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

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

Changes

This PR implements configurable aggregation cardinality limits at three priority levels as specified by the OpenTelemetry Metrics SDK specification:

View-level: Cardinality limit defined in a View's AggregationConfig (highest priority)
MetricReader-level: Default limits per instrument type in the reader (fallback)
SDK default: Fallback to 2000 if neither above is defined (final fallback)

Implementation Details

MetricReader Changes:

Added CardinalityLimitOptions struct with per-instrument-type limits
Added GetCardinalityLimit(InstrumentType) method
Added SetCardinalityLimitOptions() method

MetricCollector Changes:

Added GetCardinalityLimit() to CollectorHandle interface
Implemented delegation to underlying MetricReader

MeterContext Changes:

Added GetReaderCardinalityLimit() method
Returns maximum limit across all configured readers (prevents data loss)

Storage Classes Changes:

Updated SyncMetricStorage and AsyncMetricStorage constructors
Added optional owned_aggregation_config parameter for lifetime management
Ensures reader-level configs remain valid for storage lifetime

Meter Changes:

Modified RegisterSyncMetricStorage() and RegisterAsyncMetricStorage()
Apply reader-level fallback when view doesn't provide config
Creates temporary AggregationConfig with reader limit when needed

Configuration Changes:

Wire view-level aggregation_cardinality_limit from YAML config
Wire reader-level cardinality_limits from YAML config
Removed "cardinality limits not supported" warnings

Priority Order

The implementation follows the specification's priority order:

View config is checked first (when view is created)
Reader config is used as fallback (when storage is created)
SDK default (2000) is the final fallback (via AggregationConfig::GetOrDefault())

Testing

The existing YAML parsing tests already verify that configuration is parsed correctly. Additional integration tests are recommended to verify:

View-level limits override reader-level limits
Reader-level limits are used when no view is configured
SDK default is used when neither is configured
Per-instrument-type reader limits work correctly

For significant contributions please make sure you have completed the following items:

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

Related Issues

Fixes #3292

Note: This is a spec-compliance feature that enables users to configure cardinality limits at multiple levels, preventing memory exhaustion in high-cardinality scenarios while maintaining flexibility.

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 90.69767% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.92%. Comparing base (a56f0e2) to head (2895f04).

Files with missing lines	Patch %	Lines
...opentelemetry/sdk/metrics/state/metric_collector.h	0.00%	2 Missing ⚠️
sdk/src/metrics/metric_reader.cc	93.75%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4188      +/-   ##
==========================================
+ Coverage   82.89%   82.92%   +0.04%     
==========================================
  Files         405      405              
  Lines       17292    17335      +43     
==========================================
+ Hits        14332    14373      +41     
- Misses       2960     2962       +2

Files with missing lines	Coverage Δ
.../include/opentelemetry/sdk/metrics/metric_reader.h	`66.67% <ø> (ø)`
sdk/src/metrics/meter.cc	`81.36% <ø> (ø)`
sdk/src/metrics/meter_context.cc	`87.78% <100.00%> (+1.04%)`	⬆️
sdk/src/metrics/state/metric_collector.cc	`96.78% <100.00%> (+0.11%)`	⬆️
...opentelemetry/sdk/metrics/state/metric_collector.h	`60.00% <0.00%> (-40.00%)`	⬇️
sdk/src/metrics/metric_reader.cc	`87.70% <93.75%> (+5.88%)`	⬆️

... 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.

Add comprehensive unit tests to cover the cardinality limit implementation: metric_reader_test.cc: - CardinalityLimitOptions: Tests setting and getting all instrument-specific limits - CardinalityLimitOptionsDefaultFallback: Tests fallback to default_limit when specific limits are 0 - CardinalityLimitOptionsMixedConfiguration: Tests mixed configuration scenarios metric_collector_test.cc: - CardinalityLimitDelegation: Tests MetricCollector delegates to MetricReader - MeterContextMaxCardinalityLimit: Tests MeterContext returns max limit across readers - MeterContextCardinalityLimitWithMultipleReaders: Tests complex multi-reader scenarios These tests cover the code paths in: - sdk/src/metrics/metric_reader.cc (GetCardinalityLimit switch statement) - sdk/src/metrics/state/metric_collector.cc (delegation) - sdk/src/metrics/meter_context.cc (max-across-readers logic) - sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h (interface) Addresses Codecov coverage gaps for PR open-telemetry#4188.

Fixes open-telemetry#3292 Implements configurable aggregation cardinality limits at three priority levels as specified by the OpenTelemetry Metrics SDK specification: - View-level: cardinality limit in AggregationConfig (highest priority) - MetricReader-level: per-instrument-type limits via CardinalityLimitOptions - SDK default: fallback to 2000 when neither above is defined Changes: - metric_reader.h: Add CardinalityLimitOptions struct, GetCardinalityLimit() and SetCardinalityLimitOptions() methods - metric_reader.cc: Implement per-instrument-type limit lookup with fallback to default_limit - metric_collector.h/.cc: Add GetCardinalityLimit() to CollectorHandle interface; delegate to underlying MetricReader in MetricCollector - meter_context.h/.cc: Add GetReaderCardinalityLimit() returning max limit across all configured readers - sdk_builder.cc: Wire reader-level cardinality_limits and view-level aggregation_cardinality_limit from YAML config; remove 'not supported' warnings - CHANGELOG.md: Document the new feature - metric_reader_test.cc, metric_collector_test.cc: Add unit tests

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Metrics SDK] Implement configurable cardinality limit#4188

[Metrics SDK] Implement configurable cardinality limit#4188
om7057 wants to merge 1 commit into
open-telemetry:mainfrom
om7057:feature/configurable-cardinality-limit

om7057 commented Jun 28, 2026 •

edited

Loading

Uh oh!

om7057 commented Jun 28, 2026 •

edited

Loading

Uh oh!

marcalff left a comment

Uh oh!

om7057 commented Jun 30, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 30, 2026 •

edited

Loading

Uh oh!

lalitb Jun 30, 2026

Uh oh!

lalitb Jun 30, 2026

Uh oh!

om7057 Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Priority Order

Testing

Related Issues

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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