From 625f3062b960a547f9ce2bd8ef131b571f0e7569 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 18:26:18 -0800 Subject: [PATCH 01/24] fix --- .../sdk/common/attributemap_hash.h | 51 ++++++- .../sdk/metrics/state/attributes_hashmap.h | 126 ++++++++++++++++-- .../sdk/metrics/state/sync_metric_storage.h | 17 +-- .../sdk/metrics/view/attributes_processor.h | 21 ++- sdk/test/common/attributemap_hash_test.cc | 7 + .../metrics/attributes_hashmap_benchmark.cc | 1 + 6 files changed, 198 insertions(+), 25 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index c6f2c93ccc..d27a35b456 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -7,6 +7,23 @@ #include #include "opentelemetry/sdk/common/attribute_utils.h" +#define OTEL_IS_BIT_SET(var, bit) ((var & (1 << bit)) != 0) // true if bit is set, false otherwise +#define OTEL_SETBIT(var, bit) (var |= (1 << bit)) +#define OTEL_RESETBIT(var, bit) (var &= (0 << bit)) + +#if 0 +namespace std { + template <> struct hash + { + size_t operator()(const opentelemetry::nostd::string_view &s) const + { + return std::hash()(s.data()); + /* your code here, e.g. "return hash()(x.value);" */ + } + }; +} +#endif + OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { @@ -14,7 +31,7 @@ namespace common { template -inline void GetHashForAttributeValue(size_t &seed, const T arg) +inline void GetHashForAttributeValue(size_t &seed, const T &arg) { std::hash hasher; // reference - @@ -57,6 +74,38 @@ 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; + size_t index = 0; + attributes.ForEachKeyValue( + [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { + if (!is_key_present_callback(key)) + { + return true; + } + std::hash hasher; + // reference - + // https://www.boost.org/doc/libs/1_37_0/doc/html/hash/reference.html#boost.hash_combine + seed ^= hasher(key.data()) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + auto attr_val = nostd::visit(converter, value); + nostd::visit(GetHashForAttributeValueVisitor(seed), attr_val); + return true; + }); + return seed; +} + +template +inline size_t GetHash(T value) +{ + std::hash hasher; + return hasher(value); +} + } // namespace common } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index 5b575fd024..948175febc 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -8,6 +8,7 @@ #include "opentelemetry/sdk/common/attributemap_hash.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/instruments.h" +#include "opentelemetry/sdk/metrics/view/attributes_processor.h" #include "opentelemetry/version.h" #include @@ -19,6 +20,7 @@ namespace sdk { namespace metrics { + using opentelemetry::sdk::common::OrderedAttributeMap; class AttributeHashGenerator @@ -33,12 +35,32 @@ class AttributeHashGenerator class AttributesHashMap { public: + AttributesHashMap( + const AttributesProcessor *attributes_processor = new DefaultAttributesProcessor()) + : attributes_processor_{attributes_processor} + {} + + Aggregation *Get(const opentelemetry::common::KeyValueIterable &attributes) const + { + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( + attributes, + [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + + auto it = hash_map_.find(hash); + if (it != hash_map_.end()) + { + return it->second.second.get(); + } + return nullptr; + } + Aggregation *Get(const MetricAttributes &attributes) const { - auto it = hash_map_.find(attributes); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + auto it = hash_map_.find(hash); if (it != hash_map_.end()) { - return it->second.get(); + return it->second.second.get(); } return nullptr; } @@ -47,9 +69,20 @@ class AttributesHashMap * @return check if key is present in hash * */ + bool Has(const opentelemetry::common::KeyValueIterable &attributes) const + { + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( + attributes, + [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + + return (hash_map_.find(hash) == hash_map_.end()) ? false : true; + } + bool Has(const MetricAttributes &attributes) const { - return (hash_map_.find(attributes) == hash_map_.end()) ? false : true; + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + + return (hash_map_.find(hash) == hash_map_.end()) ? false : true; } /** @@ -57,25 +90,90 @@ class AttributesHashMap * If not present, it uses the provided callback to generate * value and store in the hash */ + Aggregation *GetOrSetDefault(const opentelemetry::common::KeyValueIterable &attributes, + std::function()> aggregation_callback) + { + + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( + attributes, + [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + + auto it = hash_map_.find(hash); + if (it != hash_map_.end()) + { + return it->second.second.get(); + } + + MetricAttributes attr{attributes}; + + hash_map_[hash] = {attr, aggregation_callback()}; + return hash_map_[hash].second.get(); + } + + Aggregation *GetOrSetDefault(std::function()> aggregation_callback) + { + auto hash = opentelemetry::sdk::common::GetHash(""); + auto it = hash_map_.find(hash); + if (it != hash_map_.end()) + { + return it->second.second.get(); + } + + hash_map_[hash] = {{}, aggregation_callback()}; + return hash_map_[hash].second.get(); + } + Aggregation *GetOrSetDefault(const MetricAttributes &attributes, std::function()> aggregation_callback) { - auto it = hash_map_.find(attributes); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + + auto it = hash_map_.find(hash); if (it != hash_map_.end()) { - return it->second.get(); + return it->second.second.get(); } - hash_map_[attributes] = aggregation_callback(); - return hash_map_[attributes].get(); + MetricAttributes attr{attributes}; + + hash_map_[hash] = {attr, aggregation_callback()}; + return hash_map_[hash].second.get(); } /** * Set the value for given key, overwriting the value if already present */ - void Set(const MetricAttributes &attributes, std::unique_ptr value) + void Set(const opentelemetry::common::KeyValueIterable &attributes, + std::unique_ptr aggr) { - hash_map_[attributes] = std::move(value); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( + attributes, + [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + auto it = hash_map_.find(hash); + if (it != hash_map_.end()) + { + it->second.second = std::move(aggr); + } + else + { + MetricAttributes attr{attributes}; + hash_map_[hash] = {attr, std::move(aggr)}; + } + } + + void Set(const MetricAttributes &attributes, std::unique_ptr aggr) + { + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + auto it = hash_map_.find(hash); + if (it != hash_map_.end()) + { + it->second.second = std::move(aggr); + } + else + { + MetricAttributes attr{attributes}; + hash_map_[hash] = {attr, std::move(aggr)}; + } } /** @@ -86,7 +184,7 @@ class AttributesHashMap { for (auto &kv : hash_map_) { - if (!callback(kv.first, *(kv.second.get()))) + if (!callback(kv.second.first, *(kv.second.second.get()))) { return false; // callback is not prepared to consume data } @@ -99,9 +197,13 @@ class AttributesHashMap */ size_t Size() { return hash_map_.size(); } + const AttributesProcessor *GetAttributesProcessor() { return attributes_processor_; } + private: - std::unordered_map, AttributeHashGenerator> - hash_map_; + std::unordered_map>> hash_map_; + const AttributesProcessor *attributes_processor_; + + // std::unique_ptr attributes_hash_generator_; }; } // namespace metrics 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 da4fb2980a..0a845c2573 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -34,13 +34,11 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage OPENTELEMETRY_MAYBE_UNUSED, const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - attributes_hashmap_(new AttributesHashMap()), - attributes_processor_{attributes_processor}, + attributes_hashmap_(new AttributesHashMap(attributes_processor)), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_reservoir_(exemplar_reservoir), #endif temporal_metric_storage_(instrument_descriptor, aggregation_type, aggregation_config) - { create_default_aggregation_ = [&, aggregation_type, aggregation_config]() -> std::unique_ptr { @@ -61,7 +59,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); #endif std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault({}, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(create_default_aggregation_)->Aggregate(value); } void RecordLong(int64_t value, @@ -77,9 +75,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, std::chrono::system_clock::now()); #endif - auto attr = attributes_processor_->process(attributes); + // auto keys_to_ignore = attributes_processor_->process(attributes); std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(attr, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_)->Aggregate(value); } void RecordDouble(double value, @@ -94,7 +92,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); #endif std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault({}, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(create_default_aggregation_)->Aggregate(value); } void RecordDouble(double value, @@ -114,9 +112,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, std::chrono::system_clock::now()); #endif - auto attr = attributes_processor_->process(attributes); + // auto keys_to_ignore = attributes_processor_->process(attributes); std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(attr, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_)->Aggregate(value); } bool Collect(CollectorHandle *collector, @@ -135,7 +133,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage unreported_metrics_; // last reported metrics stash for all the collectors. std::unordered_map last_reported_metrics_; - const AttributesProcessor *attributes_processor_; std::function()> create_default_aggregation_; #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW nostd::shared_ptr exemplar_reservoir_; diff --git a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h index 754f45e1b6..18b5986e9b 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h @@ -4,6 +4,11 @@ #pragma once #include "opentelemetry/sdk/common/attribute_utils.h" + +#define OTEL_IS_BIT_SET(var, bit) ((var & (1 << bit)) != 0) // true if bit is set, false otherwise +#define OTEL_SETBIT(var, bit) (var |= (1 << bit)) +#define OTEL_RESETBIT(var, bit) (var &= (0 << bit)) + OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { @@ -20,10 +25,14 @@ class AttributesProcessor { public: // Process the metric instrument attributes. - // @returns The processed attributes + // @returns integer with individual bits set if they are to be filtered. + virtual MetricAttributes process( const opentelemetry::common::KeyValueIterable &attributes) const noexcept = 0; - virtual ~AttributesProcessor() = default; + + virtual bool isPresent(nostd::string_view key) const noexcept = 0; + + virtual ~AttributesProcessor() = default; }; /** @@ -33,12 +42,15 @@ class AttributesProcessor class DefaultAttributesProcessor : public AttributesProcessor { +public: MetricAttributes process( const opentelemetry::common::KeyValueIterable &attributes) const noexcept override { MetricAttributes result(attributes); return result; } + + bool isPresent(nostd::string_view key) const noexcept override { return true; } }; /** @@ -70,6 +82,11 @@ class FilteringAttributesProcessor : public AttributesProcessor return result; } + bool isPresent(nostd::string_view key) const noexcept override + { + return (allowed_attribute_keys_.find(key.data()) != allowed_attribute_keys_.end()); + } + private: std::unordered_map allowed_attribute_keys_; }; diff --git a/sdk/test/common/attributemap_hash_test.cc b/sdk/test/common/attributemap_hash_test.cc index 49e53361ba..c9eaeb035b 100644 --- a/sdk/test/common/attributemap_hash_test.cc +++ b/sdk/test/common/attributemap_hash_test.cc @@ -29,4 +29,11 @@ TEST(AttributeMapHashTest, BasicTests) OrderedAttributeMap map1 = {}; EXPECT_TRUE(GetHashForAttributeMap(map1) == 0); } + + { + OrderedAttributeMap map1 = {{"k1", 1}, {"k2", true}, {"k3", 1}}; + OrderedAttributeMap map2 = {{"k2", 1}, {"k1", true}, {"k3", true}}; + EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); + EXPECT_TRUE(GetHashForAttributeMap(map1) != 0); + } } diff --git a/sdk/test/metrics/attributes_hashmap_benchmark.cc b/sdk/test/metrics/attributes_hashmap_benchmark.cc index 71aaa1cd6f..4d09cf0ce0 100644 --- a/sdk/test/metrics/attributes_hashmap_benchmark.cc +++ b/sdk/test/metrics/attributes_hashmap_benchmark.cc @@ -32,6 +32,7 @@ void BM_AttributseHashMap(benchmark::State &state) return std::unique_ptr(new DropAggregation); }; m.lock(); + hash_map.GetOrSetDefault(attributes[i % 2], create_default_aggregation)->Aggregate((int64_t)1); benchmark::DoNotOptimize(hash_map.Has(attributes[i % 2])); m.unlock(); From b243da6f21cfc02ee0b3aaa0a3f97ddc8b1a762a Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 21:59:31 -0800 Subject: [PATCH 02/24] fix --- sdk/test/common/attributemap_hash_test.cc | 2 + sdk/test/metrics/CMakeLists.txt | 5 ++ sdk/test/metrics/measurements_benchmark.cc | 85 ++++++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 sdk/test/metrics/measurements_benchmark.cc diff --git a/sdk/test/common/attributemap_hash_test.cc b/sdk/test/common/attributemap_hash_test.cc index c9eaeb035b..d8a6319379 100644 --- a/sdk/test/common/attributemap_hash_test.cc +++ b/sdk/test/common/attributemap_hash_test.cc @@ -19,6 +19,7 @@ TEST(AttributeMapHashTest, BasicTests) } { + // hashmap algo is order insensitive. OrderedAttributeMap map1 = {{"k1", 10}, {"k2", true}, {"k3", 12.22}}; OrderedAttributeMap map2 = {{"k3", 12.22}, {"k1", 10}, {"k2", true}}; EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); @@ -31,6 +32,7 @@ TEST(AttributeMapHashTest, BasicTests) } { + // true is treated as `1`, and false as `0` with hashmap algo. OrderedAttributeMap map1 = {{"k1", 1}, {"k2", true}, {"k3", 1}}; OrderedAttributeMap map2 = {{"k2", 1}, {"k1", true}, {"k3", true}}; EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 984260a544..615b8a4ab1 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -50,6 +50,11 @@ if(WITH_BENCHMARK) histogram_aggregation_benchmark benchmark::benchmark ${CMAKE_THREAD_LIBS_INIT} opentelemetry_common opentelemetry_metrics opentelemetry_resources) + + add_executable(measurements_benchmark measurements_benchmark.cc) + target_link_libraries( + measurements_benchmark benchmark::benchmark opentelemetry_metrics + opentelemetry_resources ${CMAKE_THREAD_LIBS_INIT} opentelemetry_common) endif() add_subdirectory(exemplar) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc new file mode 100644 index 0000000000..1eaef525eb --- /dev/null +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -0,0 +1,85 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include "opentelemetry/sdk/metrics/meter.h" +#include "opentelemetry/sdk/metrics/meter_context.h" +#include "opentelemetry/sdk/metrics/meter_provider.h" +#include "opentelemetry/sdk/metrics/metric_reader.h" +#include "opentelemetry/sdk/metrics/push_metric_exporter.h" + +#include +#include +#include + +using namespace opentelemetry; +using namespace opentelemetry::sdk::instrumentationscope; +using namespace opentelemetry::sdk::metrics; +using namespace opentelemetry::sdk::common; + +class MockMetricExporter : public MetricReader +{ +public: + MockMetricExporter() = default; + + opentelemetry::sdk::metrics::AggregationTemporality GetAggregationTemporality( + opentelemetry::sdk::metrics::InstrumentType) const noexcept override + { + return AggregationTemporality::kCumulative; + } + +private: + bool OnForceFlush(std::chrono::microseconds /*timeout*/) noexcept override { return true; } + + bool OnShutDown(std::chrono::microseconds /*timeout*/) noexcept override { return true; } + + void OnInitialized() noexcept override {} +}; + +namespace +{ +void BM_MeasurementsTest(benchmark::State &state) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::shared_ptr exporter(new MockMetricExporter()); + mp.AddMetricReader(exporter); + auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); + std::vector actuals; + std::vector collectionThreads; + std::function collectMetrics = [&exporter, &actuals]() { + exporter->Collect([&](ResourceMetrics &rm) { + for (const ScopeMetrics &smd : rm.scope_metric_data_) + { + for (const MetricData &md : smd.metric_data_) + { + for (const PointDataAttributes &dp : md.point_data_attr_) + { + actuals.push_back(opentelemetry::nostd::get(dp.point_data)); + } + } + } + return true; + }); + }; + constexpr int NUM_CORES = 1; + constexpr size_t MAX_MEASUREMENTS = 1000000; + std::atomic measurements_processed{0l}; + while (state.KeepRunning()) + { + measurements_processed = 0; + while (measurements_processed++ <= MAX_MEASUREMENTS) + { + size_t val1 = rand() % 10; + size_t val2 = rand() % 10; + size_t val3 = rand() % 10; + h->Add(1.0, {{"dim1", val1}, {"dim2", val2}, {"dim3", val3}}, + opentelemetry::context::Context{}); + } + } +} + +BENCHMARK(BM_MeasurementsTest); + +} // namespace +BENCHMARK_MAIN(); \ No newline at end of file From 500031728ec9912a82434716235da77e49e47fe4 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 22:10:09 -0800 Subject: [PATCH 03/24] fix --- .../sdk/common/attributemap_hash.h | 18 ------------------ .../sdk/metrics/state/attributes_hashmap.h | 6 ++---- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index d27a35b456..b9ed4c1a26 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -3,27 +3,9 @@ #pragma once -#include #include #include "opentelemetry/sdk/common/attribute_utils.h" -#define OTEL_IS_BIT_SET(var, bit) ((var & (1 << bit)) != 0) // true if bit is set, false otherwise -#define OTEL_SETBIT(var, bit) (var |= (1 << bit)) -#define OTEL_RESETBIT(var, bit) (var &= (0 << bit)) - -#if 0 -namespace std { - template <> struct hash - { - size_t operator()(const opentelemetry::nostd::string_view &s) const - { - return std::hash()(s.data()); - /* your code here, e.g. "return hash()(x.value);" */ - } - }; -} -#endif - OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index 948175febc..aac5551041 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -112,8 +112,8 @@ class AttributesHashMap Aggregation *GetOrSetDefault(std::function()> aggregation_callback) { - auto hash = opentelemetry::sdk::common::GetHash(""); - auto it = hash_map_.find(hash); + static size_t hash = opentelemetry::sdk::common::GetHash(""); + auto it = hash_map_.find(hash); if (it != hash_map_.end()) { return it->second.second.get(); @@ -202,8 +202,6 @@ class AttributesHashMap private: std::unordered_map>> hash_map_; const AttributesProcessor *attributes_processor_; - - // std::unique_ptr attributes_hash_generator_; }; } // namespace metrics From a724644ea16e8b96b28d6d3a6981df50ecb18207 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 22:16:45 -0800 Subject: [PATCH 04/24] fix --- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 3 --- .../opentelemetry/sdk/metrics/view/attributes_processor.h | 4 ---- sdk/test/common/attributemap_hash_test.cc | 4 ++-- 3 files changed, 2 insertions(+), 9 deletions(-) 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 0a845c2573..ce60677143 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -75,7 +75,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, std::chrono::system_clock::now()); #endif - // auto keys_to_ignore = attributes_processor_->process(attributes); std::lock_guard guard(attribute_hashmap_lock_); attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_)->Aggregate(value); } @@ -112,7 +111,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, std::chrono::system_clock::now()); #endif - // auto keys_to_ignore = attributes_processor_->process(attributes); std::lock_guard guard(attribute_hashmap_lock_); attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_)->Aggregate(value); } @@ -125,7 +123,6 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage private: InstrumentDescriptor instrument_descriptor_; - // hashmap to maintain the metrics for delta collection (i.e, collection since last Collect call) std::unique_ptr attributes_hashmap_; // unreported metrics stash for all the collectors diff --git a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h index 18b5986e9b..ba6868a3ec 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h @@ -5,10 +5,6 @@ #include "opentelemetry/sdk/common/attribute_utils.h" -#define OTEL_IS_BIT_SET(var, bit) ((var & (1 << bit)) != 0) // true if bit is set, false otherwise -#define OTEL_SETBIT(var, bit) (var |= (1 << bit)) -#define OTEL_RESETBIT(var, bit) (var &= (0 << bit)) - OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { diff --git a/sdk/test/common/attributemap_hash_test.cc b/sdk/test/common/attributemap_hash_test.cc index d8a6319379..227b244dde 100644 --- a/sdk/test/common/attributemap_hash_test.cc +++ b/sdk/test/common/attributemap_hash_test.cc @@ -19,7 +19,7 @@ TEST(AttributeMapHashTest, BasicTests) } { - // hashmap algo is order insensitive. + // hash algo returns same value irrespective of order of attributes. OrderedAttributeMap map1 = {{"k1", 10}, {"k2", true}, {"k3", 12.22}}; OrderedAttributeMap map2 = {{"k3", 12.22}, {"k1", 10}, {"k2", true}}; EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); @@ -32,7 +32,7 @@ TEST(AttributeMapHashTest, BasicTests) } { - // true is treated as `1`, and false as `0` with hashmap algo. + // true is treated as `1`, and false as `0` with hash algo. OrderedAttributeMap map1 = {{"k1", 1}, {"k2", true}, {"k3", 1}}; OrderedAttributeMap map2 = {{"k2", 1}, {"k1", true}, {"k3", true}}; EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); From 44f7b44a0881fb15c535cc6e8d011d2b24c7ae61 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 22:21:13 -0800 Subject: [PATCH 05/24] fix --- sdk/test/metrics/BUILD | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 210d0c2c3f..cf31370d6a 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -330,3 +330,19 @@ otel_cc_benchmark( "//sdk/src/resource", ], ) + +otel_cc_benchmark( + name = "measurements_benchmark", + srcs = [ + "measurements_benchmark.cc", + ], + tags = [ + "benchmark", + "metrics", + "test", + ], + deps = [ + "//sdk/src/metrics", + "//sdk/src/resource", + ], +) From 9e8010636f3b40127eba0f288fe23ff73fc709b6 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 22:39:37 -0800 Subject: [PATCH 06/24] fix build error --- sdk/include/opentelemetry/sdk/common/attributemap_hash.h | 3 +-- .../opentelemetry/sdk/metrics/state/attributes_hashmap.h | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index b9ed4c1a26..711368a469 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -62,8 +62,7 @@ inline size_t GetHashForAttributeMap( nostd::function_ref is_key_present_callback) { AttributeConverter converter; - size_t seed = 0UL; - size_t index = 0; + size_t seed = 0UL; attributes.ForEachKeyValue( [&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept { if (!is_key_present_callback(key)) diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index aac5551041..e2362be623 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -118,8 +118,8 @@ class AttributesHashMap { return it->second.second.get(); } - - hash_map_[hash] = {{}, aggregation_callback()}; + MetricAttributes attr{}; + hash_map_[hash] = {attr, aggregation_callback()}; return hash_map_[hash].second.get(); } From 635df613ec34374e75c349359569888dcf21e32b Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 23:13:17 -0800 Subject: [PATCH 07/24] fix --- .../opentelemetry/sdk/metrics/view/attributes_processor.h | 2 +- sdk/test/metrics/measurements_benchmark.cc | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h index ba6868a3ec..c3328bfa1e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/attributes_processor.h @@ -46,7 +46,7 @@ class DefaultAttributesProcessor : public AttributesProcessor return result; } - bool isPresent(nostd::string_view key) const noexcept override { return true; } + bool isPresent(nostd::string_view /*key*/) const noexcept override { return true; } }; /** diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 1eaef525eb..6c89fc2f72 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -62,7 +62,6 @@ void BM_MeasurementsTest(benchmark::State &state) return true; }); }; - constexpr int NUM_CORES = 1; constexpr size_t MAX_MEASUREMENTS = 1000000; std::atomic measurements_processed{0l}; while (state.KeepRunning()) From bb8f6b5e3e4c8dd08646be408becc7398ae9a707 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 23:47:40 -0800 Subject: [PATCH 08/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 42 ++++++++-------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 6c89fc2f72..bd785e98ea 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,40 +45,28 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - std::vector actuals; std::vector collectionThreads; - std::function collectMetrics = [&exporter, &actuals]() { - exporter->Collect([&](ResourceMetrics &rm) { - for (const ScopeMetrics &smd : rm.scope_metric_data_) - { - for (const MetricData &md : smd.metric_data_) - { - for (const PointDataAttributes &dp : md.point_data_attr_) - { - actuals.push_back(opentelemetry::nostd::get(dp.point_data)); - } - } - } - return true; - }); - }; - constexpr size_t MAX_MEASUREMENTS = 1000000; - std::atomic measurements_processed{0l}; + constexpr size_t MAX_MEASUREMENTS = 1000000; + constexpr size_t POSSIBLE_ATTRIBUTES = 1000; + std::map attributes[POSSIBLE_ATTRIBUTES]; + size_t total_index = 0; + for (size_t i = 0; i < 10; i++) + { + for (size_t j = 0; j < 10; j++) + for (size_t k = 0; k < 10; k++) + attributes[total_index++] = {{"dim1", i}, {"dim2", j}, {"dim3", k}}; + } while (state.KeepRunning()) { - measurements_processed = 0; - while (measurements_processed++ <= MAX_MEASUREMENTS) + for (size_t i = 0; i < MAX_MEASUREMENTS; i++) { - size_t val1 = rand() % 10; - size_t val2 = rand() % 10; - size_t val3 = rand() % 10; - h->Add(1.0, {{"dim1", val1}, {"dim2", val2}, {"dim3", val3}}, - opentelemetry::context::Context{}); + size_t index = rand() % POSSIBLE_ATTRIBUTES; + h->Add(1.0, attributes[index], opentelemetry::context::Context{}); } } + exporter->Collect([&](ResourceMetrics &rm) { return true; }); } - BENCHMARK(BM_MeasurementsTest); } // namespace -BENCHMARK_MAIN(); \ No newline at end of file +BENCHMARK_MAIN(); From 8e871396f229d7eb658542486ffd17baba326c30 Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 20 Feb 2023 23:58:22 -0800 Subject: [PATCH 09/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index bd785e98ea..99411e3895 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,7 +45,6 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - std::vector collectionThreads; constexpr size_t MAX_MEASUREMENTS = 1000000; constexpr size_t POSSIBLE_ATTRIBUTES = 1000; std::map attributes[POSSIBLE_ATTRIBUTES]; @@ -61,7 +60,10 @@ void BM_MeasurementsTest(benchmark::State &state) for (size_t i = 0; i < MAX_MEASUREMENTS; i++) { size_t index = rand() % POSSIBLE_ATTRIBUTES; - h->Add(1.0, attributes[index], opentelemetry::context::Context{}); + h->Add(1.0, + opentelemetry::common::KeyValueIterableView>( + attributes[index]), + opentelemetry::context::Context{}); } } exporter->Collect([&](ResourceMetrics &rm) { return true; }); From 9b3f8cd85ee6451268b7039be067acbe6b47a5da Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 21 Feb 2023 08:12:44 -0800 Subject: [PATCH 10/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 99411e3895..a1d08e60a7 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -66,7 +66,7 @@ void BM_MeasurementsTest(benchmark::State &state) opentelemetry::context::Context{}); } } - exporter->Collect([&](ResourceMetrics &rm) { return true; }); + exporter->Collect([&](ResourceMetrics & /*rm*/) { return true; }); } BENCHMARK(BM_MeasurementsTest); From 08b0da0246d5f2d21ff43d0786bd03b3a4563730 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 21 Feb 2023 10:22:42 -0800 Subject: [PATCH 11/24] fix --- sdk/test/common/attributemap_hash_test.cc | 8 -------- 1 file changed, 8 deletions(-) diff --git a/sdk/test/common/attributemap_hash_test.cc b/sdk/test/common/attributemap_hash_test.cc index 227b244dde..a7a893f2a8 100644 --- a/sdk/test/common/attributemap_hash_test.cc +++ b/sdk/test/common/attributemap_hash_test.cc @@ -30,12 +30,4 @@ TEST(AttributeMapHashTest, BasicTests) OrderedAttributeMap map1 = {}; EXPECT_TRUE(GetHashForAttributeMap(map1) == 0); } - - { - // true is treated as `1`, and false as `0` with hash algo. - OrderedAttributeMap map1 = {{"k1", 1}, {"k2", true}, {"k3", 1}}; - OrderedAttributeMap map2 = {{"k2", 1}, {"k1", true}, {"k3", true}}; - EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2)); - EXPECT_TRUE(GetHashForAttributeMap(map1) != 0); - } } From 552a7f5abfec20387170404ce6e0ce18ba640f17 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 21 Feb 2023 15:04:35 -0800 Subject: [PATCH 12/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index a1d08e60a7..255500d5d8 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -47,12 +47,12 @@ void BM_MeasurementsTest(benchmark::State &state) auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); constexpr size_t MAX_MEASUREMENTS = 1000000; constexpr size_t POSSIBLE_ATTRIBUTES = 1000; - std::map attributes[POSSIBLE_ATTRIBUTES]; + std::map attributes[POSSIBLE_ATTRIBUTES]; size_t total_index = 0; - for (size_t i = 0; i < 10; i++) + for (uint64_t i = 0; i < 10; i++) { - for (size_t j = 0; j < 10; j++) - for (size_t k = 0; k < 10; k++) + for (uint64_t j = 0; j < 10; j++) + for (uint64_t k = 0; k < 10; k++) attributes[total_index++] = {{"dim1", i}, {"dim2", j}, {"dim3", k}}; } while (state.KeepRunning()) From 0faff5b325e1e24b0f20d7487b416c4a545edd51 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 21 Feb 2023 15:47:27 -0800 Subject: [PATCH 13/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 255500d5d8..e574187007 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -47,12 +47,12 @@ void BM_MeasurementsTest(benchmark::State &state) auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); constexpr size_t MAX_MEASUREMENTS = 1000000; constexpr size_t POSSIBLE_ATTRIBUTES = 1000; - std::map attributes[POSSIBLE_ATTRIBUTES]; + std::map attributes[POSSIBLE_ATTRIBUTES]; size_t total_index = 0; - for (uint64_t i = 0; i < 10; i++) + for (uint32_t i = 0; i < 10; i++) { - for (uint64_t j = 0; j < 10; j++) - for (uint64_t k = 0; k < 10; k++) + for (uint32_t j = 0; j < 10; j++) + for (uint32_t k = 0; k < 10; k++) attributes[total_index++] = {{"dim1", i}, {"dim2", j}, {"dim3", k}}; } while (state.KeepRunning()) @@ -61,7 +61,7 @@ void BM_MeasurementsTest(benchmark::State &state) { size_t index = rand() % POSSIBLE_ATTRIBUTES; h->Add(1.0, - opentelemetry::common::KeyValueIterableView>( + opentelemetry::common::KeyValueIterableView>( attributes[index]), opentelemetry::context::Context{}); } From e4a29f6e3f223eb829f8597995bbd152645485ba Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 21 Feb 2023 17:14:31 -0800 Subject: [PATCH 14/24] Fix --- sdk/test/metrics/BUILD | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index cf31370d6a..53c8f61e57 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -331,18 +331,18 @@ otel_cc_benchmark( ], ) -otel_cc_benchmark( - name = "measurements_benchmark", - srcs = [ - "measurements_benchmark.cc", - ], - tags = [ - "benchmark", - "metrics", - "test", - ], - deps = [ - "//sdk/src/metrics", - "//sdk/src/resource", - ], -) +#otel_cc_benchmark( +# name = "measurements_benchmark", +# srcs = [ +# "measurements_benchmark.cc", +# ], +# tags = [ +# "benchmark", +# "metrics", +# "test", +# ], +# deps = [ +# "//sdk/src/metrics", +# "//sdk/src/resource", +# ], +#) From 11a57c865c185aa90886af6d2ad1a554a88dd9c5 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 21 Feb 2023 18:20:47 -0800 Subject: [PATCH 15/24] fix --- .../sdk/metrics/state/attributes_hashmap.h | 39 +++++++++++++++---- sdk/test/metrics/BUILD | 30 +++++++------- 2 files changed, 46 insertions(+), 23 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index e2362be623..735c4ef443 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -35,16 +35,23 @@ class AttributeHashGenerator class AttributesHashMap { public: - AttributesHashMap( - const AttributesProcessor *attributes_processor = new DefaultAttributesProcessor()) + AttributesHashMap(const AttributesProcessor *attributes_processor = nullptr) : attributes_processor_{attributes_processor} {} Aggregation *Get(const opentelemetry::common::KeyValueIterable &attributes) const { auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, - [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + attributes, [this](nostd::string_view key) { + if (attributes_processor_) + { + return attributes_processor_->isPresent(key); + } + else + { + return true; + } + }); auto it = hash_map_.find(hash); if (it != hash_map_.end()) @@ -95,8 +102,16 @@ class AttributesHashMap { auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, - [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + attributes, [this](nostd::string_view key) { + if (attributes_processor_) + { + return attributes_processor_->isPresent(key); + } + else + { + return true; + } + }); auto it = hash_map_.find(hash); if (it != hash_map_.end()) @@ -147,8 +162,16 @@ class AttributesHashMap std::unique_ptr aggr) { auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, - [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); + attributes, [this](nostd::string_view key) { + if (attributes_processor_) + { + return attributes_processor_->isPresent(key); + } + else + { + return true; + } + }); auto it = hash_map_.find(hash); if (it != hash_map_.end()) { diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 53c8f61e57..cf31370d6a 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -331,18 +331,18 @@ otel_cc_benchmark( ], ) -#otel_cc_benchmark( -# name = "measurements_benchmark", -# srcs = [ -# "measurements_benchmark.cc", -# ], -# tags = [ -# "benchmark", -# "metrics", -# "test", -# ], -# deps = [ -# "//sdk/src/metrics", -# "//sdk/src/resource", -# ], -#) +otel_cc_benchmark( + name = "measurements_benchmark", + srcs = [ + "measurements_benchmark.cc", + ], + tags = [ + "benchmark", + "metrics", + "test", + ], + deps = [ + "//sdk/src/metrics", + "//sdk/src/resource", + ], +) From 29f730be1e2183a50d3dc70ba4cf4bc8d383da1a Mon Sep 17 00:00:00 2001 From: Lalit Date: Tue, 21 Feb 2023 19:31:29 -0800 Subject: [PATCH 16/24] fix CI timeout --- sdk/test/metrics/measurements_benchmark.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index a1d08e60a7..4872d40b14 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,7 +45,7 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - constexpr size_t MAX_MEASUREMENTS = 1000000; + constexpr size_t MAX_MEASUREMENTS = 10000; // keeping low value to avoid the CI timeouts. constexpr size_t POSSIBLE_ATTRIBUTES = 1000; std::map attributes[POSSIBLE_ATTRIBUTES]; size_t total_index = 0; From 5c4daee2a9b9213c5b4cc50b3dcacba9ff89e041 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 22 Feb 2023 18:51:21 -0800 Subject: [PATCH 17/24] Update sdk/include/opentelemetry/sdk/common/attributemap_hash.h Co-authored-by: Tom Tan --- sdk/include/opentelemetry/sdk/common/attributemap_hash.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 711368a469..610751db37 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -56,7 +56,7 @@ inline size_t GetHashForAttributeMap(const OrderedAttributeMap &attribute_map) return seed; } -// Calculate hash of keys and values of KeyValueIterable, filtered using callback. +// 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) From f76b7c1c83b500be0edb84215833b2d1e68fa06a Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 24 Feb 2023 13:31:59 -0800 Subject: [PATCH 18/24] improve mutex lock --- .../sdk/metrics/state/async_metric_storage.h | 12 ++- .../sdk/metrics/state/attributes_hashmap.h | 94 +++---------------- .../sdk/metrics/state/sync_metric_storage.h | 39 +++++++- .../metrics/state/temporal_metric_storage.cc | 15 +-- .../metrics/attributes_hashmap_benchmark.cc | 7 +- sdk/test/metrics/attributes_hashmap_test.cc | 34 ++++--- sdk/test/metrics/measurements_benchmark.cc | 28 ++++-- 7 files changed, 108 insertions(+), 121 deletions(-) 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 f601691fd1..aad00bb5cf 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -47,22 +47,24 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora { auto aggr = DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_); aggr->Aggregate(measurement.second); - auto prev = cumulative_hash_map_->Get(measurement.first); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(measurement.first); + auto prev = cumulative_hash_map_->Get(hash); 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)); - delta_hash_map_->Set(measurement.first, std::move(delta)); + cumulative_hash_map_->Set(measurement.first, std::move(aggr), hash); + delta_hash_map_->Set(measurement.first, std::move(delta), hash); } else { // store received value in cumulative and delta map. cumulative_hash_map_->Set( measurement.first, - DefaultAggregation::CloneAggregation(aggregation_type_, instrument_descriptor_, *aggr)); - delta_hash_map_->Set(measurement.first, std::move(aggr)); + DefaultAggregation::CloneAggregation(aggregation_type_, instrument_descriptor_, *aggr), + hash); + delta_hash_map_->Set(measurement.first, std::move(aggr), hash); } } } diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index 735c4ef443..754eba9396 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -35,24 +35,8 @@ class AttributeHashGenerator class AttributesHashMap { public: - AttributesHashMap(const AttributesProcessor *attributes_processor = nullptr) - : attributes_processor_{attributes_processor} - {} - - Aggregation *Get(const opentelemetry::common::KeyValueIterable &attributes) const + Aggregation *Get(size_t hash) const { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, [this](nostd::string_view key) { - if (attributes_processor_) - { - return attributes_processor_->isPresent(key); - } - else - { - return true; - } - }); - auto it = hash_map_.find(hash); if (it != hash_map_.end()) { @@ -61,36 +45,11 @@ class AttributesHashMap return nullptr; } - Aggregation *Get(const MetricAttributes &attributes) const - { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto it = hash_map_.find(hash); - if (it != hash_map_.end()) - { - return it->second.second.get(); - } - return nullptr; - } - /** * @return check if key is present in hash * */ - bool Has(const opentelemetry::common::KeyValueIterable &attributes) const - { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, - [this](nostd::string_view key) { return attributes_processor_->isPresent(key); }); - - return (hash_map_.find(hash) == hash_map_.end()) ? false : true; - } - - bool Has(const MetricAttributes &attributes) const - { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - - return (hash_map_.find(hash) == hash_map_.end()) ? false : true; - } + bool Has(size_t hash) const { return (hash_map_.find(hash) == hash_map_.end()) ? false : true; } /** * @return the pointer to value for given key if present. @@ -98,21 +57,9 @@ class AttributesHashMap * value and store in the hash */ Aggregation *GetOrSetDefault(const opentelemetry::common::KeyValueIterable &attributes, - std::function()> aggregation_callback) + std::function()> aggregation_callback, + size_t hash) { - - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, [this](nostd::string_view key) { - if (attributes_processor_) - { - return attributes_processor_->isPresent(key); - } - else - { - return true; - } - }); - auto it = hash_map_.find(hash); if (it != hash_map_.end()) { @@ -125,10 +72,10 @@ class AttributesHashMap return hash_map_[hash].second.get(); } - Aggregation *GetOrSetDefault(std::function()> aggregation_callback) + Aggregation *GetOrSetDefault(std::function()> aggregation_callback, + size_t hash) { - static size_t hash = opentelemetry::sdk::common::GetHash(""); - auto it = hash_map_.find(hash); + auto it = hash_map_.find(hash); if (it != hash_map_.end()) { return it->second.second.get(); @@ -139,10 +86,9 @@ class AttributesHashMap } Aggregation *GetOrSetDefault(const MetricAttributes &attributes, - std::function()> aggregation_callback) + std::function()> aggregation_callback, + size_t hash) { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto it = hash_map_.find(hash); if (it != hash_map_.end()) { @@ -159,19 +105,9 @@ class AttributesHashMap * Set the value for given key, overwriting the value if already present */ void Set(const opentelemetry::common::KeyValueIterable &attributes, - std::unique_ptr aggr) + std::unique_ptr aggr, + size_t hash) { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap( - attributes, [this](nostd::string_view key) { - if (attributes_processor_) - { - return attributes_processor_->isPresent(key); - } - else - { - return true; - } - }); auto it = hash_map_.find(hash); if (it != hash_map_.end()) { @@ -184,10 +120,9 @@ class AttributesHashMap } } - void Set(const MetricAttributes &attributes, std::unique_ptr aggr) + void Set(const MetricAttributes &attributes, std::unique_ptr aggr, size_t hash) { - auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); - auto it = hash_map_.find(hash); + auto it = hash_map_.find(hash); if (it != hash_map_.end()) { it->second.second = std::move(aggr); @@ -220,11 +155,8 @@ class AttributesHashMap */ size_t Size() { return hash_map_.size(); } - const AttributesProcessor *GetAttributesProcessor() { return attributes_processor_; } - private: std::unordered_map>> hash_map_; - const AttributesProcessor *attributes_processor_; }; } // namespace metrics 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 ce60677143..86476c32ee 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -34,7 +34,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage OPENTELEMETRY_MAYBE_UNUSED, const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), - attributes_hashmap_(new AttributesHashMap(attributes_processor)), + attributes_hashmap_(new AttributesHashMap()), + attributes_processor_(attributes_processor), #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_reservoir_(exemplar_reservoir), #endif @@ -58,8 +59,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); #endif + static size_t hash = opentelemetry::sdk::common::GetHash(""); std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(create_default_aggregation_, hash)->Aggregate(value); } void RecordLong(int64_t value, @@ -75,8 +77,21 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, 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, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_, hash) + ->Aggregate(value); } void RecordDouble(double value, @@ -90,8 +105,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW exemplar_reservoir_->OfferMeasurement(value, {}, context, std::chrono::system_clock::now()); #endif + static size_t hash = opentelemetry::sdk::common::GetHash(""); std::lock_guard guard(attribute_hashmap_lock_); - attributes_hashmap_->GetOrSetDefault(create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(create_default_aggregation_, hash)->Aggregate(value); } void RecordDouble(double value, @@ -111,8 +127,20 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage exemplar_reservoir_->OfferMeasurement(value, attributes, context, 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, create_default_aggregation_)->Aggregate(value); + attributes_hashmap_->GetOrSetDefault(attributes, create_default_aggregation_, hash) + ->Aggregate(value); } bool Collect(CollectorHandle *collector, @@ -136,6 +164,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage #endif TemporalMetricStorage temporal_metric_storage_; opentelemetry::common::SpinLockMutex attribute_hashmap_lock_; + const AttributesProcessor *attributes_processor_; }; } // namespace metrics diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 50803a80c0..1095c21418 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -60,17 +60,19 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, { agg_hashmap->GetAllEnteries( [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { - auto agg = merged_metrics->Get(attributes); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + auto agg = merged_metrics->Get(hash); if (agg) { - merged_metrics->Set(attributes, agg->Merge(aggregation)); + merged_metrics->Set(attributes, agg->Merge(aggregation), hash); } else { merged_metrics->Set(attributes, DefaultAggregation::CreateAggregation( aggregation_type_, instrument_descriptor_, aggregation_config_) - ->Merge(aggregation)); + ->Merge(aggregation), + hash); } return true; }); @@ -94,16 +96,17 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, // merge current delta to previous cumulative last_aggr_hashmap->GetAllEnteries( [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { - auto agg = merged_metrics->Get(attributes); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes); + auto agg = merged_metrics->Get(hash); if (agg) { - merged_metrics->Set(attributes, agg->Merge(aggregation)); + merged_metrics->Set(attributes, agg->Merge(aggregation), hash); } else { auto def_agg = DefaultAggregation::CreateAggregation( aggregation_type_, instrument_descriptor_, aggregation_config_); - merged_metrics->Set(attributes, def_agg->Merge(aggregation)); + merged_metrics->Set(attributes, def_agg->Merge(aggregation), hash); } return true; }); diff --git a/sdk/test/metrics/attributes_hashmap_benchmark.cc b/sdk/test/metrics/attributes_hashmap_benchmark.cc index 4d09cf0ce0..f0286ec09e 100644 --- a/sdk/test/metrics/attributes_hashmap_benchmark.cc +++ b/sdk/test/metrics/attributes_hashmap_benchmark.cc @@ -32,9 +32,10 @@ void BM_AttributseHashMap(benchmark::State &state) return std::unique_ptr(new DropAggregation); }; m.lock(); - - hash_map.GetOrSetDefault(attributes[i % 2], create_default_aggregation)->Aggregate((int64_t)1); - benchmark::DoNotOptimize(hash_map.Has(attributes[i % 2])); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(attributes[i % 2]); + hash_map.GetOrSetDefault(attributes[i % 2], create_default_aggregation, hash) + ->Aggregate((int64_t)1); + benchmark::DoNotOptimize(hash_map.Has(hash)); m.unlock(); }; while (state.KeepRunning()) diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index dd74d5c75b..956a76dfe1 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -18,31 +18,34 @@ TEST(AttributesHashMap, BasicTests) AttributesHashMap hash_map; EXPECT_EQ(hash_map.Size(), 0); MetricAttributes m1 = {{"k1", "v1"}}; - EXPECT_EQ(hash_map.Get(m1), nullptr); - EXPECT_EQ(hash_map.Has(m1), false); + auto hash = opentelemetry::sdk::common::GetHashForAttributeMap(m1); + + EXPECT_EQ(hash_map.Get(hash), nullptr); + EXPECT_EQ(hash_map.Has(hash), false); // Set std::unique_ptr aggregation1( new DropAggregation()); // = std::unique_ptr(new DropAggregation); - hash_map.Set(m1, std::move(aggregation1)); - EXPECT_NO_THROW(hash_map.Get(m1)->Aggregate((int64_t)1)); + hash_map.Set(m1, std::move(aggregation1), hash); + EXPECT_NO_THROW(hash_map.Get(hash)->Aggregate((int64_t)1)); EXPECT_EQ(hash_map.Size(), 1); - EXPECT_EQ(hash_map.Has(m1), true); + EXPECT_EQ(hash_map.Has(hash), true); // Set same key again auto aggregation2 = std::unique_ptr(new DropAggregation()); - hash_map.Set(m1, std::move(aggregation2)); - EXPECT_NO_THROW(hash_map.Get(m1)->Aggregate((int64_t)1)); + hash_map.Set(m1, std::move(aggregation2), hash); + EXPECT_NO_THROW(hash_map.Get(hash)->Aggregate((int64_t)1)); EXPECT_EQ(hash_map.Size(), 1); - EXPECT_EQ(hash_map.Has(m1), true); + EXPECT_EQ(hash_map.Has(hash), true); // Set more enteria auto aggregation3 = std::unique_ptr(new DropAggregation()); MetricAttributes m3 = {{"k1", "v1"}, {"k2", "v2"}}; - hash_map.Set(m3, std::move(aggregation3)); - EXPECT_EQ(hash_map.Has(m1), true); - EXPECT_EQ(hash_map.Has(m3), true); - EXPECT_NO_THROW(hash_map.Get(m3)->Aggregate((int64_t)1)); + 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); + EXPECT_NO_THROW(hash_map.Get(hash3)->Aggregate((int64_t)1)); EXPECT_EQ(hash_map.Size(), 2); // GetOrSetDefault @@ -51,12 +54,15 @@ TEST(AttributesHashMap, BasicTests) return std::unique_ptr(new DropAggregation); }; MetricAttributes m4 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}}; - EXPECT_NO_THROW(hash_map.GetOrSetDefault(m4, create_default_aggregation)->Aggregate((int64_t)1)); + auto hash4 = opentelemetry::sdk::common::GetHashForAttributeMap(m4); + EXPECT_NO_THROW( + hash_map.GetOrSetDefault(m4, create_default_aggregation, hash4)->Aggregate((int64_t)1)); EXPECT_EQ(hash_map.Size(), 3); // Set attributes with different order - shouldn't create a new entry. MetricAttributes m5 = {{"k2", "v2"}, {"k1", "v1"}}; - EXPECT_EQ(hash_map.Has(m5), true); + auto hash5 = opentelemetry::sdk::common::GetHashForAttributeMap(m5); + EXPECT_EQ(hash_map.Has(hash5), true); // GetAllEnteries size_t count = 0; diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 71d364c74c..e7b9f76783 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,8 +45,10 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - constexpr size_t MAX_MEASUREMENTS = 10000; // keeping low value to avoid the CI timeouts. + constexpr size_t MAX_MEASUREMENTS = 10000; // keep low to prevent CI failure due to timeout constexpr size_t POSSIBLE_ATTRIBUTES = 1000; + constexpr size_t NUM_CORES = 1; + std::vector threads; std::map attributes[POSSIBLE_ATTRIBUTES]; size_t total_index = 0; for (uint32_t i = 0; i < 10; i++) @@ -57,13 +59,25 @@ void BM_MeasurementsTest(benchmark::State &state) } while (state.KeepRunning()) { - for (size_t i = 0; i < MAX_MEASUREMENTS; i++) + threads.clear(); + std::atomic cur_processed{0}; + for (int i = 0; i < NUM_CORES; i++) { - size_t index = rand() % POSSIBLE_ATTRIBUTES; - h->Add(1.0, - opentelemetry::common::KeyValueIterableView>( - attributes[index]), - opentelemetry::context::Context{}); + threads.push_back( + std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &POSSIBLE_ATTRIBUTES, &attributes]() { + while (cur_processed++ <= MAX_MEASUREMENTS) + { + size_t index = rand() % POSSIBLE_ATTRIBUTES; + h->Add(1.0, + opentelemetry::common::KeyValueIterableView>( + attributes[index]), + opentelemetry::context::Context{}); + } + })); + } + for (auto &thread : threads) + { + thread.join(); } } exporter->Collect([&](ResourceMetrics & /*rm*/) { return true; }); From 41535607a75772e1b6f8671de80cbcda1094d061 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 24 Feb 2023 16:20:31 -0800 Subject: [PATCH 19/24] fix --- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 86476c32ee..b8146872e3 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -159,12 +159,12 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage // last reported metrics stash for all the collectors. std::unordered_map last_reported_metrics_; std::function()> create_default_aggregation_; + const AttributesProcessor *attributes_processor_; #ifdef ENABLE_METRICS_EXEMPLAR_PREVIEW nostd::shared_ptr exemplar_reservoir_; #endif TemporalMetricStorage temporal_metric_storage_; opentelemetry::common::SpinLockMutex attribute_hashmap_lock_; - const AttributesProcessor *attributes_processor_; }; } // namespace metrics From 0c0e28fb76c4c3e7c4b0ff515c9a7bbc3d656e3f Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 24 Feb 2023 23:04:48 -0800 Subject: [PATCH 20/24] fix warning --- sdk/test/metrics/measurements_benchmark.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index e7b9f76783..6f45d3e7c6 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -61,7 +61,7 @@ void BM_MeasurementsTest(benchmark::State &state) { threads.clear(); std::atomic cur_processed{0}; - for (int i = 0; i < NUM_CORES; i++) + for (size_t i = 0; i < NUM_CORES; i++) { threads.push_back( std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &POSSIBLE_ATTRIBUTES, &attributes]() { From 43b9ea29a04018e085c48c557e83318b45d73df2 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 24 Feb 2023 23:26:39 -0800 Subject: [PATCH 21/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 6f45d3e7c6..735ed0b558 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -63,17 +63,16 @@ void BM_MeasurementsTest(benchmark::State &state) std::atomic cur_processed{0}; for (size_t i = 0; i < NUM_CORES; i++) { - threads.push_back( - std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &POSSIBLE_ATTRIBUTES, &attributes]() { - while (cur_processed++ <= MAX_MEASUREMENTS) - { - size_t index = rand() % POSSIBLE_ATTRIBUTES; - h->Add(1.0, - opentelemetry::common::KeyValueIterableView>( - attributes[index]), - opentelemetry::context::Context{}); - } - })); + threads.push_back(std::thread([&h, &cur_processed, &POSSIBLE_ATTRIBUTES, &attributes]() { + while (cur_processed++ <= MAX_MEASUREMENTS) + { + size_t index = rand() % POSSIBLE_ATTRIBUTES; + h->Add(1.0, + opentelemetry::common::KeyValueIterableView>( + attributes[index]), + opentelemetry::context::Context{}); + } + })); } for (auto &thread : threads) { From 60d616179876e370aef40dd82996d6ea2f0751c7 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 24 Feb 2023 23:56:50 -0800 Subject: [PATCH 22/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 27 +++++++++++----------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 735ed0b558..4b98c93078 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,9 +45,9 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - constexpr size_t MAX_MEASUREMENTS = 10000; // keep low to prevent CI failure due to timeout - constexpr size_t POSSIBLE_ATTRIBUTES = 1000; - constexpr size_t NUM_CORES = 1; + size_t MAX_MEASUREMENTS = 10000; // keep low to prevent CI failure due to timeout + size_t POSSIBLE_ATTRIBUTES = 1000; + size_t NUM_CORES = 1; std::vector threads; std::map attributes[POSSIBLE_ATTRIBUTES]; size_t total_index = 0; @@ -63,16 +63,17 @@ void BM_MeasurementsTest(benchmark::State &state) std::atomic cur_processed{0}; for (size_t i = 0; i < NUM_CORES; i++) { - threads.push_back(std::thread([&h, &cur_processed, &POSSIBLE_ATTRIBUTES, &attributes]() { - while (cur_processed++ <= MAX_MEASUREMENTS) - { - size_t index = rand() % POSSIBLE_ATTRIBUTES; - h->Add(1.0, - opentelemetry::common::KeyValueIterableView>( - attributes[index]), - opentelemetry::context::Context{}); - } - })); + threads.push_back( + std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &POSSIBLE_ATTRIBUTES, &attributes]() { + while (cur_processed++ <= MAX_MEASUREMENTS) + { + size_t index = rand() % POSSIBLE_ATTRIBUTES; + h->Add(1.0, + opentelemetry::common::KeyValueIterableView>( + attributes[index]), + opentelemetry::context::Context{}); + } + })); } for (auto &thread : threads) { From d9653a33c8e0d33c7bbb525e9ba88dcdd82abebe Mon Sep 17 00:00:00 2001 From: Lalit Date: Sat, 25 Feb 2023 00:24:21 -0800 Subject: [PATCH 23/24] fix --- sdk/test/metrics/measurements_benchmark.cc | 28 ++++++++++------------ 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/sdk/test/metrics/measurements_benchmark.cc b/sdk/test/metrics/measurements_benchmark.cc index 4b98c93078..06ac75a65a 100644 --- a/sdk/test/metrics/measurements_benchmark.cc +++ b/sdk/test/metrics/measurements_benchmark.cc @@ -45,11 +45,10 @@ void BM_MeasurementsTest(benchmark::State &state) std::shared_ptr exporter(new MockMetricExporter()); mp.AddMetricReader(exporter); auto h = m->CreateDoubleCounter("counter1", "counter1_description", "counter1_unit"); - size_t MAX_MEASUREMENTS = 10000; // keep low to prevent CI failure due to timeout - size_t POSSIBLE_ATTRIBUTES = 1000; - size_t NUM_CORES = 1; + size_t MAX_MEASUREMENTS = 10000; // keep low to prevent CI failure due to timeout + size_t NUM_CORES = 1; std::vector threads; - std::map attributes[POSSIBLE_ATTRIBUTES]; + std::map attributes[1000]; size_t total_index = 0; for (uint32_t i = 0; i < 10; i++) { @@ -63,17 +62,16 @@ void BM_MeasurementsTest(benchmark::State &state) std::atomic cur_processed{0}; for (size_t i = 0; i < NUM_CORES; i++) { - threads.push_back( - std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &POSSIBLE_ATTRIBUTES, &attributes]() { - while (cur_processed++ <= MAX_MEASUREMENTS) - { - size_t index = rand() % POSSIBLE_ATTRIBUTES; - h->Add(1.0, - opentelemetry::common::KeyValueIterableView>( - attributes[index]), - opentelemetry::context::Context{}); - } - })); + threads.push_back(std::thread([&h, &cur_processed, &MAX_MEASUREMENTS, &attributes]() { + while (cur_processed++ <= MAX_MEASUREMENTS) + { + size_t index = rand() % 1000; + h->Add(1.0, + opentelemetry::common::KeyValueIterableView>( + attributes[index]), + opentelemetry::context::Context{}); + } + })); } for (auto &thread : threads) { From 4f227af7065ba00f6b2adbd6c0ff54051048140d Mon Sep 17 00:00:00 2001 From: Lalit Date: Mon, 27 Feb 2023 12:55:24 -0800 Subject: [PATCH 24/24] fix --- .../sdk/common/attributemap_hash.h | 18 ++++++------------ .../sdk/metrics/state/attributes_hashmap.h | 2 +- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h index 610751db37..086f6b7901 100644 --- a/sdk/include/opentelemetry/sdk/common/attributemap_hash.h +++ b/sdk/include/opentelemetry/sdk/common/attributemap_hash.h @@ -13,7 +13,7 @@ namespace common { template -inline void GetHashForAttributeValue(size_t &seed, const T &arg) +inline void GetHash(size_t &seed, const T &arg) { std::hash hasher; // reference - @@ -22,11 +22,11 @@ inline void GetHashForAttributeValue(size_t &seed, const T &arg) } template -inline void GetHashForAttributeValue(size_t &seed, const std::vector &arg) +inline void GetHash(size_t &seed, const std::vector &arg) { for (auto v : arg) { - GetHashForAttributeValue(seed, v); + GetHash(seed, v); } } @@ -36,7 +36,7 @@ struct GetHashForAttributeValueVisitor template void operator()(T &v) { - GetHashForAttributeValue(seed_, v); + GetHash(seed_, v); } size_t &seed_; }; @@ -47,10 +47,7 @@ inline size_t GetHashForAttributeMap(const OrderedAttributeMap &attribute_map) size_t seed = 0UL; for (auto &kv : attribute_map) { - std::hash hasher; - // reference - - // https://www.boost.org/doc/libs/1_37_0/doc/html/hash/reference.html#boost.hash_combine - seed ^= hasher(kv.first) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + GetHash(seed, kv.first); nostd::visit(GetHashForAttributeValueVisitor(seed), kv.second); } return seed; @@ -69,10 +66,7 @@ inline size_t GetHashForAttributeMap( { return true; } - std::hash hasher; - // reference - - // https://www.boost.org/doc/libs/1_37_0/doc/html/hash/reference.html#boost.hash_combine - seed ^= hasher(key.data()) + 0x9e3779b9 + (seed << 6) + (seed >> 2); + GetHash(seed, key.data()); auto attr_val = nostd::visit(converter, value); nostd::visit(GetHashForAttributeValueVisitor(seed), attr_val); return true; diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h index 754eba9396..a4b53a37c5 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h @@ -49,7 +49,7 @@ class AttributesHashMap * @return check if key is present in hash * */ - bool Has(size_t hash) const { return (hash_map_.find(hash) == hash_map_.end()) ? false : true; } + bool Has(size_t hash) const { return hash_map_.find(hash) != hash_map_.end(); } /** * @return the pointer to value for given key if present.