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

Implement cardinality limits for metrics streams #1889

Closed
wants to merge 4 commits into from

Conversation

bnjjj
Copy link
Contributor

@bnjjj bnjjj commented Jun 18, 2024

Fixes #1065

Changes

Adding a new function called set_stream_cardinality_limit in opentelemetry_sdk::metrics module to set the stream cardinality limit for metric streams.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj requested a review from a team June 18, 2024 12:23
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.6%. Comparing base (da368d4) to head (0b291fb).
Report is 28 commits behind head on main.

Files Patch % Lines
...pentelemetry-sdk/src/metrics/internal/aggregate.rs 50.0% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1889     +/-   ##
=======================================
- Coverage   74.6%   74.6%   -0.1%     
=======================================
  Files        122     122             
  Lines      19952   19955      +3     
=======================================
  Hits       14902   14902             
- Misses      5050    5053      +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bnjjj bnjjj force-pushed the fix_1065 branch 2 times, most recently from 4ff39f6 to 7c6ab69 Compare June 18, 2024 12:30
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@BrynCooke
Copy link
Contributor

BrynCooke commented Jun 18, 2024

I think this needs to be implemented as per: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#configuration-1

In particular cardinality can be set on views rather than globally.
Then metric readers.
Then the global limit of 2000.

@cijothomas
Copy link
Member

I think this needs to be implemented as per: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#configuration-1

In particular cardinality can be set on views rather than globally. Then metric readers. Then the global limit of 2000.

We are removing Views from initial stable release, so this can only be on a MetricReader or most likely on MeterProvider level only for now.

@lalitb
Copy link
Member

lalitb commented Jun 18, 2024

@bnjjj - Not sure if I am missing something, but how can this method set_stream_cardinality_limit be used by application to configure the cardinality?

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

it is unclear to this works. See #1889 (comment)

The ideal way is via views, but we should hold off from offering that, given Views are being removed for 1st stable release.

@cijothomas
Copy link
Member

Closing inactive PRs. This is definitely something we need for metrics, and hoping to bring it in next 1-2 releases.

@cijothomas cijothomas closed this Jul 31, 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.

Implement cardinality limits for metrics streams
4 participants