Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use semconv span naming in ActionPack #961

Open
arielvalentin opened this issue May 2, 2024 · 5 comments
Open

Use semconv span naming in ActionPack #961

arielvalentin opened this issue May 2, 2024 · 5 comments
Labels
help wanted Extra attention is needed instrumentation keep Ensures stale-bot keeps this issue/PR open spec-compliance Required for OpenTelemetry spec compliance

Comments

@arielvalentin
Copy link
Collaborator

arielvalentin commented May 2, 2024

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

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:

rack_span.name = "#{payload[:controller]}##{payload[:action]}" unless request.env['action_dispatch.exception']

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

Footnotes

  1. https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name

@robertlaurin
Copy link
Contributor

+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
Copy link
Member

robbkidd commented May 2, 2024

... 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
Copy link

karmingc commented May 7, 2024

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

@arielvalentin
Copy link
Collaborator Author

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

@karmingc yes!

karmingc added a commit to karmingc/opentelemetry-ruby-contrib that referenced this issue May 7, 2024
karmingc added a commit to karmingc/opentelemetry-ruby-contrib that referenced this issue May 8, 2024
@arielvalentin arielvalentin added help wanted Extra attention is needed instrumentation labels May 10, 2024
Copy link
Contributor

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

@github-actions github-actions bot added the stale Marks an issue/PR stale label Jun 10, 2024
@arielvalentin arielvalentin added spec-compliance Required for OpenTelemetry spec compliance keep Ensures stale-bot keeps this issue/PR open and removed stale Marks an issue/PR stale labels Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed instrumentation keep Ensures stale-bot keeps this issue/PR open spec-compliance Required for OpenTelemetry spec compliance
Projects
None yet
Development

No branches or pull requests

4 participants