Fix a few ExplicitBucketHistogram issues (open-telemetry#2377)

* Fix a few ExplicitBucketHistogram issues * make default boundaries floats * Update helper func name Co-authored-by: Diego Hurtado <[email protected]>
codeboten · Jan 18, 2022 · 7d076cc · 7d076cc
1 parent 297b67e
commit 7d076cc
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 22 deletions.
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/_metrics/aggregation.py
@@ -17,7 +17,7 @@
 from logging import getLogger
 from math import inf
 from threading import Lock
-from typing import Generic, Optional, Sequence, TypeVar
+from typing import Generic, List, Optional, Sequence, TypeVar
 
 from opentelemetry.sdk._metrics.measurement import Measurement
 from opentelemetry.sdk._metrics.point import (
@@ -141,30 +141,32 @@ def collect(self) -> Optional[Gauge]:
 class ExplicitBucketHistogramAggregation(Aggregation[Histogram]):
     def __init__(
         self,
-        boundaries: Sequence[int] = (
-            0,
-            5,
-            10,
-            25,
-            50,
-            75,
-            100,
-            250,
-            500,
-            1000,
+        boundaries: Sequence[float] = (
+            0.0,
+            5.0,
+            10.0,
+            25.0,
+            50.0,
+            75.0,
+            100.0,
+            250.0,
+            500.0,
+            1000.0,
         ),
         record_min_max: bool = True,
     ):
         super().__init__()
-        # pylint: disable=unnecessary-comprehension
-        self._boundaries = [boundary for boundary in (*boundaries, inf)]
-        self.value = [0 for _ in range(len(self._boundaries))]
+        self._boundaries = tuple(boundaries)
+        self._bucket_counts = self._get_empty_bucket_counts()
         self._min = inf
         self._max = -inf
         self._sum = 0
         self._record_min_max = record_min_max
         self._start_time_unix_nano = _time_ns()
 
+    def _get_empty_bucket_counts(self) -> List[int]:
+        return [0] * (len(self._boundaries) + 1)
+
     def aggregate(self, measurement: Measurement) -> None:
 
         value = measurement.value
@@ -175,7 +177,7 @@ def aggregate(self, measurement: Measurement) -> None:
 
         self._sum += value
 
-        self.value[bisect_left(self._boundaries, value)] += 1
+        self._bucket_counts[bisect_left(self._boundaries, value)] += 1
 
     def collect(self) -> Optional[Histogram]:
         """
@@ -184,10 +186,10 @@ def collect(self) -> Optional[Histogram]:
         now = _time_ns()
 
         with self._lock:
-            value = self.value
+            value = self._bucket_counts
             start_time_unix_nano = self._start_time_unix_nano
 
-            self.value = [0 for _ in range(len(self._boundaries))]
+            self._bucket_counts = self._get_empty_bucket_counts()
             self._start_time_unix_nano = now + 1
 
         return Histogram(

diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py
@@ -232,16 +232,24 @@ def test_aggregate(self):
         explicit_bucket_histogram_aggregation.aggregate(Measurement(5))
 
         # The first bucket keeps count of values between (-inf, 0] (-1 and 0)
-        self.assertEqual(explicit_bucket_histogram_aggregation.value[0], 2)
+        self.assertEqual(
+            explicit_bucket_histogram_aggregation._bucket_counts[0], 2
+        )
 
         # The second bucket keeps count of values between (0, 2] (1 and 2)
-        self.assertEqual(explicit_bucket_histogram_aggregation.value[1], 2)
+        self.assertEqual(
+            explicit_bucket_histogram_aggregation._bucket_counts[1], 2
+        )
 
         # The third bucket keeps count of values between (2, 4] (3 and 4)
-        self.assertEqual(explicit_bucket_histogram_aggregation.value[2], 2)
+        self.assertEqual(
+            explicit_bucket_histogram_aggregation._bucket_counts[2], 2
+        )
 
         # The fourth bucket keeps count of values between (4, inf) (3 and 4)
-        self.assertEqual(explicit_bucket_histogram_aggregation.value[3], 1)
+        self.assertEqual(
+            explicit_bucket_histogram_aggregation._bucket_counts[3], 1
+        )
 
     def test_min_max(self):
         """