Use HTTP instead of _OTHER in HTTP span names (if method is unknown)#270
Conversation
|
EDIT sorry I see you discuss this in the issue |
daf44a4 to
739a34b
Compare
HTTP instead of _OTHER in HTTP span names (if method is unknown)
There was a problem hiding this comment.
I like d the suggestion of @trask on the issue to always prepend "HTTP ", even for the route. This will also ensure that HTTP span names don't conflict with other span names from unrelated conventions.
I added that as suggestions to this PR review.
my intention was only to use |
|
@trask Sorry, then I misunderstood you. |
joaopgrassi
left a comment
There was a problem hiding this comment.
I'm thinking since this is breaking, if we have a way/process to communicate the community/maintainers/sigs about this, so it's not forgotten? It's probably a general challenge we have here since PRs gets merged but I don't think there's a process where changes such as this are communicated to all SIGs.
Also, I'm not even sure if any instrumentation already implemented the _OTHER behavior anyway so might not be that critical.
|
Additionally: I think the changes are good, but I wonder if we can get things more "direct/clear" if we split and define proper sections for ## Name
HTTP spans MUST follow the overall [guidelines for span names](https://github.com/open-telemetry/opentelemetry-specification/tree/v1.22.0/specification/trace/api.md#span).
### Server
HTTP server span names SHOULD be `{method} {http.route}` if there is a
(low-cardinality) `http.route` available,
If there is no (low-cardinality) `http.route` available, HTTP server span names
SHOULD be `{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.
> Refer to [`method` value requirements]() for its definition
### Client
HTTP client spans have no `http.route` attribute since client-side instrumentation
is not generally aware of the "route". Therefore, HTTP client spans SHOULD be `{method}`.
> Refer to [`method` value requirements]() for its definition
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.
### {`method`} value requirements
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`.I can submit a separate PR if people think that's an improvement. |
jsuereth
left a comment
There was a problem hiding this comment.
I like the suggestion of always having HTTP as a prefix.
just confirming, you would like to revisit open-telemetry/opentelemetry-specification#3165? |
Given history there, no. Only if the idea was new and hand't been considered before. This is good to merge as-is I think. |
The idea of also prepending HTTP to routes for consistency is new IIUC |
This is true. I'm ok revisiting this, but it seems to me to come back to "what is the purpose of span names?", which to my understanding is two things:
and I'm not sure that prepending HTTP in front of well-known http methods helps either of these things. |
|
If we did go with prepending |
|
Agree with @trask that If we need a semconv-id and be able to group/visualize based on it, this would arguably go beyond default UX and would be best reported as an instrumentation scope attribute. Adding some sort of semconv-id was discussed around the open-telemetry/oteps#172 and was highly controversial. E.g. AFAIK elasticsearch reports spans that are both HTTP and DB - how they should name spans/report semconv-id for those? |
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Co-authored-by: Joao Grassi <joao@joaograssi.com>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
30a1bd9 to
9ac8f70
Compare
9ac8f70 to
88737ae
Compare
I like this idea and I tried it, but I'm not quite happy with the outcome: The sentence
applies to client and server spans ( I added an invisible anchor to |
Yep looks much nicer! Thanks for addressing it and good idea with the invisible anchor 🧠 |
Fixes #268
Changes
When
http.request.methodis set to_OTHER, useHTTPinstead of method in span names.Merge requirement checklist