Skip to content

feat(sdk-metrics): add aggregation cardinality limit#5128

Merged
pichlermarc merged 5 commits intoopen-telemetry:mainfrom
povilasv:cardinality
Nov 20, 2024
Merged

feat(sdk-metrics): add aggregation cardinality limit#5128
pichlermarc merged 5 commits intoopen-telemetry:mainfrom
povilasv:cardinality

Conversation

@povilasv
Copy link
Copy Markdown
Contributor

@povilasv povilasv commented Nov 8, 2024

Which problem is this PR solving?

implements Otel Cardinality Limit spec.

This change allows you to:

  • configure View for custom aggregation cardinality limit.
  • allows specifying cardinality selector on metric reader
  • also applies default 2000 limit if it's not set.

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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • added unit tests
  • created a small app that uses this package via npm link

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (f1ef596) to head (94cec89).

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              
Files with missing lines Coverage Δ
packages/sdk-metrics/src/export/MetricReader.ts 100.00% <100.00%> (ø)
...ckages/sdk-metrics/src/state/AsyncMetricStorage.ts 100.00% <100.00%> (ø)
...ages/sdk-metrics/src/state/DeltaMetricProcessor.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/state/MeterSharedState.ts 96.66% <100.00%> (+0.05%) ⬆️
packages/sdk-metrics/src/state/MetricCollector.ts 100.00% <100.00%> (ø)
...ackages/sdk-metrics/src/state/SyncMetricStorage.ts 100.00% <100.00%> (ø)
packages/sdk-metrics/src/view/View.ts 100.00% <100.00%> (ø)

@povilasv povilasv force-pushed the cardinality branch 6 times, most recently from 8fdf9e7 to 21c4168 Compare November 8, 2024 14:16
@povilasv povilasv marked this pull request as ready for review November 8, 2024 14:24
@povilasv povilasv requested a review from a team as a code owner November 8, 2024 14:24
Copy link
Copy Markdown
Member

@pichlermarc pichlermarc left a 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.

Comment thread packages/sdk-metrics/src/state/DeltaMetricProcessor.ts Outdated
Comment thread packages/sdk-metrics/src/view/View.ts Outdated
Comment thread packages/sdk-metrics/src/view/View.ts Outdated
Comment thread packages/sdk-metrics/src/view/View.ts Outdated
@povilasv povilasv force-pushed the cardinality branch 3 times, most recently from cc6235c to 6bdc798 Compare November 11, 2024 12:24
@povilasv povilasv requested a review from pichlermarc November 11, 2024 12:30
@povilasv
Copy link
Copy Markdown
Contributor Author

@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
pichlermarc previously approved these changes Nov 12, 2024
Copy link
Copy Markdown
Member

@pichlermarc pichlermarc left a 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 🙌

Comment thread packages/sdk-metrics/src/state/MetricCollector.ts Outdated
@pichlermarc pichlermarc dismissed their stale review November 12, 2024 09:24

selected wrong category

Copy link
Copy Markdown
Member

@pichlermarc pichlermarc left a 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. 🙌

@pichlermarc pichlermarc added spec-feature This is a request to implement a new feature which is already specified by the OTel specification pkg:sdk-metrics labels Nov 12, 2024
@povilasv povilasv requested a review from pichlermarc November 12, 2024 10:05
Copy link
Copy Markdown
Member

@pichlermarc pichlermarc left a 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 🙂

Copy link
Copy Markdown
Member

@legendecas legendecas left a 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);
Copy link
Copy Markdown
Member

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.

@povilasv
Copy link
Copy Markdown
Contributor Author

@legendecas thanks for review, updated the code

@pichlermarc pichlermarc added this pull request to the merge queue Nov 20, 2024
Merged via the queue into open-telemetry:main with commit a834861 Nov 20, 2024
@povilasv povilasv deleted the cardinality branch November 23, 2024 14:17
newkdr added a commit to newkdr/vscode-gitlens that referenced this pull request Jan 1, 2025
![snyk-top-banner](https://redirect.github.com/andygongea/OWASP-Benchmark/assets/818805/c518c423-16fe-447e-b67f-ad5a49b5d123)


<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&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;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&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;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&#x3D;@opentelemetry/resources&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;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":[]}'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:sdk-metrics spec-feature This is a request to implement a new feature which is already specified by the OTel specification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants