Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ruby example #345

Merged
merged 12 commits into from
Aug 18, 2023
Merged

Add Ruby example #345

merged 12 commits into from
Aug 18, 2023

Conversation

kaylareopelle
Copy link
Contributor

During a recent OpenTelemetry spike, the Ruby agent team created an application that closely mirrors the example in New Relic's Python OpenTelemetry Workshop.

We'd like to share this example with the community.

@alanwest
Copy link
Member

👋 Heya @kaylareopelle! Thanks for adding a Ruby example. You got this in draft, are you still working it?

@kaylareopelle kaylareopelle marked this pull request as ready for review April 27, 2023 17:13
@kaylareopelle kaylareopelle requested a review from a team April 27, 2023 17:13
@kaylareopelle
Copy link
Contributor Author

Hi @alanwest! Thanks for pinging me! This is ready for review 😄

@reese-lee
Copy link
Contributor

Hey @kaylareopelle! Thank you so much for putting this Ruby example together, and for separating it out into the directories per our Get Started Guide example in Java! I just have a few notes:

  1. I know you mentioned OTel Ruby only has support for traces atm; I haven't gotten a chance to run the example yet, but does the tracing instrumentation produce what you would see here in the screen shots for traces?

  2. I did see that it collects the same attributes, but can you rename them to fibonacci.n and fibonacci.result?

  3. The last thing (which I spaced on mentioning) is that we'd like all the examples to use the same port; could you update it to port 8080?

Thanks again!

@kaylareopelle
Copy link
Contributor Author

kaylareopelle commented Jun 7, 2023

Hi @reese-lee! Circling back to this today! I've addressed your feedback here:

  • Set default port to 8080: ca5a33e
  • Use fibonacci.n and fibonacci.result as attribute names: 6a8a298

Here's a Ruby equivalent of the screenshot you shared:
Screenshot 2023-06-07 at 4 30 20 PM

Please let me know if you need anything else!

README.md Outdated Show resolved Hide resolved
getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@reese-lee reese-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Kayla! This looks pretty good to me. Unfortunately, I ran into issues with gems on my machine so I have not been able to run the app myself; if you can confirm that the trace data the app produces matches the trace data we're looking for here, I am good to get this merged!

getting-started-guides/ruby/instrumented/README.md Outdated Show resolved Hide resolved
@kaylareopelle
Copy link
Contributor Author

@reese-lee - Sorry to hear you ran into trouble with gems! Here's a permalink to an app on staging with data from the instrumented app: https://staging.onenr.io/06vjAqpDnjP

Things look quite similar. The only difference I see involves traces with errors: https://staging.onenr.io/0znQxBDeBRV
The Ruby application doesn't have a nested span in this scenario. It has a nested span when the request completes successfully: https://staging.onenr.io/0PLREPoXewa

@reese-lee
Copy link
Contributor

@kaylareopelle I think that's good, that might just be how it's handled in Ruby.

@kaylareopelle
Copy link
Contributor Author

Great! In that case, please merge as you're able! (I don't have permission to do so)

@reese-lee
Copy link
Contributor

@kaylareopelle Okay! We chatted about your PR and it looks pretty good. We also talked about the error trace only having one span, and if you can modify the code so that the error is handled within the calculation, that will create a nested span on the error trace. Because right now it's not doing the calculation if the input is outside the valid range (see here). Does that make sense?

@kaylareopelle
Copy link
Contributor Author

Hi @reese-lee! Sorry about the delay here. I think that makes sense. Is this what you're looking for? https://staging.onenr.io/0kERzpxLlQr

@alanwest alanwest merged commit 214cdd3 into newrelic:main Aug 18, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants