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

Use semconv span naming in ActionPack #961

Open arielvalentin opened 4 months ago

arielvalentin commented 4 months ago

action_pack currently overrides the server span name to a value that does not align with semantic conventions. Per the specification^1

HTTP spans MUST follow the overall guidelines for span names.

HTTP server span names SHOULD be {method} {http.route} if there is a (low-cardinality) http.route available (see below for the exact definition of the {method} placeholder).

If there is no (low-cardinality) http.route available, HTTP server span names SHOULD be {method}.

HTTP client spans have no http.route attribute since client-side instrumentation is not generally aware of the “route”, and therefore HTTP client span names SHOULD be {method}.

The {method} MUST be {http.request.method} if the method represents the original method known to the instrumentation. In other cases (when {http.request.method} is set to _OTHER), {method} MUST be HTTP.

Instrumentation MUST NOT default to using URI path as span name, but MAY provide hooks to allow custom logic to override the default span name.

In this case the span name is set to RubyDoc naming style, with the class name of the controller followed by an # then the action:

https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/10e766e13e7734f284807af4099e0858ca85cf4f/instrumentation/action_pack/lib/opentelemetry/instrumentation/action_pack/handlers/action_controller.rb#L29

I propose the span name be set to either of the following values:

robertlaurin commented 4 months ago

+1 for ActionDispatch::Request#route_uri_pattern

Personally I would like to keep the existing naming convention as config opt if possible, it's just such a useful railism and Action Pack is rails instrumentation, just saying 😄

robbkidd commented 4 months ago

... I would like to keep the existing naming convention ... it's just such a useful railism

I've heard similar from other Rails folks instrumenting with OTel. The Rails world currently thinks in the names Rails has given and not so much thinking in the OTel semantics. Maybe that will change as more people adopt OTel and polyglot shops start wondering why their Rails web request handling spans are named differently than the spans from other services in their larger system.

karmingc commented 4 months ago

Shouldn't >= 7.1 be #{request.method} #{request.route_uri_pattern} instead?

arielvalentin commented 4 months ago

Shouldn't >= 7.1 be #{request.method} #{request.route_uri_pattern} instead?

@karmingc yes!

github-actions[bot] commented 3 months 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.