[Metrics SDK] Fix hash calculation for nostd::string (#2999)

* add test to validate cardinaity limit * format * warning * Format * maint mode CI * modify test to reproduce the issue * Format * remove vscode settings * remove redundant test * remove unused code * fix to calculate hash on string_view * remove iostream * template specialization for const char *, and varous string type tests * unused variable warning * more warnings * format * fix use-after-stack-scope * format * another try * format --------- Co-authored-by: Tom Tan <Tom.Tan@microsoft.com>
open-telemetry · Jul 17, 2024 · d8ae09e · d8ae09e · github-actions · Jul 17, 2024
1 parent 323a279
commit d8ae09e
Show file tree

Hide file tree

Showing 3 changed files with 122 additions and 6 deletions.
diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h
@@ -35,6 +35,15 @@ inline void GetHash(size_t &seed, const std::vector<T> &arg)
   }
 }
 
+// Specialization for const char*
+// this creates an intermediate string.
+template <>
+inline void GetHash<const char *>(size_t &seed, const char *const &arg)
+{
+  std::hash<std::string> hasher;
+  seed ^= hasher(std::string(arg)) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+}
+
 struct GetHashForAttributeValueVisitor
 {
   GetHashForAttributeValueVisitor(size_t &seed) : seed_(seed) {}
@@ -71,7 +80,7 @@ inline size_t GetHashForAttributeMap(
         {
           return true;
         }
-        GetHash(seed, key.data());
+        GetHash(seed, key);
         auto attr_val = nostd::visit(converter, value);
         nostd::visit(GetHashForAttributeValueVisitor(seed), attr_val);
         return true;

diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc
@@ -13,7 +13,6 @@ namespace nostd = opentelemetry::nostd;
 
 TEST(AttributesHashMap, BasicTests)
 {
-
   // Empty map
   AttributesHashMap hash_map;
   EXPECT_EQ(hash_map.Size(), 0);
@@ -73,3 +72,78 @@ TEST(AttributesHashMap, BasicTests)
       });
   EXPECT_EQ(count, hash_map.Size());
 }
+
+std::string make_unique_string(const char *str)
+{
+  return std::string(str);
+}
+
+TEST(AttributesHashMap, HashWithKeyValueIterable)
+{
+  std::string key1   = make_unique_string("k1");
+  std::string value1 = make_unique_string("v1");
+  std::string key2   = make_unique_string("k2");
+  std::string value2 = make_unique_string("v2");
+  std::string key3   = make_unique_string("k3");
+  std::string value3 = make_unique_string("v3");
+
+  // Create mock KeyValueIterable instances with the same content but different variables
+  std::map<std::string, std::string> attributes1({{key1, value1}, {key2, value2}});
+  std::map<std::string, std::string> attributes2({{key1, value1}, {key2, value2}});
+  std::map<std::string, std::string> attributes3({{key1, value1}, {key2, value2}, {key3, value3}});
+
+  // Create a callback that filters "k3" key
+  auto is_key_filter_k3_callback = [](nostd::string_view key) {
+    if (key == "k3")
+    {
+      return false;
+    }
+    return true;
+  };
+  // Calculate hash
+  size_t hash1 = opentelemetry::sdk::common::GetHashForAttributeMap(
+      opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes1),
+      is_key_filter_k3_callback);
+  size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap(
+      opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes2),
+      is_key_filter_k3_callback);
+
+  size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(
+      opentelemetry::common::KeyValueIterableView<std::map<std::string, std::string>>(attributes3),
+      is_key_filter_k3_callback);
+
+  // Expect the hashes to be the same because the content is the same
+  EXPECT_EQ(hash1, hash2);
+  // Expect the hashes to be the same because the content is the same
+  EXPECT_EQ(hash1, hash3);
+}
+
+TEST(AttributesHashMap, HashConsistencyAcrossStringTypes)
+{
+  const char *c_str                 = "teststring";
+  std::string std_str               = "teststring";
+  nostd::string_view nostd_str_view = "teststring";
+#if __cplusplus >= 201703L
+  std::string_view std_str_view = "teststring";
+#endif
+
+  size_t hash_c_str          = 0;
+  size_t hash_std_str        = 0;
+  size_t hash_nostd_str_view = 0;
+#if __cplusplus >= 201703L
+  size_t hash_std_str_view = 0;
+#endif
+
+  opentelemetry::sdk::common::GetHash(hash_c_str, c_str);
+  opentelemetry::sdk::common::GetHash(hash_std_str, std_str);
+  opentelemetry::sdk::common::GetHash(hash_nostd_str_view, nostd_str_view);
+#if __cplusplus >= 201703L
+  opentelemetry::sdk::common::GetHash(hash_std_str_view, std_str_view);
+#endif
+
+  EXPECT_EQ(hash_c_str, hash_std_str);
+  EXPECT_EQ(hash_c_str, hash_nostd_str_view);
+#if __cplusplus >= 201703L
+  EXPECT_EQ(hash_c_str, hash_std_str_view);
+#endif
+}
diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc
@@ -47,14 +47,47 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests)
         ->Aggregate(record_value);
   }
   EXPECT_EQ(hash_map.Size(), 10);  // only one more metric point should be added as overflow.
+  // record 5 more measurements to already existing (and not-overflow) metric points. They
+  // should get aggregated to these existing metric points.
+  for (auto i = 0; i < 5; i++)
+  {
+    FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}};
+    auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes);
+    static_cast<LongSumAggregation *>(
+        hash_map.GetOrSetDefault(attributes, aggregation_callback, hash))
+        ->Aggregate(record_value);
+  }
+  EXPECT_EQ(hash_map.Size(), 10);  // no new metric point added
+
   // get the overflow metric point
-  auto agg = hash_map.GetOrSetDefault(
+  auto agg1 = hash_map.GetOrSetDefault(
       FilteredOrderedAttributeMap({{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_),
+  EXPECT_NE(agg1, nullptr);
+  auto sum_agg1 = static_cast<LongSumAggregation *>(agg1);
+  EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg1->ToPoint()).value_),
             record_value * 6);  // 1 from previous 10, 5 from current 5.
+  // get remaining metric points
+  for (auto i = 0; i < 9; i++)
+  {
+    FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}};
+    auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes);
+    auto agg2 = hash_map.GetOrSetDefault(
+        FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}),
+        aggregation_callback, hash);
+    EXPECT_NE(agg2, nullptr);
+    auto sum_agg2 = static_cast<LongSumAggregation *>(agg2);
+    if (i < 5)
+    {
+      EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg2->ToPoint()).value_),
+                record_value * 2);  // 1 from first recording, 1 from third recording
+    }
+    else
+    {
+      EXPECT_EQ(nostd::get<int64_t>(nostd::get<SumPointData>(sum_agg2->ToPoint()).value_),
+                record_value);  // 1 from first recording
+    }
+  }
 }
 
 class WritableMetricStorageCardinalityLimitTestFixture