change(opentelemetry): make span name and attributes follow the standard spec #10393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous implementation did not [follow the spec][1]. I'll list
reasoning below.
Span name:
> HTTP server span names SHOULD be {http.method} {http.route} if there
is a (low-cardinality) http.route available. HTTP server span names
SHOULD be {http.method} if there is no (low-cardinality) http.route
available. HTTP client spans have no http.route attribute since
client-side instrumentation is not generally aware of the "route", and
therefore HTTP client spans SHOULD use {http.method}. 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.
It's clearly mentioned that the span name MUST NOT use URI path, but the
previous implementation did exactly that. Now it uses `http.method
http.route` where the latter is added only when available.
`http.status_code`, `http.scheme`, `http.target`, `http.method`, and
`net.host.name` are required according to the spec.
`http.route` is required if and only if it is abailable.
`http.user_agent` is recommended.
[1]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md#http-server
|
Thanks for your contribution! we will check this later |
|
@yangxikun Could you help to check this pr? |
|
@monkeyDluffy6017 @yangxikun friendly ping 🙇 , one of the last things we would like to get in before we can deploy Apisix to non-test envs. |
|
@indrekj I will check this in this week |
|
Approve, Thanks for the contribution! |
|
@yangxikun click here if you have checked |
monkeyDluffy6017
approved these changes
Nov 2, 2023
monkeyDluffy6017
changed the title
yangxikun
approved these changes
Nov 2, 2023
AlinsRan
approved these changes
Nov 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The previous implementation did not follow the spec. I'll list the reasoning below.
Span name:
It's clearly mentioned that the span name MUST NOT use URI path, but the previous implementation did exactly that. Now it uses
http.method http.routewhere the latter is added only when available.http.status_code,http.scheme,http.target,http.method, andnet.host.nameare required according to the spec.http.routeis required if and only if it is available.http.user_agentis recommended.Motivation
The most important thing for us is the span name. The previous one had unlimited cardinality which would be a very bad idea in production (it can slow down the tracing platform and when using 3rd party services it could create huge bills).
Checklist
Not relevant.
There obviously is a change, but I would consider it backward-compatible (or at least very low-risk one). I named this as
featbut could also say it's a bug fix to make sure we are aligned with the OTel spec