-
Notifications
You must be signed in to change notification settings - Fork 825
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
feat(sdk-metrics): add aggregation cardinality limit #5128
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5128 +/- ##
==========================================
+ Coverage 93.21% 93.24% +0.02%
==========================================
Files 315 315
Lines 8096 8122 +26
Branches 1622 1632 +10
==========================================
+ Hits 7547 7573 +26
Misses 549 549
|
8fdf9e7
to
21c4168
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for working on this 🙌
Are you also planning to open PRs to follow up on the rest of the cardinality limit specification as well? 🤔
I'd like to time merging this feature in a way that we have the whole spec-feature available at once, so that people can change the limit via the MetricReader
as well. Doing so would also avoid inconsistency with the async storage.
cc6235c
to
6bdc798
Compare
@pichlermarc commited your suggestions and updated PR to add cardinality limiting on async storage. I also added CardinalitySelector to MetricReader to fully implement cardinality limiting. Let me know what you think 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you for adding the other parts as well 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I selected the wrong category when posting the previous review, sorry - there's one more thing (see comment above) then this is good to merge. Thanks for working on this feature. 🙌
8371e25
to
94cec89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good, thank you for implementing this @povilasv.
I'll leave this PR open for a bit to give others a chance to review as well - then I'll merge this next week 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good! Just a nit
// If the cardinality limit is reached, we need to change the attributes | ||
if (this._cumulativeMemoStorage.size >= this._cardinalityLimit) { | ||
attributes = { 'otel.metric.overflow': true }; | ||
hashCode = hashAttributes(attributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think this hash can be a constant value, avoid re-computing.
94cec89
to
3f48f7f
Compare
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
3f48f7f
to
7455bb0
Compare
@legendecas thanks for review, updated the code |
Which problem is this PR solving?
implements Otel Cardinality Limit spec.
This change allows you to:
Link to the spec: https://opentelemetry.io/docs/specs/otel/metrics/sdk/#cardinality-limits
Updated PR to support both async and sync storage
References #4095
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: