[Metrics SDK] Implement configurable cardinality limit by om7057 · Pull Request #4188 · open-telemetry/opentelemetry-cpp

om7057 · 2026-06-28T04:12:19Z

Changes

Implements configurable aggregation cardinality limits at two priority levels
as specified by the OpenTelemetry Metrics SDK specification:

View-level: Cardinality limit defined in a View's AggregationConfig (enforced today)
MetricReader-level: Per-instrument-type limits stored on the reader via CardinalityLimits
struct (parsed and accessible via GetCardinalityLimit(); per-collector enforcement
is a follow-up, see TODO in SetCardinalityLimits())
SDK default: kDefaultCardinalityLimit = 2000 used when neither above is set

Implementation Details

cardinality_limits.h (new):

CardinalityLimits struct with per-instrument-type fields, all defaulting to kDefaultCardinalityLimit
Namespace-level constexpr kDefaultCardinalityLimit = 2000

MetricReader changes:

Added GetCardinalityLimit(InstrumentType) method
Added SetCardinalityLimits(const CardinalityLimits &) method

CollectorHandle / MetricCollector changes:

Added GetCardinalityLimit() virtual method to CollectorHandle (default returns kDefaultCardinalityLimit)
MetricCollector delegates to its underlying MetricReader

Configuration changes:

sdk_builder.cc: Wires cardinality_limits from YAML config via ToCardinalityLimits() helper
cardinality_limits_configuration.h: Initialized all fields (default_limit = 2000, others = 0)
Removed "cardinality limits not supported" warnings

Checklist

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

om7057 · 2026-06-28T04:15:36Z

Hi @dbarker
I've implemented configurable cardinality limits for the Metrics SDK to resolve issue #3292 and bring the C++ implementation into compliance with the OpenTelemetry specification.

Summary of Changes:

Added three-level cardinality limit configuration (View > Reader > SDK default)
Wired existing YAML config infrastructure to actually apply the limits
Ensured proper lifetime management for reader-level configs

This is my first contribution to OpenTelemetry C++. I'd appreciate guidance on:

Whether the approach aligns with the project's design principles
If additional tests beyond the existing YAML parsing tests are needed
If I should update the CHANGELOG.md

Looking forward to your feedback!

cc: @lalitb

marcalff

Thanks for the contribution.

There is something wrong the way clang-format was applied, causing spurious changes in many files.

This:

is most likely incorrect, and will fail CI on clang-format
makes it difficult to review the real changes from this patch.

When formatting code, a precise version of clang-format must be used. Applying a different version will cause this. The best is to use the dev container to perform formatting, as it will use the exact same version as the github CI.

In any cases, adding a file in a commit that was not changed voluntarily (i.e., with a real fix) is a red flag, and should not happen.

Please rework the patch to remove the noise, so it can be reviewed.

om7057 · 2026-06-30T03:06:17Z

@marcalff Thank you for the feedback! I've reworked the PR to remove all spurious formatting changes.

Current state
Files modified:

metric_reader.h

Added CardinalityLimitOptions
metric_reader.cc
Implemented getter/setter
metric_collector.h
Added interface method
metric_collector.cc
Implemented delegation
meter_context.h
Added reader query method
meter_context.cc
Implemented max-across-readers logic
sync_metric_storage.h
Added lifetime management
async_metric_storage.h
Added lifetime management
meter.cc
Applied reader-level fallback
sdk_builder.cc
Wired YAML config to implementation

Regarding CI: the only failing check is W3C Distributed Tracing Validation V1 (test_tracestate_key_illegal_vendor_format and test_tracestate_key_length_limit). This failure is unrelated to this PR — the changes here are entirely in the Metrics SDK and do not touch any trace context code. The W3C V1 test suite checks out the latest [w3c/trace-context]HEAD at runtime, and these two tests appear to be failing intermittently across PRs.

The PR is now clean and ready for review. Please let me know if you need any clarifications on the implementation approach!

codecov · 2026-06-30T03:57:43Z

Codecov Report

❌ Patch coverage is 44.64286% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.14%. Comparing base (cf62199) to head (7f8dc73).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/src/configuration/sdk_builder.cc	9.38%	29 Missing ⚠️
sdk/src/metrics/metric_reader.cc	90.91%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4188      +/-   ##
==========================================
- Coverage   81.25%   81.14%   -0.10%     
==========================================
  Files         446      446              
  Lines       18872    18922      +50     
