fix hasmap overflow size

open-telemetry · Nov 9, 2023 · d8ed5c3 · d8ed5c3
1 parent 9370c7b
commit d8ed5c3
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 18 deletions.
diff --git 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
@@ -22,7 +22,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests)
       []() -> std::unique_ptr<Aggregation> {
     return std::unique_ptr<Aggregation>(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,15 +43,15 @@ 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}}),
       aggregation_callback, kOverflowAttributesHash);
   EXPECT_NE(agg, nullptr);
   auto sum_agg = static_cast<LongSumAggregation *>(agg);
   EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(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<std::string, std::string> attributes = {{"key", std::to_string(i)}};
@@ -91,16 +92,15 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati
         {
           const auto &data = opentelemetry::nostd::get<SumPointData>(data_attr.point_data);
           count_attributes++;
-          if (opentelemetry::nostd::get<std::string>(data_attr.attributes.begin()->second) ==
-              kAttributesLimitOverflowValue)
+          if (data_attr.attributes.begin()->first == kAttributesLimitOverflowKey)
           {
-            EXPECT_EQ(nostd::get<int64_t>(data.value_), record_value * 5);
+            EXPECT_EQ(nostd::get<int64_t>(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,