From bbf95ff374ef31475cb56de3f096ef2bc73e804c Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 14 Dec 2022 23:00:43 -0800 Subject: [PATCH 01/15] fix histogram bucket detection logic --- .../aggregation/histogram_aggregation.cc | 11 +- sdk/test/metrics/BUILD | 16 + sdk/test/metrics/CMakeLists.txt | 1 + sdk/test/metrics/aggregation_test.cc | 20 +- sdk/test/metrics/histogram_test.cc | 278 ++++++++++++++++++ 5 files changed, 312 insertions(+), 14 deletions(-) create mode 100644 sdk/test/metrics/histogram_test.cc diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 60dcc36827..35b117a882 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -8,6 +8,7 @@ #include #include "opentelemetry/version.h" +#include #include OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -62,13 +63,14 @@ void LongHistogramAggregation::Aggregate(int64_t value, size_t index = 0; for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) { - if (value < *it) + if (value <= *it) { point_data_.counts_[index] += 1; return; } index++; } + point_data_.counts_[point_data_.boundaries_.size()] += 1; } std::unique_ptr LongHistogramAggregation::Merge( @@ -107,8 +109,8 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig * } else { - point_data_.boundaries_ = - std::list{0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 1000.0}; + point_data_.boundaries_ = {0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, + 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0}; } if (ac) { @@ -144,13 +146,14 @@ void DoubleHistogramAggregation::Aggregate(double value, size_t index = 0; for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) { - if (value < *it) + if (value <= *it) { point_data_.counts_[index] += 1; return; } index++; } + point_data_.counts_[point_data_.boundaries_.size()] += 1; } std::unique_ptr DoubleHistogramAggregation::Merge( diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 294510b571..cb7ad5e2ba 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -47,6 +47,22 @@ cc_test( ], ) +cc_test( + name = "histogram_test", + srcs = [ + "histogram_test.cc", + ], + tags = [ + "metrics", + "test", + ], + deps = [ + "//sdk/src/metrics", + "//sdk/src/resource", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "view_registry_test", srcs = [ diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 2acbc6bd18..21da8bd778 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -6,6 +6,7 @@ foreach( aggregation_test attributes_processor_test attributes_hashmap_test + histogram_test sync_metric_storage_counter_test sync_metric_storage_histogram_test sync_metric_storage_up_down_counter_test diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index fffe56b104..895d398749 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -76,17 +76,17 @@ TEST(Aggregation, LongHistogramAggregation) ASSERT_TRUE(nostd::holds_alternative(histogram_data.sum_)); EXPECT_EQ(nostd::get(histogram_data.sum_), 0); EXPECT_EQ(histogram_data.count_, 0); - aggr.Aggregate((int64_t)12, {}); // lies in fourth bucket - aggr.Aggregate((int64_t)100, {}); // lies in eight bucket + aggr.Aggregate((int64_t)12, {}); // lies in third bucket + aggr.Aggregate((int64_t)100, {}); // lies in sixth bucket histogram_data = nostd::get(aggr.ToPoint()); EXPECT_EQ(nostd::get(histogram_data.min_), 12); EXPECT_EQ(nostd::get(histogram_data.max_), 100); EXPECT_EQ(nostd::get(histogram_data.sum_), 112); EXPECT_EQ(histogram_data.count_, 2); EXPECT_EQ(histogram_data.counts_[3], 1); - EXPECT_EQ(histogram_data.counts_[7], 1); - aggr.Aggregate((int64_t)13, {}); // lies in fourth bucket - aggr.Aggregate((int64_t)252, {}); // lies in ninth bucket + EXPECT_EQ(histogram_data.counts_[6], 1); + aggr.Aggregate((int64_t)13, {}); // lies in third bucket + aggr.Aggregate((int64_t)252, {}); // lies in eigth bucket histogram_data = nostd::get(aggr.ToPoint()); EXPECT_EQ(histogram_data.count_, 4); EXPECT_EQ(histogram_data.counts_[3], 2); @@ -165,17 +165,17 @@ TEST(Aggregation, DoubleHistogramAggregation) ASSERT_TRUE(nostd::holds_alternative(histogram_data.sum_)); EXPECT_EQ(nostd::get(histogram_data.sum_), 0); EXPECT_EQ(histogram_data.count_, 0); - aggr.Aggregate(12.0, {}); // lies in fourth bucket - aggr.Aggregate(100.0, {}); // lies in eight bucket + aggr.Aggregate(12.0, {}); // lies in third bucket + aggr.Aggregate(100.0, {}); // lies in sixth bucket histogram_data = nostd::get(aggr.ToPoint()); EXPECT_EQ(nostd::get(histogram_data.sum_), 112); EXPECT_EQ(histogram_data.count_, 2); EXPECT_EQ(histogram_data.counts_[3], 1); - EXPECT_EQ(histogram_data.counts_[7], 1); + EXPECT_EQ(histogram_data.counts_[6], 1); EXPECT_EQ(nostd::get(histogram_data.min_), 12); EXPECT_EQ(nostd::get(histogram_data.max_), 100); - aggr.Aggregate(13.0, {}); // lies in fourth bucket - aggr.Aggregate(252.0, {}); // lies in ninth bucket + aggr.Aggregate(13.0, {}); // lies in third bucket + aggr.Aggregate(252.0, {}); // lies in eight bucket histogram_data = nostd::get(aggr.ToPoint()); EXPECT_EQ(histogram_data.count_, 4); EXPECT_EQ(histogram_data.counts_[3], 2); diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc new file mode 100644 index 0000000000..7cae5a05d3 --- /dev/null +++ b/sdk/test/metrics/histogram_test.cc @@ -0,0 +1,278 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include "opentelemetry/sdk/metrics/data/point_data.h" +#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 + +using namespace opentelemetry; +using namespace opentelemetry::sdk::instrumentationscope; +using namespace opentelemetry::sdk::metrics; + +class MockMetricExporter : public PushMetricExporter +{ +public: + MockMetricExporter() = default; + opentelemetry::sdk::common::ExportResult Export( + const ResourceMetrics & /* records */) noexcept override + { + return opentelemetry::sdk::common::ExportResult::kSuccess; + } + + AggregationTemporality GetAggregationTemporality( + InstrumentType /* instrument_type */) const noexcept override + { + return AggregationTemporality::kCumulative; + } + + bool ForceFlush(std::chrono::microseconds /* timeout */) noexcept override { return true; } + + bool Shutdown(std::chrono::microseconds /* timeout */) noexcept override { return true; } +}; + +class MockMetricReader : public MetricReader +{ +public: + MockMetricReader(std::unique_ptr exporter) : exporter_(std::move(exporter)) {} + AggregationTemporality GetAggregationTemporality( + InstrumentType instrument_type) const noexcept override + { + return exporter_->GetAggregationTemporality(instrument_type); + } + virtual bool OnForceFlush(std::chrono::microseconds /* timeout */) noexcept override + { + return true; + } + virtual bool OnShutDown(std::chrono::microseconds /* timeout */) noexcept override + { + return true; + } + virtual void OnInitialized() noexcept override {} + +private: + std::unique_ptr exporter_; +}; + +TEST(Histogram, Double) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + auto h = m->CreateDoubleHistogram("histogram1", "histogram1_description", "histogram1_unit"); + + h->Record(5, {}); + h->Record(10, {}); + h->Record(15, {}); + h->Record(20, {}); + h->Record(25, {}); + h->Record(30, {}); + h->Record(35, {}); + h->Record(40, {}); + h->Record(45, {}); + h->Record(50, {}); + h->Record(1e6, {}); + + std::vector actuals; + reader->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; + }); + + ASSERT_EQ(1, actuals.size()); + + const auto &actual = actuals.at(0); + ASSERT_EQ(1000275.0, opentelemetry::nostd::get(actual.sum_)); + ASSERT_EQ(11, actual.count_); + ASSERT_EQ(5.0, opentelemetry::nostd::get(actual.min_)); + ASSERT_EQ(1e6, opentelemetry::nostd::get(actual.max_)); + ASSERT_EQ( + std::list({0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), + actual.boundaries_); + ASSERT_EQ(std::vector({0, 1, 1, 3, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}), + actual.counts_); +} + +TEST(Histogram, DoubleCustomBuckets) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + std::shared_ptr config(new HistogramAggregationConfig()); + config->boundaries_ = {10, 20, 30, 40}; + std::unique_ptr view{ + new View("view1", "view1_description", AggregationType::kHistogram, config)}; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kHistogram, "histogram1")}; + std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; + mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); + + auto h = m->CreateDoubleHistogram("histogram1", "histogram1_description", "histogram1_unit"); + + h->Record(5, {}); + h->Record(10, {}); + h->Record(15, {}); + h->Record(20, {}); + h->Record(25, {}); + h->Record(30, {}); + h->Record(35, {}); + h->Record(40, {}); + h->Record(45, {}); + h->Record(50, {}); + + std::vector actuals; + reader->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; + }); + + ASSERT_EQ(1, actuals.size()); + + const auto &actual = actuals.at(0); + ASSERT_EQ(275.0, opentelemetry::nostd::get(actual.sum_)); + ASSERT_EQ(10, actual.count_); + ASSERT_EQ(5.0, opentelemetry::nostd::get(actual.min_)); + ASSERT_EQ(50.0, opentelemetry::nostd::get(actual.max_)); + ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); + ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); +} + +TEST(Histogram, UInt64) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + auto h = m->CreateUInt64Histogram("histogram1", "histogram1_description", "histogram1_unit"); + + h->Record(5, {}); + h->Record(10, {}); + h->Record(15, {}); + h->Record(20, {}); + h->Record(25, {}); + h->Record(30, {}); + h->Record(35, {}); + h->Record(40, {}); + h->Record(45, {}); + h->Record(50, {}); + h->Record(1e6, {}); + + std::vector actuals; + reader->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; + }); + + ASSERT_EQ(1, actuals.size()); + + const auto &actual = actuals.at(0); + ASSERT_EQ(1000275, opentelemetry::nostd::get(actual.sum_)); + ASSERT_EQ(11, actual.count_); + ASSERT_EQ(5, opentelemetry::nostd::get(actual.min_)); + ASSERT_EQ(1000000, opentelemetry::nostd::get(actual.max_)); + ASSERT_EQ( + std::list({0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), + actual.boundaries_); + ASSERT_EQ(std::vector({0, 1, 1, 3, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}), + actual.counts_); +} + +TEST(Histogram, UInt64CustomBuckets) +{ + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + + std::shared_ptr config(new HistogramAggregationConfig()); + config->boundaries_ = {10, 20, 30, 40}; + std::unique_ptr view{ + new View("view1", "view1_description", AggregationType::kHistogram, config)}; + std::unique_ptr instrument_selector{ + new InstrumentSelector(InstrumentType::kHistogram, "histogram1")}; + std::unique_ptr meter_selector{new MeterSelector("meter1", "version1", "schema1")}; + mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); + + auto h = m->CreateUInt64Histogram("histogram1", "histogram1_description", "histogram1_unit"); + + h->Record(5, {}); + h->Record(10, {}); + h->Record(15, {}); + h->Record(20, {}); + h->Record(25, {}); + h->Record(30, {}); + h->Record(35, {}); + h->Record(40, {}); + h->Record(45, {}); + h->Record(50, {}); + + std::vector actuals; + reader->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; + }); + + ASSERT_EQ(1, actuals.size()); + + const auto &actual = actuals.at(0); + ASSERT_EQ(275, opentelemetry::nostd::get(actual.sum_)); + ASSERT_EQ(10, actual.count_); + ASSERT_EQ(5, opentelemetry::nostd::get(actual.min_)); + ASSERT_EQ(50, opentelemetry::nostd::get(actual.max_)); + ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); + ASSERT_EQ(std::vector({2, 2, 2, 2, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}), + actual.counts_); +} \ No newline at end of file From 42ee9afc920d78844a137d917a280c117d28fc33 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 14 Dec 2022 23:09:40 -0800 Subject: [PATCH 02/15] Fix --- sdk/test/metrics/histogram_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc index 7cae5a05d3..37180d5e11 100644 --- a/sdk/test/metrics/histogram_test.cc +++ b/sdk/test/metrics/histogram_test.cc @@ -275,4 +275,4 @@ TEST(Histogram, UInt64CustomBuckets) ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); ASSERT_EQ(std::vector({2, 2, 2, 2, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}), actual.counts_); -} \ No newline at end of file +} From 6ce55d144d554f79c2baf1d17b00c2eb90a035b0 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 14 Dec 2022 23:11:15 -0800 Subject: [PATCH 03/15] fix header order --- sdk/src/metrics/aggregation/histogram_aggregation.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 35b117a882..6df289022c 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -2,14 +2,14 @@ // SPDX-License-Identifier: Apache-2.0 #include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" +#include "opentelemetry/version.h" + #include #include #include #include -#include "opentelemetry/version.h" - -#include #include + OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { From 17f3842098f36b4b6d87001594c4ec2b2e73d276 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 14 Dec 2022 23:21:10 -0800 Subject: [PATCH 04/15] use index variable for last bucket --- sdk/src/metrics/aggregation/histogram_aggregation.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 6df289022c..5f66bff50a 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -70,7 +70,8 @@ void LongHistogramAggregation::Aggregate(int64_t value, } index++; } - point_data_.counts_[point_data_.boundaries_.size()] += 1; + // value belongs to the last bucket. + point_data_.counts_[index] += 1; } std::unique_ptr LongHistogramAggregation::Merge( @@ -153,7 +154,8 @@ void DoubleHistogramAggregation::Aggregate(double value, } index++; } - point_data_.counts_[point_data_.boundaries_.size()] += 1; + // value belongs to the last bucket. + point_data_.counts_[index] += 1; } std::unique_ptr DoubleHistogramAggregation::Merge( From 6aa93d19702da2193107fa9804ac0e6c818e8bf2 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 14 Dec 2022 23:24:52 -0800 Subject: [PATCH 05/15] spell check --- sdk/test/metrics/aggregation_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 895d398749..83efe68f36 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -86,7 +86,7 @@ TEST(Aggregation, LongHistogramAggregation) EXPECT_EQ(histogram_data.counts_[3], 1); EXPECT_EQ(histogram_data.counts_[6], 1); aggr.Aggregate((int64_t)13, {}); // lies in third bucket - aggr.Aggregate((int64_t)252, {}); // lies in eigth bucket + aggr.Aggregate((int64_t)252, {}); // lies in eight bucket histogram_data = nostd::get(aggr.ToPoint()); EXPECT_EQ(histogram_data.count_, 4); EXPECT_EQ(histogram_data.counts_[3], 2); From 885940c1f6fac230c11f0aebd35dba7252b98f86 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Dec 2022 00:21:50 -0800 Subject: [PATCH 06/15] Fix --- sdk/test/metrics/histogram_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc index 37180d5e11..2aa14e3a35 100644 --- a/sdk/test/metrics/histogram_test.cc +++ b/sdk/test/metrics/histogram_test.cc @@ -188,7 +188,7 @@ TEST(Histogram, UInt64) h->Record(40, {}); h->Record(45, {}); h->Record(50, {}); - h->Record(1e6, {}); + h->Record(1000000, {}); std::vector actuals; reader->Collect([&](ResourceMetrics &rm) { From 450e46734db3b18a9e7607e9e38e2f557e61ddb2 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Dec 2022 19:06:07 -0800 Subject: [PATCH 07/15] pass config to histogram agg --- .../aggregation/histogram_aggregation.cc | 25 ++++++++++++------- sdk/test/metrics/histogram_test.cc | 3 +-- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 5f66bff50a..b020a06b45 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -80,7 +80,10 @@ std::unique_ptr LongHistogramAggregation::Merge( auto curr_value = nostd::get(ToPoint()); auto delta_value = nostd::get( (static_cast(delta).ToPoint())); - LongHistogramAggregation *aggr = new LongHistogramAggregation(); + HistogramAggregationConfig agg_config; + agg_config.boundaries_ = curr_value.boundaries_; + agg_config.record_min_max_ = record_min_max_; + LongHistogramAggregation *aggr = new LongHistogramAggregation(&agg_config); HistogramMerge(curr_value, delta_value, aggr->point_data_); return std::unique_ptr(aggr); } @@ -90,7 +93,10 @@ std::unique_ptr LongHistogramAggregation::Diff(const Aggregation &n auto curr_value = nostd::get(ToPoint()); auto next_value = nostd::get( (static_cast(next).ToPoint())); - LongHistogramAggregation *aggr = new LongHistogramAggregation(); + HistogramAggregationConfig agg_config; + agg_config.boundaries_ = curr_value.boundaries_; + agg_config.record_min_max_ = record_min_max_; + LongHistogramAggregation *aggr = new LongHistogramAggregation(&agg_config); HistogramDiff(curr_value, next_value, aggr->point_data_); return std::unique_ptr(aggr); } @@ -164,12 +170,10 @@ std::unique_ptr DoubleHistogramAggregation::Merge( auto curr_value = nostd::get(ToPoint()); auto delta_value = nostd::get( (static_cast(delta).ToPoint())); - std::shared_ptr aggregation_config(new HistogramAggregationConfig); - static_cast(aggregation_config.get()) - ->boundaries_ = curr_value.boundaries_; - static_cast(aggregation_config.get()) - ->record_min_max_ = record_min_max_; - DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(aggregation_config.get()); + HistogramAggregationConfig agg_config; + agg_config.boundaries_ = curr_value.boundaries_; + agg_config.record_min_max_ = record_min_max_; + DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(&agg_config); HistogramMerge(curr_value, delta_value, aggr->point_data_); return std::unique_ptr(aggr); } @@ -180,7 +184,10 @@ std::unique_ptr DoubleHistogramAggregation::Diff( auto curr_value = nostd::get(ToPoint()); auto next_value = nostd::get( (static_cast(next).ToPoint())); - DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(); + HistogramAggregationConfig agg_config; + agg_config.boundaries_ = curr_value.boundaries_; + agg_config.record_min_max_ = record_min_max_; + DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(&agg_config); HistogramDiff(curr_value, next_value, aggr->point_data_); return std::unique_ptr(aggr); } diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc index 2aa14e3a35..2978681dc2 100644 --- a/sdk/test/metrics/histogram_test.cc +++ b/sdk/test/metrics/histogram_test.cc @@ -273,6 +273,5 @@ TEST(Histogram, UInt64CustomBuckets) ASSERT_EQ(5, opentelemetry::nostd::get(actual.min_)); ASSERT_EQ(50, opentelemetry::nostd::get(actual.max_)); ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); - ASSERT_EQ(std::vector({2, 2, 2, 2, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}), - actual.counts_); + ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); } From a21805eeeb4c1bbedfffcbd043423884df9eacd8 Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 15 Dec 2022 23:25:23 -0800 Subject: [PATCH 08/15] remove custom histogram tests for gcc4.8 --- sdk/test/metrics/histogram_test.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc index 2978681dc2..b6e6a620ed 100644 --- a/sdk/test/metrics/histogram_test.cc +++ b/sdk/test/metrics/histogram_test.cc @@ -1,6 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include "opentelemetry/common/macros.h" #include "opentelemetry/sdk/metrics/data/point_data.h" #include "opentelemetry/sdk/metrics/meter.h" #include "opentelemetry/sdk/metrics/meter_context.h" @@ -110,6 +111,8 @@ TEST(Histogram, Double) actual.counts_); } +#if (HAVE_WORKING_REGEX) +// FIXME - View Preficate search is only supported through regex TEST(Histogram, DoubleCustomBuckets) { MeterProvider mp; @@ -166,6 +169,7 @@ TEST(Histogram, DoubleCustomBuckets) ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); } +#endif TEST(Histogram, UInt64) { @@ -219,6 +223,8 @@ TEST(Histogram, UInt64) actual.counts_); } +#if (HAVE_WORKING_REGEX) +// FIXME - View Preficate search is only supported through regex TEST(Histogram, UInt64CustomBuckets) { MeterProvider mp; @@ -275,3 +281,4 @@ TEST(Histogram, UInt64CustomBuckets) ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); } +#endif From 7cc3a914b92a2c0232861cd3367ca218b6953155 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Dec 2022 15:19:56 -0800 Subject: [PATCH 09/15] histogram performance improvements --- examples/metrics_simple/metrics_ostream.cc | 4 ++-- exporters/ostream/test/ostream_metric_test.cc | 4 ++-- .../metrics/aggregation/aggregation_config.h | 6 +++-- .../aggregation/histogram_aggregation.h | 20 ++++++++++++++++ .../sdk/metrics/data/point_data.h | 18 +++++++------- .../metrics/state/temporal_metric_storage.h | 1 + .../aggregation/histogram_aggregation.cc | 24 ++----------------- sdk/test/metrics/CMakeLists.txt | 7 ++++++ sdk/test/metrics/aggregation_test.cc | 12 +++++----- sdk/test/metrics/histogram_test.cc | 16 ++++++------- 10 files changed, 61 insertions(+), 51 deletions(-) diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index a0403d53ba..a70c80311b 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -75,8 +75,8 @@ void initMetrics(const std::string &name) std::shared_ptr aggregation_config{ new opentelemetry::sdk::metrics::HistogramAggregationConfig}; static_cast(aggregation_config.get()) - ->boundaries_ = std::list{0.0, 50.0, 100.0, 250.0, 500.0, 750.0, - 1000.0, 2500.0, 5000.0, 10000.0, 20000.0}; + ->boundaries_ = std::vector{0.0, 50.0, 100.0, 250.0, 500.0, 750.0, + 1000.0, 2500.0, 5000.0, 10000.0, 20000.0}; std::unique_ptr histogram_view{new metric_sdk::View{ name, "description", metric_sdk::AggregationType::kHistogram, aggregation_config}}; p->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), diff --git a/exporters/ostream/test/ostream_metric_test.cc b/exporters/ostream/test/ostream_metric_test.cc index 9aca741d67..cb4a8a3ab4 100644 --- a/exporters/ostream/test/ostream_metric_test.cc +++ b/exporters/ostream/test/ostream_metric_test.cc @@ -97,14 +97,14 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) std::unique_ptr(new exportermetrics::OStreamMetricExporter); metric_sdk::HistogramPointData histogram_point_data{}; - histogram_point_data.boundaries_ = std::list{10.1, 20.2, 30.2}; + histogram_point_data.boundaries_ = std::vector{10.1, 20.2, 30.2}; histogram_point_data.count_ = 3; histogram_point_data.counts_ = {200, 300, 400, 500}; histogram_point_data.sum_ = 900.5; histogram_point_data.min_ = 1.8; histogram_point_data.max_ = 12.0; metric_sdk::HistogramPointData histogram_point_data2{}; - histogram_point_data2.boundaries_ = std::list{10.0, 20.0, 30.0}; + histogram_point_data2.boundaries_ = std::vector{10.0, 20.0, 30.0}; histogram_point_data2.count_ = 3; histogram_point_data2.counts_ = {200, 300, 400, 500}; histogram_point_data2.sum_ = (int64_t)900; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index ee297881d0..69dc4c8864 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -3,8 +3,10 @@ #pragma once -#include #include "opentelemetry/version.h" + +#include + OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { @@ -19,7 +21,7 @@ class AggregationConfig class HistogramAggregationConfig : public AggregationConfig { public: - std::list boundaries_; + std::vector boundaries_; bool record_min_max_ = true; }; } // namespace metrics diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index ae9eeb4ded..10ac6c38b6 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -108,6 +108,26 @@ void HistogramDiff(HistogramPointData ¤t, HistogramPointData &next, Histog diff.record_min_max_ = false; } +template +size_t BucketBinarySearch(T value, std::vector &boundaries) +{ + size_t lower_bound = 0; + size_t upper_bound = boundaries.size(); + while (lower_bound != upper_bound) + { + size_t mid = (lower_bound + upper_bound) / 2; + if (value > boundaries[mid]) + { + lower_bound = mid + 1; + } + else + { + upper_bound = mid; + } + } + return lower_bound; +} + } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index 359da0b864..62ba3df1bc 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -8,7 +8,7 @@ #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/version.h" -#include +#include OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -55,14 +55,14 @@ class HistogramPointData HistogramPointData &operator=(HistogramPointData &&) = default; HistogramPointData(const HistogramPointData &) = default; HistogramPointData() = default; - HistogramPointData(std::list &boundaries) : boundaries_(boundaries) {} - std::list boundaries_ = {}; - ValueType sum_ = {}; - ValueType min_ = {}; - ValueType max_ = {}; - std::vector counts_ = {}; - uint64_t count_ = {}; - bool record_min_max_ = true; + HistogramPointData(std::vector &boundaries) : boundaries_(boundaries) {} + std::vector boundaries_ = {}; + ValueType sum_ = {}; + ValueType min_ = {}; + ValueType max_ = {}; + std::vector counts_ = {}; + uint64_t count_ = {}; + bool record_min_max_ = true; }; class DropPointData diff --git a/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h index 439ca3cd4d..17fe28d8bf 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h @@ -8,6 +8,7 @@ #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" #include "opentelemetry/sdk/metrics/state/metric_collector.h" +#include #include OPENTELEMETRY_BEGIN_NAMESPACE diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index b020a06b45..fe01030f13 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -60,17 +60,7 @@ void LongHistogramAggregation::Aggregate(int64_t value, point_data_.min_ = std::min(nostd::get(point_data_.min_), value); point_data_.max_ = std::max(nostd::get(point_data_.max_), value); } - size_t index = 0; - for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) - { - if (value <= *it) - { - point_data_.counts_[index] += 1; - return; - } - index++; - } - // value belongs to the last bucket. + size_t index = BucketBinarySearch(value, point_data_.boundaries_); point_data_.counts_[index] += 1; } @@ -150,17 +140,7 @@ void DoubleHistogramAggregation::Aggregate(double value, point_data_.min_ = std::min(nostd::get(point_data_.min_), value); point_data_.max_ = std::max(nostd::get(point_data_.max_), value); } - size_t index = 0; - for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) - { - if (value <= *it) - { - point_data_.counts_[index] += 1; - return; - } - index++; - } - // value belongs to the last bucket. + size_t index = BucketBinarySearch(value, point_data_.boundaries_); point_data_.counts_[index] += 1; } diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 21da8bd778..585023ebbf 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -38,6 +38,13 @@ if(WITH_BENCHMARK) add_executable(attributes_hashmap_benchmark attributes_hashmap_benchmark.cc) target_link_libraries(attributes_hashmap_benchmark benchmark::benchmark ${CMAKE_THREAD_LIBS_INIT} opentelemetry_common) + + add_executable(histogram_aggregation_benchmark + histogram_aggregation_benchmark.cc) + target_link_libraries( + histogram_aggregation_benchmark benchmark::benchmark + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_common opentelemetry_metrics + opentelemetry_resources) endif() add_subdirectory(exemplar) diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 83efe68f36..9f307cb015 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -132,9 +132,9 @@ TEST(Aggregation, LongHistogramAggregationBoundaries) { std::shared_ptr aggregation_config{ new opentelemetry::sdk::metrics::HistogramAggregationConfig}; - std::list user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, - 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; - aggregation_config->boundaries_ = user_boundaries; + std::vector user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, + 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; + aggregation_config->boundaries_ = user_boundaries; LongHistogramAggregation aggr{aggregation_config.get()}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); @@ -146,9 +146,9 @@ TEST(Aggregation, DoubleHistogramAggregationBoundaries) { std::shared_ptr aggregation_config{ new opentelemetry::sdk::metrics::HistogramAggregationConfig}; - std::list user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, - 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; - aggregation_config->boundaries_ = user_boundaries; + std::vector user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, + 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; + aggregation_config->boundaries_ = user_boundaries; DoubleHistogramAggregation aggr{aggregation_config.get()}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc index b6e6a620ed..ec77d99733 100644 --- a/sdk/test/metrics/histogram_test.cc +++ b/sdk/test/metrics/histogram_test.cc @@ -104,9 +104,9 @@ TEST(Histogram, Double) ASSERT_EQ(11, actual.count_); ASSERT_EQ(5.0, opentelemetry::nostd::get(actual.min_)); ASSERT_EQ(1e6, opentelemetry::nostd::get(actual.max_)); - ASSERT_EQ( - std::list({0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), - actual.boundaries_); + ASSERT_EQ(std::vector( + {0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), + actual.boundaries_); ASSERT_EQ(std::vector({0, 1, 1, 3, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}), actual.counts_); } @@ -166,7 +166,7 @@ TEST(Histogram, DoubleCustomBuckets) ASSERT_EQ(10, actual.count_); ASSERT_EQ(5.0, opentelemetry::nostd::get(actual.min_)); ASSERT_EQ(50.0, opentelemetry::nostd::get(actual.max_)); - ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); + ASSERT_EQ(std::vector({10, 20, 30, 40}), actual.boundaries_); ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); } #endif @@ -216,9 +216,9 @@ TEST(Histogram, UInt64) ASSERT_EQ(11, actual.count_); ASSERT_EQ(5, opentelemetry::nostd::get(actual.min_)); ASSERT_EQ(1000000, opentelemetry::nostd::get(actual.max_)); - ASSERT_EQ( - std::list({0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), - actual.boundaries_); + ASSERT_EQ(std::vector( + {0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000}), + actual.boundaries_); ASSERT_EQ(std::vector({0, 1, 1, 3, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}), actual.counts_); } @@ -278,7 +278,7 @@ TEST(Histogram, UInt64CustomBuckets) ASSERT_EQ(10, actual.count_); ASSERT_EQ(5, opentelemetry::nostd::get(actual.min_)); ASSERT_EQ(50, opentelemetry::nostd::get(actual.max_)); - ASSERT_EQ(std::list({10, 20, 30, 40}), actual.boundaries_); + ASSERT_EQ(std::vector({10, 20, 30, 40}), actual.boundaries_); ASSERT_EQ(std::vector({2, 2, 2, 2, 2}), actual.counts_); } #endif From 159a7cd5fd58171058c9d0e55391c38961a8ce6f Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Dec 2022 15:23:40 -0800 Subject: [PATCH 10/15] add benchmark test --- .../histogram_aggregation_benchmark.cc | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 sdk/test/metrics/histogram_aggregation_benchmark.cc diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc new file mode 100644 index 0000000000..0760aaa734 --- /dev/null +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -0,0 +1,88 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include + +#include "opentelemetry/sdk/metrics/data/point_data.h" +#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 + +using namespace opentelemetry; +using namespace opentelemetry::sdk::instrumentationscope; +using namespace opentelemetry::sdk::metrics; + +class MockMetricExporter : public PushMetricExporter +{ +public: + MockMetricExporter() = default; + opentelemetry::sdk::common::ExportResult Export( + const ResourceMetrics & /* records */) noexcept override + { + return opentelemetry::sdk::common::ExportResult::kSuccess; + } + + AggregationTemporality GetAggregationTemporality( + InstrumentType /* instrument_type */) const noexcept override + { + return AggregationTemporality::kCumulative; + } + + bool ForceFlush(std::chrono::microseconds /* timeout */) noexcept override { return true; } + + bool Shutdown(std::chrono::microseconds /* timeout */) noexcept override { return true; } +}; + +class MockMetricReader : public MetricReader +{ +public: + MockMetricReader(std::unique_ptr exporter) : exporter_(std::move(exporter)) {} + AggregationTemporality GetAggregationTemporality( + InstrumentType instrument_type) const noexcept override + { + return exporter_->GetAggregationTemporality(instrument_type); + } + virtual bool OnForceFlush(std::chrono::microseconds /* timeout */) noexcept override + { + return true; + } + virtual bool OnShutDown(std::chrono::microseconds /* timeout */) noexcept override + { + return true; + } + virtual void OnInitialized() noexcept override {} + +private: + std::unique_ptr exporter_; +}; + +namespace +{ +void BM_HistogramAggregation(benchmark::State &state) +{ + + MeterProvider mp; + auto m = mp.GetMeter("meter1", "version1", "schema1"); + + std::unique_ptr exporter(new MockMetricExporter()); + std::shared_ptr reader{new MockMetricReader(std::move(exporter))}; + mp.AddMetricReader(reader); + auto h = m->CreateDoubleHistogram("histogram1", "histogram1_description", "histogram1_unit"); + std::default_random_engine generator; + std::uniform_int_distribution distribution(0, 1000000); + while (state.KeepRunning()) + { + auto num = distribution(generator); + double value = (double)std::rand(); + + h->Record(value, {}); + } +} +BENCHMARK(BM_HistogramAggregation); + +} // namespace +BENCHMARK_MAIN(); From fd98b2ad9bbbf17b192d2520745f21d0fdc7399d Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Dec 2022 15:44:01 -0800 Subject: [PATCH 11/15] Fix bazel build --- .../otlp/test/otlp_metrics_serialization_test.cc | 5 +++-- .../exporters/prometheus/exporter_utils.h | 4 ++-- exporters/prometheus/src/exporter_utils.cc | 4 ++-- sdk/test/metrics/BUILD | 16 ++++++++++++++++ .../metrics/histogram_aggregation_benchmark.cc | 1 - 5 files changed, 23 insertions(+), 7 deletions(-) diff --git a/exporters/otlp/test/otlp_metrics_serialization_test.cc b/exporters/otlp/test/otlp_metrics_serialization_test.cc index adc4f488b0..fd95cab126 100644 --- a/exporters/otlp/test/otlp_metrics_serialization_test.cc +++ b/exporters/otlp/test/otlp_metrics_serialization_test.cc @@ -5,6 +5,7 @@ #include "opentelemetry/proto/metrics/v1/metrics.pb.h" #include +#include OPENTELEMETRY_BEGIN_NAMESPACE namespace exporter @@ -54,11 +55,11 @@ static metrics_sdk::MetricData CreateHistogramAggregationData() s_data_1.sum_ = 100.2; s_data_1.count_ = 22; s_data_1.counts_ = {2, 9, 4, 7}; - s_data_1.boundaries_ = std::list({0.0, 10.0, 20.0, 30.0}); + s_data_1.boundaries_ = std::vector({0.0, 10.0, 20.0, 30.0}); s_data_2.sum_ = 200.2; s_data_2.count_ = 20; s_data_2.counts_ = {0, 8, 5, 7}; - s_data_2.boundaries_ = std::list({0.0, 10.0, 20.0, 30.0}); + s_data_2.boundaries_ = std::vector({0.0, 10.0, 20.0, 30.0}); data.aggregation_temporality = metrics_sdk::AggregationTemporality::kCumulative; data.end_ts = opentelemetry::common::SystemTimestamp(std::chrono::system_clock::now()); diff --git a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h index d85d6d8a0c..ab5f50058c 100644 --- a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h +++ b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h @@ -67,7 +67,7 @@ class PrometheusExporterUtils */ template static void SetData(std::vector values, - const std::list &boundaries, + const std::vector &boundaries, const std::vector &counts, const opentelemetry::sdk::metrics::PointAttributes &labels, std::chrono::nanoseconds time, @@ -104,7 +104,7 @@ class PrometheusExporterUtils */ template static void SetValue(std::vector values, - const std::list &boundaries, + const std::vector &boundaries, const std::vector &counts, ::prometheus::ClientMetric *metric); }; diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc index a8c648132e..3fc71180e5 100644 --- a/exporters/prometheus/src/exporter_utils.cc +++ b/exporters/prometheus/src/exporter_utils.cc @@ -221,7 +221,7 @@ void PrometheusExporterUtils::SetData(std::vector values, */ template void PrometheusExporterUtils::SetData(std::vector values, - const std::list &boundaries, + const std::vector &boundaries, const std::vector &counts, const metric_sdk::PointAttributes &labels, std::chrono::nanoseconds time, @@ -340,7 +340,7 @@ void PrometheusExporterUtils::SetValue(std::vector values, */ template void PrometheusExporterUtils::SetValue(std::vector values, - const std::list &boundaries, + const std::vector &boundaries, const std::vector &counts, prometheus_client::ClientMetric *metric) { diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index cb7ad5e2ba..3904b15ea4 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -281,3 +281,19 @@ otel_cc_benchmark( "//sdk/src/metrics", ], ) + +otel_cc_benchmark( + name = "histogram_aggregation_benchmark", + srcs = [ + "histogram_aggregation_benchmark.cc", + ], + tags = [ + "benchmark", + "metrics", + "test", + ], + deps = [ + "//sdk/src/metrics", + "//sdk/src/resource", + ], +) diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index 0760aaa734..ebdcbf4eb2 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -64,7 +64,6 @@ namespace { void BM_HistogramAggregation(benchmark::State &state) { - MeterProvider mp; auto m = mp.GetMeter("meter1", "version1", "schema1"); From 3490e4403b7927e4f57f02595cfaa60e6375eedb Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 16 Dec 2022 18:00:31 -0800 Subject: [PATCH 12/15] fix --- sdk/test/metrics/histogram_aggregation_benchmark.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index ebdcbf4eb2..41ac2379ca 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -75,9 +75,7 @@ void BM_HistogramAggregation(benchmark::State &state) std::uniform_int_distribution distribution(0, 1000000); while (state.KeepRunning()) { - auto num = distribution(generator); - double value = (double)std::rand(); - + auto value = (double)distribution(generator); h->Record(value, {}); } } From 240a168516e6e98d686e8ad9e048cf8ddec4a5eb Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 21 Dec 2022 23:34:00 -0800 Subject: [PATCH 13/15] remove header already included --- exporters/otlp/test/otlp_metrics_serialization_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/exporters/otlp/test/otlp_metrics_serialization_test.cc b/exporters/otlp/test/otlp_metrics_serialization_test.cc index fd95cab126..cc3781c66f 100644 --- a/exporters/otlp/test/otlp_metrics_serialization_test.cc +++ b/exporters/otlp/test/otlp_metrics_serialization_test.cc @@ -5,7 +5,6 @@ #include "opentelemetry/proto/metrics/v1/metrics.pb.h" #include -#include OPENTELEMETRY_BEGIN_NAMESPACE namespace exporter From ea6455e4423f575876c44bf72d9d1ea7e0779d4f Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Wed, 11 Jan 2023 15:23:34 -0800 Subject: [PATCH 14/15] Update sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h Co-authored-by: Ehsan Saei <71217171+esigo@users.noreply.github.com> --- .../aggregation/histogram_aggregation.h | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index 10ac6c38b6..0c2f652f84 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -109,23 +109,10 @@ void HistogramDiff(HistogramPointData ¤t, HistogramPointData &next, Histog } template -size_t BucketBinarySearch(T value, std::vector &boundaries) +size_t BucketBinarySearch(T value, const std::vector &boundaries) { - size_t lower_bound = 0; - size_t upper_bound = boundaries.size(); - while (lower_bound != upper_bound) - { - size_t mid = (lower_bound + upper_bound) / 2; - if (value > boundaries[mid]) - { - lower_bound = mid + 1; - } - else - { - upper_bound = mid; - } - } - return lower_bound; + auto low = std::lower_bound(boundaries.begin(), boundaries.end(), value); + return low - boundaries.begin(); } } // namespace metrics From 779a840c6ddc5dae0de07d643ec89b90636245d5 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 11 Jan 2023 15:27:56 -0800 Subject: [PATCH 15/15] Fix --- .../sdk/metrics/aggregation/histogram_aggregation.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index 0c2f652f84..5d1097d93f 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -111,8 +111,8 @@ void HistogramDiff(HistogramPointData ¤t, HistogramPointData &next, Histog template size_t BucketBinarySearch(T value, const std::vector &boundaries) { - auto low = std::lower_bound(boundaries.begin(), boundaries.end(), value); - return low - boundaries.begin(); + auto low = std::lower_bound(boundaries.begin(), boundaries.end(), value); + return low - boundaries.begin(); } } // namespace metrics