feat(sdk-metrics): add aggregation cardinality limit#5128
feat(sdk-metrics): add aggregation cardinality limit#5128pichlermarc merged 5 commits intoopen-telemetry:mainfrom
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
pichlermarc
left a comment
There was a problem hiding this comment.
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 🙇 |
pichlermarc
left a comment
There was a problem hiding this comment.
Looks great, thank you for adding the other parts as well 🙌
pichlermarc
left a comment
There was a problem hiding this comment.
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
pichlermarc
left a comment
There was a problem hiding this comment.
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 🙂
legendecas
left a comment
There was a problem hiding this comment.
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.
Nit: I think this hash can be a constant value, avoid re-computing.
94cec89 to
3f48f7f
Compare
Co-authored-by: Marc Pichler <[email protected]>
Co-authored-by: Marc Pichler <[email protected]>
3f48f7f to
7455bb0
Compare
|
@legendecas thanks for review, updated the code |
 <h3>Snyk has created this PR to upgrade @opentelemetry/resources from 1.28.0 to 1.29.0.</h3> :information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project. <hr/> - The recommended version is **1 version** ahead of your current version. - The recommended version was released **a month ago**. <details> <summary><b>Release notes</b></summary> <br/> <details> <summary>Package name: <b>@opentelemetry/resources</b></summary> <ul> <li> <b>1.29.0</b> - <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/releases/tag/v1.29.0">2024-12-04</a></br><h2>1.29.0</h2> <h3>🚀 (Enhancement)</h3> <ul> <li>feat(sdk-metrics): Add support for aggregation cardinality limit with a default limit of 2000. This limit can be customized via views <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/pull/5128" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/5128/hovercard">gitkraken#5128</a></li> </ul> </li> <li> <b>1.28.0</b> - <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/releases/tag/semconv%2Fv1.28.0">2024-11-18</a></br><h2>1.28.0</h2> <h3>🚀 (Enhancement)</h3> <ul> <li>feat: update semantic conventions to 1.28.0 <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/pull/5181" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/5181/hovercard">gitkraken#5181</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://redirect.github.com/trentm">@ trentm</a></li> </ul> <h3>📚 (Refine Doc)</h3> <ul> <li>chore: Improve documentation on entry-points (top-level and "incubating") and on deprecations. <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/issues/5025" data-hovercard-type="issue" data-hovercard-url="/open-telemetry/opentelemetry-js/issues/5025/hovercard">gitkraken#5025</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://redirect.github.com/trentm">@ trentm</a></li> </ul> <h3>🏠 (Internal)</h3> <ul> <li>chore: Update the comments of some deprecated constants to point to the currently relevant replacement constant, if any. <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/pull/5160" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/5160/hovercard">gitkraken#5160</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://redirect.github.com/trentm">@ trentm</a></li> <li>chore: Minor improvements to formatting of comments. <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/pull/5100" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/5100/hovercard">gitkraken#5100</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://redirect.github.com/trentm">@ trentm</a></li> </ul> </li> </ul> from <a href="https://redirect.github.com/open-telemetry/opentelemetry-js/releases">@opentelemetry/resources GitHub release notes</a> </details> </details> --- > [!IMPORTANT] > > - Check the changes in this PR to ensure they won't cause issues with your project. > - This PR was automatically created by Snyk using the credentials of a real user. --- **Note:** _You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs._ **For more information:** <img src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiI1MDBhZWI5ZC1jZGQ2LTQ5Y2QtOTEzOC1iZGM4YTUwZGNjYjkiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjUwMGFlYjlkLWNkZDYtNDljZC05MTM4LWJkYzhhNTBkY2NiOSJ9fQ==" width="0" height="0"/> > - 🧐 [View latest project report](https://app.snyk.io/org/newkdr/project/12a8a5f5-3e19-438c-8280-eb8f4ee06d17?utm_source=github&utm_medium=referral&page=upgrade-pr) > - 📜 [Customise PR templates](https://docs.snyk.io/scan-using-snyk/pull-requests/snyk-fix-pull-or-merge-requests/customize-pr-templates?utm_source=&utm_content=fix-pr-template) > - 🛠 [Adjust upgrade PR settings](https://app.snyk.io/org/newkdr/project/12a8a5f5-3e19-438c-8280-eb8f4ee06d17/settings/integration?utm_source=github&utm_medium=referral&page=upgrade-pr) > - 🔕 [Ignore this dependency or unsubscribe from future upgrade PRs](https://app.snyk.io/org/newkdr/project/12a8a5f5-3e19-438c-8280-eb8f4ee06d17/settings/integration?pkg=@opentelemetry/resources&utm_source=github&utm_medium=referral&page=upgrade-pr#auto-dep-upgrades) [//]: # 'snyk:metadata:{"customTemplate":{"variablesUsed":[],"fieldsUsed":[]},"dependencies":[{"name":"@opentelemetry/resources","from":"1.28.0","to":"1.29.0"}],"env":"prod","hasFixes":false,"isBreakingChange":false,"isMajorUpgrade":false,"issuesToFix":[],"prId":"500aeb9d-cdd6-49cd-9138-bdc8a50dccb9","prPublicId":"500aeb9d-cdd6-49cd-9138-bdc8a50dccb9","packageManager":"npm","priorityScoreList":[],"projectPublicId":"12a8a5f5-3e19-438c-8280-eb8f4ee06d17","projectUrl":"https://app.snyk.io/org/newkdr/project/12a8a5f5-3e19-438c-8280-eb8f4ee06d17?utm_source=github&utm_medium=referral&page=upgrade-pr","prType":"upgrade","templateFieldSources":{"branchName":"default","commitMessage":"default","description":"default","title":"default"},"templateVariants":[],"type":"auto","upgrade":[],"upgradeInfo":{"versionsDiff":1,"publishedDate":"2024-12-04T16:32:24.723Z"},"vulns":[]}'
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: