open-telemetry / opentelemetry-ruby-contrib

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

feat: Add ability to customize `ActiveSupport` spans #114

Closed arielvalentin closed 10 months ago

arielvalentin commented 2 years ago

The current implementation of ActiveSupport Instrumentation only allows authors to customize the span attributes^1 and not the span name, kind, or add events that would make it possible to conform to the semconv specifications, e.g. messaging^2.

I would like for ActiveSupport instrumentation to provide a way to customize span creation as well as the ability to set the status and events.

cc: @chrisholmes

chrisholmes commented 2 years ago

There's a couple more features that we would need to port across subscriber's similar to those used in the Racecar instrumentation:

ahayworth commented 2 years ago

We might be able to do this, but I'm not sure.

I think you could sum up my feelings on this as: this feels a lot like HTTP hooks and I have the same general opinions - that makes the library harder to maintain and I don't care for that. If we can do it in a way that isn't opening the same can of worms, then I could get on board.

An alternative might be restructuring the ActiveSupport notifications instrumentation to make it more easy to subclass and override bits? What do we think of that approach?

Here are more detailed thought on the proposals though, for your consideration:


Customizing span name

The main challenge here is that the code assumes that the span name and the notification-to-subscribe-to are one and the same (and we memoize the span name at subscription time). We'd need to modify the SpanSubscriber class to take a name and a pattern. For backwards compatibility, we could fall back to old behavior (assuming name == pattern) when the pattern isn't explicitly given.

Somewhat annoyingly, the SpanSubscriber class takes keyword args, but OpenTelemetry::Instrumentation::ActiveSupport.subscribe does not. We could fix that up as well, which will be easier to do now before more instrumentation uses this. On the other hand, that's a breaking change. 😢

Customizing span creation

We could, but we also decided we didn't like the idea of hooks for HTTP instrumentation and conceptually this feels similar.

Set status and events

We provide a payload transformation option where you supply a callable and have the opportunity to sanitize that as you wish. Nothing says we couldn't provide a generic span-modification callback... but again: hooks.

Extracting parent context / links

It would be possible to choose to extract a parent context from payload rather than starting our own. This feels like an HTTP hook again, but maybe it's not as bad here.

github-actions[bot] commented 1 year 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.