Histogram Aggregation: Fix bucket detection logic, performance improvements, and benchmark tests (#1869)

lalitb · web-flow · commit 46b16ec3cf6c · 2023-01-11T16:49:35.000-08:00
diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc
@@ -74,8 +74,8 @@ void InitMetrics(const std::string &name)
   std::shared_ptr<opentelemetry::sdk::metrics::AggregationConfig> aggregation_config{
       new opentelemetry::sdk::metrics::HistogramAggregationConfig};
   static_cast<opentelemetry::sdk::metrics::HistogramAggregationConfig *>(aggregation_config.get())
-      ->boundaries_ = std::list<double>{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<double>{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<metric_sdk::View> 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
@@ -97,14 +97,14 @@ TEST(OStreamMetricsExporter, ExportHistogramPointData)
       std::unique_ptr<metric_sdk::PushMetricExporter>(new exportermetrics::OStreamMetricExporter);
 
   metric_sdk::HistogramPointData histogram_point_data{};
-  histogram_point_data.boundaries_ = std::list<double>{10.1, 20.2, 30.2};
+  histogram_point_data.boundaries_ = std::vector<double>{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<double>{10.0, 20.0, 30.0};
+  histogram_point_data2.boundaries_ = std::vector<double>{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/exporters/otlp/test/otlp_metrics_serialization_test.cc b/exporters/otlp/test/otlp_metrics_serialization_test.cc
@@ -54,11 +54,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<double>({0.0, 10.0, 20.0, 30.0});
+  s_data_1.boundaries_ = std::vector<double>({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<double>({0.0, 10.0, 20.0, 30.0});
+  s_data_2.boundaries_ = std::vector<double>({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
@@ -67,7 +67,7 @@ class PrometheusExporterUtils
    */
   template <typename T>
   static void SetData(std::vector<T> values,
-                      const std::list<double> &boundaries,
+                      const std::vector<double> &boundaries,
                       const std::vector<uint64_t> &counts,
                       const opentelemetry::sdk::metrics::PointAttributes &labels,
                       std::chrono::nanoseconds time,
@@ -104,7 +104,7 @@ class PrometheusExporterUtils
    */
   template <typename T>
   static void SetValue(std::vector<T> values,
-                       const std::list<double> &boundaries,
+                       const std::vector<double> &boundaries,
                        const std::vector<uint64_t> &counts,
                        ::prometheus::ClientMetric *metric);
 };
diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc
@@ -221,7 +221,7 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
  */
 template <typename T>
 void PrometheusExporterUtils::SetData(std::vector<T> values,
-                                      const std::list<double> &boundaries,
+                                      const std::vector<double> &boundaries,
                                       const std::vector<uint64_t> &counts,
                                       const metric_sdk::PointAttributes &labels,
                                       std::chrono::nanoseconds time,
@@ -340,7 +340,7 @@ void PrometheusExporterUtils::SetValue(std::vector<T> values,
  */
 template <typename T>
 void PrometheusExporterUtils::SetValue(std::vector<T> values,
-                                       const std::list<double> &boundaries,
+                                       const std::vector<double> &boundaries,
                                        const std::vector<uint64_t> &counts,
                                        prometheus_client::ClientMetric *metric)
 {
diff --git 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 <list>
 #include "opentelemetry/version.h"
+
+#include <vector>
+
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
 {
@@ -19,7 +21,7 @@ class AggregationConfig
 class HistogramAggregationConfig : public AggregationConfig
 {
 public:
-  std::list<double> boundaries_;
+  std::vector<double> 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
@@ -108,6 +108,13 @@ void HistogramDiff(HistogramPointData &current, HistogramPointData &next, Histog
   diff.record_min_max_ = false;
 }
 
+template <class T>
+size_t BucketBinarySearch(T value, const std::vector<double> &boundaries)
+{
+  auto low = std::lower_bound(boundaries.begin(), boundaries.end(), value);
+  return low - boundaries.begin();
+}
+
 }  // 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
@@ -8,7 +8,7 @@
 #include "opentelemetry/sdk/metrics/instruments.h"
 #include "opentelemetry/version.h"
 
-#include <list>
+#include <vector>
 
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
@@ -55,14 +55,14 @@ class HistogramPointData
   HistogramPointData &operator=(HistogramPointData &&) = default;
   HistogramPointData(const HistogramPointData &)       = default;
   HistogramPointData()                                 = default;
-  HistogramPointData(std::list<double> &boundaries) : boundaries_(boundaries) {}
-  std::list<double> boundaries_ = {};
-  ValueType sum_                = {};
-  ValueType min_                = {};
-  ValueType max_                = {};
-  std::vector<uint64_t> counts_ = {};
-  uint64_t count_               = {};
-  bool record_min_max_          = true;
+  HistogramPointData(std::vector<double> &boundaries) : boundaries_(boundaries) {}
+  std::vector<double> boundaries_ = {};
+  ValueType sum_                  = {};
+  ValueType min_                  = {};
+  ValueType max_                  = {};
+  std::vector<uint64_t> 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
@@ -8,6 +8,7 @@
 #include "opentelemetry/sdk/metrics/state/attributes_hashmap.h"
 #include "opentelemetry/sdk/metrics/state/metric_collector.h"
 
+#include <list>
 #include <memory>
 
 OPENTELEMETRY_BEGIN_NAMESPACE
diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc
@@ -2,13 +2,14 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h"
+#include "opentelemetry/version.h"
+
 #include <algorithm>
 #include <iomanip>
 #include <limits>
 #include <memory>
-#include "opentelemetry/version.h"
-
 #include <mutex>
+
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
 {
@@ -59,16 +60,8 @@ void LongHistogramAggregation::Aggregate(int64_t value,
     point_data_.min_ = std::min(nostd::get<int64_t>(point_data_.min_), value);
     point_data_.max_ = std::max(nostd::get<int64_t>(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++;
-  }
+  size_t index = BucketBinarySearch(value, point_data_.boundaries_);
+  point_data_.counts_[index] += 1;
 }
 
 std::unique_ptr<Aggregation> LongHistogramAggregation::Merge(
@@ -77,7 +70,10 @@ std::unique_ptr<Aggregation> LongHistogramAggregation::Merge(
   auto curr_value  = nostd::get<HistogramPointData>(ToPoint());
   auto delta_value = nostd::get<HistogramPointData>(
       (static_cast<const LongHistogramAggregation &>(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<int64_t>(curr_value, delta_value, aggr->point_data_);
   return std::unique_ptr<Aggregation>(aggr);
 }
@@ -87,7 +83,10 @@ std::unique_ptr<Aggregation> LongHistogramAggregation::Diff(const Aggregation &n
   auto curr_value = nostd::get<HistogramPointData>(ToPoint());
   auto next_value = nostd::get<HistogramPointData>(
       (static_cast<const LongHistogramAggregation &>(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<int64_t>(curr_value, next_value, aggr->point_data_);
   return std::unique_ptr<Aggregation>(aggr);
 }
@@ -107,8 +106,8 @@ DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig *
   }
   else
   {
-    point_data_.boundaries_ =
-        std::list<double>{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)
   {
@@ -141,16 +140,8 @@ void DoubleHistogramAggregation::Aggregate(double value,
     point_data_.min_ = std::min(nostd::get<double>(point_data_.min_), value);
     point_data_.max_ = std::max(nostd::get<double>(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++;
-  }
+  size_t index = BucketBinarySearch(value, point_data_.boundaries_);
+  point_data_.counts_[index] += 1;
 }
 
 std::unique_ptr<Aggregation> DoubleHistogramAggregation::Merge(
@@ -159,12 +150,10 @@ std::unique_ptr<Aggregation> DoubleHistogramAggregation::Merge(
   auto curr_value  = nostd::get<HistogramPointData>(ToPoint());
   auto delta_value = nostd::get<HistogramPointData>(
       (static_cast<const DoubleHistogramAggregation &>(delta).ToPoint()));
-  std::shared_ptr<AggregationConfig> aggregation_config(new HistogramAggregationConfig);
-  static_cast<opentelemetry::sdk::metrics::HistogramAggregationConfig *>(aggregation_config.get())
-      ->boundaries_ = curr_value.boundaries_;
-  static_cast<opentelemetry::sdk::metrics::HistogramAggregationConfig *>(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<double>(curr_value, delta_value, aggr->point_data_);
   return std::unique_ptr<Aggregation>(aggr);
 }
@@ -175,7 +164,10 @@ std::unique_ptr<Aggregation> DoubleHistogramAggregation::Diff(
   auto curr_value = nostd::get<HistogramPointData>(ToPoint());
   auto next_value = nostd::get<HistogramPointData>(
       (static_cast<const DoubleHistogramAggregation &>(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<double>(curr_value, next_value, aggr->point_data_);
   return std::unique_ptr<Aggregation>(aggr);
 }
diff --git 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 = [
@@ -265,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/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
@@ -37,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
diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc
diff --git a/sdk/test/metrics/histogram_test.cc b/sdk/test/metrics/histogram_test.cc
