Histogram Aggregation: Fix bucket detection logic, performance improvements, and benchmark tests

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
 {
@@ -62,13 +63,15 @@ 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++;
   }
+  // value belongs to the last bucket.
+  point_data_.counts_[index] += 1;
 }
 
 std::unique_ptr<Aggregation> LongHistogramAggregation::Merge(
@@ -107,8 +110,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)
   {
@@ -144,13 +147,15 @@ 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++;
   }
+  // value belongs to the last bucket.
+  point_data_.counts_[index] += 1;
 }
 
 std::unique_ptr<Aggregation> DoubleHistogramAggregation::Merge(

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 = [

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

sdk/test/metrics/aggregation_test.cc

@@ -76,17 +76,17 @@ TEST(Aggregation, LongHistogramAggregation)
   ASSERT_TRUE(nostd::holds_alternative<int64_t>(histogram_data.sum_));
   EXPECT_EQ(nostd::get<int64_t>(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<HistogramPointData>(aggr.ToPoint());
   EXPECT_EQ(nostd::get<int64_t>(histogram_data.min_), 12);
   EXPECT_EQ(nostd::get<int64_t>(histogram_data.max_), 100);
   EXPECT_EQ(nostd::get<int64_t>(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 eight bucket
   histogram_data = nostd::get<HistogramPointData>(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<double>(histogram_data.sum_));
   EXPECT_EQ(nostd::get<double>(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<HistogramPointData>(aggr.ToPoint());
   EXPECT_EQ(nostd::get<double>(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<double>(histogram_data.min_), 12);
   EXPECT_EQ(nostd::get<double>(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<HistogramPointData>(aggr.ToPoint());
   EXPECT_EQ(histogram_data.count_, 4);
   EXPECT_EQ(histogram_data.counts_[3], 2);