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

HTTP span name for unknown methods #268

Closed
lmolkova opened this issue Aug 17, 2023 · 9 comments · Fixed by #270
Closed

HTTP span name for unknown methods #268

lmolkova opened this issue Aug 17, 2023 · 9 comments · Fixed by #270
Assignees

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Aug 17, 2023

HTTP spec currently requires to use http.request.method:

is not generally aware of the "route", and therefore HTTP client spans SHOULD use
`{http.request.method}`.

For custom/unsupported methods, it means _OTHER, however user experience could arguably be better if span names represented the original method such as get or ACL, which are captured in http.request.method_original.

The argument for _OTHER is low cardinality, but it is not a clear requirement - open-telemetry/opentelemetry-specification#3534.

Span name is a part of stability guarantees

@trask
Copy link
Member

trask commented Aug 17, 2023

The span name SHOULD be the most general string that identifies a (statistically) interesting class of Spans

If the unknown methods are valid, then each unknown method makes an interesting group of span names

but if the unknown methods are invalid (spam), then grouping them all together makes an interesting group of span names

@lmolkova
Copy link
Contributor Author

lmolkova commented Aug 17, 2023

users and backends can still group using http.request.method attribute.

I just imagine spans looking like _OTHER in the list of span names - the immediate thing I'd do is to open the span to find out what it actually is, but if I see ThIS is SPaM, I immediately know what it is. And from my perspective, span names goal is to provide a quick summary of what it is. EDIT: and _OTHER does not serve this purpose well

@Oberon00
Copy link
Member

Oberon00 commented Aug 18, 2023

The span name's primary purpose is grouping. Low cardinality is more important than human-readability, that is what the spec says. A cardinality explosion in the span name is just as bad as in a metric attribute. See open-telemetry/opentelemetry-specification#557

Backends can (and probably should) generate a different display name based on attributes.

@trask trask moved this to Blocker for stability in Spec: HTTP Semantic Conventions Aug 18, 2023
@lmolkova
Copy link
Contributor Author

discussed at semconv SIG:

So closing this one.

@github-project-automation github-project-automation bot moved this from Blocker for stability to Done in Spec: HTTP Semantic Conventions Aug 21, 2023
@Oberon00
Copy link
Member

Oberon00 commented Aug 21, 2023

@lmolkova I think it could still make sense to define a special case for "_OTHER" like "HTTP _OTHER", but OTOH, the spans were previously named "HTTP <method>", which was removed in open-telemetry/opentelemetry-specification#3165 (personally, I thought the variant with "HTTP <method>" was better)

(EDIT: Markdown parser ate my angle brackets)

@trask
Copy link
Member

trask commented Aug 21, 2023

I kind of like the span name HTTP for the case where we don't have a known http method

EDIT: I guess unknown http methods could still have a route, in which case HTTP {http.route}

A nice this about this naming is that those spans will be more identifiable in a U/X as HTTP, as opposed to only saying _OTHER

@trask trask reopened this Aug 21, 2023
@trask trask moved this from Done to Blocker for stability in Spec: HTTP Semantic Conventions Aug 21, 2023
@joaopgrassi
Copy link
Member

joaopgrassi commented Aug 22, 2023

A nice this about this naming is that those spans will be more identifiable in a U/X as HTTP, as opposed to only saying _OTHER

But that is something a back-end can easily do as a UI-only tweak. For ex Jaeger does this already - The URL is shown together with the span name. I think other back-ends also do something similar OR can do if they want.

All to say I'm unsure if changing the spec to convey such UI feature makes sense.

@Oberon00
Copy link
Member

Oberon00 commented Aug 22, 2023

But that is something a back-end can easily do as a UI-only tweak.

No. It would need to determine that the span name comes from HTTP conventions. These can be mixed with other conventions and often HTTP is not the primary one. So this is not easy even though it may be doable, at least heuristically.

All to say I'm unsure if changing the spec to convey such UI feature makes sense.

This is not (only) an UI feature. It also avoids potential name collisions where unrelated spans would then group together (when using span name). Especially _OTHER was IIRC agreed upon as a standard enum value, so it may turn up in other conventions as well.

@lmolkova lmolkova changed the title HTTP span name: http.request.method or http.request.method_original HTTP span name for unknown methods Aug 22, 2023
@lmolkova
Copy link
Contributor Author

updated #270 to use HTTP, PTAL

@github-project-automation github-project-automation bot moved this from Blocker for stability to Done in Spec: HTTP Semantic Conventions Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

5 participants