From 5d688781ad6881d630f7ebd13c736296b5ebd17c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 2 Jul 2025 01:18:18 +0000 Subject: [PATCH 1/4] Initial plan From b26221c7693e6b5e509c9602c79f0fb0cbdb7009 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 2 Jul 2025 01:28:40 +0000 Subject: [PATCH 2/4] Add configurable cardinality limit support to View class Co-authored-by: lalitb <1196320+lalitb@users.noreply.github.com> --- .../opentelemetry/sdk/metrics/metric_reader.h | 13 +++ .../opentelemetry/sdk/metrics/view/view.h | 17 +++- sdk/src/metrics/meter.cc | 8 +- sdk/test/metrics/cardinality_limit_test.cc | 95 +++++++++++++++++++ 4 files changed, 130 insertions(+), 3 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h index 30d5e60823..34eac90f3f 100644 --- a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h @@ -41,6 +41,19 @@ class MetricReader virtual AggregationTemporality GetAggregationTemporality( InstrumentType instrument_type) const noexcept = 0; + /** + * Get the default cardinality limit for given Instrument Type for this reader. + * + * @param instrument_type The instrument type to get the cardinality limit for + * @return The cardinality limit, or 0 if no limit is set + */ + virtual size_t GetDefaultCardinalityLimit(InstrumentType instrument_type) const noexcept + { + // Default implementation returns no limit + (void)instrument_type; + return 0; + } + /** * Shutdown the metric reader. */ diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index e1c6237b36..391b3bfafa 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -31,13 +31,15 @@ class View std::shared_ptr aggregation_config = nullptr, std::unique_ptr attributes_processor = std::unique_ptr( - new opentelemetry::sdk::metrics::DefaultAttributesProcessor())) + new opentelemetry::sdk::metrics::DefaultAttributesProcessor()), + size_t aggregation_cardinality_limit = 0) : name_(name), description_(description), unit_(unit), aggregation_type_{aggregation_type}, aggregation_config_{aggregation_config}, - attributes_processor_{std::move(attributes_processor)} + attributes_processor_{std::move(attributes_processor)}, + aggregation_cardinality_limit_{aggregation_cardinality_limit} {} virtual ~View() = default; @@ -59,6 +61,16 @@ class View return attributes_processor_; } + virtual size_t GetAggregationCardinalityLimit() const noexcept + { + return aggregation_cardinality_limit_; + } + + virtual bool HasAggregationCardinalityLimit() const noexcept + { + return aggregation_cardinality_limit_ > 0; + } + private: std::string name_; std::string description_; @@ -66,6 +78,7 @@ class View AggregationType aggregation_type_; std::shared_ptr aggregation_config_; std::shared_ptr attributes_processor_; + size_t aggregation_cardinality_limit_; }; } // namespace metrics } // namespace sdk diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index d7974d7590..6b1fa0a839 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -538,6 +538,12 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( else { WarnOnDuplicateInstrument(GetInstrumentationScope(), storage_registry_, view_instr_desc); + // Calculate cardinality limit based on specification priority: + // 1. View-specific cardinality limit (if set) + // 2. Default value of 2000 + size_t cardinality_limit = view.HasAggregationCardinalityLimit() + ? view.GetAggregationCardinalityLimit() + : kAggregationCardinalityLimit; sync_storage = std::shared_ptr(new SyncMetricStorage( view_instr_desc, view.GetAggregationType(), view.GetAttributesProcessor(), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW @@ -545,7 +551,7 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( GetExemplarReservoir(view.GetAggregationType(), view.GetAggregationConfig(), view_instr_desc), #endif - view.GetAggregationConfig())); + view.GetAggregationConfig(), cardinality_limit)); storage_registry_.insert({view_instr_desc, sync_storage}); } auto sync_multi_storage = static_cast(storages.get()); diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 45e15200c3..b5ecd6a6ac 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -29,6 +29,7 @@ #include "opentelemetry/sdk/metrics/state/metric_collector.h" #include "opentelemetry/sdk/metrics/state/sync_metric_storage.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" +#include "opentelemetry/sdk/metrics/view/view.h" #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW # include "opentelemetry/sdk/metrics/exemplar/filter_type.h" @@ -39,6 +40,22 @@ using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; +TEST(CardinalityLimit, ViewCardinalityLimitConfiguration) +{ + // Test View without cardinality limit + View view_no_limit("test_view_no_limit"); + EXPECT_FALSE(view_no_limit.HasAggregationCardinalityLimit()); + EXPECT_EQ(view_no_limit.GetAggregationCardinalityLimit(), 0); + + // Test View with cardinality limit + View view_with_limit("test_view_with_limit", "", "", AggregationType::kDefault, nullptr, + std::unique_ptr( + new opentelemetry::sdk::metrics::DefaultAttributesProcessor()), + 500); + EXPECT_TRUE(view_with_limit.HasAggregationCardinalityLimit()); + EXPECT_EQ(view_with_limit.GetAggregationCardinalityLimit(), 500); +} + TEST(CardinalityLimit, AttributesHashMapBasicTests) { AttributesHashMap hash_map(10); @@ -156,3 +173,81 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati INSTANTIATE_TEST_SUITE_P(All, WritableMetricStorageCardinalityLimitTestFixture, ::testing::Values(AggregationTemporality::kDelta)); + +TEST(CardinalityLimit, SyncMetricStorageWithViewCardinalityLimit) +{ + auto sdk_start_ts = std::chrono::system_clock::now(); + InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, + InstrumentValueType::kLong}; + std::shared_ptr default_attributes_processor{ + new DefaultAttributesProcessor{}}; + + // Create a view with a cardinality limit of 5 + View view_with_limit("test_view", "", "", AggregationType::kSum, nullptr, + std::unique_ptr( + new opentelemetry::sdk::metrics::DefaultAttributesProcessor()), + 5); + + // Test that the view has the cardinality limit + EXPECT_TRUE(view_with_limit.HasAggregationCardinalityLimit()); + EXPECT_EQ(view_with_limit.GetAggregationCardinalityLimit(), 5); + + // Create SyncMetricStorage using the cardinality limit from the view + size_t cardinality_limit = view_with_limit.HasAggregationCardinalityLimit() + ? view_with_limit.GetAggregationCardinalityLimit() + : kAggregationCardinalityLimit; + SyncMetricStorage storage(instr_desc, AggregationType::kSum, default_attributes_processor, +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + ExemplarFilterType::kAlwaysOff, + ExemplarReservoir::GetNoExemplarReservoir(), +#endif + nullptr, cardinality_limit); + + int64_t record_value = 100; + // Add 5 unique metric points (should all fit within limit) + for (auto i = 0; i < 5; i++) + { + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(record_value, + KeyValueIterableView>(attributes), + opentelemetry::context::Context{}); + } + + // Add 3 more unique metric points (should trigger overflow behavior) + for (auto i = 5; i < 8; i++) + { + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(record_value, + KeyValueIterableView>(attributes), + opentelemetry::context::Context{}); + } + + AggregationTemporality temporality = AggregationTemporality::kDelta; + std::shared_ptr collector(new MockCollectorHandle(temporality)); + std::vector> collectors; + collectors.push_back(collector); + auto collection_ts = std::chrono::system_clock::now(); + size_t count_attributes = 0; + bool overflow_present = false; + + storage.Collect( + collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData &metric_data) { + for (const auto &data_attr : metric_data.point_data_attr_) + { + count_attributes++; + if (data_attr.attributes.begin()->first == kAttributesLimitOverflowKey) + { + // The overflow attribute should contain the aggregated values from the 3 excess metrics + const auto &data = opentelemetry::nostd::get(data_attr.point_data); + EXPECT_EQ(nostd::get(data.value_), record_value * 3); + overflow_present = true; + } + } + return true; + }); + + // We should have exactly 5 attributes (the cardinality limit) + EXPECT_EQ(count_attributes, 5); + // And there should be an overflow attribute + EXPECT_TRUE(overflow_present); +} From 804fa54102c28a3f7010909d6a4278e76c6ad015 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 2 Jul 2025 07:09:28 +0000 Subject: [PATCH 3/4] Fix cardinality limit test expectation to match OpenTelemetry specification Co-authored-by: lalitb <1196320+lalitb@users.noreply.github.com> --- sdk/test/metrics/cardinality_limit_test.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index b5ecd6a6ac..28c735cb54 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -205,6 +205,7 @@ TEST(CardinalityLimit, SyncMetricStorageWithViewCardinalityLimit) int64_t record_value = 100; // Add 5 unique metric points (should all fit within limit) + // With cardinality limit 5: first 4 get individual metric points for (auto i = 0; i < 5; i++) { std::map attributes = {{"key", std::to_string(i)}}; @@ -214,6 +215,7 @@ TEST(CardinalityLimit, SyncMetricStorageWithViewCardinalityLimit) } // Add 3 more unique metric points (should trigger overflow behavior) + // These will be aggregated with the 5th metric point into overflow bucket for (auto i = 5; i < 8; i++) { std::map attributes = {{"key", std::to_string(i)}}; @@ -237,9 +239,10 @@ TEST(CardinalityLimit, SyncMetricStorageWithViewCardinalityLimit) count_attributes++; if (data_attr.attributes.begin()->first == kAttributesLimitOverflowKey) { - // The overflow attribute should contain the aggregated values from the 3 excess metrics + // The overflow attribute should contain the aggregated values from the 4 excess metrics + // With cardinality limit 5: first 4 get individual points, remaining 4 go to overflow const auto &data = opentelemetry::nostd::get(data_attr.point_data); - EXPECT_EQ(nostd::get(data.value_), record_value * 3); + EXPECT_EQ(nostd::get(data.value_), record_value * 4); overflow_present = true; } } From e5393a7b0bf87eaf2bb592691e1c90aa236add9b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 2 Jul 2025 16:45:53 +0000 Subject: [PATCH 4/4] Fix IWYU warnings by adding missing includes Co-authored-by: lalitb <1196320+lalitb@users.noreply.github.com> --- sdk/include/opentelemetry/sdk/metrics/metric_reader.h | 1 + sdk/src/metrics/meter.cc | 2 ++ 2 files changed, 3 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h index 34eac90f3f..63b36cfc92 100644 --- a/sdk/include/opentelemetry/sdk/metrics/metric_reader.h +++ b/sdk/include/opentelemetry/sdk/metrics/metric_reader.h @@ -3,6 +3,7 @@ #pragma once +#include #include #include diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index 6b1fa0a839..a62a03d0e1 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include #include #include @@ -29,6 +30,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"