-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Move content length out of basic attributes #1031
Move content length out of basic attributes #1031
Conversation
semconv.httpBasicAttributesFromHTTPRequest() was including the request's content length, which is a high-cardinality label. It ended up in metric labels through the use of that function by semconv.HTTPServerMetricAttributesFromHTTPRequest().
Wait, why shouldn't it be on the exporter/receiver to deal with high cardinality fields it can't deal with? |
For traces that makes sense, but I'm not sure with metrics. In any event, I don't believe that the // HTTPServerMetricAttributesFromHTTPRequest generates low-cardinality attributes
// to be used with server-side HTTP metrics. If we want to allow metrics to be recorded with high-cardinality metrics then perhaps we need better ways to filter them out during the aggregation process. I've got a service producing many more metrics to Prometheus than expected due to this and I'm not aware of a solution at the exporter. |
I suspect the answer here is bucketing/histogramming... |
One of the produced metrics is a histogram. See https://gist.github.com/Aneurysm9/c5ce2afb9611b801027fa18d40637f56 for an example of the problem. For maximum absurdity, consider whether a metric for |
This is a difficult topic and I'm not sure there's a simple answer that will satisfy all parties. Somewhat related, I'm proposing the Metrics SDK Accumulator component add functionality for dropping labels, which is I think a good way to approach a large class of cost-related problems. See #1023 for a draft end-to-end PR, and see #1024 for the first concrete step to getting that done. |
In the general case that is probably the correct solution. I think in this particular case, however, this is a label that was mistakenly applied to the metrics through function re-use that should never have been applied and will always be filtered out if it remains once that capability is available. As a user of this instrumentation I would greatly prefer it to work out of the box vs. requiring additional filter configuration with every deployment. |
…try-go into fix_http_metric_cardinality
[Requirements](https://github.com/open-telemetry/community/blob/master/community-membership.md#requirements-2) - [X] >=10 substantive contributions (open-telemetry/opentelemetry-go-contrib#134, open-telemetry/opentelemetry-go-contrib#153, open-telemetry/opentelemetry-go-contrib#221, open-telemetry/opentelemetry-go-contrib#298, open-telemetry/opentelemetry-go-contrib#303, open-telemetry#796, open-telemetry#905, open-telemetry#986, open-telemetry#1044, open-telemetry#1031, open-telemetry#1102) - [X] Active >1mo - [X] add to CODEOWNERS (done in this pull) - [X] Add to CONTRIBUTING.md (done in this pull) - [X] Maintainer nomination: @MrAlias - [ ] Other maintainer(s) (@Aneurysm9) sign-off - [ ] Add to @open-telemetry/go-approvers
semconv.httpBasicAttributesFromHTTPRequest() was including the request's content length,
which is a high-cardinality label. It ended up in metric labels through the use of that function
by semconv.HTTPServerMetricAttributesFromHTTPRequest().