==========================================
+ Hits        15332    15353      +21     
- Misses       3540     3569      +29

Files with missing lines	Coverage Δ
.../include/opentelemetry/sdk/metrics/metric_reader.h	`100.00% <ø> (ø)`
...opentelemetry/sdk/metrics/state/metric_collector.h	`100.00% <ø> (ø)`
sdk/src/metrics/state/metric_collector.cc	`96.78% <100.00%> (+0.11%)`	⬆️
sdk/src/metrics/metric_reader.cc	`85.46% <90.91%> (+3.64%)`	⬆️
sdk/src/configuration/sdk_builder.cc	`48.31% <9.38%> (-1.21%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lalitb · 2026-06-30T15:05:21Z

+            auto ctx_ptr = meter_context_.lock();
+            if (ctx_ptr)
+            {
+              size_t reader_limit = ctx_ptr->GetReaderCardinalityLimit(instrument_descriptor.type_);


I think this breaks per-reader semantics. Example: reader A has limit 10, reader B has limit 1000. The storage uses 1000, so reader A can export far more series than configured. This is also a heap/memory regression for low-limit readers.

One possible fix is to avoid resolving reader-level limits into a single storage-level config. The view-level limit can stay on the shared stream/storage, but the MetricReader fallback should be applied per collector/reader, since each reader may have a different configured limit. So instead of taking the max across readers, the collection path likely needs to use the current CollectorHandle's GetCardinalityLimit(instrument_type) when no view-level limit exists.

Good catch @lalitb, thanks! You're right, resolving the reader-level fallback into shared storage breaks the per-reader semantics you described.

I've removed that logic here and reverted the storage constructors. For now, this PR only enforces view-level aggregation_cardinality_limit (plus the existing SDK default of 2000). Reader-level limits are still parsed, but not enforced.

I agree the right place to apply the MetricReader fallback is in the collection path on a per-CollectorHandle basis when no view-level limit exists. Since that's a more involved change, I'd rather tackle it in a follow-up PR.

Also fixed the IWYU warning in the latest commit.

dbarker

Thanks for the contribution. Please see some minor feedback and questions below.

dbarker

Thanks for the PR! Approved with some feedback below.

om7057 · 2026-07-14T03:24:12Z

Thanks for the contribution.

There is something wrong the way clang-format was applied, causing spurious changes in many files.

This:

is most likely incorrect, and will fail CI on clang-format

makes it difficult to review the real changes from this patch.

When formatting code, a precise version of clang-format must be used. Applying a different version will cause this. The best is to use the dev container to perform formatting, as it will use the exact same version as the github CI.

In any cases, adding a file in a commit that was not changed voluntarily (i.e., with a real fix) is a red flag, and should not happen.

Please rework the patch to remove the noise, so it can be reviewed.

@marcalff please approve the PR if all the relevant changes are being addressed according to you.

om7057 · 2026-07-14T03:59:36Z

Thanks for the PR! Approved with some feedback below.

Thank you for the approval.
Is the PR ready for merge?

dbarker · 2026-07-15T17:53:17Z

Is the PR ready for merge?

The open change request might need to be cleared first.

@ThomsonTan and @lalitb please also take a look since you have open comments.

om7057 · 2026-07-16T08:53:33Z

Is the PR ready for merge?

The open change request might need to be cleared first.

@ThomsonTan and @lalitb please also take a look since you have open comments.

I've resolved the open change request, @dbarker.
So I request @ThomsonTan, @lalitb & @marcalff to go through the PR once and approve if all their changes and comments have been addressed.

ThomsonTan · 2026-07-16T15:25:40Z

@om7057 thanks for the update. Could you please also resolve the merge conflict?

om7057 · 2026-07-16T16:32:39Z

@om7057 thanks for the update. Could you please also resolve the merge conflict?

@ThomsonTan Sure, I've resolved it. Please check and approve if it looks good.

Fixes open-telemetry#3292 Implements configurable aggregation cardinality limits at view-level and reader-level as specified by the OpenTelemetry Metrics SDK spec. Changes: - cardinality_limits.h: New header with CardinalityLimits struct and constexpr kDefaultCardinalityLimit = 2000 - metric_reader.h: Include cardinality_limits.h; add GetCardinalityLimit() and SetCardinalityLimits() methods - metric_reader.cc: Implement per-instrument-type limit lookup (plain setter, no sentinel logic) - metric_collector.h/.cc: GetCardinalityLimit() in CollectorHandle defaults to CardinalityLimits::kDefaultCardinalityLimit; MetricCollector delegates to underlying MetricReader - meter_context.h/.cc: Remove GetReaderCardinalityLimit() — per-reader limits are accessed via GetCollectors() directly - sdk_builder.cc: Add ToCardinalityLimits() helper in anonymous namespace to convert CardinalityLimitsConfiguration (0 = unset) to CardinalityLimits; used by both periodic and pull reader creation paths - cardinality_limits_configuration.h: Initialize default_limit to 2000, per-instrument fields to 0 (unset sentinel); eliminates UB - CHANGELOG.md: Document the new feature - metric_reader_test.cc, metric_collector_test.cc: Unit tests; collector tests use GetCollectors() instead of removed GetReaderCardinalityLimit(); added ReaderCardinalityLimitEnforcedOnCollection integration test - sdk/test/metrics/BUILD: Add //sdk/src/metrics dep to all_tests

format issues resolved.

marcalff

LGTM, see minor comments.

marcalff · 2026-07-16T23:31:02Z

+  virtual std::size_t GetCardinalityLimit(InstrumentType instrument_type) noexcept
+  {
+    (void)instrument_type;
+    return kDefaultCardinalityLimit;
+  }


Could this be a pure virtual method instead ?

Also, make it const ?

Sure, I believe both can be implemented.
GetCardinalityLimit is now a pure virtual const method on CollectorHandle. MetricCollector overrides it as const noexcept override, and MockCollectorHandle in common.h provides the implementation returning kDefaultCardinalityLimit. The default non-virtual body is gone.
Please review the changes @marcalff and if you feel PR is ready to merge from your end, I'll appreciate the approval.

Fixes open-telemetry#3292 Implements configurable aggregation cardinality limits at view-level and reader-level as specified by the OpenTelemetry Metrics SDK spec. Changes: - cardinality_limits.h: New header with CardinalityLimits struct and constexpr kDefaultCardinalityLimit = 2000 - metric_reader.h: Include cardinality_limits.h; add GetCardinalityLimit() and SetCardinalityLimits() methods - metric_reader.cc: Implement per-instrument-type limit lookup (plain setter, no sentinel logic) - metric_collector.h/.cc: GetCardinalityLimit() in CollectorHandle defaults to CardinalityLimits::kDefaultCardinalityLimit; MetricCollector delegates to underlying MetricReader - meter_context.h/.cc: Remove GetReaderCardinalityLimit() — per-reader limits are accessed via GetCollectors() directly - sdk_builder.cc: Add ToCardinalityLimits() helper in anonymous namespace to convert CardinalityLimitsConfiguration (0 = unset) to CardinalityLimits; used by both periodic and pull reader creation paths - cardinality_limits_configuration.h: Initialize default_limit to 2000, per-instrument fields to 0 (unset sentinel); eliminates UB - CHANGELOG.md: Document the new feature - metric_reader_test.cc, metric_collector_test.cc: Unit tests; collector tests use GetCollectors() instead of removed GetReaderCardinalityLimit(); added ReaderCardinalityLimitEnforcedOnCollection integration test - sdk/test/metrics/BUILD: Add //sdk/src/metrics dep to all_tests

Add <cstddef> and cardinality_limits.h directly to common.h since MockCollectorHandle::GetCardinalityLimit uses std::size_t and kDefaultCardinalityLimit directly.

om7057 · 2026-07-23T02:10:53Z

Hi @marcalff @lalitb @dbarker, all the CI checks have been passed, and the PR is approved as well. If you feel it's ready, please consider merging.
Thank you.

linux-foundation-easycla · 2026-07-25T06:37:04Z

The committers listed above are authorized under a signed CLA.

✅ login: dbarker / name: Doug Barker (ace38fc)
✅ login: marcalff / name: Marc Alff (099f8c1, 43dbe46, a74a859)
✅ login: om7057 / name: Om Kulkarni (456b92e, c1fa36e)
✅ login: om7057 / name: om7057 (3c5a4ee, 4822e27, 7f8dc73, fd95d28)

The merge of main's registry-based PeriodicMetricReaderBuilder (open-telemetry#4277) left CreatePeriodicMetricReader() with unreachable dead code and a reference to the now-removed PeriodicExportingMetricReaderFactory direct-construction path, breaking the build across all CI jobs. Restore the registry builder flow and apply cardinality-limit enforcement to the reader it produces.

om7057 requested a review from a team as a code owner June 28, 2026 04:12

om7057 mentioned this pull request Jun 28, 2026

[Metrics SDK] Make cardinality limit configurable #3292

Open

marcalff previously requested changes Jun 29, 2026

View reviewed changes

om7057 force-pushed the feature/configurable-cardinality-limit branch 4 times, most recently from a385928 to 7758926 Compare June 30, 2026 02:51

om7057 force-pushed the feature/configurable-cardinality-limit branch 4 times, most recently from 1f771a3 to 8d2967e Compare June 30, 2026 06:02

lalitb reviewed Jun 30, 2026

View reviewed changes

om7057 force-pushed the feature/configurable-cardinality-limit branch from 2895f04 to 126fba1 Compare July 8, 2026 02:46

dbarker reviewed Jul 10, 2026

View reviewed changes

om7057 force-pushed the feature/configurable-cardinality-limit branch 4 times, most recently from 3881f4c to 26d3cb3 Compare July 10, 2026 14:36

marcalff self-requested a review July 10, 2026 14:52

ThomsonTan reviewed Jul 10, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

om7057 force-pushed the feature/configurable-cardinality-limit branch 6 times, most recently from 22f8fb7 to b7f7bcb Compare July 12, 2026 06:27

dbarker approved these changes Jul 13, 2026

View reviewed changes

Comment thread sdk/include/opentelemetry/sdk/metrics/metric_reader.h

Comment thread sdk/src/metrics/metric_reader.cc

Comment thread sdk/test/metrics/metric_collector_test.cc Outdated

om7057 force-pushed the feature/configurable-cardinality-limit branch 3 times, most recently from 4ed7073 to 9fc411c Compare July 14, 2026 02:52

om7057 force-pushed the feature/configurable-cardinality-limit branch from 9fc411c to 2924ea0 Compare July 16, 2026 08:48

om7057 force-pushed the feature/configurable-cardinality-limit branch from 2924ea0 to bc5708a Compare July 16, 2026 16:27

om7057 force-pushed the feature/configurable-cardinality-limit branch from bc5708a to fd95d28 Compare July 16, 2026 16:52

Merge branch 'main' into feature/configurable-cardinality-limit

43dbe46

marcalff reviewed Jul 16, 2026

View reviewed changes

om7057 force-pushed the feature/configurable-cardinality-limit branch from 800ba57 to 4822e27 Compare July 17, 2026 14:47

om7057 and others added 2 commits July 22, 2026 00:50

Merge branch 'main' into feature/configurable-cardinality-limit

456b92e

fix(metrics): add missing IWYU includes to common.h

3c5a4ee

Add <cstddef> and cardinality_limits.h directly to common.h since MockCollectorHandle::GetCardinalityLimit uses std::size_t and kDefaultCardinalityLimit directly.

om7057 force-pushed the feature/configurable-cardinality-limit branch from 140fafa to 3c5a4ee Compare July 22, 2026 18:28

dbarker and others added 4 commits July 23, 2026 17:45

Merge branch 'main' into feature/configurable-cardinality-limit

ace38fc

Merge branch 'main' into feature/configurable-cardinality-limit

099f8c1

Merge branch 'main' into feature/configurable-cardinality-limit

a74a859

Merge branch 'main' into feature/configurable-cardinality-limit

c1fa36e

om7057 force-pushed the feature/configurable-cardinality-limit branch from 3192821 to 7f8dc73 Compare July 25, 2026 06:40

Uh oh!

Conversation

om7057 commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Implementation Details

Checklist

Uh oh!

om7057 commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

om7057 commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

lalitb Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

om7057 Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

dbarker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dbarker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

om7057 commented Jul 14, 2026

Uh oh!

om7057 commented Jul 14, 2026

Uh oh!

dbarker commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

om7057 commented Jul 16, 2026

Uh oh!

ThomsonTan commented Jul 16, 2026

Uh oh!

om7057 commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcalff left a comment

Choose a reason for hiding this comment

Uh oh!

marcalff Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

om7057 Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

om7057 commented Jul 23, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

om7057 commented Jun 28, 2026 •

edited

Loading

om7057 commented Jun 28, 2026 •

edited

Loading

om7057 commented Jun 30, 2026 •

edited

Loading

codecov Bot commented Jun 30, 2026 •

edited

Loading

dbarker commented Jul 15, 2026 •

edited

Loading

om7057 commented Jul 16, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Jul 25, 2026 •

edited

Loading