sdk/metric: apply default cardinality limit of 2000#8247
sdk/metric: apply default cardinality limit of 2000#8247pellared wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8247 +/- ##
=====================================
Coverage 82.6% 82.6%
=====================================
Files 309 309
Lines 24621 24621
=====================================
Hits 20356 20356
- Misses 3888 3889 +1
+ Partials 377 376 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Applies the Metrics SDK specification-recommended default cardinality limit (2000) in go.opentelemetry.io/otel/sdk/metric, updating tests, docs, and changelog to reflect the new default and overflow behavior.
Changes:
- Set the SDK default cardinality limit to 2000 (previously unlimited by default).
- Update cardinality-limit tests to validate overflow aggregation behavior.
- Refresh package docs, examples, and changelog to reflect the new default and migration guidance.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/metric/config.go | Changes the default cardinality limit constant to 2000 and updates option docs. |
| sdk/metric/provider_test.go | Adjusts cardinality limit tests for the new default and checks overflow datapoints. |
| sdk/metric/config_test.go | Extends env-var parsing tests (including 0 to disable the limit). |
| sdk/metric/doc.go | Updates package documentation to state the new default and describe overflow behavior. |
| sdk/metric/example_test.go | Removes redundant explicit WithCardinalityLimit(2000) since it’s now default. |
| CHANGELOG.md | Documents the default change and how to preserve unlimited cardinality. |
|
I remember us making an explicit decision not to change that, as it was a breaking change. |
dashpole
left a comment
There was a problem hiding this comment.
I was initially opposed to doing this, but its becoming more of a wart with all of the CVE reports and with users expecting consistency with declarative config. I think as long as we keep the env var around for a while this is the best path forward.
Per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#cardinality-limits: