From d8ed5c3a7dd655abf4620287cff8036164e818e9 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 9 Nov 2023 09:36:34 -0800 Subject: [PATCH] fix hasmap overflow size --- .../sdk/metrics/state/attributes_hashmap.h | 20 ++++++++++--------- sdk/test/metrics/cardinality_limit_test.cc | 18 ++++++++--------- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index a2af5a5a54..85009a9fcb 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -23,12 +23,12 @@ namespace metrics using opentelemetry::sdk::common::OrderedAttributeMap; -constexpr uint16_t kAggregationCardinalityLimit = 2000; +constexpr size_t kAggregationCardinalityLimit = 2000; const std::string kAggregationCardinalityLimitOverflowError = "Maximum data points for metric stream exceeded. Entry added to overflow"; -const std::string kAttributesLimitOverflowKey = "otel.metrics.overflow"; -const std::string kAttributesLimitOverflowValue = "true"; -const size_t kOverflowAttributesHash = common::GetHashForAttributeMap( +const std::string kAttributesLimitOverflowKey = "otel.metrics.overflow"; +const bool kAttributesLimitOverflowValue = true; +const size_t kOverflowAttributesHash = common::GetHashForAttributeMap( {{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}); // precalculated for optimization @@ -78,7 +78,7 @@ class AttributesHashMap return it->second.second.get(); } - if (hash_map_.size() >= attributes_limit_) + if (IsOverflowAttributes()) { return GetOrSetOveflowAttributes(aggregation_callback); } @@ -98,7 +98,7 @@ class AttributesHashMap return it->second.second.get(); } - if (hash_map_.size() >= attributes_limit_) + if (IsOverflowAttributes()) { return GetOrSetOveflowAttributes(aggregation_callback); } @@ -118,7 +118,7 @@ class AttributesHashMap return it->second.second.get(); } - if (hash_map_.size() >= attributes_limit_) + if (IsOverflowAttributes()) { return GetOrSetOveflowAttributes(aggregation_callback); } @@ -141,7 +141,7 @@ class AttributesHashMap { it->second.second = std::move(aggr); } - else if (hash_map_.size() >= attributes_limit_) + else if (IsOverflowAttributes()) { hash_map_[kOverflowAttributesHash] = { MetricAttributes{{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}, @@ -161,7 +161,7 @@ class AttributesHashMap { it->second.second = std::move(aggr); } - else if (hash_map_.size() >= attributes_limit_) + else if (IsOverflowAttributes()) { hash_map_[kOverflowAttributesHash] = { MetricAttributes{{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}, @@ -218,6 +218,8 @@ class AttributesHashMap hash_map_[kOverflowAttributesHash] = {attr, std::move(agg)}; return hash_map_[kOverflowAttributesHash].second.get(); } + + bool IsOverflowAttributes() const { return (hash_map_.size() + 1 >= attributes_limit_); } }; } // namespace metrics diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 834e87ef56..d290f889b5 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -22,7 +22,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) []() -> std::unique_ptr { return std::unique_ptr(new LongSumAggregation(true)); }; - // add 10 unique metric points. + // add 10 unique metric points. 9 should be added to hashmap, 10th should be overflow. long record_value = 100; for (auto i = 0; i < 10; i++) { @@ -33,7 +33,8 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) ->Aggregate(record_value); } EXPECT_EQ(hash_map.Size(), 10); - // add 5 unique metric points above limit, they should get consolidated as single metric point. + // add 5 unique metric points above limit, they all should get consolidated as single + // overflowmetric point. for (auto i = 10; i < 15; i++) { OrderedAttributeMap attributes = {{"key", std::to_string(i)}}; @@ -42,7 +43,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) hash_map.GetOrSetDefault(attributes, aggregation_callback, hash)) ->Aggregate(record_value); } - EXPECT_EQ(hash_map.Size(), 11); // only one more metric point should be added as overflow. + EXPECT_EQ(hash_map.Size(), 10); // only one more metric point should be added as overflow. // get the overflow metric point auto agg = hash_map.GetOrSetDefault( OrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), @@ -50,7 +51,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) EXPECT_NE(agg, nullptr); auto sum_agg = static_cast(agg); EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), - record_value * 5); + record_value * 6); // 1 from previous 10, 5 from current 5. } class WritableMetricStorageCardinalityLimitTestFixture @@ -69,7 +70,7 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati ExemplarReservoir::GetNoExemplarReservoir(), nullptr, attributes_limit); long record_value = 100; - // add 10 unique metric points, and 5 more above limit. + // add 9 unique metric points, and 6 more above limit. for (auto i = 0; i < 15; i++) { std::map attributes = {{"key", std::to_string(i)}}; @@ -91,16 +92,15 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati { const auto &data = opentelemetry::nostd::get(data_attr.point_data); count_attributes++; - if (opentelemetry::nostd::get(data_attr.attributes.begin()->second) == - kAttributesLimitOverflowValue) + if (data_attr.attributes.begin()->first == kAttributesLimitOverflowKey) { - EXPECT_EQ(nostd::get(data.value_), record_value * 5); + EXPECT_EQ(nostd::get(data.value_), record_value * 6); overflow_present = true; } } return true; }); - EXPECT_EQ(count_attributes, attributes_limit + 1); // +1 for overflow metric point. + EXPECT_EQ(count_attributes, attributes_limit); EXPECT_EQ(overflow_present, true); } INSTANTIATE_TEST_SUITE_P(All,