open-telemetry / opentelemetry-ruby-contrib

Contrib Packages for the OpenTelemetry Ruby API and SDK implementation.
https://opentelemetry.io
Apache License 2.0
80 stars 163 forks source link

Customising Rack instrumentation inside a Grape service #1011

Closed chrisholmes closed 2 weeks ago

chrisholmes commented 2 months ago

Description of the bug

When using a Grape service it is not possible to customise the behaviour of the rack instrumentation. This is due to the load order of instrumentations (alphabetical) and the approach used by the grape, rails, and sinatra web frameworks to initialise the rack instrumentation with default parameters. When we attempt to load the rack instrumentation later with custom parameters it returns early as the instrumentation was loaded already with default configruation by grape's instrumentation.

This is problematic for us as we'd likely to use the rack instrumentation configuration to suppress HTTP healthcheck spans and potentially other features.

This problem doesn't occur with the rails or sinatra instrumentation as they're loaded after the rack instrumentation becuase of the alphabetical ordering.

I'm can think of a few options for solutions, but I'd like to hold off to see what the manitainers think.

Share details about your runtime

Applies to all runtimes

Share a simplified reproduction if possible

Can share if required, but the simplest version is a change to add a configuration to rack here: https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/grape/example/trace_demonstration.rb#L18

arielvalentin commented 2 months ago

cc: @muripic

robertlaurin commented 2 months ago

What is the current sentiment of forwarding config options?

If grape is taking responsibility for installing rack instrumentation, can we also supply rack config opts through grape?

Possible no code change alternatively, have you tried configuring rack via env vars? If the env vars are present at install time it should still apply them whether its the SDK installing it or grape.

arielvalentin commented 2 months ago

@robertlaurin Yes, using envars is the most reliable way to configure the app but we do not do a good job of documenting how to customize the instrumentations using envars on opentelemetry.io. It is centered around using the DSL to accomplish this and it is very surprising to experience Rack configurations being ignored when configured manually.

Using "nested" configurations becomes complicated in applications where you have multiple frameworks involved all trying to configure the Rack instrumentation, including auto-inserting the Rack middleware.

In my case, the ActionPack and Sinatra instrumentation compete with each other in the GitHub monolith to insert the Rack middleware. I have a hack here to have Sinatra disable Rack installation so I have a PR here to actually optionally disable the installation:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/1019

This has come up several times before and it may be resolved by either introducing some sort of dependency tree or ordering in the Registry to ensure no matter the order you declare the instrumentations in the DSL, the registry installs them in the appropriate order with the correct configuration options.

chrisholmes commented 2 months ago

@arielvalentin a similar change to the grape instrumentation would also help my use case as we're often mounting Grape within rails. I can see about raising a PR for this.

chrisholmes commented 2 months ago

@arielvalentin, I've raised this PR for it: https://github.com/open-telemetry/opentelemetry-ruby-contrib/pull/1043

kaylareopelle commented 1 month ago

@chrisholmes - Does #1043 resolve this bug or is there more that needs to be done?

github-actions[bot] commented 2 weeks ago

👋 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.

chrisholmes commented 2 weeks ago

hi @kaylareopelle, I missed your message. #1043 works for us.

kaylareopelle commented 2 weeks ago

@chrisholmes, Great to hear! I'll close this issue.