diff --git a/CHANGELOG.md b/CHANGELOG.md index 62262afe08..62914b9c4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [METRICS SDK] Fix hash collision in MetricAttributes + [#3322](https://github.com/open-telemetry/opentelemetry-cpp/pull/3322) + * [BUILD] Fix misssing exported definition for OTLP file exporter and forceflush [#3319](https://github.com/open-telemetry/opentelemetry-cpp/pull/3319) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 39df29b278..e56d6aaf75 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -10,10 +10,6 @@ #include #include -#include "opentelemetry/common/attribute_value.h" -#include "opentelemetry/common/key_value_iterable.h" -#include "opentelemetry/nostd/function_ref.h" -#include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/version.h" @@ -74,27 +70,6 @@ inline size_t GetHashForAttributeMap(const OrderedAttributeMap &attribute_map) return seed; } -// Calculate hash of keys and values of KeyValueIterable, filtered using callback. -inline size_t GetHashForAttributeMap( - const opentelemetry::common::KeyValueIterable &attributes, - nostd::function_ref is_key_present_callback) -{ - AttributeConverter converter; - size_t seed = 0UL; - attributes.ForEachKeyValue( - [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { - if (!is_key_present_callback(key)) - { - return true; - } - GetHash(seed, key); - auto attr_val = nostd::visit(converter, value); - nostd::visit(GetHashForAttributeValueVisitor(seed), attr_val); - return true; - }); - return seed; -} - template inline size_t GetHash(T value) { diff --git a/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_cell.h b/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_cell.h index 03941870c0..cc315b7131 100644 --- a/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_cell.h +++ b/sdk/include/opentelemetry/sdk/metrics/exemplar/reservoir_cell.h @@ -125,6 +125,7 @@ class ReservoirCell res.erase(it); } } + res.UpdateHash(); return res; } diff --git a/sdk/include/opentelemetry/sdk/metrics/instruments.h b/sdk/include/opentelemetry/sdk/metrics/instruments.h index 76ad07b2e3..6473a267f2 100644 --- a/sdk/include/opentelemetry/sdk/metrics/instruments.h +++ b/sdk/include/opentelemetry/sdk/metrics/instruments.h @@ -65,6 +65,7 @@ struct InstrumentDescriptor }; using MetricAttributes = opentelemetry::sdk::metrics::FilteredOrderedAttributeMap; +using MetricAttributesHash = opentelemetry::sdk::metrics::FilteredOrderedAttributeMapHash; using AggregationTemporalitySelector = std::function; /*class InstrumentSelector { diff --git a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h index 14e9a3fbfa..93ffccc48c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -71,24 +71,22 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora auto aggr = DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_); aggr->Aggregate(measurement.second); - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(measurement.first); - auto prev = cumulative_hash_map_->Get(hash); + auto prev = cumulative_hash_map_->Get(measurement.first); if (prev) { auto delta = prev->Diff(*aggr); // store received value in cumulative map, and the diff in delta map (to pass it to temporal // storage) - cumulative_hash_map_->Set(measurement.first, std::move(aggr), hash); - delta_hash_map_->Set(measurement.first, std::move(delta), hash); + cumulative_hash_map_->Set(measurement.first, std::move(aggr)); + delta_hash_map_->Set(measurement.first, std::move(delta)); } else { // store received value in cumulative and delta map. cumulative_hash_map_->Set( measurement.first, - DefaultAggregation::CloneAggregation(aggregation_type_, instrument_descriptor_, *aggr), - hash); - delta_hash_map_->Set(measurement.first, std::move(aggr), hash); + DefaultAggregation::CloneAggregation(aggregation_type_, instrument_descriptor_, *aggr)); + delta_hash_map_->Set(measurement.first, std::move(aggr)); } } } diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index ddf2a8b206..bcc7610aae 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -15,6 +15,7 @@ #include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" #include "opentelemetry/version.h" @@ -29,9 +30,9 @@ using opentelemetry::sdk::common::OrderedAttributeMap; constexpr size_t kAggregationCardinalityLimit = 2000; const std::string kAttributesLimitOverflowKey = "otel.metrics.overflow"; const bool kAttributesLimitOverflowValue = true; -const size_t kOverflowAttributesHash = opentelemetry::sdk::common::GetHashForAttributeMap( - {{kAttributesLimitOverflowKey, - kAttributesLimitOverflowValue}}); // precalculated for optimization +const MetricAttributes kOverflowAttributes = { + {kAttributesLimitOverflowKey, + kAttributesLimitOverflowValue}}; // precalculated for optimization class AttributeHashGenerator { @@ -42,18 +43,19 @@ class AttributeHashGenerator } }; -class AttributesHashMap +template +class AttributesHashMapWithCustomHash { public: - AttributesHashMap(size_t attributes_limit = kAggregationCardinalityLimit) + AttributesHashMapWithCustomHash(size_t attributes_limit = kAggregationCardinalityLimit) : attributes_limit_(attributes_limit) {} - Aggregation *Get(size_t hash) const + Aggregation *Get(const MetricAttributes &attributes) const { - auto it = hash_map_.find(hash); + auto it = hash_map_.find(attributes); if (it != hash_map_.end()) { - return it->second.second.get(); + return it->second.get(); } return nullptr; } @@ -62,7 +64,10 @@ class AttributesHashMap * @return check if key is present in hash * */ - bool Has(size_t hash) const { return hash_map_.find(hash) != hash_map_.end(); } + bool Has(const MetricAttributes &attributes) const + { + return hash_map_.find(attributes) != hash_map_.end(); + } /** * @return the pointer to value for given key if present. @@ -71,13 +76,16 @@ class AttributesHashMap */ Aggregation *GetOrSetDefault(const opentelemetry::common::KeyValueIterable &attributes, const AttributesProcessor *attributes_processor, - std::function()> aggregation_callback, - size_t hash) + std::function()> aggregation_callback) { - auto it = hash_map_.find(hash); + // TODO: avoid constructing MetricAttributes from KeyValueIterable for + // hash_map_.find which is a heavy operation + MetricAttributes attr{attributes, attributes_processor}; + + auto it = hash_map_.find(attr); if (it != hash_map_.end()) { - return it->second.second.get(); + return it->second.get(); } if (IsOverflowAttributes()) @@ -85,19 +93,17 @@ class AttributesHashMap return GetOrSetOveflowAttributes(aggregation_callback); } - MetricAttributes attr{attributes, attributes_processor}; - - hash_map_[hash] = {attr, aggregation_callback()}; - return hash_map_[hash].second.get(); + auto result = hash_map_.emplace(std::move(attr), aggregation_callback()); + return result.first->second.get(); } - Aggregation *GetOrSetDefault(std::function()> aggregation_callback, - size_t hash) + Aggregation *GetOrSetDefault(const MetricAttributes &attributes, + std::function()> aggregation_callback) { - auto it = hash_map_.find(hash); + auto it = hash_map_.find(attributes); if (it != hash_map_.end()) { - return it->second.second.get(); + return it->second.get(); } if (IsOverflowAttributes()) @@ -105,19 +111,17 @@ class AttributesHashMap return GetOrSetOveflowAttributes(aggregation_callback); } - MetricAttributes attr{}; - hash_map_[hash] = {attr, aggregation_callback()}; - return hash_map_[hash].second.get(); + hash_map_[attributes] = aggregation_callback(); + return hash_map_[attributes].get(); } - Aggregation *GetOrSetDefault(const MetricAttributes &attributes, - std::function()> aggregation_callback, - size_t hash) + Aggregation *GetOrSetDefault(MetricAttributes &&attributes, + std::function()> aggregation_callback) { - auto it = hash_map_.find(hash); + auto it = hash_map_.find(attributes); if (it != hash_map_.end()) { - return it->second.second.get(); + return it->second.get(); } if (IsOverflowAttributes()) @@ -125,54 +129,50 @@ class AttributesHashMap return GetOrSetOveflowAttributes(aggregation_callback); } - MetricAttributes attr{attributes}; - - hash_map_[hash] = {attr, aggregation_callback()}; - return hash_map_[hash].second.get(); + auto result = hash_map_.emplace(std::move(attributes), aggregation_callback()); + return result.first->second.get(); } - /** * Set the value for given key, overwriting the value if already present */ void Set(const opentelemetry::common::KeyValueIterable &attributes, const AttributesProcessor *attributes_processor, - std::unique_ptr aggr, - size_t hash) + std::unique_ptr aggr) + { + Set(MetricAttributes{attributes, attributes_processor}, std::move(aggr)); + } + + void Set(const MetricAttributes &attributes, std::unique_ptr aggr) { - auto it = hash_map_.find(hash); + auto it = hash_map_.find(attributes); if (it != hash_map_.end()) { - it->second.second = std::move(aggr); + it->second = std::move(aggr); } else if (IsOverflowAttributes()) { - hash_map_[kOverflowAttributesHash] = { - MetricAttributes{{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}, - std::move(aggr)}; + hash_map_[kOverflowAttributes] = std::move(aggr); } else { - MetricAttributes attr{attributes, attributes_processor}; - hash_map_[hash] = {attr, std::move(aggr)}; + hash_map_[attributes] = std::move(aggr); } } - void Set(const MetricAttributes &attributes, std::unique_ptr aggr, size_t hash) + void Set(MetricAttributes &&attributes, std::unique_ptr aggr) { - auto it = hash_map_.find(hash); + auto it = hash_map_.find(attributes); if (it != hash_map_.end()) { - it->second.second = std::move(aggr); + it->second = std::move(aggr); } else if (IsOverflowAttributes()) { - hash_map_[kOverflowAttributesHash] = { - MetricAttributes{{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}, - std::move(aggr)}; + hash_map_[kOverflowAttributes] = std::move(aggr); } else { - hash_map_[hash] = {attributes, std::move(aggr)}; + hash_map_[std::move(attributes)] = std::move(aggr); } } @@ -184,7 +184,7 @@ class AttributesHashMap { for (auto &kv : hash_map_) { - if (!callback(kv.second.first, *(kv.second.second.get()))) + if (!callback(kv.first, *(kv.second.get()))) { return false; // callback is not prepared to consume data } @@ -197,8 +197,13 @@ class AttributesHashMap */ size_t Size() { return hash_map_.size(); } +#ifdef UNIT_TESTING + size_t BucketCount() { return hash_map_.bucket_count(); } + size_t BucketSize(size_t n) { return hash_map_.bucket_size(n); } +#endif + private: - std::unordered_map>> hash_map_; + std::unordered_map, CustomHash> hash_map_; size_t attributes_limit_; Aggregation *GetOrSetOveflowAttributes( @@ -210,19 +215,21 @@ class AttributesHashMap Aggregation *GetOrSetOveflowAttributes(std::unique_ptr agg) { - auto it = hash_map_.find(kOverflowAttributesHash); + auto it = hash_map_.find(kOverflowAttributes); if (it != hash_map_.end()) { - return it->second.second.get(); + return it->second.get(); } - MetricAttributes attr{{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}; - hash_map_[kOverflowAttributesHash] = {attr, std::move(agg)}; - return hash_map_[kOverflowAttributesHash].second.get(); + auto result = hash_map_.emplace(kOverflowAttributes, std::move(agg)); + return result.first->second.get(); } bool IsOverflowAttributes() const { return (hash_map_.size() + 1 >= attributes_limit_); } }; + +using AttributesHashMap = AttributesHashMapWithCustomHash<>; + } // namespace metrics } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h b/sdk/include/opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h index 329e75bfa7..6a0562e0e9 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h @@ -3,13 +3,21 @@ #pragma once +#include +#include +#include #include +#include +#include +#include #include - +#include #include "opentelemetry/common/attribute_value.h" #include "opentelemetry/common/key_value_iterable.h" #include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/attribute_utils.h" +#include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE @@ -22,22 +30,65 @@ class AttributesProcessor; // IWYU pragma: keep class FilteredOrderedAttributeMap : public opentelemetry::sdk::common::OrderedAttributeMap { public: - FilteredOrderedAttributeMap() = default; + FilteredOrderedAttributeMap() : OrderedAttributeMap() { UpdateHash(); } + FilteredOrderedAttributeMap( std::initializer_list> attributes) : OrderedAttributeMap(attributes) - {} + { + UpdateHash(); + } + FilteredOrderedAttributeMap(const opentelemetry::common::KeyValueIterable &attributes) : FilteredOrderedAttributeMap(attributes, nullptr) - {} + { + // No need to update hash here as it is already updated in the constructor above + } + FilteredOrderedAttributeMap(const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::sdk::metrics::AttributesProcessor *processor); + FilteredOrderedAttributeMap( std::initializer_list> attributes, const opentelemetry::sdk::metrics::AttributesProcessor *processor); + + // + // Copy and move constructors, assignment operators + // + FilteredOrderedAttributeMap(const FilteredOrderedAttributeMap &other) = default; + FilteredOrderedAttributeMap(FilteredOrderedAttributeMap &&other) = default; + FilteredOrderedAttributeMap &operator=(const FilteredOrderedAttributeMap &other) = default; + FilteredOrderedAttributeMap &operator=(FilteredOrderedAttributeMap &&other) = default; + + // + // equality operator + // + bool operator==(const FilteredOrderedAttributeMap &other) const + { + return hash_ == other.hash_ && static_cast(*this) == + static_cast(other); + } + + size_t GetHash() const { return hash_; } + + void UpdateHash() { hash_ = GetHashForAttributeMap(*this); } + +private: + size_t hash_ = (std::numeric_limits::max)(); }; + +class FilteredOrderedAttributeMapHash +{ +public: + size_t operator()( + const opentelemetry::sdk::metrics::FilteredOrderedAttributeMap &attributes) const + { + return attributes.GetHash(); + } +}; + } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h index 6e5b799e3f..0c9dea9c90 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -95,9 +95,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); } #endif - static size_t hash = opentelemetry::sdk::common::GetHash(""); + static MetricAttributes attr = MetricAttributes{}; std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(create_default_aggregation_, hash)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attr, create_default_aggregation_)->Aggregate(value); } void RecordLong(int64_t value, @@ -116,21 +116,10 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage std::chrono::system_clock::now()); } #endif - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, [this](nostd::string_view key) { - if (attributes_processor_) - { - return attributes_processor_->isPresent(key); - } - else - { - return true; - } - }); std::lock_guard guard(attribute_hashmap_lock_); attributes_hashmap_ - ->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_, hash) + ->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_) ->Aggregate(value); } @@ -148,9 +137,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); } #endif - static size_t hash = opentelemetry::sdk::common::GetHash(""); + static MetricAttributes attr = MetricAttributes{}; std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(create_default_aggregation_, hash)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attr, create_default_aggregation_)->Aggregate(value); } void RecordDouble(double value, @@ -169,20 +158,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage std::chrono::system_clock::now()); } #endif - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, [this](nostd::string_view key) { - if (attributes_processor_) - { - return attributes_processor_->isPresent(key); - } - else - { - return true; - } - }); std::lock_guard guard(attribute_hashmap_lock_); attributes_hashmap_ - ->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_, hash) + ->GetOrSetDefault(attributes, attributes_processor_, create_default_aggregation_) ->Aggregate(value); } diff --git a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h index 4e20e7d65f..7ab8cafb13 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h @@ -87,6 +87,8 @@ class FilteringAttributesProcessor : public AttributesProcessor } return true; }); + + result.UpdateHash(); return result; } diff --git a/sdk/src/metrics/state/filtered_ordered_attribute_map.cc b/sdk/src/metrics/state/filtered_ordered_attribute_map.cc index baed0fce9a..85f4bc9d6e 100644 --- a/sdk/src/metrics/state/filtered_ordered_attribute_map.cc +++ b/sdk/src/metrics/state/filtered_ordered_attribute_map.cc @@ -3,6 +3,7 @@ #include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/nostd/function_ref.h" +#include "opentelemetry/sdk/common/attribute_utils.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" OPENTELEMETRY_BEGIN_NAMESPACE @@ -23,6 +24,8 @@ FilteredOrderedAttributeMap::FilteredOrderedAttributeMap( } return true; }); + + UpdateHash(); } FilteredOrderedAttributeMap::FilteredOrderedAttributeMap( @@ -38,6 +41,8 @@ FilteredOrderedAttributeMap::FilteredOrderedAttributeMap( SetAttribute(kv.first, kv.second); } } + + UpdateHash(); } } // namespace metrics } // namespace sdk diff --git a/sdk/src/metrics/state/observable_registry.cc b/sdk/src/metrics/state/observable_registry.cc index 8db11d4605..d8d6afb01b 100644 --- a/sdk/src/metrics/state/observable_registry.cc +++ b/sdk/src/metrics/state/observable_registry.cc @@ -3,10 +3,8 @@ #include #include -#include #include #include -#include #include #include @@ -14,7 +12,6 @@ #include "opentelemetry/metrics/async_instruments.h" #include "opentelemetry/metrics/observer_result.h" #include "opentelemetry/nostd/shared_ptr.h" -#include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/common/global_log_handler.h" #include "opentelemetry/sdk/metrics/async_instruments.h" #include "opentelemetry/sdk/metrics/instruments.h" diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 1302f9d642..d027c9883c 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -13,7 +13,6 @@ #include "opentelemetry/common/timestamp.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/span.h" -#include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" @@ -104,19 +103,17 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, { agg_hashmap->GetAllEnteries( [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto agg = merged_metrics->Get(hash); + auto agg = merged_metrics->Get(attributes); if (agg) { - merged_metrics->Set(attributes, agg->Merge(aggregation), hash); + merged_metrics->Set(attributes, agg->Merge(aggregation)); } else { merged_metrics->Set(attributes, DefaultAggregation::CreateAggregation( aggregation_type_, instrument_descriptor_, aggregation_config_) - ->Merge(aggregation), - hash); + ->Merge(aggregation)); } return true; }); @@ -139,17 +136,16 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, // merge current delta to previous cumulative last_aggr_hashmap->GetAllEnteries( [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto agg = merged_metrics->Get(hash); + auto agg = merged_metrics->Get(attributes); if (agg) { - merged_metrics->Set(attributes, agg->Merge(aggregation), hash); + merged_metrics->Set(attributes, agg->Merge(aggregation)); } else { auto def_agg = DefaultAggregation::CreateAggregation( aggregation_type_, instrument_descriptor_, aggregation_config_); - merged_metrics->Set(attributes, def_agg->Merge(aggregation), hash); + merged_metrics->Set(attributes, def_agg->Merge(aggregation)); } return true; }); diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 929511a89b..519cb1fc1f 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -36,6 +36,9 @@ cc_test( cc_test( name = "all_tests", srcs = glob(["*_test.cc"]), + copts = [ + "-DUNIT_TESTING", + ], tags = [ "metrics", "test", diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 4e09af4d2e..79b8c28a2f 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -38,6 +38,7 @@ foreach( target_link_libraries( ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} metrics_common_test_utils opentelemetry_resources) + target_compile_definitions(${testname} PRIVATE UNIT_TESTING) gtest_add_tests( TARGET ${testname} TEST_PREFIX metrics. diff --git a/sdk/test/metrics/attributes_hashmap_benchmark.cc b/sdk/test/metrics/attributes_hashmap_benchmark.cc index 8c4bc69f9e..8225f8ba12 100644 --- a/sdk/test/metrics/attributes_hashmap_benchmark.cc +++ b/sdk/test/metrics/attributes_hashmap_benchmark.cc @@ -11,11 +11,9 @@ #include #include -#include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/drop_aggregation.h" #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" -#include "opentelemetry/sdk/metrics/state/filtered_ordered_attribute_map.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h" using namespace opentelemetry::sdk::metrics; @@ -39,10 +37,9 @@ void BM_AttributseHashMap(benchmark::State &state) return std::unique_ptr(new DropAggregation); }; m.lock(); - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes[i % 2]); - hash_map.GetOrSetDefault(attributes[i % 2], create_default_aggregation, hash) + hash_map.GetOrSetDefault(attributes[i % 2], create_default_aggregation) ->Aggregate(static_cast(1)); - benchmark::DoNotOptimize(hash_map.Has(hash)); + benchmark::DoNotOptimize(hash_map.Has(attributes[i % 2])); m.unlock(); }; while (state.KeepRunning()) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 153c6ada25..c51f4fe800 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -5,12 +5,10 @@ #include #include #include -#include #include #include #include -#include "opentelemetry/common/key_value_iterable_view.h" #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/attributemap_hash.h" @@ -28,34 +26,32 @@ TEST(AttributesHashMap, BasicTests) AttributesHashMap hash_map; EXPECT_EQ(hash_map.Size(), 0); MetricAttributes m1 = {{"k1", "v1"}}; - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(m1); - EXPECT_EQ(hash_map.Get(hash), nullptr); - EXPECT_EQ(hash_map.Has(hash), false); + EXPECT_EQ(hash_map.Get(m1), nullptr); + EXPECT_EQ(hash_map.Has(m1), false); // Set std::unique_ptr aggregation1( new DropAggregation()); // = std::unique_ptr(new DropAggregation); - hash_map.Set(m1, std::move(aggregation1), hash); - hash_map.Get(hash)->Aggregate(static_cast(1)); + hash_map.Set(m1, std::move(aggregation1)); + hash_map.Get(m1)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 1); - EXPECT_EQ(hash_map.Has(hash), true); + EXPECT_EQ(hash_map.Has(m1), true); // Set same key again auto aggregation2 = std::unique_ptr(new DropAggregation()); - hash_map.Set(m1, std::move(aggregation2), hash); - hash_map.Get(hash)->Aggregate(static_cast(1)); + hash_map.Set(m1, std::move(aggregation2)); + hash_map.Get(m1)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 1); - EXPECT_EQ(hash_map.Has(hash), true); + EXPECT_EQ(hash_map.Has(m1), true); // Set more enteria auto aggregation3 = std::unique_ptr(new DropAggregation()); MetricAttributes m3 = {{"k1", "v1"}, {"k2", "v2"}}; - auto hash3 = opentelemetry::sdk::common::GetHashForAttributeMap(m3); - hash_map.Set(m3, std::move(aggregation3), hash3); - EXPECT_EQ(hash_map.Has(hash), true); - EXPECT_EQ(hash_map.Has(hash3), true); - hash_map.Get(hash3)->Aggregate(static_cast(1)); + hash_map.Set(m3, std::move(aggregation3)); + EXPECT_EQ(hash_map.Has(m1), true); + EXPECT_EQ(hash_map.Has(m3), true); + hash_map.Get(m3)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 2); // GetOrSetDefault @@ -64,15 +60,16 @@ TEST(AttributesHashMap, BasicTests) return std::unique_ptr(new DropAggregation); }; MetricAttributes m4 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}}; - auto hash4 = opentelemetry::sdk::common::GetHashForAttributeMap(m4); - hash_map.GetOrSetDefault(m4, create_default_aggregation, hash4) - ->Aggregate(static_cast(1)); + hash_map.GetOrSetDefault(m4, create_default_aggregation)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 3); // Set attributes with different order - shouldn't create a new entry. MetricAttributes m5 = {{"k2", "v2"}, {"k1", "v1"}}; - auto hash5 = opentelemetry::sdk::common::GetHashForAttributeMap(m5); - EXPECT_EQ(hash_map.Has(hash5), true); + EXPECT_EQ(hash_map.Has(m5), true); + + // Set attributes with different order - shouldn't create a new entry. + MetricAttributes m6 = {{"k1", "v2"}, {"k2", "v1"}}; + EXPECT_EQ(hash_map.Has(m6), false); // GetAllEnteries size_t count = 0; @@ -84,49 +81,52 @@ TEST(AttributesHashMap, BasicTests) EXPECT_EQ(count, hash_map.Size()); } -std::string make_unique_string(const char *str) +class MetricAttributeMapHashForCollision { - return std::string(str); -} +public: + size_t operator()(const MetricAttributes & /*attributes*/) const { return 42; } +}; -TEST(AttributesHashMap, HashWithKeyValueIterable) +TEST(AttributesHashMap, CollisionTest) { - 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") + // The hash on MetricsAttributes will be ignored by MetricAttributeMapHashForCollision + MetricAttributes m1 = {{"k1", "v1"}}; + MetricAttributes m2 = {{"k2", "v2"}}; + MetricAttributes m3 = {{"k1", "v1"}, {"k2", "v2"}}; + MetricAttributes m4 = {}; + + AttributesHashMapWithCustomHash hash_map; + + hash_map.Set(m1, std::unique_ptr(new DropAggregation())); + hash_map.Set(m2, std::unique_ptr(new DropAggregation())); + hash_map.Set(m3, std::unique_ptr(new DropAggregation())); + hash_map.Set(m4, std::unique_ptr(new DropAggregation())); + + EXPECT_EQ(hash_map.Size(), 4); + EXPECT_EQ(hash_map.Has(m1), true); + EXPECT_EQ(hash_map.Has(m2), true); + EXPECT_EQ(hash_map.Has(m3), true); + EXPECT_EQ(hash_map.Has(m4), true); + + MetricAttributes m5 = {{"k2", "v1"}}; + EXPECT_EQ(hash_map.Has(m5), false); + + // + // Verify only one bucket used based on the custom hash + // + size_t total_active_buckets = 0; + size_t total_elements = 0; + for (size_t i = 0; i < hash_map.BucketCount(); i++) + { + size_t bucket_size = hash_map.BucketSize(i); + if (bucket_size > 0) { - return false; + total_active_buckets++; + total_elements += bucket_size; } - 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); + } + EXPECT_EQ(total_active_buckets, 1); + EXPECT_EQ(total_elements, 4); } TEST(AttributesHashMap, HashConsistencyAcrossStringTypes) diff --git a/sdk/test/metrics/cardinality_limit_test.cc b/sdk/test/metrics/cardinality_limit_test.cc index c785eeb5d5..801096b269 100644 --- a/sdk/test/metrics/cardinality_limit_test.cc +++ b/sdk/test/metrics/cardinality_limit_test.cc @@ -19,7 +19,6 @@ #include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/variant.h" -#include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/sum_aggregation.h" #include "opentelemetry/sdk/metrics/data/metric_data.h" @@ -51,9 +50,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) for (auto i = 0; i < 10; i++) { FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}}; - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - static_cast( - hash_map.GetOrSetDefault(attributes, aggregation_callback, hash)) + static_cast(hash_map.GetOrSetDefault(attributes, aggregation_callback)) ->Aggregate(record_value); } EXPECT_EQ(hash_map.Size(), 10); @@ -62,9 +59,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) for (auto i = 10; i < 15; i++) { FilteredOrderedAttributeMap attributes = {{"key", std::to_string(i)}}; - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - static_cast( - hash_map.GetOrSetDefault(attributes, aggregation_callback, hash)) + static_cast(hash_map.GetOrSetDefault(attributes, aggregation_callback)) ->Aggregate(record_value); } EXPECT_EQ(hash_map.Size(), 10); // only one more metric point should be added as overflow. @@ -73,17 +68,13 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) 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)) + static_cast(hash_map.GetOrSetDefault(attributes, aggregation_callback)) ->Aggregate(record_value); } EXPECT_EQ(hash_map.Size(), 10); // no new metric point added // get the overflow metric point - auto agg1 = hash_map.GetOrSetDefault( - FilteredOrderedAttributeMap({{kAttributesLimitOverflowKey, kAttributesLimitOverflowValue}}), - aggregation_callback, kOverflowAttributesHash); + auto agg1 = hash_map.GetOrSetDefault(kOverflowAttributes, aggregation_callback); EXPECT_NE(agg1, nullptr); auto sum_agg1 = static_cast(agg1); EXPECT_EQ(nostd::get(nostd::get(sum_agg1->ToPoint()).value_), @@ -92,10 +83,7 @@ TEST(CardinalityLimit, AttributesHashMapBasicTests) 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); + auto agg2 = hash_map.GetOrSetDefault(attributes, aggregation_callback); EXPECT_NE(agg2, nullptr); auto sum_agg2 = static_cast(agg2); if (i < 5) diff --git a/sdk/test/metrics/observer_result_test.cc b/sdk/test/metrics/observer_result_test.cc index fc4835786c..b33141b4f7 100644 --- a/sdk/test/metrics/observer_result_test.cc +++ b/sdk/test/metrics/observer_result_test.cc @@ -6,10 +6,8 @@ #include #include #include -#include #include "opentelemetry/common/key_value_iterable_view.h" -#include "opentelemetry/nostd/variant.h" #include "opentelemetry/sdk/metrics/observer_result.h" #include "opentelemetry/sdk/metrics/view/attributes_processor.h"