From f3c38a3063b7d79a26652d650f85f45d770fab87 Mon Sep 17 00:00:00 2001 From: PradSenn Date: Tue, 11 Mar 2025 17:45:41 -0700 Subject: [PATCH 01/29] Implementing configurable aggregation cardinality limit and collector emitting all views instead of last view Issues Cardinality of the Aggregation limit (2000) is hardcoded and no ability to configure as part of the meter/aggregation Meter stores meter-name to storage in storage_registry_. When multiple views are added, the last view overrides the previous view, and collector can only emit the last view's metrics that are added. Fixes Introducing cardinality as configurable parameter in AggregationConfig, and implementing the limit from AggregationConfig in Storage Changing the storage_registry_ to keep track of view_name to storage, instead of metric_name to storage. So when multiple views are added, different view-names will be collected by collector. Please provide a brief description of the changes here. --- .../metrics/aggregation/aggregation_config.h | 3 + sdk/include/opentelemetry/sdk/metrics/meter.h | 2 +- .../sdk/metrics/state/async_metric_storage.h | 8 +- .../sdk/metrics/state/attributes_hashmap.h | 8 +- .../sdk/metrics/state/sync_metric_storage.h | 8 +- sdk/src/metrics/meter.cc | 4 +- sdk/src/metrics/state/sync_metric_storage.cc | 2 +- .../metrics/state/temporal_metric_storage.cc | 2 +- sdk/test/metrics/cardinality_limit_test.cc | 4 +- sdk/test/metrics/sum_aggregation_test.cc | 146 ++++++++++++++++++ 10 files changed, 174 insertions(+), 13 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index f5a48d7ddb..58361855ad 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -6,6 +6,7 @@ #include #include "opentelemetry/version.h" +#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -15,6 +16,8 @@ namespace metrics class AggregationConfig { public: + AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) : cardinality_limit_(cardinality_limit) {} + size_t cardinality_limit_; virtual ~AggregationConfig() = default; }; diff --git a/sdk/include/opentelemetry/sdk/metrics/meter.h b/sdk/include/opentelemetry/sdk/metrics/meter.h index bf1b0e6c37..810312d8d7 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter.h @@ -136,7 +136,7 @@ class Meter final : public opentelemetry::metrics::Meter // meter-context. std::unique_ptr scope_; std::weak_ptr meter_context_; - // Mapping between instrument-name and Aggregation Storage. + // Mapping between view-name and Aggregation Storage. std::unordered_map> storage_registry_; std::shared_ptr observable_registry_; MeterConfig meter_config_; 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 14e9a3fbfa..975357a536 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -42,8 +42,9 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, - cumulative_hash_map_(new AttributesHashMap()), - delta_hash_map_(new AttributesHashMap()), + aggregation_config_{aggregation_config}, + cumulative_hash_map_(new AttributesHashMap(aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), + delta_hash_map_(new AttributesHashMap(aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), exemplar_reservoir_(exemplar_reservoir), @@ -126,7 +127,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora { std::lock_guard guard(hashmap_lock_); delta_metrics = std::move(delta_hash_map_); - delta_hash_map_.reset(new AttributesHashMap); + delta_hash_map_.reset(new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ : kAggregationCardinalityLimit)); } auto status = @@ -138,6 +139,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora private: InstrumentDescriptor instrument_descriptor_; AggregationType aggregation_type_; + const AggregationConfig *aggregation_config_; 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/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index ddf2a8b206..03aaca3ef9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -47,7 +47,13 @@ class AttributesHashMap public: AttributesHashMap(size_t attributes_limit = kAggregationCardinalityLimit) : attributes_limit_(attributes_limit) - {} + { + if (attributes_limit_ > kAggregationCardinalityLimit) + { + hash_map_.reserve(attributes_limit_); + } + } + Aggregation *Get(size_t hash) const { auto it = hash_map_.find(hash); 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 6e5b799e3f..b142b18730 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -63,10 +63,11 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage ExemplarFilterType exempler_filter_type, nostd::shared_ptr &&exemplar_reservoir, #endif - const AggregationConfig *aggregation_config, - size_t attributes_limit = kAggregationCardinalityLimit) + const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - attributes_hashmap_(new AttributesHashMap(attributes_limit)), + aggregation_config_(aggregation_config), + attributes_hashmap_(new AttributesHashMap( + aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), attributes_processor_(attributes_processor), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), @@ -202,6 +203,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage ExemplarFilterType exemplar_filter_type_; nostd::shared_ptr exemplar_reservoir_; #endif + const AggregationConfig *aggregation_config_; TemporalMetricStorage temporal_metric_storage_; opentelemetry::common::SpinLockMutex attribute_hashmap_lock_; }; diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index f89de3b6b0..3a7d2c3be5 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -496,7 +496,7 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( instrument_descriptor), #endif view.GetAggregationConfig())); - storage_registry_[instrument_descriptor.name_] = storage; + storage_registry_[view_instr_desc.name_] = storage; multi_storage->AddStorage(storage); return true; }); @@ -554,7 +554,7 @@ std::unique_ptr Meter::RegisterAsyncMetricStorage( instrument_descriptor), #endif view.GetAggregationConfig())); - storage_registry_[instrument_descriptor.name_] = storage; + storage_registry_[view_instr_desc.name_] = storage; static_cast(storages.get())->AddStorage(storage); return true; }); diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index ad6ea27821..2380bbb6c8 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -35,7 +35,7 @@ bool SyncMetricStorage::Collect(CollectorHandle *collector, { std::lock_guard guard(attribute_hashmap_lock_); delta_metrics = std::move(attributes_hashmap_); - attributes_hashmap_.reset(new AttributesHashMap); + attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ : kAggregationCardinalityLimit)); } return temporal_metric_storage_.buildMetrics(collector, collectors, sdk_start_ts, collection_ts, diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 1302f9d642..90e09c0523 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -99,7 +99,7 @@ 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); + std::unique_ptr merged_metrics(new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ : kAggregationCardinalityLimit)); for (auto &agg_hashmap : unreported_list) { agg_hashmap->GetAllEnteries( diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index c785eeb5d5..8e07e4ceac 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -121,6 +121,8 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati const size_t attributes_limit = 10; InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, InstrumentValueType::kLong}; + + AggregationConfig aggConfig(attributes_limit); std::unique_ptr default_attributes_processor{ new DefaultAttributesProcessor{}}; SyncMetricStorage storage(instr_desc, AggregationType::kSum, default_attributes_processor.get(), @@ -128,7 +130,7 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(), #endif - nullptr, attributes_limit); + &aggConfig); int64_t record_value = 100; // add 9 unique metric points, and 6 more above limit. diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 72783f6492..7bd46947dd 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -30,6 +30,7 @@ #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" #include "opentelemetry/sdk/metrics/view/view.h" +#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #if OPENTELEMETRY_HAVE_WORKING_REGEX @@ -144,6 +145,78 @@ TEST(HistogramToSumFilterAttributes, Double) }); } +TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + std::string instrument_unit = "ms"; + std::string instrument_name = "historgram1"; + std::string instrument_desc = "histogram metrics"; + size_t cardinality_limit = 10000; + + std::unordered_map allowedattr; + allowedattr["attr1"] = true; + std::unique_ptr attrproc{ + new opentelemetry::sdk::metrics::FilteringAttributesProcessor(allowedattr)}; + + std::shared_ptr dummy_aggregation_config{ + new opentelemetry::sdk::metrics::AggregationConfig(cardinality_limit)}; + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + std::unique_ptr view{new View("view1", "view1_description", instrument_unit, + AggregationType::kSum, dummy_aggregation_config, + std::move(attrproc))}; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kHistogram, instrument_name, instrument_unit)}; + std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; + mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); + + auto h = m->CreateDoubleHistogram(instrument_name, instrument_desc, instrument_unit); + size_t total_metrics_times = 5; + + size_t agg_repeat_count = 5; + for (size_t repeat = 0; repeat < agg_repeat_count; repeat++) + { + + for (size_t times = 0; times < total_metrics_times; times++) + { + for (size_t i = 0; i < 2 * cardinality_limit; i++) + { + std::unordered_map attr = {{"attr1", std::to_string(i)}, {"attr2", "val2"}}; + h->Record(1, attr, opentelemetry::context::Context{}); + } + } + + reader->Collect([&](ResourceMetrics &rm) { + for (const ScopeMetrics &smd : rm.scope_metric_data_) + { + for (const MetricData &md : smd.metric_data_) + { + // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow. Just making the logic generic here to succeed for n or n-1 total cardinality. + EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); + EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); + for (size_t i = 0; i < md.point_data_attr_.size(); i++) + { + EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); + if (md.point_data_attr_[i].attributes.end() != md.point_data_attr_[i].attributes.find("attr1")) + { + EXPECT_EQ(total_metrics_times * (repeat + 1), opentelemetry::nostd::get(opentelemetry::nostd::get( + md.point_data_attr_[i].point_data) + .value_)); + } else { + EXPECT_NE(md.point_data_attr_[i].attributes.end(), + md.point_data_attr_[i].attributes.find(sdk::metrics::kAttributesLimitOverflowKey)); + } + } + } + } + return true; + }); + } +} + TEST(CounterToSum, Double) { MeterProvider mp; @@ -247,6 +320,79 @@ TEST(CounterToSumFilterAttributes, Double) }); } +TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + std::string instrument_unit = "ms"; + std::string instrument_name = "counter1"; + std::string instrument_desc = "counter metrics"; + size_t cardinality_limit = 10000; + + std::unordered_map allowedattr; + allowedattr["attr1"] = true; + std::unique_ptr attrproc{ + new opentelemetry::sdk::metrics::FilteringAttributesProcessor(allowedattr)}; + + std::shared_ptr dummy_aggregation_config{ + new opentelemetry::sdk::metrics::AggregationConfig(cardinality_limit)}; + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + std::unique_ptr view{new View("view1", "view1_description", instrument_unit, + AggregationType::kSum, dummy_aggregation_config, + std::move(attrproc))}; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kCounter, instrument_name, instrument_unit)}; + std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; + mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); + + auto c = m->CreateDoubleCounter(instrument_name, instrument_desc, instrument_unit); + + size_t agg_repeat_count = 5; + for (size_t repeat = 0; repeat < agg_repeat_count; repeat++) + { + size_t total_metrics_times = 5; + + for (size_t times = 0; times < total_metrics_times; times++) + { + for (size_t i = 0; i < 2 * cardinality_limit; i++) + { + std::unordered_map attr = {{"attr1", std::to_string(i)}, {"attr2", "val2"}}; + c->Add(1, attr, opentelemetry::context::Context{}); + } + } + + reader->Collect([&](ResourceMetrics &rm) { + for (const ScopeMetrics &smd : rm.scope_metric_data_) + { + for (const MetricData &md : smd.metric_data_) + { + // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow. Just making the logic generic here to succeed for n or n-1 total cardinality. + EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); + EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); + for (int i = 0; i < md.point_data_attr_.size(); i++) + { + EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); + if (md.point_data_attr_[i].attributes.find("attr1") != md.point_data_attr_[i].attributes.end()) + { + EXPECT_EQ(total_metrics_times * (repeat + 1), opentelemetry::nostd::get(opentelemetry::nostd::get( + md.point_data_attr_[i].point_data) + .value_)); + } else { + EXPECT_NE(md.point_data_attr_[i].attributes.end(), + md.point_data_attr_[i].attributes.find(sdk::metrics::kAttributesLimitOverflowKey)); + } + } + } + } + return true; + }); + } +} + + class UpDownCounterToSumFixture : public ::testing::TestWithParam {}; From 0d80b39f413f401cea8f07d707cb6cd515d7deeb Mon Sep 17 00:00:00 2001 From: Prad Senniappan Date: Wed, 12 Mar 2025 16:37:34 +0000 Subject: [PATCH 02/29] Fixing initialization order issue --- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b142b18730..05b97a36eb 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -196,6 +196,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage private: InstrumentDescriptor instrument_descriptor_; // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) + const AggregationConfig *aggregation_config_; std::unique_ptr attributes_hashmap_; std::function()> create_default_aggregation_; const AttributesProcessor *attributes_processor_; @@ -203,7 +204,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage ExemplarFilterType exemplar_filter_type_; nostd::shared_ptr exemplar_reservoir_; #endif - const AggregationConfig *aggregation_config_; TemporalMetricStorage temporal_metric_storage_; opentelemetry::common::SpinLockMutex attribute_hashmap_lock_; }; From 671e4caccfe6fd425d5211cb16e8af492ce1fc78 Mon Sep 17 00:00:00 2001 From: Prad Senniappan Date: Fri, 14 Mar 2025 03:52:45 +0000 Subject: [PATCH 03/29] Fixing integer size mismatch error --- sdk/test/metrics/sum_aggregation_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 7bd46947dd..7980819e0a 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -372,7 +372,7 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow. Just making the logic generic here to succeed for n or n-1 total cardinality. EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); - for (int i = 0; i < md.point_data_attr_.size(); i++) + for (size_t i = 0; i < md.point_data_attr_.size(); i++) { EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); if (md.point_data_attr_[i].attributes.find("attr1") != md.point_data_attr_[i].attributes.end()) From db29ab1a2ec17fccb6b90a5ec5d5cdd17b7d5c62 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 15:08:08 -0700 Subject: [PATCH 04/29] Resolve conflict in meter.cc --- sdk/src/metrics/meter.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index 299a7b63ca..d7974d7590 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -618,17 +618,11 @@ std::unique_ptr Meter::RegisterAsyncMetricStorage( GetExemplarReservoir(view.GetAggregationType(), view.GetAggregationConfig(), view_instr_desc), #endif -<<<<<<< HEAD - view.GetAggregationConfig())); - storage_registry_[view_instr_desc.name_] = storage; - static_cast(storages.get())->AddStorage(storage); -======= view.GetAggregationConfig())); storage_registry_.insert({view_instr_desc, async_storage}); } auto async_multi_storage = static_cast(storages.get()); async_multi_storage->AddStorage(async_storage); ->>>>>>> main return true; }); if (!success) From 14aa407dc400b5daf40a594b2bd0ed9e38e637a4 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 17:02:45 -0700 Subject: [PATCH 05/29] Fix View constructor --- sdk/test/metrics/sum_aggregation_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 2c502424be..f6af3ed7ac 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -163,7 +163,7 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; mp.AddMetricReader(reader); - std::unique_ptr view{new View("view1", "view1_description", instrument_unit, + std::unique_ptr view{new View("view1", "view1_description", AggregationType::kSum, dummy_aggregation_config, std::move(attrproc))}; std::unique_ptr instrument_selector{ @@ -337,7 +337,7 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; mp.AddMetricReader(reader); - std::unique_ptr view{new View("view1", "view1_description", instrument_unit, + std::unique_ptr view{new View("view1", "view1_description", AggregationType::kSum, dummy_aggregation_config, std::move(attrproc))}; std::unique_ptr instrument_selector{ From 5f153a252c14e8b82de18d03420bf1a1ca947a99 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 17:11:35 -0700 Subject: [PATCH 06/29] Fix format --- .../metrics/aggregation/aggregation_config.h | 6 +- .../sdk/metrics/state/async_metric_storage.h | 12 +++- .../sdk/metrics/state/sync_metric_storage.h | 5 +- sdk/src/metrics/state/sync_metric_storage.cc | 4 +- .../metrics/state/temporal_metric_storage.cc | 4 +- sdk/test/metrics/sum_aggregation_test.cc | 65 +++++++++++-------- 6 files changed, 61 insertions(+), 35 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index a9231a3f47..c2bf664dc0 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -5,8 +5,8 @@ #include -#include "opentelemetry/version.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" +#include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -16,7 +16,9 @@ namespace metrics class AggregationConfig { public: - AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) : cardinality_limit_(cardinality_limit) {} + AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) + : cardinality_limit_(cardinality_limit) + {} size_t cardinality_limit_; virtual ~AggregationConfig() = default; }; 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 d5c1cebf18..1a35ddd67d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -43,8 +43,12 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, aggregation_config_{aggregation_config}, - cumulative_hash_map_(new AttributesHashMap(aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), - delta_hash_map_(new AttributesHashMap(aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), + cumulative_hash_map_(new AttributesHashMap(aggregation_config + ? aggregation_config->cardinality_limit_ + : kAggregationCardinalityLimit)), + delta_hash_map_(new AttributesHashMap(aggregation_config + ? aggregation_config->cardinality_limit_ + : kAggregationCardinalityLimit)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), exemplar_reservoir_(exemplar_reservoir), @@ -125,7 +129,9 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora { std::lock_guard guard(hashmap_lock_); delta_metrics = std::move(delta_hash_map_); - delta_hash_map_.reset(new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ : kAggregationCardinalityLimit)); + delta_hash_map_.reset(new AttributesHashMap(aggregation_config_ + ? aggregation_config_->cardinality_limit_ + : kAggregationCardinalityLimit)); } auto status = 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 e9af9822d6..d81251204e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -66,8 +66,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_config_(aggregation_config), - attributes_hashmap_(new AttributesHashMap( - aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), + attributes_hashmap_(new AttributesHashMap(aggregation_config + ? aggregation_config->cardinality_limit_ + : kAggregationCardinalityLimit)), attributes_processor_(std::move(attributes_processor)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index 2380bbb6c8..ade9b92545 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -35,7 +35,9 @@ 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_ ? aggregation_config_->cardinality_limit_ : kAggregationCardinalityLimit)); + attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_ + ? aggregation_config_->cardinality_limit_ + : kAggregationCardinalityLimit)); } return temporal_metric_storage_.buildMetrics(collector, collectors, sdk_start_ts, collection_ts, diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index d8b73c2be9..9e991f73f9 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -97,7 +97,9 @@ 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)); + std::unique_ptr merged_metrics( + new AttributesHashMap(aggregation_config_ ? aggregation_config_->cardinality_limit_ + : kAggregationCardinalityLimit)); for (auto &agg_hashmap : unreported_list) { agg_hashmap->GetAllEnteries( diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index f6af3ed7ac..36f8211d57 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -26,11 +26,11 @@ #include "opentelemetry/sdk/metrics/meter_provider.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/view/attributes_processor.h" #include "opentelemetry/sdk/metrics/view/instrument_selector.h" #include "opentelemetry/sdk/metrics/view/meter_selector.h" #include "opentelemetry/sdk/metrics/view/view.h" -#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #if OPENTELEMETRY_HAVE_WORKING_REGEX @@ -150,7 +150,7 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) std::string instrument_unit = "ms"; std::string instrument_name = "historgram1"; std::string instrument_desc = "histogram metrics"; - size_t cardinality_limit = 10000; + size_t cardinality_limit = 10000; std::unordered_map allowedattr; allowedattr["attr1"] = true; @@ -163,9 +163,8 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; mp.AddMetricReader(reader); - std::unique_ptr view{new View("view1", "view1_description", - AggregationType::kSum, dummy_aggregation_config, - std::move(attrproc))}; + std::unique_ptr view{new View("view1", "view1_description", AggregationType::kSum, + dummy_aggregation_config, std::move(attrproc))}; std::unique_ptr instrument_selector{ new InstrumentSelector(InstrumentType::kHistogram, instrument_name, instrument_unit)}; std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; @@ -182,7 +181,8 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) { for (size_t i = 0; i < 2 * cardinality_limit; i++) { - std::unordered_map attr = {{"attr1", std::to_string(i)}, {"attr2", "val2"}}; + std::unordered_map attr = {{"attr1", std::to_string(i)}, + {"attr2", "val2"}}; h->Record(1, attr, opentelemetry::context::Context{}); } } @@ -192,20 +192,27 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow. Just making the logic generic here to succeed for n or n-1 total cardinality. + // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 + // including overflow. Just making the logic generic here to succeed for n or n-1 total + // cardinality. EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); - if (md.point_data_attr_[i].attributes.end() != md.point_data_attr_[i].attributes.find("attr1")) + if (md.point_data_attr_[i].attributes.end() != + md.point_data_attr_[i].attributes.find("attr1")) + { + EXPECT_EQ(total_metrics_times * (repeat + 1), + opentelemetry::nostd::get(opentelemetry::nostd::get( + md.point_data_attr_[i].point_data) + .value_)); + } + else { - EXPECT_EQ(total_metrics_times * (repeat + 1), opentelemetry::nostd::get(opentelemetry::nostd::get( - md.point_data_attr_[i].point_data) - .value_)); - } else { EXPECT_NE(md.point_data_attr_[i].attributes.end(), - md.point_data_attr_[i].attributes.find(sdk::metrics::kAttributesLimitOverflowKey)); + md.point_data_attr_[i].attributes.find( + sdk::metrics::kAttributesLimitOverflowKey)); } } } @@ -324,7 +331,7 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) std::string instrument_unit = "ms"; std::string instrument_name = "counter1"; std::string instrument_desc = "counter metrics"; - size_t cardinality_limit = 10000; + size_t cardinality_limit = 10000; std::unordered_map allowedattr; allowedattr["attr1"] = true; @@ -337,9 +344,8 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; mp.AddMetricReader(reader); - std::unique_ptr view{new View("view1", "view1_description", - AggregationType::kSum, dummy_aggregation_config, - std::move(attrproc))}; + std::unique_ptr view{new View("view1", "view1_description", AggregationType::kSum, + dummy_aggregation_config, std::move(attrproc))}; std::unique_ptr instrument_selector{ new InstrumentSelector(InstrumentType::kCounter, instrument_name, instrument_unit)}; std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; @@ -356,7 +362,8 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) { for (size_t i = 0; i < 2 * cardinality_limit; i++) { - std::unordered_map attr = {{"attr1", std::to_string(i)}, {"attr2", "val2"}}; + std::unordered_map attr = {{"attr1", std::to_string(i)}, + {"attr2", "val2"}}; c->Add(1, attr, opentelemetry::context::Context{}); } } @@ -366,20 +373,27 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 including overflow. Just making the logic generic here to succeed for n or n-1 total cardinality. + // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 + // including overflow. Just making the logic generic here to succeed for n or n-1 total + // cardinality. EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); - if (md.point_data_attr_[i].attributes.find("attr1") != md.point_data_attr_[i].attributes.end()) + if (md.point_data_attr_[i].attributes.find("attr1") != + md.point_data_attr_[i].attributes.end()) + { + EXPECT_EQ(total_metrics_times * (repeat + 1), + opentelemetry::nostd::get(opentelemetry::nostd::get( + md.point_data_attr_[i].point_data) + .value_)); + } + else { - EXPECT_EQ(total_metrics_times * (repeat + 1), opentelemetry::nostd::get(opentelemetry::nostd::get( - md.point_data_attr_[i].point_data) - .value_)); - } else { EXPECT_NE(md.point_data_attr_[i].attributes.end(), - md.point_data_attr_[i].attributes.find(sdk::metrics::kAttributesLimitOverflowKey)); + md.point_data_attr_[i].attributes.find( + sdk::metrics::kAttributesLimitOverflowKey)); } } } @@ -389,7 +403,6 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) } } - class UpDownCounterToSumFixture : public ::testing::TestWithParam {}; From b446e6f5d0369fcb0313c90a04dbc5ea0d37da84 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 17:36:37 -0700 Subject: [PATCH 07/29] Fix iwyu --- sdk/test/metrics/async_metric_storage_test.cc | 4 +++- sdk/test/metrics/cardinality_limit_test.cc | 3 +++ sdk/test/metrics/sum_aggregation_test.cc | 2 ++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index f621379384..640e68891c 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -17,15 +17,17 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" +#include "opentelemetry/sdk/metrics/data/exemplar_data.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" +#include "opentelemetry/sdk/metrics/exemplar/filter_type.h" +#include "opentelemetry/sdk/metrics/exemplar/reservoir.h" #include "opentelemetry/sdk/metrics/export/metric_producer.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/state/async_metric_storage.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" -#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index f0482c4979..f783ce0db1 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -20,9 +20,12 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/aggregation/sum_aggregation.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" +#include "opentelemetry/sdk/metrics/exemplar/filter_type.h" +#include "opentelemetry/sdk/metrics/exemplar/reservoir.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 36f8211d57..402d5ea60d 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -2,7 +2,9 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include +#include #include #include #include From 9b34954b9dbb6483d8a74a9c6571356ffe66f6b2 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 18:27:47 -0700 Subject: [PATCH 08/29] Fix more iwyu --- sdk/src/metrics/state/sync_metric_storage.cc | 1 + sdk/test/metrics/async_metric_storage_test.cc | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index ade9b92545..a303365eaf 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -9,6 +9,7 @@ #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/metric_collector.h" diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index 640e68891c..1491354bc8 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -17,7 +17,6 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" -#include "opentelemetry/sdk/metrics/data/exemplar_data.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" #include "opentelemetry/sdk/metrics/exemplar/filter_type.h" @@ -28,6 +27,7 @@ #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" +#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" From 5186013484675c674de11eaf0abe76f11c8ba094 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 19:15:10 -0700 Subject: [PATCH 09/29] More fix on iywu --- sdk/test/metrics/async_metric_storage_test.cc | 5 ----- sdk/test/metrics/cardinality_limit_test.cc | 5 ----- 2 files changed, 10 deletions(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index 1491354bc8..ae07799de5 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -29,11 +29,6 @@ #include "opentelemetry/sdk/metrics/state/metric_collector.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" -#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW -# include "opentelemetry/sdk/metrics/exemplar/filter_type.h" -# include "opentelemetry/sdk/metrics/exemplar/reservoir.h" -#endif - using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::sdk::instrumentationscope; using namespace opentelemetry::sdk::resource; diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index f783ce0db1..493cb77de7 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -33,11 +33,6 @@ #include "opentelemetry/sdk/metrics/state/sync_metric_storage.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" -#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW -# include "opentelemetry/sdk/metrics/exemplar/filter_type.h" -# include "opentelemetry/sdk/metrics/exemplar/reservoir.h" -#endif - using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; From 969ad8530a4be682c96dab2325a41577ec7646a3 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 20:18:54 -0700 Subject: [PATCH 10/29] More iwyu --- sdk/test/metrics/async_metric_storage_test.cc | 2 -- sdk/test/metrics/cardinality_limit_test.cc | 2 -- 2 files changed, 4 deletions(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index ae07799de5..98972267d6 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -19,8 +19,6 @@ #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" -#include "opentelemetry/sdk/metrics/exemplar/filter_type.h" -#include "opentelemetry/sdk/metrics/exemplar/reservoir.h" #include "opentelemetry/sdk/metrics/export/metric_producer.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/state/async_metric_storage.h" diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 493cb77de7..e8e63fa90d 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -24,8 +24,6 @@ #include "opentelemetry/sdk/metrics/aggregation/sum_aggregation.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" -#include "opentelemetry/sdk/metrics/exemplar/filter_type.h" -#include "opentelemetry/sdk/metrics/exemplar/reservoir.h" #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" From 8a5b7a10962e6caea94dd66d1b1a14a2372a9334 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 3 Sep 2025 22:31:08 -0700 Subject: [PATCH 11/29] More fix on iwyu --- sdk/test/metrics/async_metric_storage_test.cc | 5 +++++ sdk/test/metrics/cardinality_limit_test.cc | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index 98972267d6..f621379384 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -27,6 +27,11 @@ #include "opentelemetry/sdk/metrics/state/metric_collector.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW +# include "opentelemetry/sdk/metrics/exemplar/filter_type.h" +# include "opentelemetry/sdk/metrics/exemplar/reservoir.h" +#endif + using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::sdk::instrumentationscope; using namespace opentelemetry::sdk::resource; diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index e8e63fa90d..957e17ae5a 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -31,6 +31,12 @@ #include "opentelemetry/sdk/metrics/state/sync_metric_storage.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW +# include "opentelemetry/sdk/metrics/exemplar/filter_type.h" +# include "opentelemetry/sdk/metrics/exemplar/reservoir.h" +#endif + + using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; From 6c91fbba2b10f0851e1dfde641f9759c660eabd3 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 07:54:55 -0700 Subject: [PATCH 12/29] More iwyu fix --- sdk/test/metrics/async_metric_storage_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index f621379384..b2015eb6ce 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -17,6 +17,7 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" +#include "opentelemetry/sdk/metrics/data/exemplar_data.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" #include "opentelemetry/sdk/metrics/export/metric_producer.h" @@ -25,7 +26,6 @@ #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" -#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" From 2c9305c2d0d5e2109182e2deae5cf350b5928e2b Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 07:56:39 -0700 Subject: [PATCH 13/29] Remove extra empty line --- sdk/test/metrics/cardinality_limit_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 957e17ae5a..61a47c5983 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -36,7 +36,6 @@ # include "opentelemetry/sdk/metrics/exemplar/reservoir.h" #endif - using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; From 100959c601ac79ab3d6f9382dde99039f25900e5 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 09:49:03 -0700 Subject: [PATCH 14/29] More iwyu fix --- sdk/test/metrics/async_metric_storage_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index b2015eb6ce..f621379384 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -17,7 +17,6 @@ #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" -#include "opentelemetry/sdk/metrics/data/exemplar_data.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" #include "opentelemetry/sdk/metrics/data/point_data.h" #include "opentelemetry/sdk/metrics/export/metric_producer.h" @@ -26,6 +25,7 @@ #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" +#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" From f6719f269b0a784ede6f4d73c455980a6ef2eddc Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 10:27:55 -0700 Subject: [PATCH 15/29] One more iwyu --- sdk/test/metrics/async_metric_storage_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index f621379384..d1411fcaaa 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -25,11 +25,13 @@ #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" -#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW +# include "opentelemetry/sdk/metrics/data/exemplar_data.h" # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" # include "opentelemetry/sdk/metrics/exemplar/reservoir.h" +#else +# include "opentelemetry/sdk/metrics/view/attributes_processor.h" #endif using namespace opentelemetry::sdk::metrics; From d0c4b952883ac8c7c6dc9bc1735eee0f45066253 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 10:53:30 -0700 Subject: [PATCH 16/29] Increase test timeout for valgrind --- ci/do_ci.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 2eff9cd200..71a9a82cbd 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -558,7 +558,7 @@ elif [[ "$1" == "bazel.tsan" ]]; then exit 0 elif [[ "$1" == "bazel.valgrind" ]]; then bazel $BAZEL_STARTUP_OPTIONS build $BAZEL_OPTIONS_ASYNC //... - bazel $BAZEL_STARTUP_OPTIONS test --run_under="/usr/bin/valgrind --leak-check=full --error-exitcode=1 --errors-for-leak-kinds=definite --suppressions=\"${SRC_DIR}/ci/valgrind-suppressions\"" $BAZEL_TEST_OPTIONS_ASYNC //... + bazel $BAZEL_STARTUP_OPTIONS test --test_timeout=600 --run_under="/usr/bin/valgrind --leak-check=full --error-exitcode=1 --errors-for-leak-kinds=definite --suppressions=\"${SRC_DIR}/ci/valgrind-suppressions\"" $BAZEL_TEST_OPTIONS_ASYNC //... exit 0 elif [[ "$1" == "bazel.e2e" ]]; then cd examples/e2e From d4aaf6a756e075effda8327da93185395d994a4d Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 4 Sep 2025 11:20:55 -0700 Subject: [PATCH 17/29] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b79f94cba..d3bf93576d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,10 @@ Increment the: * [BUILD] Use -dev versions in main branch [#3609](https://github.com/open-telemetry/opentelemetry-cpp/pull/3609) +* [SDK] Implementing configurable aggregation cardinality limit and collector + emitting all views instead of last view + [#3624](https://github.com/open-telemetry/opentelemetry-cpp/pull/3624) + Important changes: * [CMAKE] Upgrade CMake minimum version to 3.16 From f3ea8f00345eb20d3c62517484784b04921a5ce1 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Fri, 5 Sep 2025 09:47:14 -0700 Subject: [PATCH 18/29] Address feedback --- sdk/test/metrics/async_metric_storage_test.cc | 5 +++-- sdk/test/metrics/sum_aggregation_test.cc | 13 +++++++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index d1411fcaaa..3e17da5a26 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -194,8 +194,9 @@ TEST_P(WritableMetricStorageTestUpDownFixture, TestAggregation) } return true; }); - // subsequent recording after collection shouldn't fail - // monotonic increasing values; + // Note: When the cardinality limit is set to n, the attributes hashmap emits n-1 distinct attribute sets, + // plus an overflow bucket for additional attributes. The test logic below is made generic to succeed for either + // n or n-1 total cardinality. If this behavior is unexpected, please investigate and file an issue. int64_t get_count2 = -50; int64_t put_count2 = -70; diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 402d5ea60d..03b3138893 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -45,7 +45,7 @@ TEST(HistogramToSum, Double) MeterProvider mp; auto m = mp.GetMeter("meter1", "version1", "schema1"); std::string instrument_unit = "ms"; - std::string instrument_name = "historgram1"; + std::string instrument_name = "histogram1"; std::string instrument_desc = "histogram metrics"; std::unique_ptr exporter(new MockMetricExporter()); @@ -97,7 +97,7 @@ TEST(HistogramToSumFilterAttributes, Double) MeterProvider mp; auto m = mp.GetMeter("meter1", "version1", "schema1"); std::string instrument_unit = "ms"; - std::string instrument_name = "historgram1"; + std::string instrument_name = "histogram1"; std::string instrument_desc = "histogram metrics"; std::unordered_map allowedattr; @@ -150,7 +150,7 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) MeterProvider mp; auto m = mp.GetMeter("meter1", "version1", "schema1"); std::string instrument_unit = "ms"; - std::string instrument_name = "historgram1"; + std::string instrument_name = "histogram1"; std::string instrument_desc = "histogram metrics"; size_t cardinality_limit = 10000; @@ -375,9 +375,10 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 - // including overflow. Just making the logic generic here to succeed for n or n-1 total - // cardinality. + // When the number of unique attribute sets exceeds the cardinality limit, the implementation + // emits up to (cardinality_limit - 1) unique sets and one overflow set, resulting in a total + // of cardinality_limit sets. This test checks that the number of emitted attribute sets is + // within the expected range, accounting for the overflow behavior. EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) From d8d4fdd890b43452a632092fe8f849e7d3d69320 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Fri, 5 Sep 2025 10:37:08 -0700 Subject: [PATCH 19/29] Format code --- sdk/test/metrics/async_metric_storage_test.cc | 7 ++++--- sdk/test/metrics/sum_aggregation_test.cc | 9 +++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index 3e17da5a26..380ddd0564 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -194,9 +194,10 @@ TEST_P(WritableMetricStorageTestUpDownFixture, TestAggregation) } return true; }); - // Note: When the cardinality limit is set to n, the attributes hashmap emits n-1 distinct attribute sets, - // plus an overflow bucket for additional attributes. The test logic below is made generic to succeed for either - // n or n-1 total cardinality. If this behavior is unexpected, please investigate and file an issue. + // Note: When the cardinality limit is set to n, the attributes hashmap emits n-1 distinct + // attribute sets, plus an overflow bucket for additional attributes. The test logic below is made + // generic to succeed for either n or n-1 total cardinality. If this behavior is unexpected, + // please investigate and file an issue. int64_t get_count2 = -50; int64_t put_count2 = -70; diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 03b3138893..dd33ed7c2a 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -375,10 +375,11 @@ TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // When the number of unique attribute sets exceeds the cardinality limit, the implementation - // emits up to (cardinality_limit - 1) unique sets and one overflow set, resulting in a total - // of cardinality_limit sets. This test checks that the number of emitted attribute sets is - // within the expected range, accounting for the overflow behavior. + // When the number of unique attribute sets exceeds the cardinality limit, the + // implementation emits up to (cardinality_limit - 1) unique sets and one overflow set, + // resulting in a total of cardinality_limit sets. This test checks that the number of + // emitted attribute sets is within the expected range, accounting for the overflow + // behavior. EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) From 121d6c6be7ce012089c2ee0097c2ffa1a2a70e5c Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 8 Sep 2025 16:15:43 -0700 Subject: [PATCH 20/29] Address feedback --- CHANGELOG.md | 3 +-- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3bf93576d..a0f9e4b0dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,8 +33,7 @@ Increment the: * [BUILD] Use -dev versions in main branch [#3609](https://github.com/open-telemetry/opentelemetry-cpp/pull/3609) -* [SDK] Implementing configurable aggregation cardinality limit and collector - emitting all views instead of last view +* [SDK] Implementing configurable aggregation cardinality limit [#3624](https://github.com/open-telemetry/opentelemetry-cpp/pull/3624) Important changes: 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 d81251204e..5a3c6c33d7 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -66,7 +66,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_config_(aggregation_config), - attributes_hashmap_(new AttributesHashMap(aggregation_config + attributes_hashmap_(std::make_unique(aggregation_config ? aggregation_config->cardinality_limit_ : kAggregationCardinalityLimit)), attributes_processor_(std::move(attributes_processor)), From 9c6fb5e24ca825a5d176ca0cc4dc88afbfba3c35 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 8 Sep 2025 17:22:24 -0700 Subject: [PATCH 21/29] Address more feedback --- .../sdk/metrics/state/async_metric_storage.h | 19 +++++++++---------- .../sdk/metrics/state/sync_metric_storage.h | 9 +++++---- 2 files changed, 14 insertions(+), 14 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 1a35ddd67d..5746f7f4e9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -9,6 +9,7 @@ #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/sdk/common/attributemap_hash.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW @@ -42,13 +43,11 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, - aggregation_config_{aggregation_config}, - cumulative_hash_map_(new AttributesHashMap(aggregation_config - ? aggregation_config->cardinality_limit_ - : kAggregationCardinalityLimit)), - delta_hash_map_(new AttributesHashMap(aggregation_config - ? aggregation_config->cardinality_limit_ - : kAggregationCardinalityLimit)), + aggregation_config_{aggregation_config ? aggregation_config : &default_aggregation_config_}, + cumulative_hash_map_( + std::make_unique(aggregation_config_->cardinality_limit_)), + delta_hash_map_( + std::make_unique(aggregation_config_->cardinality_limit_)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), exemplar_reservoir_(exemplar_reservoir), @@ -129,9 +128,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora { std::lock_guard guard(hashmap_lock_); delta_metrics = std::move(delta_hash_map_); - delta_hash_map_.reset(new AttributesHashMap(aggregation_config_ - ? aggregation_config_->cardinality_limit_ - : kAggregationCardinalityLimit)); + delta_hash_map_ = + std::make_unique(aggregation_config_->cardinality_limit_); } auto status = @@ -141,6 +139,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora } private: + static inline const AggregationConfig default_aggregation_config_{}; InstrumentDescriptor instrument_descriptor_; AggregationType aggregation_type_; 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 5a3c6c33d7..2a05e7b10e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -18,6 +18,7 @@ #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" @@ -65,10 +66,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - aggregation_config_(aggregation_config), - attributes_hashmap_(std::make_unique(aggregation_config - ? aggregation_config->cardinality_limit_ - : kAggregationCardinalityLimit)), + aggregation_config_(aggregation_config ? aggregation_config : &default_aggregation_config_), + attributes_hashmap_( + std::make_unique(aggregation_config_->cardinality_limit_)), attributes_processor_(std::move(attributes_processor)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_filter_type_(exempler_filter_type), @@ -173,6 +173,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::function_ref callback) noexcept override; private: + static inline const AggregationConfig default_aggregation_config_{}; InstrumentDescriptor instrument_descriptor_; // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) const AggregationConfig *aggregation_config_; From f81f8bdf7866077934f71491a267828840fd346e Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 8 Sep 2025 19:15:22 -0700 Subject: [PATCH 22/29] Remove static inline which is C++ 17 --- .../opentelemetry/sdk/metrics/state/async_metric_storage.h | 7 +++++-- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 7 +++++-- 2 files changed, 10 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 5746f7f4e9..6f29b70b35 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -43,7 +43,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, - aggregation_config_{aggregation_config ? aggregation_config : &default_aggregation_config_}, + aggregation_config_{aggregation_config ? aggregation_config : &GetDefaultAggregationConfig()}, cumulative_hash_map_( std::make_unique(aggregation_config_->cardinality_limit_)), delta_hash_map_( @@ -139,7 +139,10 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora } private: - static inline const AggregationConfig default_aggregation_config_{}; + static const AggregationConfig& GetDefaultAggregationConfig() { + static const AggregationConfig default_config{}; + return default_config; + } InstrumentDescriptor instrument_descriptor_; AggregationType aggregation_type_; 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 2a05e7b10e..91dc98d8d1 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -66,7 +66,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - aggregation_config_(aggregation_config ? aggregation_config : &default_aggregation_config_), + aggregation_config_(aggregation_config ? aggregation_config : &GetDefaultAggregationConfig()), attributes_hashmap_( std::make_unique(aggregation_config_->cardinality_limit_)), attributes_processor_(std::move(attributes_processor)), @@ -173,7 +173,10 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::function_ref callback) noexcept override; private: - static inline const AggregationConfig default_aggregation_config_{}; + static const AggregationConfig& GetDefaultAggregationConfig() { + static const AggregationConfig default_config{}; + return default_config; + } InstrumentDescriptor instrument_descriptor_; // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) const AggregationConfig *aggregation_config_; From 93b99145824b5945a85a1c5ae1b9b79f06dc840f Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Mon, 8 Sep 2025 19:20:09 -0700 Subject: [PATCH 23/29] Reformat --- .../opentelemetry/sdk/metrics/state/async_metric_storage.h | 6 ++++-- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 6 ++++-- 2 files changed, 8 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 6f29b70b35..e7b9e4c215 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -43,7 +43,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, - aggregation_config_{aggregation_config ? aggregation_config : &GetDefaultAggregationConfig()}, + aggregation_config_{aggregation_config ? aggregation_config + : &GetDefaultAggregationConfig()}, cumulative_hash_map_( std::make_unique(aggregation_config_->cardinality_limit_)), delta_hash_map_( @@ -139,7 +140,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora } private: - static const AggregationConfig& GetDefaultAggregationConfig() { + static const AggregationConfig &GetDefaultAggregationConfig() + { static const AggregationConfig default_config{}; return default_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 91dc98d8d1..19753314c5 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -66,7 +66,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - aggregation_config_(aggregation_config ? aggregation_config : &GetDefaultAggregationConfig()), + aggregation_config_(aggregation_config ? aggregation_config + : &GetDefaultAggregationConfig()), attributes_hashmap_( std::make_unique(aggregation_config_->cardinality_limit_)), attributes_processor_(std::move(attributes_processor)), @@ -173,7 +174,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::function_ref callback) noexcept override; private: - static const AggregationConfig& GetDefaultAggregationConfig() { + static const AggregationConfig &GetDefaultAggregationConfig() + { static const AggregationConfig default_config{}; return default_config; } From 7dde674557c6746f7719b2d9ee2bdfe2e4ac79df Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 10 Sep 2025 09:57:24 -0700 Subject: [PATCH 24/29] Move GetOrDefault to AggregationConfig --- .../sdk/metrics/aggregation/aggregation_config.h | 11 +++++++++++ .../sdk/metrics/state/async_metric_storage.h | 8 +------- .../sdk/metrics/state/sync_metric_storage.h | 8 +------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index c2bf664dc0..2effa4933c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -19,6 +19,17 @@ class AggregationConfig AggregationConfig(size_t cardinality_limit = kAggregationCardinalityLimit) : cardinality_limit_(cardinality_limit) {} + + static const AggregationConfig *GetOrDefault(const AggregationConfig *config) + { + if (config) + { + return config; + } + static const AggregationConfig default_config{}; + return &default_config; + } + size_t cardinality_limit_; virtual ~AggregationConfig() = default; }; 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 e7b9e4c215..968de7aa82 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -43,8 +43,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, - aggregation_config_{aggregation_config ? aggregation_config - : &GetDefaultAggregationConfig()}, + aggregation_config_{AggregationConfig::GetOrDefault(aggregation_config)}, cumulative_hash_map_( std::make_unique(aggregation_config_->cardinality_limit_)), delta_hash_map_( @@ -140,11 +139,6 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora } private: - static const AggregationConfig &GetDefaultAggregationConfig() - { - static const AggregationConfig default_config{}; - return default_config; - } InstrumentDescriptor instrument_descriptor_; AggregationType aggregation_type_; 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 19753314c5..f517375fd8 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -66,8 +66,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - aggregation_config_(aggregation_config ? aggregation_config - : &GetDefaultAggregationConfig()), + aggregation_config_(AggregationConfig::GetOrDefault(aggregation_config)), attributes_hashmap_( std::make_unique(aggregation_config_->cardinality_limit_)), attributes_processor_(std::move(attributes_processor)), @@ -174,11 +173,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage nostd::function_ref callback) noexcept override; private: - static const AggregationConfig &GetDefaultAggregationConfig() - { - static const AggregationConfig default_config{}; - return default_config; - } InstrumentDescriptor instrument_descriptor_; // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) const AggregationConfig *aggregation_config_; From dc5661c1efa91292a77d9d6b897575bea9763933 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 10 Sep 2025 13:06:31 -0700 Subject: [PATCH 25/29] Address feedback --- sdk/test/metrics/sum_aggregation_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index dd33ed7c2a..853cfa02ef 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -194,11 +194,11 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 - // including overflow. Just making the logic generic here to succeed for n or n-1 total - // cardinality. - EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); - EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); + // When the number of unique attribute sets exceeds the cardinality limit, the + // implementation emits up to (cardinality_limit - 1) unique sets and one overflow set, + // resulting in a total of cardinality_limit sets. This test checks that the number of + // emitted attribute sets is within the expected range, accounting for the overflow behavior. + EXPECT_EQ(cardinality_limit, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); From df2c45113fde827ff0bb136858421e2d263d81c4 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 10 Sep 2025 13:11:04 -0700 Subject: [PATCH 26/29] Run format --- sdk/test/metrics/sum_aggregation_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 853cfa02ef..42a12de716 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -197,7 +197,8 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) // When the number of unique attribute sets exceeds the cardinality limit, the // implementation emits up to (cardinality_limit - 1) unique sets and one overflow set, // resulting in a total of cardinality_limit sets. This test checks that the number of - // emitted attribute sets is within the expected range, accounting for the overflow behavior. + // emitted attribute sets is within the expected range, accounting for the overflow + // behavior. EXPECT_EQ(cardinality_limit, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { From 49eac751c57d39814d6f18299f58e36ce10950ba Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 10 Sep 2025 17:21:39 -0700 Subject: [PATCH 27/29] Revert "Run format" This reverts commit df2c45113fde827ff0bb136858421e2d263d81c4. --- sdk/test/metrics/sum_aggregation_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 42a12de716..853cfa02ef 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -197,8 +197,7 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) // When the number of unique attribute sets exceeds the cardinality limit, the // implementation emits up to (cardinality_limit - 1) unique sets and one overflow set, // resulting in a total of cardinality_limit sets. This test checks that the number of - // emitted attribute sets is within the expected range, accounting for the overflow - // behavior. + // emitted attribute sets is within the expected range, accounting for the overflow behavior. EXPECT_EQ(cardinality_limit, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { From 02fc44ec5a2a8899c61ed8fb75c5bb1e4daa4060 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Wed, 10 Sep 2025 17:21:53 -0700 Subject: [PATCH 28/29] Revert "Address feedback" This reverts commit dc5661c1efa91292a77d9d6b897575bea9763933. --- sdk/test/metrics/sum_aggregation_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index 853cfa02ef..dd33ed7c2a 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -194,11 +194,11 @@ TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) { for (const MetricData &md : smd.metric_data_) { - // When the number of unique attribute sets exceeds the cardinality limit, the - // implementation emits up to (cardinality_limit - 1) unique sets and one overflow set, - // resulting in a total of cardinality_limit sets. This test checks that the number of - // emitted attribute sets is within the expected range, accounting for the overflow behavior. - EXPECT_EQ(cardinality_limit, md.point_data_attr_.size()); + // Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 + // including overflow. Just making the logic generic here to succeed for n or n-1 total + // cardinality. + EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); + EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); for (size_t i = 0; i < md.point_data_attr_.size(); i++) { EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); From a42ae4373819817e49dfd920f0d8d89f20e7b8c4 Mon Sep 17 00:00:00 2001 From: Tom Tan Date: Thu, 11 Sep 2025 00:13:44 -0700 Subject: [PATCH 29/29] Address feedback --- sdk/src/metrics/state/sync_metric_storage.cc | 4 +--- sdk/test/metrics/sum_aggregation_test.cc | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/sdk/src/metrics/state/sync_metric_storage.cc b/sdk/src/metrics/state/sync_metric_storage.cc index a303365eaf..3844d2b05d 100644 --- a/sdk/src/metrics/state/sync_metric_storage.cc +++ b/sdk/src/metrics/state/sync_metric_storage.cc @@ -36,9 +36,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_ - ? aggregation_config_->cardinality_limit_ - : kAggregationCardinalityLimit)); + attributes_hashmap_.reset(new AttributesHashMap(aggregation_config_->cardinality_limit_)); } return temporal_metric_storage_.buildMetrics(collector, collectors, sdk_start_ts, collection_ts, diff --git a/sdk/test/metrics/sum_aggregation_test.cc b/sdk/test/metrics/sum_aggregation_test.cc index dd33ed7c2a..7722db1afa 100644 --- a/sdk/test/metrics/sum_aggregation_test.cc +++ b/sdk/test/metrics/sum_aggregation_test.cc @@ -145,7 +145,7 @@ TEST(HistogramToSumFilterAttributes, Double) }); } -TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) +TEST(HistogramToSumFilterAttributesWithCardinalityLimit, Double) { MeterProvider mp; auto m = mp.GetMeter("meter1", "version1", "schema1");