Skip to content

Move content length out of basic attributes#1031

Merged
MrAlias merged 4 commits intoopen-telemetry:masterfrom
Aneurysm9:fix_http_metric_cardinality
Aug 5, 2020
Merged

Move content length out of basic attributes#1031
MrAlias merged 4 commits intoopen-telemetry:masterfrom
Aneurysm9:fix_http_metric_cardinality

Conversation

@Aneurysm9
Copy link
Copy Markdown
Member

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().

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().
@lizthegrey
Copy link
Copy Markdown
Member

Wait, why shouldn't it be on the exporter/receiver to deal with high cardinality fields it can't deal with?

@Aneurysm9
Copy link
Copy Markdown
Member Author

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 function was intended to generate high-cardinality labels given the comment it bears:

// 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.

@lizthegrey
Copy link
Copy Markdown
Member

For traces that makes sense, but I'm not sure with metrics. In any event, I don't believe that the HTTPServerMetricAttributesFromHTTPRequest function was intended to generate high-cardinality labels given the comment it bears:

// 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...

@Aneurysm9
Copy link
Copy Markdown
Member Author

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 http_server_request_content_length needs http_request_content_length as a label.

@jmacd
Copy link
Copy Markdown
Contributor

jmacd commented Aug 5, 2020

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.

@Aneurysm9
Copy link
Copy Markdown
Member Author

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.

@MrAlias MrAlias merged commit b40fdf1 into open-telemetry:master Aug 5, 2020
@Aneurysm9 Aneurysm9 deleted the fix_http_metric_cardinality branch August 5, 2020 21:01
@Aneurysm9 Aneurysm9 mentioned this pull request Aug 24, 2020
MrAlias added a commit to MrAlias/opentelemetry-go that referenced this pull request Aug 28, 2020
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

6 participants