Move content length out of basic attributes#1031
Move content length out of basic attributes#1031MrAlias merged 4 commits intoopen-telemetry:masterfrom
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().