bug: Rack configuration options ignored due to ActionPack instrumentation

[nvolker]

Summary

When thinning out our instrumentations, solely using Rack, I found that the middleware wasn't being installed.
I then stumbled across this: https://cloud-native.slack.com/archives/C01NWKKMKMY/p1629852592034300?thread_ts=1629850975.033500&cid=C01NWKKMKMY

After adding ActievJob, as follows:

OpenTelemetry::SDK.configure do |config|
  config.service_name = "foo"
  config.use "OpenTelemetry::Instrumentation::ActionPack"
  config.use "OpenTelemetry::Instrumentation::Rack", { untraced_endpoints: ["/health"] }
end

I noticed the following INFO logs:

[09:47] INFO Instrumentation: OpenTelemetry::Instrumentation::ActionPack was successfully installed with the following options {:enable_recognize_route=>false} Context: { schema: rails }
[09:47] INFO Instrumentation: OpenTelemetry::Instrumentation::Rack was successfully installed with the following options {:allowed_request_headers=>[], :allowed_response_headers=>[], :application=>nil, :record_frontend_span=>false, :retain_middleware_names=>false, :untraced_endpoints=>[], :url_quantization=>nil, :untraced_requests=>nil} Context: { schema: rails }

Seeing that untraced_endpoints was an empty Array and not taking in my config (see ☝️ logs), I then switch the ordering of ActionPack & Rack and got this (👇):

[09:50] INFO Instrumentation: OpenTelemetry::Instrumentation::Rack was successfully installed with the following options {:allowed_request_headers=>[], :allowed_response_headers=>[], :application=>nil, :record_frontend_span=>false, :retain_middleware_names=>false, :untraced_endpoints=>["/health", "/ping"], :url_quantization=>nil, :untraced_requests=>nil} Context: { schema: rails }
[09:50] INFO Instrumentation: OpenTelemetry::Instrumentation::ActionPack was successfully installed with the following options {:enable_recognize_route=>false} Context: { schema: rails }

Now :untraced_endpoints is correctly populated.

I believe this is happening due to the ActiveJob instrumentation installing Rack with an empty config {}, here: https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/railtie.rb#L13

Share details about your runtime

Operating system details: macOS, Catalina 10.15.7
RUBY_ENGINE: "ruby"
RUBY_VERSION: "2.7.1" & "3.0.3"
RAILS_VERSION: "6.0.4.8" & "6.1.5"

[nvolker]

As suggested by @arielvalentin, here, using ENV variables works as expected as this pulls this is pulled in separately from the user-supplied configuration:

export OTEL_RUBY_INSTRUMENTATION_RACK_CONFIG_OPTS="untraced_endpoints=/health"

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/base/lib/opentelemetry/instrumentation/base.rb#L272

[ahayworth]

I believe this is something of a known issue (although I do not recall where we have discussed prior). It’s probably worth figuring out how to make this work correctly regardless of the ordering in the config block. Or, we could at least document this behavior (and the workarounds) somewhere more prominently.

If I remember correctly, we had presumed that folks wouldn’t typically be installing Rack instrumentation in addition to something that builds on it. However, I think that presumption originated in a time when we didn’t have as many interesting Rack options for one to set. :)

[johnnyshields]

I am experiencing this too. Tried the following:

  config.use "OpenTelemetry::Instrumentation::Rack", ...
  config.use_all

But it raises:

OpenTelemetry error: unexpected configuration error due to Use either `use_all` or `use`, but not both

Would make sense if we could combine these two.

[ahayworth]

Yes, they are exclusive (although we may be able to make it more permissive).

When I think about the reasons someone would wish to do this, it sounds like you wish to use all instrumentation but pass specific options to one of the libraries. That is possible with use_all:

OpenTelemetry::SDK.configure do |c|
  c.use_all { 'OpenTelemetry::Instrumentation::Rack' => { untraced_endpoints: ['probes/ready'] } }
end

...however, I don't know that this is documented well enough. I'll make a note on our documentation issue that this is a thing that could be more clear.

[johnnyshields]

Yes, but as noted above, this currently doesn't work b/c ActionPack auto-loads Rack without the options.

[arielvalentin]

@robertlaurin et.al. I looked into this a bit today and a proper fix for this issue is going to be a bit difficult due to state management of the configuration options and the interactions between the SDK Configurator, Registry, and the Instrumentations.

The instrumentation options are passed as arguments to the registry at configuration time:

https://github.com/open-telemetry/opentelemetry-ruby/blob/03e36cec755df97e08139494b85473a7ad547001/sdk/lib/opentelemetry/sdk/configurator.rb#L161

The registry is unaware of any dependent installation order and installs them in the order they were registered:

https://github.com/open-telemetry/opentelemetry-ruby/blob/03e36cec755df97e08139494b85473a7ad547001/registry/lib/opentelemetry/instrumentation/registry.rb#L78

The config parameter for in install is specific to the instrumentation and does not contain any programmatic options for dependent instrumentations.

As @johnnyshields pointed out the ActionPack Railtie installs the Rack instrumentation without any configuration options, which means the only way to override these options is via environment variables or to manually register the instrumentations knowing the dependent order.

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/railtie.rb#L13

I think we need to review the Configurator's design and consider an approach where either...
... we add configuration state to the individual instrumentations during registration time or
... the Registry takes on the responsibility of holding the state of programatic configurations or
... introduce a dependency graph to ensure instrumentations are installed in the appropriate order

[ahayworth]

introduce a dependency graph to ensure instrumentations are installed in the appropriate order

This is perhaps a bold take, but I suspect this will be useful. I also don't think it would be terribly hard to implement; I think Rails has some examples we could stealborrow. ^{1 2}

Other approaches could work, though - collecting all installation options and deferring the actual installation until the end of the configure block (and merging them somehow) maybe could work, although that idea is not fully formed in my mind.

Since this is only stuff run at SDK initialization, we can probably do something friendly and backwards-compatible with the registry (something something something metaprogramming) to make it a non-breaking enhancement, if we're careful?

---

Footnotes

1. https://github.com/rails/rails/blob/18707ab17fa492eb25ad2e8f9818a320dc20b823/railties/lib/rails/initializable.rb ↩

2. https://ruby-doc.org/stdlib-3.1.2/libdoc/tsort/rdoc/TSort.html ↩

[johnnyshields]

@ahayworth dependency graphs are lovely, but I think there is a much simpler solution, which is to allow using both use_all and use X, and load the configs in the order declared (reloading if necessary). Any configs declared in use_all's block arg should be loaded at the end, in the order declared.

[ahayworth]

@johnnyshields That's true, but if you squint at it that feels (to me) like it's a shortcut to a dependency graph.

Or more specifically, making our users construct the appropriate dependency graph 😆 😆

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.