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

Clarify cardinality requirements of a span name #3534

Open
lmolkova opened this issue Jun 2, 2023 · 9 comments
Open

Clarify cardinality requirements of a span name #3534

lmolkova opened this issue Jun 2, 2023 · 9 comments
Labels
spec:trace Related to the specification/trace directory triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Jun 2, 2023

Came up in:
open-telemetry/semantic-conventions#35 (comment)

In #3470 we realized that http.request.method can have high cardinality when custom HTTP methods are used.
While it clearly represents a problem for metrics, it's not clear what the consequences are for the span name.

The spec does not clearly require span name to have low cardinality, instead, it talks about names being general, identifying a class of operations rather than individual ones:

The *span name* concisely identifies the work represented by the Span,
for example, an RPC method name, a function name,
or the name of a subtask or stage within a larger computation.
The span name SHOULD be the most general string that identifies a
(statistically) interesting *class of Spans*,
rather than individual Span instances while still being human-readable.
That is, "get_user" is a reasonable name, while "get_user/314159",
where "314159" is a user ID, is not a good name due to its high cardinality.
Generality SHOULD be prioritized over human-readability.

Based on this and the history of the changes (#557, #270), the idea is to identify a class of operations and work as a grouping key.
In this sense, the cardinality restrictions are soft and the cardinality term doesn't quite apply.

In the context of http method, grouping operations should work well even when invalid methods are recorded in the span name - there just will be a long tail of rare custom methods.

Question:
Do we use/envision using the span name as a dimension on metrics?

  • if yes: We should clearly state that span names SHOULD have low cardinality
  • if no: Let's keep recommending instrumentations to use the name that identifies a class of operations, but clarify that telemetry pipeline/consumers of telemetry should not assume span names to have low cardinality.
@lmolkova lmolkova added the spec:trace Related to the specification/trace directory label Jun 2, 2023
@Oberon00
Copy link
Member

Oberon00 commented Jun 5, 2023

Generally, I feel it is not good for a span name to have high cardinality.

The question is if we want to live with the "defect" we currently have in the HTTP conventions, or find another solution like normalization (nontrivial) or using a rather unhelpful span name (like "HTTP client" instead of "HTTP <method>")

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 5, 2023

Generally, I feel it is not good for a span name to have high cardinality.

I agree. And yes, the questions are:

  • is high cardinality in edge cases a problem?
  • is there any benefit in normalization?

@trask
Copy link
Member

trask commented Jun 5, 2023

The question is if we want to live with the "defect" we currently have in the HTTP conventions, or find another solution like normalization (nontrivial)

isn't this defect solved if we go with Limit HTTP request method cardinality: original_method (option A)?

this is the one reason I'm currently leaning towards "Option A"

cc @reyang @jsuereth @arminru

@Oberon00
Copy link
Member

Oberon00 commented Jun 6, 2023

I think normally one would not use high-cardinality attributes as input for the span name, instead of normalizing. But that would leave nothing at all for (client-side) HTTP except for "HTTP client"... (Server-side, one could consider reverting #2998, or clarifying that it only applies to accepted requests)

@pyohannes
Copy link
Contributor

What's exactly the scope of this issue? Just span names recommended by semantic conventions, or all span names in general?

If it's the latter, I'd be reluctant to add this as a SHOULD into the API specification, as there is no way to enforce this (as it's given by the end user of the API).

@Oberon00
Copy link
Member

Oberon00 commented Jun 6, 2023

IMHO the wording should/could address the consumers here: SHOULD they be able to handle high cardinality in a reasonable manner or MAY they assume span names to be low-cardinality (and exhibit bad behavior like dropping spans entirely in an unsystematic way, or creating UIs pages that are so large that they freeze the browser on load, etc.)

A (at least as strict) recommendation for the semantic conventions can be made additionally.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 6, 2023

I would rather be strict about assumptions and say "Consumers SHOULD NOT assume span names to have low cardinality".

Even when every instrumentation does its best and creates names from low-cardinality attributes, in a big system number of distinct operations can reach hundreds (thousands?). We don't define what constitutes low or high cardinality (#2996), but it feels like at least middle-ish.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 6, 2023

What's exactly the scope of this issue? Just span names recommended by semantic conventions, or all span names in general?

great question. Agree we can't control span names in general. The question is how much effort we need to do for semconv and otel-authored instrumentations.

@austinlparker
Copy link
Member

Discussed by GC - We concur that clarifying language to consumers of OTel Spans should indicate that span names MAY be high cardinality, but that the interpretation of 'statistically interesting' in the linked spec document should be clarified to read as 'low cardinality'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor
Projects
Status: Spec - Accepted
Development

No branches or pull requests

7 participants