indrekj opened a new pull request, #10393:
URL: https://github.com/apache/apisix/pull/10393
## Description
The previous implementation did not [follow the spec][1]. I'll list the
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
## 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
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
Not relevant.
- [x] I have verified that this change is backward compatible (If not,
please discuss on the [APISIX mailing
list](https://github.com/apache/apisix/tree/master#community) first)
There obviously is a change, but I would consider it backward-compatible
(or at least very low-risk one). I named this as `feat` but could also say it's
a bug fix to make sure we are aligned with the OTel spec
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
