feat: OTel Railtie #1111
feat: OTel Railtie #1111
Conversation
arielvalentin
force-pushed
the
arielvalentin/railtie
branch
2 times, most recently
from
e1204ce
to
71ba3ed
Compare
|
@SomalianIvan @robertlaurin This is rather perplexing and I am curious if you all have run into this error if the course of testing auto-instrumentation with JRuby. Tests are failing for Linux JRuby 9.3.3.0 + Rails/ActiveRecord 6.1.4.4 with a https://github.com/open-telemetry/opentelemetry-ruby/runs/5074238711?check_suite_focus=true |
|
DOAH Never mind this is JRuby being stuck at Rails 6 && Ruby 2.6.8 :table-flip: It still makes 0 sense to me that my testes do not fail locally |
arielvalentin
force-pushed
the
arielvalentin/railtie
branch
from
71ba3ed
to
405b1e6
Compare
arielvalentin
requested review from
ahayworth,
dazuma,
ericmustin,
fbogsany,
mwear and
robertlaurin
as code owners
|
Been thinking about this PR and where this railtie could or should live.
The more I think about the more I think that this should probably be part of the rails instrumentation gem, the user experience then becomes:
Would be good to discuss this on the sig today. |
There was a problem hiding this comment.
Firstly, great stuff as usual. The actual implementation I don't have much feedback on, lgtm broadly, i think pulling in a few rails specific defaults for resource attributes and service name "makes sense". I had one small point of feedback on the initializer order but its pretty minor.
As far as the tests, my opinion is that you've taken a good direction here although it's a lot of boilerplate. If we're worried about the test suite taking too long to run, i think we can probably hack that by having any auto-instrumentation tests be "off by default" and enabled via env var, and we can choose to selectively enable those tests on certain ci steps (perhaps just on latest ruby and on jruby, or something along those lines).
as @robertlaurin pointed out i think the uncertain thing is packaging.
I think the simplest way to get this out the door, and manage it long term without boxing ourselves in, is to simply include it as a file within opentelemetry-instrumentation-rails that a user can require from theGemfile.
gem 'opentelemetry-instrumentation-rails', require: 'lib/opentelemetry-railtie'
I haven't actually tested this specific incantation but I "think" it should "just work". This was the approach we took with dd-trace-rb(which was, to be fair, structured differently, as 1 big gem and not a bunch of small ones), and it seemed to be well received in terms of user adoption and also in terms of not having a ton of angry support tickets :)
Thinking about it for different types of users:
- For existing users, or those who don't want the magical OOTB experience, there's no breaking changes because they aren't going to be requiring
lib/opentelemetry-railtie - For new users, they get a magical OOTB experience via both "quickstart documentation" and perhaps eventually the otel operator, if they want it.
gem 'opentelemetry-instrumentation-all
gem 'opentelemetry-instrumentation-rails', require: 'lib/opentelemetry-railtie'
New rails users (the vast majority of our new users) can blindly c/p that code snippet and get everything instrumented and lots of control via Env var. And If new users eventually want to start tuning what instrumentation packages they want to include they can still do it via just their gemfile(by switching out instrumentation-all for specific subset of packages they want) and env vars.
By packaging it within the instrumentation-rails gem, we ensure that there's never going to be a scenario where the railtie "doesn't work", or the user expects a no-code auto-instrumentation experience and doesn't get it. and I also think we offering the right message that this only works for rails.
Also, we aren't boxing ourselves into maintaining a new gem, releasing that gem, etc etc, which i think is practically speaking a good thing.
| class Railtie < ::Rails::Railtie | ||
| railtie_name :opentelemetry | ||
| config.opentelemetry = ActiveSupport::OrderedOptions.new | ||
| initializer 'opentelemetry.configure' do |app| |
There was a problem hiding this comment.
The before and after options lets us determine where we want to insert the railtie in the initializer process. New Relic inserts before load_config_initializers , which makes sense to me since this would ensure our railtie runs before anything in config/initializers/
| initializer 'opentelemetry.configure' do |app| | |
| initializer 'opentelemetry.configure', before: :load_config_initializers do |app| |
There was a problem hiding this comment.
There was a problem hiding this comment.
hmmm... I need to dig into this a bit more to understand what effect this will have.
It seems New Relic is dealing with an issue where their instrumentation is inadvertently loading the framework but I have not looked closely enough to see why they are running into this problem:
| class Railtie < ::Rails::Railtie | ||
| railtie_name :opentelemetry | ||
| config.opentelemetry = ActiveSupport::OrderedOptions.new | ||
| initializer 'opentelemetry.configure' do |app| |
There was a problem hiding this comment.
kinda scope creep but how do we feel about wrapping this in an on/off environment variable?
If the goal is "just update the gemfile once and control everything with env vars" it might be nice to have a break-glass env-var option to completely disable instrumentation.
|
I will move this into the Rails auto-instrumentation gem.
Are you saying there's too much structural duplication and you me to consolidate them?
What gives you that impression? |
|
In the spirit of making things easier to do for our users, I'm inclined to include the Railtie if the class is present and then add an option that could be set to skip it entirely both in code and via envar. wdyt? @robertlaurin @ericmustin |
This is how I generally expect rails-related things to work: add the gem to my Rails is magic, as its detractors enjoy reminding everyone! I also like the idea of including a config option to disable the auto-configuration, if you need. I suspect some big users of Rails with highly specialized initialization routines might benefit from disabling that in certain monolithic applications, while still taking advantage of the easy autoconfig in other Rails apps they run. Hypothetically, of course. |
|
if the railtie is invoked by default and we include it in Re, the tests, i don't think there's too much duplication, and i am not sure if the tests take too long or not, i just recall @arielvalentin mentioning the tests took awhile in the SIG mtg, but it's possible i have my wires crossed. |
We have not concerned ourselves too much with stability guarantees in the past for any gems that are pre-1.0 or GA. Are we changing our position on this?
I was referring to our existing GH actions setup and it is not specific to these changes. The current test suite completes in a little under 10 minutes per matrix version. |
I don't think that's accurate. We have multiple "deprecated" configuration options, for example, and I think this represents a significant change in behavior, certainly the most significant since we've 1.0'd the SDK. As @robertlaurin pointed out, since Multiple vendors are also informing their users to rely on this package so i think a bit of care is warranted since we'd be breaking real folks out there's real monitoring for real stuff, and, i'd like to avoid that if possible? I don't think that's unreasonable. |
|
I agree that I don't want to surprise our users in a negative way but I'm also keen on pushing towards making as much available to our users by default as we possibly can. w/r/t breaking changes... IIRC the one option we did that for was DB statement obfuscation and that was because it posed a risk of leaking PII so we wanted to be careful about it. If there are others then yeah I will concede that we need a less disruptive rollout but we should also make that an explicit rule as opposed to exception. As far as vendors are concerned, they should probably defer to our documentation or (as we've discussed in side bars) manage their own distros and pin to specific versions so they can mitigate upstream changes. If we worry about vendor documentation being stale all of the time we'll slow down our ability to make significant changes especially around auto-instrumentation, which is moving target. |
This gem provides a minimal Railtie to use in lieu of manual configuration. Operators may customize SDK settings using environment variables but this gem will provide some defaults: Sets a minimal set of values for OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES when left unset Users may override SDK configurations using standard OTel SDK environment variables Users may override Instrumentation libraries are configurable using OTEL_RUBY_*_CONFIG_OPTS
arielvalentin
force-pushed
the
arielvalentin/railtie
branch
2 times, most recently
from
117c6e3
to
955089a
Compare
This commit moves the Railtie from its own gem into opentelemetry-instrumentation-rails. Users will have to explicitly require the railtie in their bundler file or as part of Rails bootstrap after bundler has required dependencies. It also reverses the decision to set default OTEL_RESOURCES_ATTRIBUTES based on the Rails.env since we have run into some issues in production. cc: @robertlaurin @ericmustin
arielvalentin
force-pushed
the
arielvalentin/railtie
branch
from
955089a
to
c2a11bb
Compare
|
@robertlaurin @ericmustin looking for some feedback before I proceed with writing up docs. |
There was a problem hiding this comment.
Hey @arielvalentin we did a mob review on this PR during the SIG.
The only contentious point (raised by me) was whether or not it is worth adding all the test scaffolding to support testing the railtie itself.
We landed on if you think it is worth keeping, and because you've already put in the work to test it. We can keep it.
If in the future it becomes a maintenance burden, we may reserve the right to do away with the railtie tests.
Thanks for pushing this through.
Sounds reasonable to me. |
This gem provides a minimal Railtie to use in lieu of manual configuration.
Operators may customize SDK settings using environment variables but this gem will provide some defaults:
OTEL_SERVICE_NAMEandwhen left unsetOTEL_RESOURCE_ATTRIBUTESOTEL_RUBY_*_CONFIG_OPTS