From b93d1f31833ceec8b412daf9487b0bc620ee4df1 Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 10 Aug 2026 09:12:54 +0530 Subject: [PATCH 1/7] [Metrics SDK] Enforce MetricReader-level cardinality limits as a fallback during collection Fixes #4387 Follow-up to #4188 and #4314. Reader-level cardinality limits were parsed and stored on MetricReader but never enforced. The initial attempt to enforce them in #4188 by resolving the reader's limit directly into shared per-view storage was reverted during review since it broke per-reader semantics: with two readers sharing one instrument's storage, a stricter reader would let the laxer reader lose data, or a laxer reader would force every reader down to its limit. This enforces limits at both ends instead: - Shared recording storage (SyncMetricStorage/AsyncMetricStorage) is now sized at the max cardinality limit across all attached readers when the view has no explicit limit, so no reader loses data. - Each reader's own (possibly stricter) limit is re-applied to just its own output during collection (TemporalMetricStorage:: buildMetrics), via AttributesHashMap's existing overflow mechanism. View-level limits still take precedence over reader-level ones, unchanged, per the View > Reader > SDK default spec precedence. --- CHANGELOG.md | 9 ++ .../opentelemetry/sdk/metrics/metric_reader.h | 5 +- .../sdk/metrics/state/async_metric_storage.h | 34 ++++++-- .../sdk/metrics/state/sync_metric_storage.h | 36 +++++++- sdk/src/metrics/meter.cc | 40 ++++++++- sdk/src/metrics/metric_reader.cc | 6 -- sdk/src/metrics/state/sync_metric_storage.cc | 2 +- .../metrics/state/temporal_metric_storage.cc | 14 ++- sdk/test/metrics/metric_collector_test.cc | 86 +++++++++++++++++++ 9 files changed, 206 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f1d54a64a..f3a72b09ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,15 @@ Increment the: ## [Unreleased] +* [METRICS SDK] Enforce `MetricReader`-level cardinality limits as a fallback + during collection, when the matching view has no explicit + `aggregation_cardinality_limit` of its own (View > Reader > SDK default). + Shared recording storage for a view with no explicit limit is now sized at + the max limit across all attached readers, so a reader with a higher limit + does not lose data; each reader's own (possibly stricter) limit is then + re-applied to just its own collected output. + [#4387](https://github.com/open-telemetry/opentelemetry-cpp/issues/4387) + * [CONFIGURATION] Add support for the composite sampler configuration (programmatic and from yaml) ([#4366](https://github.com/open-telemetry/opentelemetry-cpp/pull/4366)) diff --git a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h index 4813861314..eec71eb468 100644 --- a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h @@ -61,9 +61,8 @@ class MetricReader /** * Set per-instrument-type cardinality limits for this reader. * - * TODO: Reader-level limits are stored but not yet enforced as a per-collector - * fallback during the collection path. Enforcement will be added in a follow-up. - * View-level limits (via AggregationConfig) are enforced today. + * Enforced as a fallback during the collection path whenever the matching view has no + * explicit AggregationConfig cardinality limit of its own (View > Reader > SDK default). * * @param limits The cardinality limits to apply */ diff --git a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h index a3f56ef64d..e538b8f3f4 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -34,6 +34,8 @@ namespace metrics class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStorage { public: + // Back-compat overload preserving the original constructor signature for any external caller. + // See SyncMetricStorage's constructor comment for what `recording_cardinality_limit` is for. AsyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, const AggregationType aggregation_type, #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW @@ -41,13 +43,30 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config) + : AsyncMetricStorage(instrument_descriptor, + aggregation_type, +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + exempler_filter_type, + std::move(exemplar_reservoir), +#endif + aggregation_config, + AggregationConfig::GetOrDefault(aggregation_config)->cardinality_limit_) + {} + + AsyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, + const AggregationType aggregation_type, +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + ExemplarFilterType exempler_filter_type, + nostd::shared_ptr &&exemplar_reservoir, +#endif + const AggregationConfig *aggregation_config, + std::size_t recording_cardinality_limit) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, aggregation_config_{AggregationConfig::GetOrDefault(aggregation_config)}, - cumulative_hash_map_( - std::make_unique(aggregation_config_->cardinality_limit_)), - delta_hash_map_( - std::make_unique(aggregation_config_->cardinality_limit_)), + recording_cardinality_limit_(recording_cardinality_limit), + cumulative_hash_map_(std::make_unique(recording_cardinality_limit_)), + delta_hash_map_(std::make_unique(recording_cardinality_limit_)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), exemplar_reservoir_(std::move(exemplar_reservoir)), @@ -127,9 +146,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora std::shared_ptr delta_metrics = nullptr; { std::lock_guard guard(hashmap_lock_); - delta_metrics = std::move(delta_hash_map_); - delta_hash_map_ = - std::make_unique(aggregation_config_->cardinality_limit_); + delta_metrics = std::move(delta_hash_map_); + delta_hash_map_ = std::make_unique(recording_cardinality_limit_); } auto status = @@ -142,6 +160,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora InstrumentDescriptor instrument_descriptor_; AggregationType aggregation_type_; const AggregationConfig *aggregation_config_; + // Capacity used to (re)size cumulative_hash_map_/delta_hash_map_. See the constructor comment. + const std::size_t recording_cardinality_limit_; std::unique_ptr cumulative_hash_map_; std::unique_ptr delta_hash_map_; opentelemetry::common::SpinLockMutex hashmap_lock_; diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h index be94f926e2..7ef30674bc 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -62,6 +62,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif // ENABLE_METRICS_EXEMPLAR_PREVIEW public: + // Back-compat overload preserving the original constructor signature for any external caller. + // Sizes recording storage using the view's own cardinality limit (or the SDK default when no + // AggregationConfig is supplied), matching this class's original behavior exactly. SyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, const AggregationType aggregation_type, std::shared_ptr attributes_processor, @@ -70,10 +73,37 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config) + : SyncMetricStorage(instrument_descriptor, + aggregation_type, + std::move(attributes_processor), +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + exempler_filter_type, + std::move(exemplar_reservoir), +#endif + aggregation_config, + AggregationConfig::GetOrDefault(aggregation_config)->cardinality_limit_) + {} + + // `recording_cardinality_limit` sizes the storage that raw measurements are recorded into, + // separately from `aggregation_config`. When a view sets an explicit cardinality limit, + // callers should pass that same limit (the overload above does this). When a view has no + // explicit limit, callers may pass the max cardinality limit configured across all attached + // MetricReaders, so no reader loses data purely because the shared recording storage was + // capped too low for it; each reader's own (possibly stricter) limit is then re-applied to + // its own output during collection. See TemporalMetricStorage::buildMetrics(). + SyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, + const AggregationType aggregation_type, + std::shared_ptr attributes_processor, +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + ExemplarFilterType exempler_filter_type, + nostd::shared_ptr &&exemplar_reservoir, +#endif + const AggregationConfig *aggregation_config, + std::size_t recording_cardinality_limit) : instrument_descriptor_(instrument_descriptor), aggregation_config_(AggregationConfig::GetOrDefault(aggregation_config)), - attributes_hashmap_( - std::make_unique(aggregation_config_->cardinality_limit_)), + recording_cardinality_limit_(recording_cardinality_limit), + attributes_hashmap_(std::make_unique(recording_cardinality_limit_)), attributes_processor_(std::move(attributes_processor)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), @@ -292,6 +322,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage InstrumentDescriptor instrument_descriptor_; // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) const AggregationConfig *aggregation_config_; + // Capacity used to (re)size attributes_hashmap_. See the constructor comment above. + const std::size_t recording_cardinality_limit_; std::unique_ptr attributes_hashmap_; std::function()> create_default_aggregation_; std::shared_ptr attributes_processor_; diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index 3daf0b4017..f92eb488fe 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -21,6 +21,7 @@ #include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" #include "opentelemetry/sdk/instrumentationscope/scope_configurator.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/async_instruments.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/instruments.h" @@ -28,6 +29,7 @@ #include "opentelemetry/sdk/metrics/meter_config.h" #include "opentelemetry/sdk/metrics/meter_context.h" #include "opentelemetry/sdk/metrics/state/async_metric_storage.h" +#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" #include "opentelemetry/sdk/metrics/state/metric_storage.h" #include "opentelemetry/sdk/metrics/state/multi_metric_storage.h" @@ -83,6 +85,32 @@ std::ostream &operator<<(std::ostream &os, return os; } +// When a view sets an explicit cardinality limit, it wins outright (View > Reader > SDK +// default), so the raw recording storage is simply sized to that limit, unchanged. +// +// When a view has no explicit limit, the shared recording storage must be sized to the highest +// limit configured across all MetricReaders currently attached, so no reader loses data purely +// because the shared cap was sized for a stricter reader. Each reader's own (possibly lower) +// limit is then re-applied to just its own output during collection; see +// TemporalMetricStorage::buildMetrics(). +std::size_t ResolveRecordingCardinalityLimit( + const opentelemetry::sdk::metrics::AggregationConfig *aggregation_config, + opentelemetry::nostd::span> + collectors, + opentelemetry::sdk::metrics::InstrumentType instrument_type) +{ + if (aggregation_config) + { + return aggregation_config->cardinality_limit_; + } + std::size_t max_limit = opentelemetry::sdk::metrics::kAggregationCardinalityLimit; + for (auto &collector : collectors) + { + max_limit = (std::max)(max_limit, collector->GetCardinalityLimit(instrument_type)); + } + return max_limit; +} + } // namespace OPENTELEMETRY_BEGIN_NAMESPACE @@ -513,7 +541,7 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( auto success = view_registry->FindViews( instrument_descriptor, *scope_, - [this, &instrument_descriptor, &storages + [this, &instrument_descriptor, &storages, ctx #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW , exemplar_filter_type @@ -541,6 +569,8 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( else { WarnOnDuplicateInstrument(GetInstrumentationScope(), storage_registry_, view_instr_desc); + auto recording_cardinality_limit = ResolveRecordingCardinalityLimit( + view.GetAggregationConfig(), ctx->GetCollectors(), view_instr_desc.type_); sync_storage = std::shared_ptr(new SyncMetricStorage( view_instr_desc, view.GetAggregationType(), view.GetAttributesProcessor(), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW @@ -548,7 +578,7 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( GetExemplarReservoir(view.GetAggregationType(), view.GetAggregationConfig(), view_instr_desc), #endif - view.GetAggregationConfig())); + view.GetAggregationConfig(), recording_cardinality_limit)); storage_registry_.insert({view_instr_desc, sync_storage}); } auto sync_multi_storage = static_cast(storages.get()); @@ -586,7 +616,7 @@ std::unique_ptr Meter::RegisterAsyncMetricStorage( auto success = view_registry->FindViews( instrument_descriptor, *GetInstrumentationScope(), - [this, &instrument_descriptor, &storages + [this, &instrument_descriptor, &storages, ctx #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW , exemplar_filter_type @@ -614,6 +644,8 @@ std::unique_ptr Meter::RegisterAsyncMetricStorage( else { WarnOnDuplicateInstrument(GetInstrumentationScope(), storage_registry_, view_instr_desc); + auto recording_cardinality_limit = ResolveRecordingCardinalityLimit( + view.GetAggregationConfig(), ctx->GetCollectors(), view_instr_desc.type_); async_storage = std::shared_ptr(new AsyncMetricStorage( view_instr_desc, view.GetAggregationType(), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW @@ -621,7 +653,7 @@ std::unique_ptr Meter::RegisterAsyncMetricStorage( GetExemplarReservoir(view.GetAggregationType(), view.GetAggregationConfig(), view_instr_desc), #endif - view.GetAggregationConfig())); + view.GetAggregationConfig(), recording_cardinality_limit)); storage_registry_.insert({view_instr_desc, async_storage}); } auto async_multi_storage = static_cast(storages.get()); diff --git a/sdk/src/metrics/metric_reader.cc b/sdk/src/metrics/metric_reader.cc index c9312fc183..549fb0f83e 100644 --- a/sdk/src/metrics/metric_reader.cc +++ b/sdk/src/metrics/metric_reader.cc @@ -111,12 +111,6 @@ std::size_t MetricReader::GetCardinalityLimit(InstrumentType instrument_type) co void MetricReader::SetCardinalityLimits(const CardinalityLimits &limits) noexcept { cardinality_limits_ = limits; - // TODO: Reader-level limits are stored but not yet enforced as a per-collector - // fallback during the collection path. Enforcement will be added in a follow-up. - OTEL_INTERNAL_LOG_WARN( - "MetricReader::SetCardinalityLimits - reader-level cardinality limits are stored " - "but not yet enforced during collection. Use view-level AggregationConfig to " - "enforce limits today."); } } // namespace metrics diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index 245ef77454..3bdbd98536 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -53,7 +53,7 @@ bool SyncMetricStorage::Collect(CollectorHandle *collector, { std::lock_guard guard(attribute_hashmap_lock_); delta_metrics = std::move(attributes_hashmap_); - attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_->cardinality_limit_)); + attributes_hashmap_.reset(new AttributesHashMap(recording_cardinality_limit_)); #ifdef OPENTELEMETRY_HAVE_METRICS_BOUND_INSTRUMENTS_PREVIEW // Garbage-collect entries the user has dropped that have no pending data. // Cleanup happens during Collect(); if no collection runs, dropped bound diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 85a0a4b3c0..a1255efa1d 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -114,9 +114,17 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, } auto unreported_list = std::move(present->second); // Iterate over the unreporter metrics for `collector` and store result in `merged_metrics` - std::unique_ptr merged_metrics( - new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ - : kAggregationCardinalityLimit)); + // + // When no view-level limit is configured (aggregation_config_ == nullptr), fall back to this + // specific collector's own MetricReader-level limit rather than a flat SDK default. The + // underlying recording storage may be sized larger (to avoid losing data for readers with a + // higher limit, see SyncMetricStorage/AsyncMetricStorage), so capping merged_metrics here to + // this collector's own limit re-applies the View > Reader > SDK default precedence per reader: + // AttributesHashMap::Set() below routes anything beyond this capacity into the overflow point, + // without affecting what other collectors of the same storage see. + std::unique_ptr merged_metrics(new AttributesHashMap( + aggregation_config_ ? aggregation_config_->cardinality_limit_ + : collector->GetCardinalityLimit(instrument_descriptor_.type_))); for (auto &agg_hashmap : unreported_list) { agg_hashmap->GetAllEntries( diff --git a/sdk/test/metrics/metric_collector_test.cc b/sdk/test/metrics/metric_collector_test.cc index c97f9221f9..15505fae57 100644 --- a/sdk/test/metrics/metric_collector_test.cc +++ b/sdk/test/metrics/metric_collector_test.cc @@ -594,6 +594,92 @@ TEST_F(MetricCollectorTest, ViewCardinalityLimitEnforcedOnCollection) EXPECT_TRUE(overflow_present); } +namespace +{ +size_t CountRealPoints(const MetricProducer::Result &result, bool *overflow_present) +{ + size_t total_points = 0; + for (const ScopeMetrics &sm : result.points_.scope_metric_data_) + { + for (const MetricData &md : sm.metric_data_) + { + for (const PointDataAttributes &pda : md.point_data_attr_) + { + if (!pda.attributes.GetAttributes().empty() && + pda.attributes.GetAttributes().begin()->first == kAttributesLimitOverflowKey) + { + if (overflow_present) + { + *overflow_present = true; + } + continue; + } + ++total_points; + } + } + } + return total_points; +} +} // namespace + +// Regression test for the bug found during review of #4188: resolving a reader-level +// cardinality limit fallback into shared storage broke per-reader semantics, since a +// stricter reader would force every other reader sharing the same storage down to its limit. +// +// With no view-level limit configured, a stricter reader must only ever report up to its own +// limit (extra series collapse into its own overflow point), while a laxer reader sharing the +// same underlying storage must still see every recorded series with no data loss and no +// spurious overflow. +TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackWithMultipleReaders) +{ + constexpr size_t kLowLimit = 3; + constexpr size_t kHighLimit = 50; + constexpr size_t kUniqueAttributeSets = 10; + + auto context = std::shared_ptr(new MeterContext(ViewRegistryFactory::Create())); + auto scope = InstrumentationScope::Create("ReaderCardinalityLimitFallbackWithMultipleReaders"); + auto meter = std::shared_ptr(new Meter(context, std::move(scope))); + context->AddMeter(meter); + + auto low_reader = std::shared_ptr(new MockMetricReader()); + CardinalityLimits low_limits; + low_limits.counter = kLowLimit; + low_reader->SetCardinalityLimits(low_limits); + auto low_collector = AddMetricReaderToMeterContext(context, low_reader).lock(); + + auto high_reader = std::shared_ptr(new MockMetricReader()); + CardinalityLimits high_limits; + high_limits.counter = kHighLimit; + high_reader->SetCardinalityLimits(high_limits); + auto high_collector = AddMetricReaderToMeterContext(context, high_reader).lock(); + + // Both readers must already be attached before the instrument is first used: recording + // storage capacity is resolved (as the max limit across attached readers) at that point. + auto counter = meter->CreateUInt64Counter("shared_counter"); + + for (size_t i = 0; i < kUniqueAttributeSets; ++i) + { + std::map attrs = {{"key", std::to_string(i)}}; + counter->Add( + 1, opentelemetry::common::KeyValueIterableView>(attrs), + opentelemetry::context::Context{}); + } + + bool low_overflow = false; + bool high_overflow = false; + size_t low_points = CountRealPoints(low_collector->Produce(), &low_overflow); + size_t high_points = CountRealPoints(high_collector->Produce(), &high_overflow); + + // The stricter reader must be capped at its own limit, with overflow absorbing the rest. + EXPECT_LE(low_points, kLowLimit); + EXPECT_TRUE(low_overflow); + + // The laxer reader, sharing the same underlying storage, must see every recorded series: + // it must not be capped down to the stricter reader's limit, and must not report overflow. + EXPECT_EQ(high_points, kUniqueAttributeSets); + EXPECT_FALSE(high_overflow); +} + #if defined(__GNUC__) || defined(__clang__) || defined(__apple_build_version__) # pragma GCC diagnostic pop #endif From 0f545ac4afc06d744f1ae45c92139be6e6e949e9 Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 10 Aug 2026 21:07:43 +0530 Subject: [PATCH 2/7] Fix IWYU: meter.cc needs /, sync_metric_storage.cc no longer uses aggregation_config.h directly --- sdk/src/metrics/meter.cc | 2 ++ sdk/src/metrics/state/sync_metric_storage.cc | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index f92eb488fe..dde87ceb85 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -1,6 +1,8 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include +#include #include #include #include diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index 3bdbd98536..ac119b36a2 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -8,7 +8,6 @@ #include "opentelemetry/common/timestamp.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/span.h" -#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/sync_metric_storage.h" From c1df3f53f908d6c56b1ff9671387e2c060db7715 Mon Sep 17 00:00:00 2001 From: om7057 Date: Tue, 11 Aug 2026 08:09:53 +0530 Subject: [PATCH 3/7] Address review: fix overflow-merge data loss, floor bug, and ambiguous view-explicit detection Addresses three issues @lalitb found in review: 1. AttributesHashMap::Set() overwrote the existing overflow aggregation instead of merging into it. With several distinct attribute sets routed to overflow (as now happens routinely with a low reader-level limit), only the last one survived, silently dropping the others. Fixed by merging via Aggregation::Merge(), matching the existing GetOrSetDefault() overflow behavior. 2. ResolveRecordingCardinalityLimit() floored the recording storage size at kAggregationCardinalityLimit (2000) even when every attached reader's own limit was lower, so a single delta reader with e.g. limit=3 could still see up to 2000 series via the single-collector delta fast path in TemporalMetricStorage::buildMetrics(). Fixed by removing the floor: the max is now taken purely across attached readers' configured limits (falling back to the SDK default only when there are no readers yet). 3. A non-null AggregationConfig doesn't necessarily mean a view explicitly set a cardinality limit: SdkBuilder::AddView() may build one purely to carry histogram boundaries, leaving cardinality_limit_ at its compiled-in default. Treating that as "explicit" silently skipped the MetricReader-level fallback. Added AggregationConfig::cardinality_limit_explicit_ (defaults to true, preserving existing programmatic/test behavior) and have AddView() set it false when a config is synthesized without aggregation_cardinality_limit, true when it is set. Tests added: overflow-merge total-value assertion on the existing multi-reader test, a new single-delta-reader fast-path regression test, and two SdkBuilder tests pinning cardinality_limit_explicit_ for the histogram-boundaries-only and boundaries-plus-limit cases. --- .../metrics/aggregation/aggregation_config.h | 10 +- .../sdk/metrics/state/attributes_hashmap.h | 28 +++++- sdk/src/configuration/sdk_builder.cc | 11 ++- sdk/src/metrics/meter.cc | 22 +++-- .../metrics/state/temporal_metric_storage.cc | 18 ++-- sdk/test/configuration/sdk_builder_test.cc | 92 +++++++++++++++++++ sdk/test/metrics/metric_collector_test.cc | 74 ++++++++++++++- 7 files changed, 235 insertions(+), 20 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 8c80715846..f5adfceb9e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -40,7 +40,15 @@ class AggregationConfig } size_t cardinality_limit_; - virtual ~AggregationConfig() = default; + // Whether cardinality_limit_ reflects an intentionally-configured value, as opposed to + // just the compiled-in default left untouched because this config was built for some other + // reason (e.g. histogram boundaries). Defaults to true so any caller that constructs an + // AggregationConfig directly (tests, programmatic API) keeps prior behavior: the config's + // cardinality_limit_ is always honored as-is. SdkBuilder::AddView() is the one place that + // knows when a config was synthesized without an explicit `aggregation_cardinality_limit`, + // and sets this to false there so a MetricReader-level fallback can apply instead. + bool cardinality_limit_explicit_ = true; + virtual ~AggregationConfig() = default; }; class HistogramAggregationConfig : public AggregationConfig diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index 1ca165f5af..c9f4e914a1 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -122,7 +122,12 @@ class AttributesHashMapWithCustomHash return result.first->second.get(); } /** - * Set the value for given key, overwriting the value if already present + * Set the value for given key, overwriting the value if already present. + * + * When `attributes` is a new key that has to be routed to the overflow entry (capacity + * reached), this merges into any existing overflow aggregation rather than replacing it, so + * that a second and later over-capacity attribute set doesn't discard values already + * accumulated there from an earlier one. */ void Set(const MetricAttributes &attributes, std::unique_ptr aggr) { @@ -133,7 +138,7 @@ class AttributesHashMapWithCustomHash } else if (IsOverflowAttributes(attributes)) { - hash_map_[GetOverflowAttributes()] = std::move(aggr); + SetOverflowMerged(std::move(aggr)); } else { @@ -150,7 +155,7 @@ class AttributesHashMapWithCustomHash } else if (IsOverflowAttributes(attributes)) { - hash_map_[GetOverflowAttributes()] = std::move(aggr); + SetOverflowMerged(std::move(aggr)); } else { @@ -207,6 +212,23 @@ class AttributesHashMapWithCustomHash return result.first->second.get(); } + // Merges `agg` into the existing overflow aggregation if one is already present, otherwise + // installs it as the overflow entry. Used by Set() when routing a new over-capacity key to + // overflow, so multiple distinct attribute sets funneled into the single overflow bucket + // accumulate rather than each replacing the last. + void SetOverflowMerged(std::unique_ptr agg) + { + auto it = hash_map_.find(GetOverflowAttributes()); + if (it != hash_map_.end()) + { + hash_map_[GetOverflowAttributes()] = it->second->Merge(*agg); + } + else + { + hash_map_[GetOverflowAttributes()] = std::move(agg); + } + } + bool IsOverflowAttributes(const MetricAttributes &attributes) const { // If the incoming attributes are exactly the overflow sentinel, route diff --git a/sdk/src/configuration/sdk_builder.cc b/sdk/src/configuration/sdk_builder.cc index de22c29f00..3c11c66804 100644 --- a/sdk/src/configuration/sdk_builder.cc +++ b/sdk/src/configuration/sdk_builder.cc @@ -1933,6 +1933,14 @@ void SdkBuilder::AddView( if (stream->aggregation) { sdk_aggregation_config = CreateAggregationConfig(stream->aggregation, sdk_aggregation_type); + if (sdk_aggregation_config) + { + // CreateAggregationConfig() may build a config purely for non-cardinality reasons (e.g. + // histogram boundaries), leaving cardinality_limit_ at its compiled-in default. Mark it + // as not explicit so it doesn't shadow a MetricReader-level fallback; the block below + // sets this back to true if aggregation_cardinality_limit is also configured. + sdk_aggregation_config->cardinality_limit_explicit_ = false; + } } // Apply aggregation_cardinality_limit from the view stream configuration @@ -1940,7 +1948,8 @@ void SdkBuilder::AddView( { if (sdk_aggregation_config) { - sdk_aggregation_config->cardinality_limit_ = stream->aggregation_cardinality_limit; + sdk_aggregation_config->cardinality_limit_ = stream->aggregation_cardinality_limit; + sdk_aggregation_config->cardinality_limit_explicit_ = true; } else { diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index dde87ceb85..aeee1a5c57 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -87,13 +87,19 @@ std::ostream &operator<<(std::ostream &os, return os; } -// When a view sets an explicit cardinality limit, it wins outright (View > Reader > SDK -// default), so the raw recording storage is simply sized to that limit, unchanged. +// When a view sets an explicit cardinality limit (cardinality_limit_explicit_), it wins outright +// (View > Reader > SDK default), so the raw recording storage is simply sized to that limit, +// unchanged. A non-null AggregationConfig does not by itself mean the view set an explicit +// cardinality limit: e.g. SdkBuilder::AddView() may build one purely to carry histogram +// boundaries, leaving cardinality_limit_ at its compiled-in default and +// cardinality_limit_explicit_ false. // // When a view has no explicit limit, the shared recording storage must be sized to the highest // limit configured across all MetricReaders currently attached, so no reader loses data purely -// because the shared cap was sized for a stricter reader. Each reader's own (possibly lower) -// limit is then re-applied to just its own output during collection; see +// because the shared cap was sized for a stricter reader. Do not floor this at the SDK default: +// a reader may configure a limit lower than the default, and that stricter limit must still +// apply when it is the only (or the strictest) reader attached. Each reader's own (possibly +// lower) limit is then re-applied to just its own output during collection; see // TemporalMetricStorage::buildMetrics(). std::size_t ResolveRecordingCardinalityLimit( const opentelemetry::sdk::metrics::AggregationConfig *aggregation_config, @@ -101,11 +107,15 @@ std::size_t ResolveRecordingCardinalityLimit( collectors, opentelemetry::sdk::metrics::InstrumentType instrument_type) { - if (aggregation_config) + if (aggregation_config && aggregation_config->cardinality_limit_explicit_) { return aggregation_config->cardinality_limit_; } - std::size_t max_limit = opentelemetry::sdk::metrics::kAggregationCardinalityLimit; + if (collectors.empty()) + { + return opentelemetry::sdk::metrics::kAggregationCardinalityLimit; + } + std::size_t max_limit = 0; for (auto &collector : collectors) { max_limit = (std::max)(max_limit, collector->GetCardinalityLimit(instrument_type)); diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index a1255efa1d..48391fba65 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -115,16 +115,20 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, auto unreported_list = std::move(present->second); // Iterate over the unreporter metrics for `collector` and store result in `merged_metrics` // - // When no view-level limit is configured (aggregation_config_ == nullptr), fall back to this - // specific collector's own MetricReader-level limit rather than a flat SDK default. The - // underlying recording storage may be sized larger (to avoid losing data for readers with a - // higher limit, see SyncMetricStorage/AsyncMetricStorage), so capping merged_metrics here to - // this collector's own limit re-applies the View > Reader > SDK default precedence per reader: + // When the view has no explicit cardinality limit (aggregation_config_ == nullptr, or its + // cardinality_limit_ was left at the compiled-in default for an unrelated reason such as + // histogram boundaries; see cardinality_limit_explicit_), fall back to this specific + // collector's own MetricReader-level limit rather than a flat SDK default. The underlying + // recording storage may be sized larger (to avoid losing data for readers with a higher + // limit, see SyncMetricStorage/AsyncMetricStorage), so capping merged_metrics here to this + // collector's own limit re-applies the View > Reader > SDK default precedence per reader: // AttributesHashMap::Set() below routes anything beyond this capacity into the overflow point, // without affecting what other collectors of the same storage see. + const bool has_explicit_view_limit = + aggregation_config_ && aggregation_config_->cardinality_limit_explicit_; std::unique_ptr merged_metrics(new AttributesHashMap( - aggregation_config_ ? aggregation_config_->cardinality_limit_ - : collector->GetCardinalityLimit(instrument_descriptor_.type_))); + has_explicit_view_limit ? aggregation_config_->cardinality_limit_ + : collector->GetCardinalityLimit(instrument_descriptor_.type_))); for (auto &agg_hashmap : unreported_list) { agg_hashmap->GetAllEntries( diff --git a/sdk/test/configuration/sdk_builder_test.cc b/sdk/test/configuration/sdk_builder_test.cc index 2d8bb243da..ef11739012 100644 --- a/sdk/test/configuration/sdk_builder_test.cc +++ b/sdk/test/configuration/sdk_builder_test.cc @@ -30,6 +30,7 @@ #include "opentelemetry/sdk/configuration/composable_rule_based_sampler_rule_attribute_values_configuration.h" #include "opentelemetry/sdk/configuration/composable_rule_based_sampler_rule_configuration.h" #include "opentelemetry/sdk/configuration/composable_sampler_configuration.h" +#include "opentelemetry/sdk/configuration/explicit_bucket_histogram_aggregation_configuration.h" #include "opentelemetry/sdk/configuration/extension_push_metric_exporter_builder.h" #include "opentelemetry/sdk/configuration/extension_push_metric_exporter_configuration.h" #include "opentelemetry/sdk/configuration/instrument_type.h" @@ -829,3 +830,94 @@ TEST(SdkBuilder, AddViewCounterCardinalityLimitOnly) EXPECT_EQ(matched, 1); } + +// Regression test for a bug found during review of #4388: a view that configures an +// `aggregation` block for reasons unrelated to cardinality (here, explicit histogram +// boundaries) without also setting `aggregation_cardinality_limit` must not be treated as +// having an explicit view-level cardinality limit, since the resulting AggregationConfig's +// cardinality_limit_ is just the compiled-in default (kAggregationCardinalityLimit), not a +// user choice. Otherwise a MetricReader-level fallback would be silently skipped. +TEST(SdkBuilder, AddViewHistogramBoundariesWithoutCardinalityLimitIsNotExplicit) +{ + namespace metrics_sdk = opentelemetry::sdk::metrics; + + auto model = std::make_unique(); + model->selector = std::make_unique(); + model->selector->instrument_type = config_sdk::InstrumentType::histogram; + + model->stream = std::make_unique(); + auto histogram_aggr = + std::make_unique(); + histogram_aggr->boundaries = {1.0, 2.0, 3.0}; + model->stream->aggregation = std::move(histogram_aggr); + // aggregation_cardinality_limit intentionally left at its default (0 = kInheritFromReader). + + auto registry = std::make_shared(); + config_sdk::SdkBuilder builder(registry); + + metrics_sdk::ViewRegistry view_registry; + builder.AddView(&view_registry, model); + + metrics_sdk::InstrumentDescriptor instrument_descriptor{ + "", "", "", metrics_sdk::InstrumentType::kHistogram, metrics_sdk::InstrumentValueType::kLong}; + auto instrumentation_scope = scope_sdk::InstrumentationScope::Create(""); + + int matched = 0; + view_registry.FindViews(instrument_descriptor, *instrumentation_scope, + [&](const metrics_sdk::View &view) { + matched++; + auto *aggregation_config = view.GetAggregationConfig(); + EXPECT_NE(aggregation_config, nullptr); + if (aggregation_config) + { + EXPECT_FALSE(aggregation_config->cardinality_limit_explicit_); + } + return true; + }); + + EXPECT_EQ(matched, 1); +} + +// Companion to the test above: when the same stream also sets aggregation_cardinality_limit, +// the resulting config must be marked explicit and carry that value. +TEST(SdkBuilder, AddViewHistogramBoundariesWithCardinalityLimitIsExplicit) +{ + namespace metrics_sdk = opentelemetry::sdk::metrics; + + auto model = std::make_unique(); + model->selector = std::make_unique(); + model->selector->instrument_type = config_sdk::InstrumentType::histogram; + + model->stream = std::make_unique(); + auto histogram_aggr = + std::make_unique(); + histogram_aggr->boundaries = {1.0, 2.0, 3.0}; + model->stream->aggregation = std::move(histogram_aggr); + model->stream->aggregation_cardinality_limit = 99; + + auto registry = std::make_shared(); + config_sdk::SdkBuilder builder(registry); + + metrics_sdk::ViewRegistry view_registry; + builder.AddView(&view_registry, model); + + metrics_sdk::InstrumentDescriptor instrument_descriptor{ + "", "", "", metrics_sdk::InstrumentType::kHistogram, metrics_sdk::InstrumentValueType::kLong}; + auto instrumentation_scope = scope_sdk::InstrumentationScope::Create(""); + + int matched = 0; + view_registry.FindViews(instrument_descriptor, *instrumentation_scope, + [&](const metrics_sdk::View &view) { + matched++; + auto *aggregation_config = view.GetAggregationConfig(); + EXPECT_NE(aggregation_config, nullptr); + if (aggregation_config) + { + EXPECT_TRUE(aggregation_config->cardinality_limit_explicit_); + EXPECT_EQ(aggregation_config->cardinality_limit_, 99u); + } + return true; + }); + + EXPECT_EQ(matched, 1); +} diff --git a/sdk/test/metrics/metric_collector_test.cc b/sdk/test/metrics/metric_collector_test.cc index 15505fae57..55b596cf5b 100644 --- a/sdk/test/metrics/metric_collector_test.cc +++ b/sdk/test/metrics/metric_collector_test.cc @@ -620,6 +620,25 @@ size_t CountRealPoints(const MetricProducer::Result &result, bool *overflow_pres } return total_points; } + +// Sums the counter value across every point, including the overflow point. Used to prove no +// data is silently dropped when several distinct attribute sets are funneled into overflow. +int64_t SumCounterValue(const MetricProducer::Result &result) +{ + int64_t total = 0; + for (const ScopeMetrics &sm : result.points_.scope_metric_data_) + { + for (const MetricData &md : sm.metric_data_) + { + for (const PointDataAttributes &pda : md.point_data_attr_) + { + auto sum_point_data = nostd::get(pda.point_data); + total += nostd::get(sum_point_data.value_); + } + } + } + return total; +} } // namespace // Regression test for the bug found during review of #4188: resolving a reader-level @@ -665,14 +684,20 @@ TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackWithMultipleReaders) opentelemetry::context::Context{}); } + auto low_produced = low_collector->Produce(); + auto high_produced = high_collector->Produce(); + bool low_overflow = false; bool high_overflow = false; - size_t low_points = CountRealPoints(low_collector->Produce(), &low_overflow); - size_t high_points = CountRealPoints(high_collector->Produce(), &high_overflow); + size_t low_points = CountRealPoints(low_produced, &low_overflow); + size_t high_points = CountRealPoints(high_produced, &high_overflow); // The stricter reader must be capped at its own limit, with overflow absorbing the rest. EXPECT_LE(low_points, kLowLimit); EXPECT_TRUE(low_overflow); + // No data may be silently dropped: every one of the kUniqueAttributeSets Add(1, ...) calls + // must still be reflected somewhere, whether as its own point or merged into overflow. + EXPECT_EQ(SumCounterValue(low_produced), static_cast(kUniqueAttributeSets)); // The laxer reader, sharing the same underlying storage, must see every recorded series: // it must not be capped down to the stricter reader's limit, and must not report overflow. @@ -680,6 +705,51 @@ TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackWithMultipleReaders) EXPECT_FALSE(high_overflow); } +// Regression test for a second bug found during review: the single-collector delta fast path +// in TemporalMetricStorage::buildMetrics() reads straight from the raw recording storage, +// bypassing the per-collector re-cap. With a single delta reader this is only safe if the +// *recording* storage itself was already sized to that reader's own limit (rather than floored +// at the SDK default), which is what ResolveRecordingCardinalityLimit() in meter.cc must do. +TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackSingleDeltaReader) +{ + constexpr size_t kLimit = 3; + constexpr size_t kUniqueAttributeSets = 10; + + auto context = std::shared_ptr(new MeterContext(ViewRegistryFactory::Create())); + auto scope = InstrumentationScope::Create("ReaderCardinalityLimitFallbackSingleDeltaReader"); + auto meter = std::shared_ptr(new Meter(context, std::move(scope))); + context->AddMeter(meter); + + // MockMetricReader reports kDelta aggregation temporality, and with exactly one reader + // attached this exercises TemporalMetricStorage::buildMetrics()'s single-collector-delta + // fast path. + auto reader = std::shared_ptr(new MockMetricReader()); + CardinalityLimits limits; + limits.counter = kLimit; + reader->SetCardinalityLimits(limits); + auto collector = AddMetricReaderToMeterContext(context, reader).lock(); + + auto counter = meter->CreateUInt64Counter("delta_counter"); + + for (size_t i = 0; i < kUniqueAttributeSets; ++i) + { + std::map attrs = {{"key", std::to_string(i)}}; + counter->Add( + 1, opentelemetry::common::KeyValueIterableView>(attrs), + opentelemetry::context::Context{}); + } + + auto produced = collector->Produce(); + bool overflow_present = false; + size_t real_points = CountRealPoints(produced, &overflow_present); + + // The reader's own limit must be honored even via the single-collector delta fast path, + // not silently widened to the SDK default (2000). + EXPECT_LE(real_points, kLimit); + EXPECT_TRUE(overflow_present); + EXPECT_EQ(SumCounterValue(produced), static_cast(kUniqueAttributeSets)); +} + #if defined(__GNUC__) || defined(__clang__) || defined(__apple_build_version__) # pragma GCC diagnostic pop #endif From 618b270da98a63e94cbbef3dbaed81db5b03185d Mon Sep 17 00:00:00 2001 From: om7057 Date: Wed, 12 Aug 2026 20:05:01 +0530 Subject: [PATCH 4/7] Fix silent merge break: exemplar filter parameter renamed on main, my delegating ctors still used the old name The exemplar-filters refactor (#4267) that landed on main renamed the long-standing typo'd parameter exempler_filter_type to exemplar_filter_type in some but not all of the lines my branch also touched, so git's line-based auto-merge produced no conflict marker but left the two delegating-constructor overloads in sync_metric_storage.h/async_metric_storage.h referencing the old name in their forwarding call / parameter declaration while the member-init list used the new one. Only builds with ENABLE_METRICS_EXEMPLAR_PREVIEW defined actually instantiate that code path, which is why local default-config builds didn't catch it before pushing. Verified with a throwaway build with WITH_METRICS_EXEMPLAR_PREVIEW=ON (opentelemetry_metrics, metric_collector_test, metric_reader_test, sync/async_instruments_test, async_metric_storage_test all build and pass), and re-verified the default (flag off) build still passes. --- .../opentelemetry/sdk/metrics/state/async_metric_storage.h | 4 ++-- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h index a8989641e9..7fa46f4811 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -47,7 +47,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora : AsyncMetricStorage(instrument_descriptor, aggregation_type, #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - exempler_filter_type, + exemplar_filter_type, std::move(exemplar_reservoir), #endif aggregation_config, @@ -57,7 +57,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora AsyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, const AggregationType aggregation_type, #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - ExemplarFilterType exempler_filter_type, + ExemplarFilterType exemplar_filter_type, nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config, diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h index 1ac5e4a9b0..b48e7f4370 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -65,7 +65,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage aggregation_type, std::move(attributes_processor), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - exempler_filter_type, + exemplar_filter_type, std::move(exemplar_reservoir), #endif aggregation_config, @@ -83,7 +83,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage const AggregationType aggregation_type, std::shared_ptr attributes_processor, #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - ExemplarFilterType exempler_filter_type, + ExemplarFilterType exemplar_filter_type, nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config, From 07ad1981be6cc7695b4410b8cfac33f4a9de24ab Mon Sep 17 00:00:00 2001 From: om7057 Date: Mon, 17 Aug 2026 23:54:04 +0530 Subject: [PATCH 5/7] Address review: represent unspecified cardinality limit for programmatic configs, document late-reader recording-capacity limitation Per lalitb's review: - AggregationConfig/HistogramAggregationConfig/ Base2ExponentialHistogramAggregationConfig previously defaulted cardinality_limit_explicit_ to true unconditionally, so a programmatically-constructed config built only for some other reason (e.g. HistogramAggregationConfig() to set histogram boundaries) was wrongly treated as having set an explicit cardinality limit, silently shadowing the MetricReader-level fallback. Added a sentinel (kCardinalityLimitUnspecified) so the constructor can tell apart "no cardinality_limit argument was given" from "cardinality_limit was explicitly passed", without needing (avoided elsewhere in the SDK for ABI reasons). This is correct for every construction path - tests, programmatic API, and SdkBuilder - without each caller having to set the flag manually. - Recording capacity for an instrument is resolved once, as the highest limit across readers attached at instrument-creation time, and is not grown retroactively. A reader added later with a higher limit cannot recover attribute sets that already collapsed into overflow before it was attached. Documented this on MeterContext::AddMetricReader and MeterProvider::AddMetricReader, and added a regression test pinning the behavior. --- CHANGELOG.md | 6 ++- .../metrics/aggregation/aggregation_config.h | 38 ++++++++----- .../opentelemetry/sdk/metrics/meter_context.h | 7 +++ .../sdk/metrics/meter_provider.h | 7 +++ sdk/test/metrics/metric_collector_test.cc | 54 +++++++++++++++++++ 5 files changed, 98 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d102b6e310..e35afa1142 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,11 @@ Increment the: Shared recording storage for a view with no explicit limit is now sized at the max limit across all attached readers, so a reader with a higher limit does not lose data; each reader's own (possibly stricter) limit is then - re-applied to just its own collected output. + re-applied to just its own collected output. A programmatically-constructed + `AggregationConfig`/`HistogramAggregationConfig`/ + `Base2ExponentialHistogramAggregationConfig` (e.g. one built only to carry + histogram boundaries) is now also correctly treated as not having an + explicit cardinality limit, matching the declarative-configuration path. [#4387](https://github.com/open-telemetry/opentelemetry-cpp/issues/4387) * [CONFIGURATION] Build the configured resource detectors in SdkBuilder, apply the `detection.attributes` include/exclude filter to the detected attributes, diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index f5adfceb9e..e03d263a16 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -3,6 +3,8 @@ #pragma once +#include +#include #include #include "opentelemetry/sdk/metrics/instruments.h" @@ -15,11 +17,21 @@ namespace sdk namespace metrics { +// Sentinel passed to AggregationConfig's cardinality_limit constructor argument to mean "the +// caller did not specify a limit", distinct from any real limit value (which is always a small +// positive count). Lets the constructor tell apart e.g. HistogramAggregationConfig() built only +// to carry boundaries_ from HistogramAggregationConfig(500) built to set a real limit of 500, +// without needing (avoided elsewhere in the SDK for ABI reasons). +constexpr size_t kCardinalityLimitUnspecified = (std::numeric_limits::max)(); + class AggregationConfig { public: - AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) - : cardinality_limit_(cardinality_limit) + AggregationConfig(size_t cardinality_limit = kCardinalityLimitUnspecified) + : cardinality_limit_(cardinality_limit == kCardinalityLimitUnspecified + ? kAggregationCardinalityLimit + : cardinality_limit), + cardinality_limit_explicit_(cardinality_limit != kCardinalityLimitUnspecified) {} AggregationConfig(const AggregationConfig &) = default; @@ -40,21 +52,21 @@ class AggregationConfig } size_t cardinality_limit_; - // Whether cardinality_limit_ reflects an intentionally-configured value, as opposed to - // just the compiled-in default left untouched because this config was built for some other - // reason (e.g. histogram boundaries). Defaults to true so any caller that constructs an - // AggregationConfig directly (tests, programmatic API) keeps prior behavior: the config's - // cardinality_limit_ is always honored as-is. SdkBuilder::AddView() is the one place that - // knows when a config was synthesized without an explicit `aggregation_cardinality_limit`, - // and sets this to false there so a MetricReader-level fallback can apply instead. - bool cardinality_limit_explicit_ = true; - virtual ~AggregationConfig() = default; + // Whether cardinality_limit_ reflects an intentionally-configured value, as opposed to just + // the compiled-in default it was left at because this config was constructed for some other + // reason (e.g. histogram boundaries) without a cardinality_limit argument. Derived from + // whether the constructor's cardinality_limit argument was kCardinalityLimitUnspecified, so + // this is correct for every construction path (tests, programmatic API, SdkBuilder) without + // each caller having to set it manually. A MetricReader-level fallback applies whenever this + // is false. + bool cardinality_limit_explicit_; + virtual ~AggregationConfig() = default; }; class HistogramAggregationConfig : public AggregationConfig { public: - HistogramAggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) + HistogramAggregationConfig(size_t cardinality_limit = kCardinalityLimitUnspecified) : AggregationConfig(cardinality_limit) {} @@ -84,7 +96,7 @@ class Base2ExponentialHistogramAggregationConfig : public AggregationConfig { public: Base2ExponentialHistogramAggregationConfig( - size_t cardinality_limit = kAggregationCardinalityLimit) + size_t cardinality_limit = kCardinalityLimitUnspecified) : AggregationConfig(cardinality_limit) {} diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_context.h b/sdk/include/opentelemetry/sdk/metrics/meter_context.h index 318c0c5391..ed30931450 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_context.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_context.h @@ -142,6 +142,13 @@ class MeterContext : public std::enable_shared_from_this * Note: This reader may not receive any in-flight meter data, but will get newly created meter * data. * Note: This method is not thread safe, and should ideally be called from main thread. + * Note: For an instrument that already exists when this reader is added, this reader's + * cardinality limit for that instrument only takes effect up to whatever recording capacity + * was already resolved (as the highest limit across readers attached at instrument-creation + * time) when the instrument was first created; that capacity is fixed for the lifetime of the + * instrument and is not grown retroactively. A reader added with a higher limit than any + * reader present at instrument-creation time will not recover attribute sets that had already + * collapsed into the instrument's overflow point before this reader was attached. */ void AddMetricReader(std::shared_ptr reader, std::unique_ptr metric_filter = nullptr) noexcept; diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h index fccaae690b..d7159a2049 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h @@ -104,6 +104,13 @@ class OPENTELEMETRY_EXPORT MeterProvider final : public opentelemetry::metrics:: * Note: This reader may not receive any in-flight meter data, but will get newly created meter * data. * Note: This method is not thread safe, and should ideally be called from main thread. + * Note: For an instrument that already exists when this reader is added, this reader's + * cardinality limit for that instrument only takes effect up to whatever recording capacity + * was already resolved (as the highest limit across readers attached at instrument-creation + * time) when the instrument was first created; that capacity is fixed for the lifetime of the + * instrument and is not grown retroactively. A reader added with a higher limit than any + * reader present at instrument-creation time will not recover attribute sets that had already + * collapsed into the instrument's overflow point before this reader was attached. */ void AddMetricReader(std::shared_ptr reader, std::unique_ptr metric_filter = nullptr) noexcept; diff --git a/sdk/test/metrics/metric_collector_test.cc b/sdk/test/metrics/metric_collector_test.cc index 55b596cf5b..eb07b647b5 100644 --- a/sdk/test/metrics/metric_collector_test.cc +++ b/sdk/test/metrics/metric_collector_test.cc @@ -750,6 +750,60 @@ TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackSingleDeltaReader) EXPECT_EQ(SumCounterValue(produced), static_cast(kUniqueAttributeSets)); } +// Documents and pins a known limitation raised during review of #4188: recording capacity is +// resolved once, as the highest limit across readers attached at instrument-creation time (see +// ResolveRecordingCardinalityLimit() in meter.cc), and is not grown retroactively. A reader +// added later with a higher limit cannot recover attribute sets that had already collapsed into +// the overflow point before it was attached; see the Note on MeterContext::AddMetricReader(). +TEST_F(MetricCollectorTest, ReaderAddedAfterInstrumentCreationDoesNotGrowRecordingCapacity) +{ + constexpr size_t kLowLimit = 3; + constexpr size_t kHighLimit = 50; + constexpr size_t kUniqueAttributeSets = 10; + + auto context = std::shared_ptr(new MeterContext(ViewRegistryFactory::Create())); + auto scope = InstrumentationScope::Create( + "ReaderAddedAfterInstrumentCreationDoesNotGrowRecordingCapacity"); + auto meter = std::shared_ptr(new Meter(context, std::move(scope))); + context->AddMeter(meter); + + auto low_reader = std::shared_ptr(new MockMetricReader()); + CardinalityLimits low_limits; + low_limits.counter = kLowLimit; + low_reader->SetCardinalityLimits(low_limits); + AddMetricReaderToMeterContext(context, low_reader); + + // The instrument is created (and its recording capacity resolved) with only the low-limit + // reader attached. + auto counter = meter->CreateUInt64Counter("late_reader_counter"); + + for (size_t i = 0; i < kUniqueAttributeSets; ++i) + { + std::map attrs = {{"key", std::to_string(i)}}; + counter->Add( + 1, opentelemetry::common::KeyValueIterableView>(attrs), + opentelemetry::context::Context{}); + } + + // A higher-limit reader is attached only after the instrument already exists and has recorded + // more unique attribute sets than the low limit. + auto high_reader = std::shared_ptr(new MockMetricReader()); + CardinalityLimits high_limits; + high_limits.counter = kHighLimit; + high_reader->SetCardinalityLimits(high_limits); + auto high_collector = AddMetricReaderToMeterContext(context, high_reader).lock(); + + auto high_produced = high_collector->Produce(); + bool high_overflow = false; + size_t high_points = CountRealPoints(high_produced, &high_overflow); + + // Despite its own limit of kHighLimit, the late-attached reader is still capped by the + // recording capacity resolved when the instrument was created: attribute sets already + // collapsed into overflow before this reader existed cannot be recovered. + EXPECT_LE(high_points, kLowLimit); + EXPECT_TRUE(high_overflow); +} + #if defined(__GNUC__) || defined(__clang__) || defined(__apple_build_version__) # pragma GCC diagnostic pop #endif From 2ee55e578a71c3fdca6be063ca9859be5c46bde7 Mon Sep 17 00:00:00 2001 From: om7057 Date: Wed, 19 Aug 2026 08:39:27 +0530 Subject: [PATCH 6/7] Address review: encapsulate cardinality limit fields, fix delta fast-path re-cap, fix bound-instrument admission limit Three findings from the recheck of the earlier fixes: - AggregationConfig::cardinality_limit_ was still a public field, so direct assignment (config.cardinality_limit_ = 100) could desync it from cardinality_limit_explicit_, silently letting a MetricReader fallback override a value the caller thought was set explicitly. Made both fields private; added SetCardinalityLimit() to set them together, and GetCardinalityLimit()/IsCardinalityLimitExplicit() accessors. Updated every call site (meter.cc, temporal_metric_storage.cc, sync/async storage headers, sdk_builder.cc, sdk_builder_test.cc); the manual cardinality_limit_explicit_ = false workaround in SdkBuilder::AddView() is no longer needed since AggregationConfig's constructor now derives it correctly for every construction path. - The single-collector delta fast path in TemporalMetricStorage::buildMetrics() returned the raw recording storage directly, bypassing the per-collector re-cap. When recording capacity was resolved larger than this reader's own limit (e.g. the reader was attached after the instrument already existed, so its limit was never part of the max-across-readers resolution at creation time), the fast path could emit more attribute sets than the reader's configured limit. Now re-caps through a hashmap sized to the collector's effective limit before emitting, when the recorded size exceeds it. - SyncMetricStorage::ResolveCardinality() (bound-instrument admission, preview) used aggregation_config_->GetCardinalityLimit() instead of recording_cardinality_limit_. These can differ when the limit comes from a MetricReader fallback rather than an explicit view limit, since attributes_hashmap_ (the unbound path) is sized to recording_cardinality_limit_. Now uses the same resolved limit so bound and unbound admission agree. Also fixed a pre-existing test bug found while verifying the second fix: ReaderCardinalityLimitFallbackSingleDeltaReader used MockMetricReader's default exporter, which reports kCumulative, not kDelta, so it never actually exercised the single-collector-delta fast path its name and comment claimed to test. Fixed to construct an explicit kDelta exporter, and added ReaderCardinalityLimitFallbackSingleDeltaReaderAttachedLate, which does exercise that path and fails without the fix (verified by temporarily reverting it locally: 10 points emitted with no overflow instead of capped at the reader's limit of 3). --- CHANGELOG.md | 7 +++ .../metrics/aggregation/aggregation_config.h | 27 +++++--- .../sdk/metrics/state/async_metric_storage.h | 13 ++-- .../sdk/metrics/state/sync_metric_storage.h | 22 ++++--- sdk/src/configuration/sdk_builder.cc | 16 ++--- sdk/src/metrics/meter.cc | 16 ++--- .../metrics/state/temporal_metric_storage.cc | 55 ++++++++++++++--- sdk/test/configuration/sdk_builder_test.cc | 14 ++--- sdk/test/metrics/metric_collector_test.cc | 61 +++++++++++++++++-- 9 files changed, 170 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e35afa1142..d90ac0ac57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,13 @@ Increment the: `Base2ExponentialHistogramAggregationConfig` (e.g. one built only to carry histogram boundaries) is now also correctly treated as not having an explicit cardinality limit, matching the declarative-configuration path. + `AggregationConfig::cardinality_limit_`/`cardinality_limit_explicit_` are + now private, set together via `SetCardinalityLimit()`, so they cannot be + desynced by direct field assignment. The single-collector delta fast path + in `TemporalMetricStorage::buildMetrics()` now re-caps to the collector's + own limit instead of emitting the raw recording storage unchecked, and the + bound-instrument admission path (preview) now uses the same resolved + recording limit as the unbound path. [#4387](https://github.com/open-telemetry/opentelemetry-cpp/issues/4387) * [CONFIGURATION] Build the configured resource detectors in SdkBuilder, apply the `detection.attributes` include/exclude filter to the detected attributes, diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index e03d263a16..ec31378016 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -51,16 +51,27 @@ class AggregationConfig return &default_config; } + std::size_t GetCardinalityLimit() const noexcept { return cardinality_limit_; } + + // Whether the cardinality limit reflects an intentionally-configured value, as opposed to + // just the compiled-in default it was left at because this config was constructed for some + // other reason (e.g. histogram boundaries) without a cardinality_limit argument. A + // MetricReader-level fallback applies whenever this is false. + bool IsCardinalityLimitExplicit() const noexcept { return cardinality_limit_explicit_; } + + // Sets the cardinality limit and marks it explicit, atomically. Use this instead of assigning + // a limit some other way, so the limit and its explicit-ness can never go out of sync. + void SetCardinalityLimit(std::size_t cardinality_limit) noexcept + { + cardinality_limit_ = cardinality_limit; + cardinality_limit_explicit_ = true; + } + + virtual ~AggregationConfig() = default; + +private: size_t cardinality_limit_; - // Whether cardinality_limit_ reflects an intentionally-configured value, as opposed to just - // the compiled-in default it was left at because this config was constructed for some other - // reason (e.g. histogram boundaries) without a cardinality_limit argument. Derived from - // whether the constructor's cardinality_limit argument was kCardinalityLimitUnspecified, so - // this is correct for every construction path (tests, programmatic API, SdkBuilder) without - // each caller having to set it manually. A MetricReader-level fallback applies whenever this - // is false. bool cardinality_limit_explicit_; - virtual ~AggregationConfig() = default; }; class HistogramAggregationConfig : public AggregationConfig diff --git a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h index 7fa46f4811..6ec0fbb6b1 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -44,14 +44,15 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config) - : AsyncMetricStorage(instrument_descriptor, - aggregation_type, + : AsyncMetricStorage( + instrument_descriptor, + aggregation_type, #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - exemplar_filter_type, - std::move(exemplar_reservoir), + exemplar_filter_type, + std::move(exemplar_reservoir), #endif - aggregation_config, - AggregationConfig::GetOrDefault(aggregation_config)->cardinality_limit_) + aggregation_config, + AggregationConfig::GetOrDefault(aggregation_config)->GetCardinalityLimit()) {} AsyncMetricStorage(const InstrumentDescriptor &instrument_descriptor, diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h index b48e7f4370..7150433f2d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -61,15 +61,16 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::shared_ptr &&exemplar_reservoir, #endif const AggregationConfig *aggregation_config) - : SyncMetricStorage(instrument_descriptor, - aggregation_type, - std::move(attributes_processor), + : SyncMetricStorage( + instrument_descriptor, + aggregation_type, + std::move(attributes_processor), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - exemplar_filter_type, - std::move(exemplar_reservoir), + exemplar_filter_type, + std::move(exemplar_reservoir), #endif - aggregation_config, - AggregationConfig::GetOrDefault(aggregation_config)->cardinality_limit_) + aggregation_config, + AggregationConfig::GetOrDefault(aggregation_config)->GetCardinalityLimit()) {} // `recording_cardinality_limit` sizes the storage that raw measurements are recorded into, @@ -289,7 +290,12 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage { return filtered; } - const size_t limit = aggregation_config_->cardinality_limit_; + // Use the resolved recording capacity, not aggregation_config_->GetCardinalityLimit(): + // when the limit comes from a MetricReader fallback rather than an explicit view limit, + // these can differ, and attributes_hashmap_ (the unbound path) is sized to + // recording_cardinality_limit_. Using the same value here keeps bound and unbound + // admission consistent. + const size_t limit = recording_cardinality_limit_; const bool has_overflow = active_keys_.find(GetOverflowAttributes()) != active_keys_.end(); // Mirror AttributesHashMap::IsOverflowAttributes() exactly. The configured // limit applies to non-overflow attribute sets, while overflow is reserved. diff --git a/sdk/src/configuration/sdk_builder.cc b/sdk/src/configuration/sdk_builder.cc index 0be18e46f4..8e5c20bddb 100644 --- a/sdk/src/configuration/sdk_builder.cc +++ b/sdk/src/configuration/sdk_builder.cc @@ -1989,15 +1989,12 @@ void SdkBuilder::AddView( if (stream->aggregation) { + // CreateAggregationConfig() may build a config purely for non-cardinality reasons (e.g. + // histogram boundaries) via a config's default constructor, which already leaves the + // cardinality limit not-explicit (see AggregationConfig's constructor) so it doesn't + // shadow a MetricReader-level fallback; the block below marks it explicit if + // aggregation_cardinality_limit is also configured. sdk_aggregation_config = CreateAggregationConfig(stream->aggregation, sdk_aggregation_type); - if (sdk_aggregation_config) - { - // CreateAggregationConfig() may build a config purely for non-cardinality reasons (e.g. - // histogram boundaries), leaving cardinality_limit_ at its compiled-in default. Mark it - // as not explicit so it doesn't shadow a MetricReader-level fallback; the block below - // sets this back to true if aggregation_cardinality_limit is also configured. - sdk_aggregation_config->cardinality_limit_explicit_ = false; - } } // Apply aggregation_cardinality_limit from the view stream configuration @@ -2005,8 +2002,7 @@ void SdkBuilder::AddView( { if (sdk_aggregation_config) { - sdk_aggregation_config->cardinality_limit_ = stream->aggregation_cardinality_limit; - sdk_aggregation_config->cardinality_limit_explicit_ = true; + sdk_aggregation_config->SetCardinalityLimit(stream->aggregation_cardinality_limit); } else { diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index d5053c51af..985c2552b9 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -87,12 +87,12 @@ std::ostream &operator<<(std::ostream &os, return os; } -// When a view sets an explicit cardinality limit (cardinality_limit_explicit_), it wins outright -// (View > Reader > SDK default), so the raw recording storage is simply sized to that limit, -// unchanged. A non-null AggregationConfig does not by itself mean the view set an explicit -// cardinality limit: e.g. SdkBuilder::AddView() may build one purely to carry histogram -// boundaries, leaving cardinality_limit_ at its compiled-in default and -// cardinality_limit_explicit_ false. +// When a view sets an explicit cardinality limit (IsCardinalityLimitExplicit()), it wins +// outright (View > Reader > SDK default), so the raw recording storage is simply sized to that +// limit, unchanged. A non-null AggregationConfig does not by itself mean the view set an +// explicit cardinality limit: e.g. SdkBuilder::AddView() may build one purely to carry +// histogram boundaries, leaving the limit at its compiled-in default and +// IsCardinalityLimitExplicit() false. // // When a view has no explicit limit, the shared recording storage must be sized to the highest // limit configured across all MetricReaders currently attached, so no reader loses data purely @@ -107,9 +107,9 @@ std::size_t ResolveRecordingCardinalityLimit( collectors, opentelemetry::sdk::metrics::InstrumentType instrument_type) { - if (aggregation_config && aggregation_config->cardinality_limit_explicit_) + if (aggregation_config && aggregation_config->IsCardinalityLimitExplicit()) { - return aggregation_config->cardinality_limit_; + return aggregation_config->GetCardinalityLimit(); } if (collectors.empty()) { diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 48391fba65..e559c35897 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -84,8 +84,43 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, metric_data.end_ts = collection_ts; last_delta_collection_ts_ = collection_ts; - // Direct conversion of delta metrics to point data - delta_metrics->GetAllEntries( + // Recording capacity is resolved once at instrument-creation time (as the highest limit + // across readers attached then; see ResolveRecordingCardinalityLimit() in meter.cc) and is + // not shrunk retroactively, so it can exceed this single collector's own (possibly + // stricter) current limit, e.g. if this reader was attached after instrument creation with + // a lower limit than was resolved then. Re-cap through a hashmap sized to this collector's + // effective limit before emitting, so its own limit is honored on this fast path too; + // mirrors the precedence applied to the general merge path below (View > Reader > SDK + // default). + const bool has_explicit_view_limit = + aggregation_config_ && aggregation_config_->IsCardinalityLimitExplicit(); + const std::size_t effective_limit = + has_explicit_view_limit ? aggregation_config_->GetCardinalityLimit() + : collector->GetCardinalityLimit(instrument_descriptor_.type_); + + if (delta_metrics->Size() <= effective_limit) + { + // Direct conversion of delta metrics to point data + delta_metrics->GetAllEntries( + [&metric_data](const MetricAttributes &attributes, Aggregation &aggregation) { + PointDataAttributes point_data_attr; + point_data_attr.point_data = aggregation.ToPoint(); + point_data_attr.attributes = attributes; + metric_data.point_data_attr_.emplace_back(std::move(point_data_attr)); + return true; + }); + return callback(metric_data); + } + + AttributesHashMap recapped(effective_limit); + delta_metrics->GetAllEntries([&recapped, this](const MetricAttributes &attributes, + Aggregation &aggregation) { + recapped.Set(attributes, DefaultAggregation::CreateAggregation( + aggregation_type_, instrument_descriptor_, aggregation_config_) + ->Merge(aggregation)); + return true; + }); + recapped.GetAllEntries( [&metric_data](const MetricAttributes &attributes, Aggregation &aggregation) { PointDataAttributes point_data_attr; point_data_attr.point_data = aggregation.ToPoint(); @@ -116,18 +151,18 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, // Iterate over the unreporter metrics for `collector` and store result in `merged_metrics` // // When the view has no explicit cardinality limit (aggregation_config_ == nullptr, or its - // cardinality_limit_ was left at the compiled-in default for an unrelated reason such as - // histogram boundaries; see cardinality_limit_explicit_), fall back to this specific - // collector's own MetricReader-level limit rather than a flat SDK default. The underlying - // recording storage may be sized larger (to avoid losing data for readers with a higher - // limit, see SyncMetricStorage/AsyncMetricStorage), so capping merged_metrics here to this - // collector's own limit re-applies the View > Reader > SDK default precedence per reader: + // limit was left at the compiled-in default for an unrelated reason such as histogram + // boundaries; see IsCardinalityLimitExplicit()), fall back to this specific collector's own + // MetricReader-level limit rather than a flat SDK default. The underlying recording storage + // may be sized larger (to avoid losing data for readers with a higher limit, see + // SyncMetricStorage/AsyncMetricStorage), so capping merged_metrics here to this collector's + // own limit re-applies the View > Reader > SDK default precedence per reader: // AttributesHashMap::Set() below routes anything beyond this capacity into the overflow point, // without affecting what other collectors of the same storage see. const bool has_explicit_view_limit = - aggregation_config_ && aggregation_config_->cardinality_limit_explicit_; + aggregation_config_ && aggregation_config_->IsCardinalityLimitExplicit(); std::unique_ptr merged_metrics(new AttributesHashMap( - has_explicit_view_limit ? aggregation_config_->cardinality_limit_ + has_explicit_view_limit ? aggregation_config_->GetCardinalityLimit() : collector->GetCardinalityLimit(instrument_descriptor_.type_))); for (auto &agg_hashmap : unreported_list) { diff --git a/sdk/test/configuration/sdk_builder_test.cc b/sdk/test/configuration/sdk_builder_test.cc index cd1dcf8d64..e541ae9124 100644 --- a/sdk/test/configuration/sdk_builder_test.cc +++ b/sdk/test/configuration/sdk_builder_test.cc @@ -1079,7 +1079,7 @@ TEST(SdkBuilder, AddViewEmptySelectorMatchesAllSupportedInstrumentTypes) EXPECT_NE(config, nullptr); if (config != nullptr) { - EXPECT_EQ(config->cardinality_limit_, 42u); + EXPECT_EQ(config->GetCardinalityLimit(), 42u); matched++; } return true; @@ -1117,7 +1117,7 @@ TEST(SdkBuilder, AddViewHistogramCardinalityLimitOnly) if (aggregation_config) { EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kHistogram); - EXPECT_EQ(aggregation_config->cardinality_limit_, 42u); + EXPECT_EQ(aggregation_config->GetCardinalityLimit(), 42u); // Pin what users actually receive: building the aggregation from this config // must keep the SDK's default bucket boundaries, not silently collapse to a @@ -1168,7 +1168,7 @@ TEST(SdkBuilder, AddViewCounterCardinalityLimitOnly) if (aggregation_config) { EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kDefault); - EXPECT_EQ(aggregation_config->cardinality_limit_, 7u); + EXPECT_EQ(aggregation_config->GetCardinalityLimit(), 7u); } return true; }); @@ -1215,7 +1215,7 @@ TEST(SdkBuilder, AddViewHistogramBoundariesWithoutCardinalityLimitIsNotExplicit) EXPECT_NE(aggregation_config, nullptr); if (aggregation_config) { - EXPECT_FALSE(aggregation_config->cardinality_limit_explicit_); + EXPECT_FALSE(aggregation_config->IsCardinalityLimitExplicit()); } return true; }); @@ -1258,8 +1258,8 @@ TEST(SdkBuilder, AddViewHistogramBoundariesWithCardinalityLimitIsExplicit) EXPECT_NE(aggregation_config, nullptr); if (aggregation_config) { - EXPECT_TRUE(aggregation_config->cardinality_limit_explicit_); - EXPECT_EQ(aggregation_config->cardinality_limit_, 99u); + EXPECT_TRUE(aggregation_config->IsCardinalityLimitExplicit()); + EXPECT_EQ(aggregation_config->GetCardinalityLimit(), 99u); } return true; }); @@ -1297,7 +1297,7 @@ TEST(SdkBuilder, AddViewWithCardinalityLimitPreservesExplicitAggregation) if (aggregation_config) { EXPECT_EQ(aggregation_config->GetType(), metrics_sdk::AggregationType::kHistogram); - EXPECT_EQ(aggregation_config->cardinality_limit_, 42u); + EXPECT_EQ(aggregation_config->GetCardinalityLimit(), 42u); auto *histogram_config = static_cast(aggregation_config); EXPECT_EQ(histogram_config->boundaries_, (std::vector{1.0, 2.0})); diff --git a/sdk/test/metrics/metric_collector_test.cc b/sdk/test/metrics/metric_collector_test.cc index eb07b647b5..a8f9d9cc5e 100644 --- a/sdk/test/metrics/metric_collector_test.cc +++ b/sdk/test/metrics/metric_collector_test.cc @@ -720,10 +720,12 @@ TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackSingleDeltaReader) auto meter = std::shared_ptr(new Meter(context, std::move(scope))); context->AddMeter(meter); - // MockMetricReader reports kDelta aggregation temporality, and with exactly one reader - // attached this exercises TemporalMetricStorage::buildMetrics()'s single-collector-delta - // fast path. - auto reader = std::shared_ptr(new MockMetricReader()); + // MockMetricReader's default exporter reports kCumulative, not kDelta (see + // MockMetricExporter's default member initializer), so an explicit kDelta exporter is + // required here to actually exercise TemporalMetricStorage::buildMetrics()'s + // single-collector-delta fast path with exactly one reader attached. + auto reader = std::shared_ptr(new MockMetricReader( + std::unique_ptr(new MockMetricExporter(AggregationTemporality::kDelta)))); CardinalityLimits limits; limits.counter = kLimit; reader->SetCardinalityLimits(limits); @@ -750,6 +752,57 @@ TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackSingleDeltaReader) EXPECT_EQ(SumCounterValue(produced), static_cast(kUniqueAttributeSets)); } +// Regression test for a bug found during review of #4388: the single-collector delta fast path +// in TemporalMetricStorage::buildMetrics() returned recorded data directly, without re-capping +// to the collector's own limit. When recording capacity was resolved larger than this reader's +// limit (because the reader was attached after the instrument already existed, so its stricter +// limit was never part of the max-across-readers resolution at creation time), the fast path +// could emit more attribute sets than the reader's own configured limit allows. +TEST_F(MetricCollectorTest, ReaderCardinalityLimitFallbackSingleDeltaReaderAttachedLate) +{ + constexpr size_t kLimit = 3; + constexpr size_t kUniqueAttributeSets = 10; + + auto context = std::shared_ptr(new MeterContext(ViewRegistryFactory::Create())); + auto scope = + InstrumentationScope::Create("ReaderCardinalityLimitFallbackSingleDeltaReaderAttachedLate"); + auto meter = std::shared_ptr(new Meter(context, std::move(scope))); + context->AddMeter(meter); + + // No reader attached yet: recording capacity resolves to the SDK default (2000). + ASSERT_EQ(context->GetCollectors().size(), 0u); + auto counter = meter->CreateUInt64Counter("late_delta_counter"); + + // A single delta reader with a much stricter limit is attached only after the instrument + // already exists, exercising the single-collector-delta fast path with recording capacity + // (2000) larger than this reader's own limit (kLimit). An explicit kDelta exporter is + // required: MockMetricReader's default exporter reports kCumulative. + auto reader = std::shared_ptr(new MockMetricReader( + std::unique_ptr(new MockMetricExporter(AggregationTemporality::kDelta)))); + CardinalityLimits limits; + limits.counter = kLimit; + reader->SetCardinalityLimits(limits); + auto collector = AddMetricReaderToMeterContext(context, reader).lock(); + + for (size_t i = 0; i < kUniqueAttributeSets; ++i) + { + std::map attrs = {{"key", std::to_string(i)}}; + counter->Add( + 1, opentelemetry::common::KeyValueIterableView>(attrs), + opentelemetry::context::Context{}); + } + + auto produced = collector->Produce(); + bool overflow_present = false; + size_t real_points = CountRealPoints(produced, &overflow_present); + + // Even though recording capacity was resolved without this reader in the picture, its own + // limit must still be honored on the single-collector delta fast path. + EXPECT_LE(real_points, kLimit); + EXPECT_TRUE(overflow_present); + EXPECT_EQ(SumCounterValue(produced), static_cast(kUniqueAttributeSets)); +} + // Documents and pins a known limitation raised during review of #4188: recording capacity is // resolved once, as the highest limit across readers attached at instrument-creation time (see // ResolveRecordingCardinalityLimit() in meter.cc), and is not grown retroactively. A reader From 1302f76ed3d497fe22b397ff17aca95402478a9c Mon Sep 17 00:00:00 2001 From: om7057 Date: Thu, 20 Aug 2026 00:25:45 +0530 Subject: [PATCH 7/7] Fix IWYU: temporal_metric_storage.cc needs , metric_collector_test.cc needs push_metric_exporter.h temporal_metric_storage.cc's new effective_limit/AttributesHashMap recap logic uses std::size_t directly. metric_collector_test.cc now constructs MockMetricExporter/PushMetricExporter directly instead of only through common.h. --- sdk/src/metrics/state/temporal_metric_storage.cc | 1 + sdk/test/metrics/metric_collector_test.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index e559c35897..dc8c3fcf78 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include diff --git a/sdk/test/metrics/metric_collector_test.cc b/sdk/test/metrics/metric_collector_test.cc index a8f9d9cc5e..bd10c8c9ea 100644 --- a/sdk/test/metrics/metric_collector_test.cc +++ b/sdk/test/metrics/metric_collector_test.cc @@ -31,6 +31,7 @@ #include "opentelemetry/sdk/metrics/meter.h" #include "opentelemetry/sdk/metrics/meter_context.h" #include "opentelemetry/sdk/metrics/metric_reader.h" +#include "opentelemetry/sdk/metrics/push_metric_exporter.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h"