[Metrics SDK] Change boundry type to double for Explicit Bucket His…

…togram Aggregation, and change default bucket range (open-telemetry#1626) * fix bucket boundaries type for histogram aggregation * update bucket range, and fix build
yxue · Dec 5, 2022 · aa47ee5 · aa47ee5
1 parent 5203552
commit aa47ee5
Show file tree

Hide file tree

Showing 11 changed files with 33 additions and 66 deletions.
diff --git 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<std::list<double>>(histogram_point_data.boundaries_))
-    {
-      auto &double_boundaries = nostd::get<std::list<double>>(histogram_point_data.boundaries_);
-      printVec(sout_, double_boundaries);
-    }
-    else if (nostd::holds_alternative<std::list<long>>(histogram_point_data.boundaries_))
-    {
-      auto &long_boundaries = nostd::get<std::list<long>>(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
@@ -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<long>{10, 20, 30};
+  histogram_point_data2.boundaries_ = std::list<double>{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
@@ -126,21 +126,10 @@ void OtlpMetricUtils::ConvertHistogramMetric(
       }
     }
     // buckets
-    if ((nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_)))
-    {
-      auto boundaries = nostd::get<std::list<double>>(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<std::list<long>>(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
@@ -482,14 +482,14 @@ class OtlpHttpMetricExporterTestPeer : public ::testing::Test
     auto exporter = GetExporter(std::unique_ptr<OtlpHttpClient>{mock_otlp_http_client});
 
     opentelemetry::sdk::metrics::HistogramPointData histogram_point_data{};
-    histogram_point_data.boundaries_ = std::list<double>{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<long>{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<double>{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<long>{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
@@ -68,7 +68,7 @@ class PrometheusExporterUtils
    */
   template <typename T>
   static void SetData(std::vector<T> values,
-                      const opentelemetry::sdk::metrics::ListType &boundaries,
+                      const std::list<double> &boundaries,
                       const std::vector<uint64_t> &counts,
                       const opentelemetry::sdk::metrics::PointAttributes &labels,
                       std::chrono::nanoseconds time,
@@ -103,9 +103,9 @@ class PrometheusExporterUtils
   /**
    * Handle Histogram
    */
-  template <typename T, typename U>
+  template <typename T>
   static void SetValue(std::vector<T> values,
-                       const std::list<U> &boundaries,
+                       const std::list<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
@@ -197,7 +197,7 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
  */
 template <typename T>
 void PrometheusExporterUtils::SetData(std::vector<T> values,
-                                      const opentelemetry::sdk::metrics::ListType &boundaries,
+                                      const std::list<double> &boundaries,
                                       const std::vector<uint64_t> &counts,
                                       const metric_sdk::PointAttributes &labels,
                                       std::chrono::nanoseconds time,
@@ -206,14 +206,7 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
   metric_family->metric.emplace_back();
   prometheus_client::ClientMetric &metric = metric_family->metric.back();
   SetMetricBasic(metric, time, labels);
-  if (nostd::holds_alternative<std::list<long>>(boundaries))
-  {
-    SetValue(values, nostd::get<std::list<long>>(boundaries), counts, &metric);
-  }
-  else
-  {
-    SetValue(values, nostd::get<std::list<double>>(boundaries), counts, &metric);
-  }
+  SetValue(values, boundaries, counts, &metric);
 }
 
 /**
@@ -321,9 +314,9 @@ void PrometheusExporterUtils::SetValue(std::vector<T> values,
 /**
  * Handle Histogram
  */
-template <typename T, typename U>
+template <typename T>
 void PrometheusExporterUtils::SetValue(std::vector<T> values,
-                                       const std::list<U> &boundaries,
+                                       const std::list<double> &boundaries,
                                        const std::vector<uint64_t> &counts,
                                        prometheus_client::ClientMetric *metric)
 {

diff --git 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<double>{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<long>{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
@@ -20,7 +20,7 @@ template <typename T>
 class HistogramAggregationConfig : public AggregationConfig
 {
 public:
-  std::list<T> boundaries_;
+  std::list<double> 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
@@ -17,7 +17,6 @@ namespace metrics
 {
 
 using ValueType = nostd::variant<long, double>;
-using ListType  = nostd::variant<std::list<long>, std::list<double>>;
 
 // 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<double> &boundaries) : boundaries_(boundaries) {}
+  std::list<double> 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
@@ -24,14 +24,15 @@ LongHistogramAggregation::LongHistogramAggregation(
   }
   else
   {
-    point_data_.boundaries_ = std::list<long>{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<uint64_t>(nostd::get<std::list<long>>(point_data_.boundaries_).size() + 1, 0);
+  point_data_.counts_         = std::vector<uint64_t>(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<long>(point_data_.max_), value);
   }
   size_t index = 0;
-  for (auto it = nostd::get<std::list<long>>(point_data_.boundaries_).begin();
-       it != nostd::get<std::list<long>>(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<uint64_t>(nostd::get<std::list<double>>(point_data_.boundaries_).size() + 1, 0);
+  point_data_.counts_         = std::vector<uint64_t>(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<double>(point_data_.max_), value);
   }
   size_t index = 0;
-  for (auto it = nostd::get<std::list<double>>(point_data_.boundaries_).begin();
-       it != nostd::get<std::list<double>>(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
@@ -75,7 +75,6 @@ TEST(Aggregation, LongHistogramAggregation)
   ASSERT_TRUE(nostd::holds_alternative<HistogramPointData>(data));
   auto histogram_data = nostd::get<HistogramPointData>(data);
   ASSERT_TRUE(nostd::holds_alternative<long>(histogram_data.sum_));
-  ASSERT_TRUE(nostd::holds_alternative<std::list<long>>(histogram_data.boundaries_));
   EXPECT_EQ(nostd::get<long>(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<opentelemetry::sdk::metrics::HistogramAggregationConfig<long>>
       aggregation_config{new opentelemetry::sdk::metrics::HistogramAggregationConfig<long>};
-  std::list<long> user_boundaries = {0, 50, 100, 250, 500, 750, 1000, 2500, 5000, 10000};
-  aggregation_config->boundaries_ = user_boundaries;
+  std::list<double> 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<HistogramPointData>(data));
   auto histogram_data = nostd::get<HistogramPointData>(data);
-  ASSERT_TRUE(nostd::holds_alternative<std::list<long>>(histogram_data.boundaries_));
-  EXPECT_EQ(nostd::get<std::list<long>>(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<HistogramPointData>(data));
   auto histogram_data = nostd::get<HistogramPointData>(data);
-  ASSERT_TRUE(nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_));
-  EXPECT_EQ(nostd::get<std::list<double>>(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<HistogramPointData>(data));
   auto histogram_data = nostd::get<HistogramPointData>(data);
   ASSERT_TRUE(nostd::holds_alternative<double>(histogram_data.sum_));
-  ASSERT_TRUE(nostd::holds_alternative<std::list<double>>(histogram_data.boundaries_));
   EXPECT_EQ(nostd::get<double>(histogram_data.sum_), 0);
   EXPECT_EQ(histogram_data.count_, 0);
   aggr.Aggregate(12.0, {});   // lies in fourth bucket