Roll the dice for ruby #2655
Roll the dice for ruby #2655
Conversation
Signed-off-by: svrnm <[email protected]>
Co-authored-by: Patrice Chalin <[email protected]>
Co-authored-by: Patrice Chalin <[email protected]>
|
@chalin thanks for reviewing! I addressed all of your comments, anything else or are we good to merge?:-) |
| lifecycle. For Rails applications, the usual way to initialize OpenTelemetry is | ||
| in a Rails initializer. For other Ruby services, perform this initialization as | ||
| early as possible in the start-up process. | ||
| lifecycle. Perform this initialization as early as possible in the start-up |
There was a problem hiding this comment.
My only hesitation here is that the majority of our contributors, maintainers, and support requests are specifically around using OTel with Ruby On Rails.
I suspect removing references to initializing Rails applications vs using Sinatra will increase our support requests for documentation from our user base.
I would like to keep the reference to Rails initializers in the documentation in addition to the Sinatra example.
There was a problem hiding this comment.
I would agree with this. While it's generally good to be as neutral as possible, I think we should keep mentioning Rails specifically because it's just so prominent in the world of Ruby.
There was a problem hiding this comment.
I initially wanted to build the sample app with rails but Sinatra appeared to be easier and less steps involved. I think there are 2 options:
- Replace sinatra with Rails
- Add a "Note" for the rails specific initalizers
I can do both, I lean towards the first, although it is a little bit more work
There was a problem hiding this comment.
I think in most cases folks will be retrofitting into existing Rails apps, which is one reason our original docs where intended to be copy/paste modify and we didn't build an entire sample app.
Of course if you have the bandwidth to put that together it will be most welcome.
I do realize that the goal here is to have a symmetric example, so I think the Sinatra app is pretty good. I really appreciate you adding this example in the docs. Thank you for doing that!
There was a problem hiding this comment.
I'll try to retrofit it into rails, let me see how far I get :)
There was a problem hiding this comment.
so... I reworked it and it's now rails, please give it another look, hope the things I did make sense, ruby is not a language I am fluent in ;-)
There was a problem hiding this comment.
Haven't stepped through it but LGTM ... pending the issue raised by @arielvalentin.
Signed-off-by: svrnm <[email protected]>
There was a problem hiding this comment.
Assuming @chalin comments are addressed. Thank you for doing this!
Co-authored-by: Patrice Chalin <[email protected]>
|
thanks for the additional review, @chalin |
Signed-off-by: svrnm <[email protected]>
|
@cartermp - this good to go from your p.o.v.? |
|
@arielvalentin & @open-telemetry/ruby-approvers: please take another look :-) |
|
Just saw that you already did a review & approval @arielvalentin, thank you very much! |
One more for #2623: ruby!
Preview: