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

Limit HTTP request method cardinality: use one attribute (option C/E) #35

Closed

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented May 16, 2023

Option C/E from #17 :

  • don't try to fix the method on the instrumentation side
  • recommend instrumentation to call a metric-only advice API
    • the SHOULD provide a (short/common) method list by default
    • OTel will let users override it with a custom set
  • Give up on span name low cardinality in edge cases, see #3534 for details

Pros:

  • generic solution for any attribute
  • advice API can use the same value for extra values (e.g. __DIMENSION_CAP or another magic string consistent for all attributes value)

Trade-offs:

  • querying across signals is still non-trivial. It can still work with backend help translating match to __DIMENSION_CAP magic string to a query like not in(methods_used_on_metrics_set). Or there could be another backend solution to the query-by-capped-method-across-signals problem
  • HTTP semconv can go stable as is. Instrumentations would probably wait for advice API to materialize before they can go stable to avoid breaking changes.

@lmolkova
Copy link
Contributor Author

discussed at the Spec SIG meeting. Doing dim-capping for metrics on the same attribute or defining different semantics for signals blocks cross-signal joins and goes against OTel vision.

@trask
Copy link
Member

trask commented Jun 2, 2023

http span name may have high cardinality when invalid methods are used

I'd be in favor of still normalizing the "http method" portion of the span name if we go with this option (i.e. producing same span names as Option A)

@Oberon00
Copy link
Member

Oberon00 commented Jun 2, 2023

I think that would defeat part of the purpose of this option. Ok, maybe not of the purpose, but if the assumption behind this option is that backends can cope with high cardinality of these things, I don't think the span name should get special treatment?

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 2, 2023

I'd be in favor of still normalizing the "http method" portion of the span name if we go with this option (i.e. producing same span names as Option A

This would need another (tracing-only) hint API/common processor. How critical do you think it is?

If I had a magic wand, I'd rather ask users to provide low-cardinality names similar to server-side like "GET /container/{c}/blob/{b}" and I would design hint API around this

@trask
Copy link
Member

trask commented Jun 2, 2023

the assumption behind this option is that backends can cope with high cardinality of these things

I think the assumption behind this option is that there will be a new metric hint (#68) that will solve it (and will not pass on high cardinality to the backend)

@trask
Copy link
Member

trask commented Jun 2, 2023

I'd be in favor of still normalizing the "http method" portion of the span name if we go with this option (i.e. producing same span names as Option A

This would need another (tracing-only) hint API/common processor. How critical do you think it is?

It's not clear to me how critical low-cardinality span names are. I thought the intention of making span names low-cardinality was to allow metrics to be produced with span name as a dimension, which I think would give it the same importance as having low-cardinality metric dimensions.

@lmolkova
Copy link
Contributor Author

lmolkova commented Jun 2, 2023

I thought the intention of making span names low-cardinality was to allow metrics to be produced with span name as dimensions

hm, what would be the point, if all the components of a span name are already on the HTTP span as individual attributes?
If it's a more general approach that can apply to any span then the derived metrics could only have a span name/status as a dimension.

Let me create an issue in the spec repo on it.

@lmolkova lmolkova force-pushed the http-request-method-high-card branch from 72532e9 to cf0cc05 Compare June 7, 2023 22:11
@trask
Copy link
Member

trask commented Jun 8, 2023

I've opened open-telemetry/opentelemetry-specification#3545 to work through what the metric advice would need to look like to make this option a reality, and to discuss whether it can address the "correlation problem" which has been the main (only?) objection to this option.

@lmolkova
Copy link
Contributor Author

closing in favor of #17 based on open-telemetry/opentelemetry-specification#3470 (comment)

@lmolkova lmolkova closed this Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants