[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 45.83333% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.06%. Comparing base (ed9286c) to head (2924ea0).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4188      +/-   ##
==========================================
- Coverage   78.13%   78.06%   -0.07%     
==========================================
  Files         440      440              
  Lines       18727    18771      +44     
==========================================
+ Hits        14631    14651      +20     
- Misses       4096     4120      +24

Files with missing lines	Coverage Δ
.../include/opentelemetry/sdk/metrics/metric_reader.h	`66.67% <ø> (ø)`
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	`85.46% <90.91%> (+3.64%)`	⬆️
sdk/src/configuration/sdk_builder.cc	`4.91% <0.00%> (-0.09%)`	⬇️

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

dbarker

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

dbarker

Thanks for the updates. Please see feedback below. Approval to follow.

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.

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

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.

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

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 reviewed Jul 12, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

om7057 force-pushed the feature/configurable-cardinality-limit branch 5 times, most recently from f101219 to 5a9978c Compare July 13, 2026 19:11

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 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

dbarker commented Jul 15, 2026 •

edited

Loading