From 1bf392a7b6b7c21e2fcfdc94e626b13bf95bc4e7 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 9 Jul 2024 18:30:11 -0700 Subject: [PATCH 01/20] add test to validate cardinaity limit --- sdk/test/metrics/attributes_hashmap_test.cc | 57 ++++++++++- sdk/test/metrics/cardinality_limit_test.cc | 101 +++++++++++++++++++- 2 files changed, 154 insertions(+), 4 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 7e03deed47..f71796867d 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -11,9 +11,38 @@ using namespace opentelemetry::sdk::metrics; namespace nostd = opentelemetry::nostd; -TEST(AttributesHashMap, BasicTests) + +// Mock KeyValueIterable implementation +class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable { +public: + MockKeyValueIterable(std::initializer_list> init) + : attributes_(init) + {} + + bool ForEachKeyValue(nostd::function_ref callback) const noexcept override + { + for (const auto &kv : attributes_) + { + if (!callback(kv.first, kv.second)) + { + return false; + } + } + return true; + } + + size_t size() const noexcept { + return attributes_.size(); + } + +private: + std::vector> attributes_; +}; + +TEST(AttributesHashMap, BasicTests) +{ // Empty map AttributesHashMap hash_map; EXPECT_EQ(hash_map.Size(), 0); @@ -73,3 +102,29 @@ TEST(AttributesHashMap, BasicTests) }); EXPECT_EQ(count, hash_map.Size()); } + +TEST(AttributesHashMap, HashWithKeyValueIterable) +{ + // Create mock KeyValueIterable instances with the same content but different variables + MockKeyValueIterable attributes1({{"k1", "v1"}, {"k2", "v2"}}); + MockKeyValueIterable attributes2({{"k1", "v1"}, {"k2", "v2"}}); + + // Create a callback that accepts all keys + auto is_key_present_callback = [](nostd::string_view key) { + return true; // Consider all keys + }; + + // Calculate hash + size_t hash1 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes1, is_key_present_callback); + size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_present_callback); + + // Expect the hashes to be the same because the content is the same + EXPECT_EQ(hash1, hash2); + + // Create mock KeyValueIterable instance with different order + MockKeyValueIterable attributes3({{"k2", "v2"}, {"k1", "v1"}}); + size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes3, is_key_present_callback); + + // Expect the hash to be different because the order is different + EXPECT_NE(hash1, hash3); +} diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 8c2edb4b0c..9313a9871c 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -18,7 +18,7 @@ using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; -TEST(CardinalityLimit, AttributesHashMapBasicTests) +/*TEST(CardinalityLimit, AttributesHashMapBasicTests) { AttributesHashMap hash_map(10); std::function()> aggregation_callback = @@ -47,6 +47,18 @@ 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( FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), @@ -55,13 +67,96 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) auto sum_agg = static_cast(agg); EXPECT_EQ(nostd::get(nostd::get(sum_agg->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 agg = hash_map.GetOrSetDefault( + FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), + aggregation_callback, hash); + EXPECT_NE(agg, nullptr); + auto sum_agg = static_cast(agg); + if (i < 5) { + EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), + record_value * 2); // 1 from first recording, 1 from third recording + } else { + EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), + record_value); // 1 from first recording + } + } +}*/ + +TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) +{ + const auto kCardinalityLimit = 10; + AggregationTemporality temporality = AggregationTemporality::kCumulative; + auto sdk_start_ts = std::chrono::system_clock::now(); + int64_t expected_total_get_requests = 0; + int64_t expected_total_put_requests = 0; + InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, + InstrumentValueType::kLong}; + + std::unique_ptr default_attributes_processor{ + new DefaultAttributesProcessor{}}; + opentelemetry::sdk::metrics::SyncMetricStorage storage( + instr_desc, AggregationType::kSum, default_attributes_processor.get(), +#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW + ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(), +#endif + nullptr, kCardinalityLimit); + + int64_t first_recorded_value = 10; + for (auto i = 0 ; i < 10 ;i ++){ + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(first_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + } + + // record again + int64_t second_recorded_value = 30; + for (auto i = 0 ; i < 10 ;i ++){ + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(second_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + } + //record 5 more with new attributes to go to overflow metrics + int64_t third_recorded_value = 40; + for (auto i = 11 ; i < 16 ; i++){ + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(third_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + } + + std::shared_ptr collector(new MockCollectorHandle(temporality)); + std::vector> collectors; + collectors.push_back(collector); + + // Some computation here + auto collection_ts = std::chrono::system_clock::now(); + size_t count_attributes = 0; + storage.Collect( + collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData &metric_data) { + for (const auto &data_attr : metric_data.point_data_attr_) + { + const auto &data = opentelemetry::nostd::get(data_attr.point_data); + if (data_attr.attributes.find("key") != data_attr.attributes.end()) + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), first_recorded_value + second_recorded_value); + count_attributes++; + } + else if (data_attr.attributes.find(kAttributesLimitOverflowKey) != data_attr.attributes.end()) + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), first_recorded_value + second_recorded_value + 5 * third_recorded_value); + count_attributes++; + } + } + return true; + }); + EXPECT_EQ(count_attributes, kCardinalityLimit); // Max cardinality limit } class WritableMetricStorageCardinalityLimitTestFixture : public ::testing::TestWithParam {}; -TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregation) +/*TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregation) { auto sdk_start_ts = std::chrono::system_clock::now(); const size_t attributes_limit = 10; @@ -112,4 +207,4 @@ TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregati } INSTANTIATE_TEST_SUITE_P(All, WritableMetricStorageCardinalityLimitTestFixture, - ::testing::Values(AggregationTemporality::kDelta)); + ::testing::Values(AggregationTemporality::kDelta));*/ From 140d8aab461c17f24609326c0dce44db4367cffa Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 01:39:47 +0000 Subject: [PATCH 02/20] format --- sdk/test/metrics/attributes_hashmap_test.cc | 25 +++++++------ sdk/test/metrics/cardinality_limit_test.cc | 40 +++++++++++++-------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index f71796867d..b6fc0fce57 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -11,16 +11,19 @@ using namespace opentelemetry::sdk::metrics; namespace nostd = opentelemetry::nostd; - // Mock KeyValueIterable implementation class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable { public: - MockKeyValueIterable(std::initializer_list> init) + MockKeyValueIterable( + std::initializer_list> + init) : attributes_(init) {} - bool ForEachKeyValue(nostd::function_ref callback) const noexcept override + bool ForEachKeyValue( + nostd::function_ref callback) + const noexcept override { for (const auto &kv : attributes_) { @@ -32,15 +35,12 @@ class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable return true; } - size_t size() const noexcept { - return attributes_.size(); - } + size_t size() const noexcept { return attributes_.size(); } private: std::vector> attributes_; }; - TEST(AttributesHashMap, BasicTests) { // Empty map @@ -111,19 +111,22 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) // Create a callback that accepts all keys auto is_key_present_callback = [](nostd::string_view key) { - return true; // Consider all keys + return true; // Consider all keys }; // Calculate hash - size_t hash1 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes1, is_key_present_callback); - size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_present_callback); + size_t hash1 = + opentelemetry::sdk::common::GetHashForAttributeMap(attributes1, is_key_present_callback); + size_t hash2 = + opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_present_callback); // Expect the hashes to be the same because the content is the same EXPECT_EQ(hash1, hash2); // Create mock KeyValueIterable instance with different order MockKeyValueIterable attributes3({{"k2", "v2"}, {"k1", "v1"}}); - size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes3, is_key_present_callback); + size_t hash3 = + opentelemetry::sdk::common::GetHashForAttributeMap(attributes3, is_key_present_callback); // Expect the hash to be different because the order is different EXPECT_NE(hash1, hash3); diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 9313a9871c..5d999bbeff 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -88,7 +88,7 @@ namespace nostd = opentelemetry::nostd; TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) { - const auto kCardinalityLimit = 10; + const auto kCardinalityLimit = 10; AggregationTemporality temporality = AggregationTemporality::kCumulative; auto sdk_start_ts = std::chrono::system_clock::now(); int64_t expected_total_get_requests = 0; @@ -106,22 +106,31 @@ TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) nullptr, kCardinalityLimit); int64_t first_recorded_value = 10; - for (auto i = 0 ; i < 10 ;i ++){ - std::map attributes = {{"key", std::to_string(i)}}; - storage.RecordLong(first_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + for (auto i = 0; i < 10; i++) + { + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(first_recorded_value, + KeyValueIterableView>(attributes), + opentelemetry::context::Context{}); } // record again int64_t second_recorded_value = 30; - for (auto i = 0 ; i < 10 ;i ++){ - std::map attributes = {{"key", std::to_string(i)}}; - storage.RecordLong(second_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + for (auto i = 0; i < 10; i++) + { + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(second_recorded_value, + KeyValueIterableView>(attributes), + opentelemetry::context::Context{}); } - //record 5 more with new attributes to go to overflow metrics + // record 5 more with new attributes to go to overflow metrics int64_t third_recorded_value = 40; - for (auto i = 11 ; i < 16 ; i++){ - std::map attributes = {{"key", std::to_string(i)}}; - storage.RecordLong(third_recorded_value, KeyValueIterableView>(attributes),opentelemetry::context::Context{}); + for (auto i = 11; i < 16; i++) + { + std::map attributes = {{"key", std::to_string(i)}}; + storage.RecordLong(third_recorded_value, + KeyValueIterableView>(attributes), + opentelemetry::context::Context{}); } std::shared_ptr collector(new MockCollectorHandle(temporality)); @@ -138,12 +147,15 @@ TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) const auto &data = opentelemetry::nostd::get(data_attr.point_data); if (data_attr.attributes.find("key") != data_attr.attributes.end()) { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), first_recorded_value + second_recorded_value); + EXPECT_EQ(opentelemetry::nostd::get(data.value_), + first_recorded_value + second_recorded_value); count_attributes++; } - else if (data_attr.attributes.find(kAttributesLimitOverflowKey) != data_attr.attributes.end()) + else if (data_attr.attributes.find(kAttributesLimitOverflowKey) != + data_attr.attributes.end()) { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), first_recorded_value + second_recorded_value + 5 * third_recorded_value); + EXPECT_EQ(opentelemetry::nostd::get(data.value_), + first_recorded_value + second_recorded_value + 5 * third_recorded_value); count_attributes++; } } From 0e595a3040b7ca258db84ba49ad48a068f9b668c Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 9 Jul 2024 19:15:05 -0700 Subject: [PATCH 03/20] warning --- sdk/test/metrics/attributes_hashmap_test.cc | 2 +- sdk/test/metrics/cardinality_limit_test.cc | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index b6fc0fce57..16c9806616 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -35,7 +35,7 @@ class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable return true; } - size_t size() const noexcept { return attributes_.size(); } + size_t size() const noexcept override { return attributes_.size(); } private: std::vector> attributes_; diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 5d999bbeff..140cfc3e8b 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -18,7 +18,7 @@ using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; -/*TEST(CardinalityLimit, AttributesHashMapBasicTests) +TEST(CardinalityLimit, AttributesHashMapBasicTests) { AttributesHashMap hash_map(10); std::function()> aggregation_callback = @@ -84,15 +84,13 @@ namespace nostd = opentelemetry::nostd; record_value); // 1 from first recording } } -}*/ +} TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) { const auto kCardinalityLimit = 10; AggregationTemporality temporality = AggregationTemporality::kCumulative; auto sdk_start_ts = std::chrono::system_clock::now(); - int64_t expected_total_get_requests = 0; - int64_t expected_total_put_requests = 0; InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, InstrumentValueType::kLong}; @@ -168,7 +166,7 @@ class WritableMetricStorageCardinalityLimitTestFixture : public ::testing::TestWithParam {}; -/*TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregation) +TEST_P(WritableMetricStorageCardinalityLimitTestFixture, LongCounterSumAggregation) { auto sdk_start_ts = std::chrono::system_clock::now(); const size_t attributes_limit = 10; @@ -219,4 +217,4 @@ class WritableMetricStorageCardinalityLimitTestFixture } INSTANTIATE_TEST_SUITE_P(All, WritableMetricStorageCardinalityLimitTestFixture, - ::testing::Values(AggregationTemporality::kDelta));*/ + ::testing::Values(AggregationTemporality::kDelta)); From e269a9e183f3357dfff07e5449351986b5f94f4f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 02:19:03 +0000 Subject: [PATCH 04/20] Format --- sdk/test/metrics/cardinality_limit_test.cc | 32 ++++++++++++---------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 140cfc3e8b..8ba0a728dd 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -57,7 +57,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) hash_map.GetOrSetDefault(attributes, aggregation_callback, hash)) ->Aggregate(record_value); } - EXPECT_EQ(hash_map.Size(), 10); // no new metric point added + EXPECT_EQ(hash_map.Size(), 10); // no new metric point added // get the overflow metric point auto agg = hash_map.GetOrSetDefault( @@ -68,31 +68,35 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), record_value * 6); // 1 from previous 10, 5 from current 5. // get remaining metric points - for (auto i = 0 ; i < 9 ; i ++) { + for (auto i = 0; i < 9; i++) + { FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}}; auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto agg = hash_map.GetOrSetDefault( - FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), - aggregation_callback, hash); + auto agg = hash_map.GetOrSetDefault( + FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), + aggregation_callback, hash); EXPECT_NE(agg, nullptr); auto sum_agg = static_cast(agg); - if (i < 5) { + if (i < 5) + { EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), - record_value * 2); // 1 from first recording, 1 from third recording - } else { + record_value * 2); // 1 from first recording, 1 from third recording + } + else + { EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), - record_value); // 1 from first recording + record_value); // 1 from first recording } } } TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) { - const auto kCardinalityLimit = 10; - AggregationTemporality temporality = AggregationTemporality::kCumulative; - auto sdk_start_ts = std::chrono::system_clock::now(); - InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, - InstrumentValueType::kLong}; + const auto kCardinalityLimit = 10; + AggregationTemporality temporality = AggregationTemporality::kCumulative; + auto sdk_start_ts = std::chrono::system_clock::now(); + InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, + InstrumentValueType::kLong}; std::unique_ptr default_attributes_processor{ new DefaultAttributesProcessor{}}; From 60cac1c8100883143390327bcefda2ba9cb85267 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 02:34:38 +0000 Subject: [PATCH 05/20] maint mode CI --- sdk/test/metrics/attributes_hashmap_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 16c9806616..699ec2f71c 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -110,7 +110,7 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) MockKeyValueIterable attributes2({{"k1", "v1"}, {"k2", "v2"}}); // Create a callback that accepts all keys - auto is_key_present_callback = [](nostd::string_view key) { + auto is_key_present_callback = [](nostd::string_view /*key*/) { return true; // Consider all keys }; From 2058aa377e2dfafe26f999ff35b8d565a59101ab Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 9 Jul 2024 23:49:01 -0700 Subject: [PATCH 06/20] modify test to reproduce the issue --- sdk/test/metrics/attributes_hashmap_test.cc | 59 +++++++++++++-------- sdk/test/metrics/cardinality_limit_test.cc | 7 +-- 2 files changed, 41 insertions(+), 25 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 16c9806616..cbaca33fae 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -16,10 +16,13 @@ class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable { public: MockKeyValueIterable( - std::initializer_list> - init) - : attributes_(init) - {} + std::initializer_list> init) + { + for (const auto &kv : init) + { + attributes_.emplace_back(kv.first, kv.second); + } + } bool ForEachKeyValue( nostd::function_ref callback) @@ -38,9 +41,10 @@ class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable size_t size() const noexcept override { return attributes_.size(); } private: - std::vector> attributes_; + std::vector> attributes_; }; + TEST(AttributesHashMap, BasicTests) { // Empty map @@ -103,31 +107,42 @@ TEST(AttributesHashMap, BasicTests) EXPECT_EQ(count, hash_map.Size()); } +std::string make_unique_string(const char* str) +{ + return std::string(str); +} + TEST(AttributesHashMap, HashWithKeyValueIterable) { // Create mock KeyValueIterable instances with the same content but different variables - MockKeyValueIterable attributes1({{"k1", "v1"}, {"k2", "v2"}}); - MockKeyValueIterable attributes2({{"k1", "v1"}, {"k2", "v2"}}); - - // Create a callback that accepts all keys - auto is_key_present_callback = [](nostd::string_view key) { - return true; // Consider all keys - }; - + MockKeyValueIterable attributes1({{make_unique_string("k1"), make_unique_string("v1")}, + {make_unique_string("k2"), make_unique_string("v2")}}); + MockKeyValueIterable attributes2({{make_unique_string("k1"), make_unique_string("v1")}, + {make_unique_string("k2"), make_unique_string("v2")}}); + MockKeyValueIterable attributes3({{make_unique_string("k1"), make_unique_string("v1")}, + {make_unique_string("k2"), make_unique_string("v2")}, + {make_unique_string("k3"), make_unique_string("v3")}}); + + + // 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(attributes1, is_key_present_callback); + opentelemetry::sdk::common::GetHashForAttributeMap(attributes1, is_key_filter_k3_callback); size_t hash2 = - opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_present_callback); + opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_filter_k3_callback); + + size_t hash3 = + opentelemetry::sdk::common::GetHashForAttributeMap(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); - // Create mock KeyValueIterable instance with different order - MockKeyValueIterable attributes3({{"k2", "v2"}, {"k1", "v1"}}); - size_t hash3 = - opentelemetry::sdk::common::GetHashForAttributeMap(attributes3, is_key_present_callback); - - // Expect the hash to be different because the order is different - EXPECT_NE(hash1, hash3); } diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 8ba0a728dd..e42950f57c 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -110,7 +110,7 @@ TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) int64_t first_recorded_value = 10; for (auto i = 0; i < 10; i++) { - std::map attributes = {{"key", std::to_string(i)}}; + std::map attributes = {{std::string{"key"}, std::to_string(i)}}; storage.RecordLong(first_recorded_value, KeyValueIterableView>(attributes), opentelemetry::context::Context{}); @@ -120,16 +120,17 @@ TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) int64_t second_recorded_value = 30; for (auto i = 0; i < 10; i++) { - std::map attributes = {{"key", std::to_string(i)}}; + std::map attributes = {{std::string{"key"}, std::to_string(i)}}; storage.RecordLong(second_recorded_value, KeyValueIterableView>(attributes), opentelemetry::context::Context{}); } + // record 5 more with new attributes to go to overflow metrics int64_t third_recorded_value = 40; for (auto i = 11; i < 16; i++) { - std::map attributes = {{"key", std::to_string(i)}}; + std::map attributes = {{std::string{"key"}, std::to_string(i)}}; storage.RecordLong(third_recorded_value, KeyValueIterableView>(attributes), opentelemetry::context::Context{}); From 1c61fa55ab80c3b385a9d4bd29235ee9ee78e99f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 06:55:30 +0000 Subject: [PATCH 07/20] Format --- sdk/test/metrics/attributes_hashmap_test.cc | 35 ++++++++++----------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index cbaca33fae..643a5bd46e 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -15,8 +15,7 @@ namespace nostd = opentelemetry::nostd; class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable { public: - MockKeyValueIterable( - std::initializer_list> init) + MockKeyValueIterable(std::initializer_list> init) { for (const auto &kv : init) { @@ -44,7 +43,6 @@ class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable std::vector> attributes_; }; - TEST(AttributesHashMap, BasicTests) { // Empty map @@ -107,42 +105,41 @@ TEST(AttributesHashMap, BasicTests) EXPECT_EQ(count, hash_map.Size()); } -std::string make_unique_string(const char* str) +std::string make_unique_string(const char *str) { - return std::string(str); + return std::string(str); } TEST(AttributesHashMap, HashWithKeyValueIterable) { // Create mock KeyValueIterable instances with the same content but different variables MockKeyValueIterable attributes1({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}}); + {make_unique_string("k2"), make_unique_string("v2")}}); MockKeyValueIterable attributes2({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}}); + {make_unique_string("k2"), make_unique_string("v2")}}); MockKeyValueIterable attributes3({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}, - {make_unique_string("k3"), make_unique_string("v3")}}); - + {make_unique_string("k2"), make_unique_string("v2")}, + {make_unique_string("k3"), make_unique_string("v3")}}); // Create a callback that filters "k3" key -auto is_key_filter_k3_callback = [](nostd::string_view key) { - if (key == "k3") { - return false; - } - return true; -}; + 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(attributes1, is_key_filter_k3_callback); size_t hash2 = opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_filter_k3_callback); - size_t hash3 = + size_t hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(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 the hashes to be the same because the content is the same EXPECT_EQ(hash1, hash3); - } From ddc0634da39a21bea710fb4c1013a187b029eacd Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 06:59:26 +0000 Subject: [PATCH 08/20] remove vscode settings --- .vscode/settings.json | 73 ------------------------------------------- 1 file changed, 73 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 12e3da1658..0000000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,73 +0,0 @@ -{ - "files.associations": { - "cctype": "cpp", - "clocale": "cpp", - "cmath": "cpp", - "cstdarg": "cpp", - "cstddef": "cpp", - "cstdio": "cpp", - "cstdlib": "cpp", - "cstring": "cpp", - "ctime": "cpp", - "cwchar": "cpp", - "cwctype": "cpp", - "array": "cpp", - "atomic": "cpp", - "bit": "cpp", - "*.tcc": "cpp", - "bitset": "cpp", - "chrono": "cpp", - "compare": "cpp", - "concepts": "cpp", - "condition_variable": "cpp", - "cstdint": "cpp", - "deque": "cpp", - "list": "cpp", - "map": "cpp", - "set": "cpp", - "string": "cpp", - "unordered_map": "cpp", - "unordered_set": "cpp", - "vector": "cpp", - "exception": "cpp", - "algorithm": "cpp", - "functional": "cpp", - "iterator": "cpp", - "memory": "cpp", - "memory_resource": "cpp", - "numeric": "cpp", - "optional": "cpp", - "random": "cpp", - "ratio": "cpp", - "regex": "cpp", - "string_view": "cpp", - "system_error": "cpp", - "tuple": "cpp", - "type_traits": "cpp", - "utility": "cpp", - "fstream": "cpp", - "future": "cpp", - "initializer_list": "cpp", - "iomanip": "cpp", - "iosfwd": "cpp", - "iostream": "cpp", - "istream": "cpp", - "limits": "cpp", - "mutex": "cpp", - "new": "cpp", - "numbers": "cpp", - "ostream": "cpp", - "semaphore": "cpp", - "shared_mutex": "cpp", - "span": "cpp", - "sstream": "cpp", - "stdexcept": "cpp", - "stop_token": "cpp", - "streambuf": "cpp", - "thread": "cpp", - "cinttypes": "cpp", - "typeinfo": "cpp", - "variant": "cpp", - "pointers": "cpp" - } -} \ No newline at end of file From 2e678b4e8eeab5e83c24012df7ac572ca4edbf75 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 10 Jul 2024 00:31:50 -0700 Subject: [PATCH 09/20] remove redundant test --- sdk/test/metrics/cardinality_limit_test.cc | 115 +++++++-------------- 1 file changed, 38 insertions(+), 77 deletions(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index e42950f57c..49aa21eee4 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -18,6 +18,44 @@ using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; +// Mock KeyValueIterable implementation +class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable +{ +public: + MockKeyValueIterable(std::initializer_list> init) + { + for (const auto &kv : init) + { + attributes_.emplace_back(kv.first, kv.second); + } + } + + bool ForEachKeyValue( + nostd::function_ref callback) + const noexcept override + { + for (const auto &kv : attributes_) + { + if (!callback(kv.first, kv.second)) + { + return false; + } + } + return true; + } + + size_t size() const noexcept override { return attributes_.size(); } + +private: + std::vector> attributes_; +}; + + +std::string make_unique_string(const char *str) +{ + return std::string(str); +} + TEST(CardinalityLimit, AttributesHashMapBasicTests) { AttributesHashMap hash_map(10); @@ -90,83 +128,6 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) } } -TEST(WritableMetricStorageTestFixture, SyncStorageCardinalityLimit) -{ - const auto kCardinalityLimit = 10; - AggregationTemporality temporality = AggregationTemporality::kCumulative; - auto sdk_start_ts = std::chrono::system_clock::now(); - InstrumentDescriptor instr_desc = {"name", "desc", "1unit", InstrumentType::kCounter, - InstrumentValueType::kLong}; - - std::unique_ptr default_attributes_processor{ - new DefaultAttributesProcessor{}}; - opentelemetry::sdk::metrics::SyncMetricStorage storage( - instr_desc, AggregationType::kSum, default_attributes_processor.get(), -#ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW - ExemplarFilterType::kAlwaysOff, ExemplarReservoir::GetNoExemplarReservoir(), -#endif - nullptr, kCardinalityLimit); - - int64_t first_recorded_value = 10; - for (auto i = 0; i < 10; i++) - { - std::map attributes = {{std::string{"key"}, std::to_string(i)}}; - storage.RecordLong(first_recorded_value, - KeyValueIterableView>(attributes), - opentelemetry::context::Context{}); - } - - // record again - int64_t second_recorded_value = 30; - for (auto i = 0; i < 10; i++) - { - std::map attributes = {{std::string{"key"}, std::to_string(i)}}; - storage.RecordLong(second_recorded_value, - KeyValueIterableView>(attributes), - opentelemetry::context::Context{}); - } - - // record 5 more with new attributes to go to overflow metrics - int64_t third_recorded_value = 40; - for (auto i = 11; i < 16; i++) - { - std::map attributes = {{std::string{"key"}, std::to_string(i)}}; - storage.RecordLong(third_recorded_value, - KeyValueIterableView>(attributes), - opentelemetry::context::Context{}); - } - - std::shared_ptr collector(new MockCollectorHandle(temporality)); - std::vector> collectors; - collectors.push_back(collector); - - // Some computation here - auto collection_ts = std::chrono::system_clock::now(); - size_t count_attributes = 0; - storage.Collect( - collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData &metric_data) { - for (const auto &data_attr : metric_data.point_data_attr_) - { - const auto &data = opentelemetry::nostd::get(data_attr.point_data); - if (data_attr.attributes.find("key") != data_attr.attributes.end()) - { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), - first_recorded_value + second_recorded_value); - count_attributes++; - } - else if (data_attr.attributes.find(kAttributesLimitOverflowKey) != - data_attr.attributes.end()) - { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), - first_recorded_value + second_recorded_value + 5 * third_recorded_value); - count_attributes++; - } - } - return true; - }); - EXPECT_EQ(count_attributes, kCardinalityLimit); // Max cardinality limit -} - class WritableMetricStorageCardinalityLimitTestFixture : public ::testing::TestWithParam {}; From fd733ea0ccd87394b25e858596b3f88c68edc1bd Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 10 Jul 2024 00:33:27 -0700 Subject: [PATCH 10/20] remove unused code --- sdk/test/metrics/cardinality_limit_test.cc | 38 ---------------------- 1 file changed, 38 deletions(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 49aa21eee4..1af151422b 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -18,44 +18,6 @@ using namespace opentelemetry::sdk::metrics; using namespace opentelemetry::common; namespace nostd = opentelemetry::nostd; -// Mock KeyValueIterable implementation -class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable -{ -public: - MockKeyValueIterable(std::initializer_list> init) - { - for (const auto &kv : init) - { - attributes_.emplace_back(kv.first, kv.second); - } - } - - bool ForEachKeyValue( - nostd::function_ref callback) - const noexcept override - { - for (const auto &kv : attributes_) - { - if (!callback(kv.first, kv.second)) - { - return false; - } - } - return true; - } - - size_t size() const noexcept override { return attributes_.size(); } - -private: - std::vector> attributes_; -}; - - -std::string make_unique_string(const char *str) -{ - return std::string(str); -} - TEST(CardinalityLimit, AttributesHashMapBasicTests) { AttributesHashMap hash_map(10); From b55f513f4624032b3c94597117cd13d8bb49f2cc Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 10 Jul 2024 00:55:35 -0700 Subject: [PATCH 11/20] fix to calculate hash on string_view --- sdk/include/opentelemetry/sdk/common/attributemap_hash.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 408070255a..ea23827ae8 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -10,6 +10,7 @@ #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/version.h" +#include OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -71,7 +72,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; From 9e28d46a1afe71140e47801cb24ffaad7e538c82 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 10 Jul 2024 01:02:11 -0700 Subject: [PATCH 12/20] remove iostream --- sdk/include/opentelemetry/sdk/common/attributemap_hash.h | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index ea23827ae8..592fbc1a0f 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -10,7 +10,6 @@ #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/version.h" -#include OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk From f9d636fb76d7085e8dfaff3747b3b012d7c1a50f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 13:59:38 -0700 Subject: [PATCH 13/20] template specialization for const char *, and varous string type tests --- .../sdk/common/attributemap_hash.h | 9 ++++++ sdk/test/metrics/attributes_hashmap_test.cc | 29 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 592fbc1a0f..a2e94a5b6d 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) {} diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 643a5bd46e..154aa3fc9c 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -143,3 +143,32 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) // 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; + size_t hash_std_str_view = 0; + + 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 +} From 74839b48916799c1d183f41ec7ab4816f9827422 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 14:20:57 -0700 Subject: [PATCH 14/20] unused variable warning --- sdk/test/metrics/attributes_hashmap_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 154aa3fc9c..e6b51b34f0 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -157,7 +157,9 @@ TEST(AttributesHashMap, HashConsistencyAcrossStringTypes) 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); From f4bb581ef3ce4033cee289fbeb292b9d4091fdc4 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 14:40:44 -0700 Subject: [PATCH 15/20] more warnings --- sdk/test/metrics/cardinality_limit_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index 1af151422b..eefacb8885 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -60,31 +60,31 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) 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 agg = hash_map.GetOrSetDefault( + auto agg2 = hash_map.GetOrSetDefault( FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), aggregation_callback, hash); - EXPECT_NE(agg, nullptr); - auto sum_agg = static_cast(agg); + EXPECT_NE(agg2, nullptr); + auto sum_agg2 = static_cast(agg2); if (i < 5) { - EXPECT_EQ(nostd::get(nostd::get(sum_agg->ToPoint()).value_), + 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_agg->ToPoint()).value_), + EXPECT_EQ(nostd::get(nostd::get(sum_agg2->ToPoint()).value_), record_value); // 1 from first recording } } From 63819b3cfc15bc6c958e99f59ed5111c517dcc7e Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 10 Jul 2024 21:45:39 +0000 Subject: [PATCH 16/20] format --- .../sdk/common/attributemap_hash.h | 4 +-- sdk/test/metrics/attributes_hashmap_test.cc | 25 +++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index a2e94a5b6d..e09b03d08b 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -36,9 +36,9 @@ inline void GetHash(size_t &seed, const std::vector &arg) } // Specialization for const char* -// this creates an intermediate string. +// this creates an intermediate string. template <> -inline void GetHash(size_t &seed, const char* const &arg) +inline void GetHash(size_t &seed, const char *const &arg) { std::hash hasher; seed ^= hasher(std::string(arg)) + 0x9e3779b9 + (seed << 6) + (seed >> 2); diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index e6b51b34f0..687d80ecc3 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -144,33 +144,32 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) EXPECT_EQ(hash1, hash3); } - TEST(AttributesHashMap, HashConsistencyAcrossStringTypes) { - const char* c_str = "teststring"; - std::string std_str = "teststring"; + const char *c_str = "teststring"; + std::string std_str = "teststring"; nostd::string_view nostd_str_view = "teststring"; - #if __cplusplus >= 201703L +#if __cplusplus >= 201703L std::string_view std_str_view = "teststring"; - #endif +#endif - size_t hash_c_str = 0; - size_t hash_std_str = 0; + size_t hash_c_str = 0; + size_t hash_std_str = 0; size_t hash_nostd_str_view = 0; - #if __cplusplus >= 201703L +#if __cplusplus >= 201703L size_t hash_std_str_view = 0; - #endif +#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 +#if __cplusplus >= 201703L opentelemetry::sdk::common::GetHash(hash_std_str_view, std_str_view); - #endif +#endif EXPECT_EQ(hash_c_str, hash_std_str); EXPECT_EQ(hash_c_str, hash_nostd_str_view); - #if __cplusplus >= 201703L +#if __cplusplus >= 201703L EXPECT_EQ(hash_c_str, hash_std_str_view); - #endif +#endif } From fcc9fd143b28167faf4290c699a101a3eca10241 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 16 Jul 2024 16:06:25 -0700 Subject: [PATCH 17/20] fix use-after-stack-scope --- sdk/test/metrics/attributes_hashmap_test.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 687d80ecc3..df2cc1906d 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -112,14 +112,17 @@ std::string make_unique_string(const char *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 - MockKeyValueIterable attributes1({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}}); - MockKeyValueIterable attributes2({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}}); - MockKeyValueIterable attributes3({{make_unique_string("k1"), make_unique_string("v1")}, - {make_unique_string("k2"), make_unique_string("v2")}, - {make_unique_string("k3"), make_unique_string("v3")}}); + MockKeyValueIterable attributes1({{key1, value1}, {key2, value2}}); + MockKeyValueIterable attributes2({{key1, value1}, {key2, value2}}); + MockKeyValueIterable attributes3({{key1, value1}, {key2, value2}, {key3, value3}}); // Create a callback that filters "k3" key auto is_key_filter_k3_callback = [](nostd::string_view key) { From c170c761bde6eeffcb42a828c4d74e6c80481d93 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 16 Jul 2024 16:09:24 -0700 Subject: [PATCH 18/20] format --- sdk/test/metrics/attributes_hashmap_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index df2cc1906d..843c5bc458 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -112,11 +112,11 @@ std::string make_unique_string(const char *str) TEST(AttributesHashMap, HashWithKeyValueIterable) { - std::string key1 = make_unique_string("k1"); + std::string key1 = make_unique_string("k1"); std::string value1 = make_unique_string("v1"); - std::string key2 = make_unique_string("k2"); + std::string key2 = make_unique_string("k2"); std::string value2 = make_unique_string("v2"); - std::string key3 = make_unique_string("k3"); + 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 From a2590e3659467199eb89b97092df24c546eeb026 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 16 Jul 2024 17:10:09 -0700 Subject: [PATCH 19/20] another try --- sdk/test/metrics/attributes_hashmap_test.cc | 44 +++------------------ 1 file changed, 6 insertions(+), 38 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 843c5bc458..8412aa2f26 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -11,38 +11,6 @@ using namespace opentelemetry::sdk::metrics; namespace nostd = opentelemetry::nostd; -// Mock KeyValueIterable implementation -class MockKeyValueIterable : public opentelemetry::common::KeyValueIterable -{ -public: - MockKeyValueIterable(std::initializer_list> init) - { - for (const auto &kv : init) - { - attributes_.emplace_back(kv.first, kv.second); - } - } - - bool ForEachKeyValue( - nostd::function_ref callback) - const noexcept override - { - for (const auto &kv : attributes_) - { - if (!callback(kv.first, kv.second)) - { - return false; - } - } - return true; - } - - size_t size() const noexcept override { return attributes_.size(); } - -private: - std::vector> attributes_; -}; - TEST(AttributesHashMap, BasicTests) { // Empty map @@ -120,9 +88,9 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) std::string value3 = make_unique_string("v3"); // Create mock KeyValueIterable instances with the same content but different variables - MockKeyValueIterable attributes1({{key1, value1}, {key2, value2}}); - MockKeyValueIterable attributes2({{key1, value1}, {key2, value2}}); - MockKeyValueIterable attributes3({{key1, value1}, {key2, value2}, {key3, value3}}); + 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) { @@ -134,12 +102,12 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) }; // Calculate hash size_t hash1 = - opentelemetry::sdk::common::GetHashForAttributeMap(attributes1, is_key_filter_k3_callback); + opentelemetry::sdk::common::GetHashForAttributeMap(opentelemetry::common::KeyValueIterableView>(attributes1), is_key_filter_k3_callback); size_t hash2 = - opentelemetry::sdk::common::GetHashForAttributeMap(attributes2, is_key_filter_k3_callback); + opentelemetry::sdk::common::GetHashForAttributeMap(opentelemetry::common::KeyValueIterableView>(attributes2), is_key_filter_k3_callback); size_t hash3 = - opentelemetry::sdk::common::GetHashForAttributeMap(attributes3, is_key_filter_k3_callback); + 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); From bb92dc581d8700d2b6d2db2cfde7ec7a9bfcb933 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 16 Jul 2024 17:14:05 -0700 Subject: [PATCH 20/20] format --- sdk/test/metrics/attributes_hashmap_test.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 8412aa2f26..9ae8f5cfd5 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -101,13 +101,16 @@ TEST(AttributesHashMap, HashWithKeyValueIterable) 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); + 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);