From d127140564ce7c06884389ad4c39d9f85cdcd4a3 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Sun, 2 Oct 2022 11:39:06 -0700 Subject: [PATCH] [Metrics SDK] Change boundry type to `double` for Explicit Bucket Histogram Aggregation, and change default bucket range (#1626) * fix bucket boundaries type for histogram aggregation * update bucket range, and fix build --- exporters/ostream/src/metric_exporter.cc | 11 +---------- exporters/ostream/test/ostream_metric_test.cc | 2 +- exporters/otlp/src/otlp_metric_utils.cc | 17 +++-------------- .../otlp/test/otlp_http_metric_exporter_test.cc | 8 ++++---- .../exporters/prometheus/exporter_utils.h | 6 +++--- exporters/prometheus/src/exporter_utils.cc | 15 ++++----------- .../prometheus/test/prometheus_test_helper.h | 4 ++-- .../metrics/aggregation/aggregation_config.h | 2 +- .../opentelemetry/sdk/metrics/data/point_data.h | 5 ++--- .../aggregation/histogram_aggregation.cc | 16 +++++++--------- sdk/test/metrics/aggregation_test.cc | 13 +++++-------- 11 files changed, 33 insertions(+), 66 deletions(-) diff --git a/exporters/ostream/src/metric_exporter.cc b/exporters/ostream/src/metric_exporter.cc index 8491929533..24d799c034 100644 --- a/exporters/ostream/src/metric_exporter.cc +++ b/exporters/ostream/src/metric_exporter.cc @@ -205,16 +205,7 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po } sout_ << "\n buckets : "; - if (nostd::holds_alternative>(histogram_point_data.boundaries_)) - { - auto &double_boundaries = nostd::get>(histogram_point_data.boundaries_); - printVec(sout_, double_boundaries); - } - else if (nostd::holds_alternative>(histogram_point_data.boundaries_)) - { - auto &long_boundaries = nostd::get>(histogram_point_data.boundaries_); - printVec(sout_, long_boundaries); - } + printVec(sout_, histogram_point_data.boundaries_); sout_ << "\n counts : "; printVec(sout_, histogram_point_data.counts_); diff --git a/exporters/ostream/test/ostream_metric_test.cc b/exporters/ostream/test/ostream_metric_test.cc index ea46c09bbe..5ed226f26b 100644 --- a/exporters/ostream/test/ostream_metric_test.cc +++ b/exporters/ostream/test/ostream_metric_test.cc @@ -105,7 +105,7 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData) 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, 20, 30}; + histogram_point_data2.boundaries_ = std::list{10.0, 20.0, 30.0}; histogram_point_data2.count_ = 3; histogram_point_data2.counts_ = {200, 300, 400, 500}; histogram_point_data2.sum_ = 900l; diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index 82c99684ef..67686a93ad 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -126,21 +126,10 @@ void OtlpMetricUtils::ConvertHistogramMetric( } } // buckets - if ((nostd::holds_alternative>(histogram_data.boundaries_))) - { - auto boundaries = nostd::get>(histogram_data.boundaries_); - for (auto bound : boundaries) - { - proto_histogram_point_data->add_explicit_bounds(bound); - } - } - else + + for (auto bound : histogram_data.boundaries_) { - auto boundaries = nostd::get>(histogram_data.boundaries_); - for (auto bound : boundaries) - { - proto_histogram_point_data->add_explicit_bounds(bound); - } + proto_histogram_point_data->add_explicit_bounds(bound); } // bucket counts for (auto bucket_value : histogram_data.counts_) diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index 295e18cb1b..aaa1cda1a6 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -482,14 +482,14 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test auto exporter = GetExporter(std::unique_ptr{mock_otlp_http_client}); opentelemetry::sdk::metrics::HistogramPointData histogram_point_data{}; - histogram_point_data.boundaries_ = std::list{10.1, 20.2, 30.2}; + histogram_point_data.boundaries_ = {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_ = 19.0; opentelemetry::sdk::metrics::HistogramPointData histogram_point_data2{}; - histogram_point_data2.boundaries_ = std::list{10, 20, 30}; + histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0}; histogram_point_data2.count_ = 3; histogram_point_data2.counts_ = {200, 300, 400, 500}; histogram_point_data2.sum_ = 900l; @@ -619,12 +619,12 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test "library_name", "1.5.0"); opentelemetry::sdk::metrics::HistogramPointData histogram_point_data{}; - histogram_point_data.boundaries_ = std::list{10.1, 20.2, 30.2}; + histogram_point_data.boundaries_ = {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; opentelemetry::sdk::metrics::HistogramPointData histogram_point_data2{}; - histogram_point_data2.boundaries_ = std::list{10, 20, 30}; + histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0}; histogram_point_data2.count_ = 3; histogram_point_data2.counts_ = {200, 300, 400, 500}; histogram_point_data2.sum_ = 900l; diff --git a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h index c8df4f6cfd..09486a438e 100644 --- a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h +++ b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h @@ -68,7 +68,7 @@ class PrometheusExporterUtils */ template static void SetData(std::vector values, - const opentelemetry::sdk::metrics::ListType &boundaries, + const std::list &boundaries, const std::vector &counts, const opentelemetry::sdk::metrics::PointAttributes &labels, std::chrono::nanoseconds time, @@ -103,9 +103,9 @@ class PrometheusExporterUtils /** * Handle Histogram */ - template + template static void SetValue(std::vector values, - const std::list &boundaries, + const std::list &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 44f4395ece..93b27f38ee 100644 --- a/exporters/prometheus/src/exporter_utils.cc +++ b/exporters/prometheus/src/exporter_utils.cc @@ -197,7 +197,7 @@ void PrometheusExporterUtils::SetData(std::vector values, */ template void PrometheusExporterUtils::SetData(std::vector values, - const opentelemetry::sdk::metrics::ListType &boundaries, + const std::list &boundaries, const std::vector &counts, const metric_sdk::PointAttributes &labels, std::chrono::nanoseconds time, @@ -206,14 +206,7 @@ void PrometheusExporterUtils::SetData(std::vector values, metric_family->metric.emplace_back(); prometheus_client::ClientMetric &metric = metric_family->metric.back(); SetMetricBasic(metric, time, labels); - if (nostd::holds_alternative>(boundaries)) - { - SetValue(values, nostd::get>(boundaries), counts, &metric); - } - else - { - SetValue(values, nostd::get>(boundaries), counts, &metric); - } + SetValue(values, boundaries, counts, &metric); } /** @@ -321,9 +314,9 @@ void PrometheusExporterUtils::SetValue(std::vector values, /** * Handle Histogram */ -template +template void PrometheusExporterUtils::SetValue(std::vector values, - const std::list &boundaries, + const std::list &boundaries, const std::vector &counts, prometheus_client::ClientMetric *metric) { diff --git a/exporters/prometheus/test/prometheus_test_helper.h b/exporters/prometheus/test/prometheus_test_helper.h index 6f9bc73a49..f4db88d2fc 100644 --- a/exporters/prometheus/test/prometheus_test_helper.h +++ b/exporters/prometheus/test/prometheus_test_helper.h @@ -46,12 +46,12 @@ inline metric_sdk::ResourceMetrics CreateSumPointData() inline metric_sdk::ResourceMetrics CreateHistogramPointData() { metric_sdk::HistogramPointData histogram_point_data{}; - histogram_point_data.boundaries_ = std::list{10.1, 20.2, 30.2}; + histogram_point_data.boundaries_ = {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; metric_sdk::HistogramPointData histogram_point_data2{}; - histogram_point_data2.boundaries_ = std::list{10, 20, 30}; + histogram_point_data2.boundaries_ = {10.0, 20.0, 30.0}; histogram_point_data2.count_ = 3; histogram_point_data2.counts_ = {200, 300, 400, 500}; histogram_point_data2.sum_ = 900l; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 67a092ff9c..d2a07f91d2 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -20,7 +20,7 @@ template class HistogramAggregationConfig : public AggregationConfig { public: - std::list boundaries_; + std::list boundaries_; bool record_min_max_ = true; }; } // namespace metrics diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index ebc4bb98f2..dfeda3d33d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -17,7 +17,6 @@ namespace metrics { using ValueType = nostd::variant; -using ListType = nostd::variant, std::list>; // TODO: remove ctors and initializers from below classes when GCC<5 stops shipping on Ubuntu @@ -55,8 +54,8 @@ class HistogramPointData HistogramPointData &operator=(HistogramPointData &&) = default; HistogramPointData(const HistogramPointData &) = default; HistogramPointData() = default; - - ListType boundaries_ = {}; + HistogramPointData(std::list &boundaries) : boundaries_(boundaries) {} + std::list boundaries_ = {}; ValueType sum_ = {}; ValueType min_ = {}; ValueType max_ = {}; diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 5dcb277d6a..4dec404d5d 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -24,14 +24,15 @@ LongHistogramAggregation::LongHistogramAggregation( } else { - point_data_.boundaries_ = std::list{0l, 5l, 10l, 25l, 50l, 75l, 100l, 250l, 500l, 1000l}; + 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 (aggregation_config) { record_min_max_ = aggregation_config->record_min_max_; } - point_data_.counts_ = - std::vector(nostd::get>(point_data_.boundaries_).size() + 1, 0); + point_data_.counts_ = std::vector(point_data_.boundaries_.size() + 1, 0); point_data_.sum_ = 0l; point_data_.count_ = 0; point_data_.record_min_max_ = record_min_max_; @@ -59,8 +60,7 @@ void LongHistogramAggregation::Aggregate(long value, point_data_.max_ = std::max(nostd::get(point_data_.max_), value); } size_t index = 0; - for (auto it = nostd::get>(point_data_.boundaries_).begin(); - it != nostd::get>(point_data_.boundaries_).end(); ++it) + for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) { if (value < *it) { @@ -114,8 +114,7 @@ DoubleHistogramAggregation::DoubleHistogramAggregation( { record_min_max_ = aggregation_config->record_min_max_; } - point_data_.counts_ = - std::vector(nostd::get>(point_data_.boundaries_).size() + 1, 0); + point_data_.counts_ = std::vector(point_data_.boundaries_.size() + 1, 0); point_data_.sum_ = 0.0; point_data_.count_ = 0; point_data_.record_min_max_ = record_min_max_; @@ -143,8 +142,7 @@ void DoubleHistogramAggregation::Aggregate(double value, point_data_.max_ = std::max(nostd::get(point_data_.max_), value); } size_t index = 0; - for (auto it = nostd::get>(point_data_.boundaries_).begin(); - it != nostd::get>(point_data_.boundaries_).end(); ++it) + for (auto it = point_data_.boundaries_.begin(); it != point_data_.boundaries_.end(); ++it) { if (value < *it) { diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index c10ccae366..bc82863803 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -75,7 +75,6 @@ TEST(Aggregation, LongHistogramAggregation) ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); ASSERT_TRUE(nostd::holds_alternative(histogram_data.sum_)); - ASSERT_TRUE(nostd::holds_alternative>(histogram_data.boundaries_)); EXPECT_EQ(nostd::get(histogram_data.sum_), 0); EXPECT_EQ(histogram_data.count_, 0); aggr.Aggregate(12l, {}); // lies in fourth bucket @@ -134,14 +133,14 @@ TEST(Aggregation, LongHistogramAggregationBoundaries) { nostd::shared_ptr> aggregation_config{new opentelemetry::sdk::metrics::HistogramAggregationConfig}; - std::list user_boundaries = {0, 50, 100, 250, 500, 750, 1000, 2500, 5000, 10000}; - aggregation_config->boundaries_ = user_boundaries; + 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; LongHistogramAggregation aggr{aggregation_config.get()}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); - ASSERT_TRUE(nostd::holds_alternative>(histogram_data.boundaries_)); - EXPECT_EQ(nostd::get>(histogram_data.boundaries_), user_boundaries); + EXPECT_EQ(histogram_data.boundaries_, user_boundaries); } TEST(Aggregation, DoubleHistogramAggregationBoundaries) @@ -155,8 +154,7 @@ TEST(Aggregation, DoubleHistogramAggregationBoundaries) auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); - ASSERT_TRUE(nostd::holds_alternative>(histogram_data.boundaries_)); - EXPECT_EQ(nostd::get>(histogram_data.boundaries_), user_boundaries); + EXPECT_EQ(histogram_data.boundaries_, user_boundaries); } TEST(Aggregation, DoubleHistogramAggregation) @@ -166,7 +164,6 @@ TEST(Aggregation, DoubleHistogramAggregation) ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); ASSERT_TRUE(nostd::holds_alternative(histogram_data.sum_)); - ASSERT_TRUE(nostd::holds_alternative>(histogram_data.boundaries_)); EXPECT_EQ(nostd::get(histogram_data.sum_), 0); EXPECT_EQ(histogram_data.count_, 0); aggr.Aggregate(12.0, {}); // lies in fourth bucket