From d8ae09e53d6988a063364c86a8362053432a3f9f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 17 Jul 2024 07:00:20 -0700 Subject: [PATCH] [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 --- .../sdk/common/attributemap_hash.h | 11 ++- sdk/test/metrics/attributes_hashmap_test.cc | 76 ++++++++++++++++++- sdk/test/metrics/cardinality_limit_test.cc | 41 +++++++++- 3 files changed, 122 insertions(+), 6 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 408070255a..e09b03d08b 100644 --- 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 &arg) } } +// Specialization for const char* +// this creates an intermediate string. +template <> +inline void GetHash(size_t &seed, const char *const &arg) +{ + std::hash 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 index 7e03deed47..9ae8f5cfd5 100644 --- 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 attributes1({{key1, value1}, {key2, value2}}); + std::map attributes2({{key1, value1}, {key2, value2}}); + std::map 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>(attributes1), + is_key_filter_k3_callback); + size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap( + opentelemetry::common::KeyValueIterableView>(attributes2), + is_key_filter_k3_callback); + + size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap( + opentelemetry::common::KeyValueIterableView>(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 index 3f9483ac1c..cf94112f1a 100644 --- 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( + 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(agg); - EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), + EXPECT_NE(agg1, nullptr); + auto sum_agg1 = static_cast(agg1); + EXPECT_EQ(nostd::get(nostd::get(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(agg2); + if (i < 5) + { + EXPECT_EQ(nostd::get(nostd::get(sum_agg2->ToPoint()).value_), + record_value * 2); // 1 from first recording, 1 from third recording + } + else + { + EXPECT_EQ(nostd::get(nostd::get(sum_agg2->ToPoint()).value_), + record_value); // 1 from first recording + } + } } class WritableMetricStorageCardinalityLimitTestFixture