Merge pull request #508 from open-telemetry/main

[METRICS SDK] Fix copying overflow attributes in attribute hashmap (#…

Author: malkia

CHANGELOG.md

@@ -36,6 +36,9 @@ Increment the:
 * [SDK] Implementing configurable aggregation cardinality limit
   [#3624](https://github.com/open-telemetry/opentelemetry-cpp/pull/3624)
 
+* [SDK] Fix copying overflow attributes in metric AttributesHashMap
+  [#3651](https://github.com/open-telemetry/opentelemetry-cpp/pull/3651)
+
 Important changes:
 
 * [CMAKE] Upgrade CMake minimum version to 3.16

sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h

@@ -95,7 +95,7 @@ class AttributesHashMapWithCustomHash
       return it->second.get();
     }
 
-    if (IsOverflowAttributes())
+    if (IsOverflowAttributes(attr))
     {
       return GetOrSetOveflowAttributes(aggregation_callback);
     }
@@ -114,7 +114,7 @@ class AttributesHashMapWithCustomHash
       return it->second.get();
     }
 
-    if (IsOverflowAttributes())
+    if (IsOverflowAttributes(attributes))
     {
       return GetOrSetOveflowAttributes(aggregation_callback);
     }
@@ -133,7 +133,7 @@ class AttributesHashMapWithCustomHash
       return it->second.get();
     }
 
-    if (IsOverflowAttributes())
+    if (IsOverflowAttributes(attributes))
     {
       return GetOrSetOveflowAttributes(aggregation_callback);
     }
@@ -158,7 +158,7 @@ class AttributesHashMapWithCustomHash
     {
       it->second = std::move(aggr);
     }
-    else if (IsOverflowAttributes())
+    else if (IsOverflowAttributes(attributes))
     {
       hash_map_[kOverflowAttributes] = std::move(aggr);
     }
@@ -175,7 +175,7 @@ class AttributesHashMapWithCustomHash
     {
       it->second = std::move(aggr);
     }
-    else if (IsOverflowAttributes())
+    else if (IsOverflowAttributes(attributes))
     {
       hash_map_[kOverflowAttributes] = std::move(aggr);
     }
@@ -234,7 +234,27 @@ class AttributesHashMapWithCustomHash
     return result.first->second.get();
   }
 
-  bool IsOverflowAttributes() const { return (hash_map_.size() + 1 >= attributes_limit_); }
+  bool IsOverflowAttributes(const MetricAttributes &attributes) const
+  {
+    // If the incoming attributes are exactly the overflow sentinel, route
+    // directly to the overflow entry.
+    if (attributes == kOverflowAttributes)
+    {
+      return true;
+    }
+    // Determine if overflow entry already exists.
+    bool has_overflow = (hash_map_.find(kOverflowAttributes) != hash_map_.end());
+    // If overflow already present, total size already includes it; trigger overflow
+    // when current size (including overflow) is >= limit.
+    if (has_overflow)
+    {
+      return hash_map_.size() >= attributes_limit_;
+    }
+    // If overflow not present yet, simulate adding a new distinct key. If that
+    // would exceed the limit, we redirect to overflow instead of creating a
+    // new real attribute entry.
+    return (hash_map_.size() + 1) >= attributes_limit_;
+  }
 };
 
 using AttributesHashMap = AttributesHashMapWithCustomHash<>;

sdk/test/metrics/attributes_hashmap_test.cc

@@ -158,3 +158,67 @@ TEST(AttributesHashMap, HashConsistencyAcrossStringTypes)
   EXPECT_EQ(hash_c_str, hash_std_str_view);
 #endif
 }
+
+TEST(AttributesHashMap, OverflowCardinalityLimitBehavior)
+{
+  // Configure a very small limit to exercise overflow logic easily.
+  const size_t limit = 4;  // real attributes limit
+  AttributesHashMapWithCustomHash<> map(limit);
+
+  // We expect to be able to insert exactly 'limit' distinct real attribute sets.
+  // After that, further distinct attributes should route to the overflow bucket,
+  // which should appear only once regardless of how many additional unique sets arrive.
+
+  // Insert distinct attributes up to the limit.
+  for (size_t i = 0; i < limit; ++i)
+  {
+    MetricAttributes attr = {{"k", std::to_string(i)}};
+    map.GetOrSetDefault(attr, []() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
+        ->Aggregate(static_cast<int64_t>(1));
+  }
+
+  // Size should be exactly 'limit' (no overflow yet)
+  EXPECT_EQ(map.Size(), limit);
+
+  // Insert one more distinct attribute; this should not increase the real attributes count
+  MetricAttributes overflow_trigger = {{"k", "overflow"}};
+  map.GetOrSetDefault(overflow_trigger,
+                      []() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
+      ->Aggregate(static_cast<int64_t>(1));
+
+  EXPECT_EQ(map.Size(), limit);
+
+  // Insert several more unique attributes - size must remain constant (limit)
+  for (size_t i = 0; i < limit - 1; ++i)
+  {
+    MetricAttributes extra_attr = {{"k", std::string("extra") + std::to_string(i)}};
+    map.GetOrSetDefault(extra_attr,
+                        []() { return std::unique_ptr<Aggregation>(new DropAggregation()); })
+        ->Aggregate(static_cast<int64_t>(1));
+  }
+  EXPECT_EQ(map.Size(), limit);
+
+  // Ensure overflow key was actually created and accessible via Get
+  EXPECT_NE(map.Get(kOverflowAttributes), nullptr);
+
+  // Ensure original real attributes still present
+  for (size_t i = 0; i < limit - 1; ++i)
+  {
+    MetricAttributes attr = {{"k", std::to_string(i)}};
+    EXPECT_NE(map.Get(attr), nullptr);
+  }
+
+  // Copy the hash map to a new map in non-determistic order and verify all entries are present
+  AttributesHashMapWithCustomHash<> map_copy(limit);
+  map.GetAllEnteries([&map_copy](const MetricAttributes &attributes, Aggregation &) {
+    map_copy.Set(attributes, std::unique_ptr<Aggregation>(new DropAggregation()));
+    return true;
+  });
+  EXPECT_EQ(map_copy.Size(), map.Size());
+  EXPECT_NE(map_copy.Get(kOverflowAttributes), nullptr);
+  for (size_t i = 0; i < limit - 1; ++i)
+  {
+    MetricAttributes attr = {{"k", std::to_string(i)}};
+    EXPECT_NE(map_copy.Get(attr), nullptr);
+  }
+}