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

Move content length out of basic attributes #1031

Merged

Conversation

Aneurysm9
Copy link
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
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
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
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
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
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
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
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.

5 participants