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

[metrics] Document view level cardinality limit #5321

Merged
merged 9 commits into from
Feb 7, 2024

Conversation

reyang
Copy link
Member

@reyang reyang commented Feb 7, 2024

Follow up #5312.

@reyang reyang requested a review from a team February 7, 2024 04:20
@reyang reyang changed the title Document view level cardinality Document view level cardinality limit Feb 7, 2024
@reyang reyang requested a review from Yun-Ting February 7, 2024 04:21
reyang and others added 2 commits February 7, 2024 11:47
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
`SetMaxMetricPointsPerMetricStream` method, or at individual
[view](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#view)
level. Refer to this
level using `MetricStreamConfiguration.CardinalityLimit`. Refer to this
[doc](../../docs/metrics/customizing-the-sdk/README.md#changing-maximum-metricpoints-per-metricstream)
Copy link
Contributor

@Yun-Ting Yun-Ting Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section can be updated to be: "../../docs/metrics/customizing-the-sdk/README.md#changing-the-cardinalityLimit", given the name standardization effort: https://github.com/open-telemetry/opentelemetry-dotnet/pull/5328/files.

@@ -356,18 +356,13 @@ predictable and reliable behavior when excessive cardinality happens, whether it
was due to a malicious attack or developer making mistakes while writing code.

OpenTelemetry has a default cardinality limit of `2000` per metric. This limit
can be configured at `MeterProvider` level using
can be configured at `MeterProvider` level using the
`SetMaxMetricPointsPerMetricStream` method, or at individual
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`SetMaxMetricPointsPerMetricStream` method, or at individual
`SetCardinalityLimit` method, or at individual

@Yun-Ting
Copy link
Contributor

Yun-Ting commented Feb 7, 2024

Might be nice to explicitly point out in the metrics readme file that cardinality limit will be pre-allocated to prevent user from abusing it.

@reyang
Copy link
Member Author

reyang commented Feb 7, 2024

Might be nice to explicitly point out in the metrics readme file that cardinality limit will be pre-allocated to prevent user from abusing it.

This is captured here https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/metrics#memory-preallocation.

@CodeBlanch CodeBlanch added documentation Documentation related metrics Metrics signal related labels Feb 7, 2024
@CodeBlanch CodeBlanch changed the title Document view level cardinality limit [metrics] Document view level cardinality limit Feb 7, 2024
@CodeBlanch CodeBlanch merged commit 05700f6 into open-telemetry:main Feb 7, 2024
30 checks passed
@CodeBlanch
Copy link
Member

I merged this and then put some updates on #5328

@reyang reyang deleted the reyang/doc-view-cardinality branch February 9, 2024 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related metrics Metrics signal related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants