Provide guidelines for low-cardinality span names#416
Provide guidelines for low-cardinality span names#416carlosalberto merged 5 commits intoopen-telemetry:masterfrom
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
yurishkuro
requested review from
AloisReitbauer,
SergeyKanzhelev,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
songy23,
tedsuo and
tigrannajaryan
as code owners
Signed-off-by: Yuri Shkuro <ys@uber.com>
| | ----------------- | ------------ | | ||
| | `get` | Too general | | ||
| | `get_account/42` | Too specific | | ||
| | `get_account` | Good, and account_id=42 would make a nice Span attribute | |
There was a problem hiding this comment.
What do you do in the case that you have the url, but don't know which part of it is an id or dynamic?
There was a problem hiding this comment.
then you can't use it. It's covered in the 2nd file.
| unless another value that represents the identity of the request and has a lower cardinality can be identified | ||
| (e.g. the route for server spans; see below). | ||
| HTTP spans MUST follow the overall [guidelines for span names](./api-tracing.md#span). | ||
| Many REST APIs encode parameters into URI path, e.g. `/api/users/123` where `123` |
There was a problem hiding this comment.
Please make a more specific recommendation. E.g. using one of the formats "HTTP {METHOD-NAME}" or "{METHOD-NAME} {HOST-NAME}" sounds reasonable.
There was a problem hiding this comment.
Method is already suggested in the following sentences, I can change to use the exact pattern.
Host name is not acceptable. If clients are directly integrated with service discovery, then host name may be very high cardinality.
There was a problem hiding this comment.
Yeah, but I would make it more clear that this not merely a suggestion but actually what should be used (in absence of a better option like endpoint name, route, ...). And I would definitely prescribe an exact (fallback) format, because that would allow grouping HTTP requests from different languages together on the back end.
There was a problem hiding this comment.
Added exact recommendation in L34, but I don't know how strongly prescriptive we want to be.
Signed-off-by: Yuri Shkuro <ys@uber.com>
|
cc @open-telemetry/specs-approvers |
This patch moves the B3 and trace context propagators into the api/trace module and renames the distributed context module to `propagation`. It also incorporates the doc changes from open-telemetry/opentelemetry-specification#416
This patch moves the B3 and trace context propagators into the api/trace module and renames the distributed context module to `propagation`. It also incorporates the doc changes from open-telemetry/opentelemetry-specification#416
This patch moves the B3 and trace context propagators into the api/trace module and renames the distributed context module to `propagation`. It also incorporates the doc changes from open-telemetry/opentelemetry-specification#416
7 participants
Resolves #270