move midpoint calculation into base metrics

elasticapm/metrics/base_metrics.py

@@ -241,7 +241,30 @@ def collect(self):
                 if histo is not noop_metric:
                     counts = histo.val
                     if counts or not histo.reset_on_collect:
-                        samples[labels].update({name: {"counts": counts, "values": histo.buckets, "type": "histogram"}})
+                        # For the bucket values, we follow the approach described by Prometheus's
+                        # histogram_quantile function
+                        # (https://prometheus.io/docs/prometheus/latest/querying/functions/#histogram_quantile)
+                        # to achieve consistent percentile aggregation results:
+                        #
+                        # "The histogram_quantile() function interpolates quantile values by assuming a linear
+                        # distribution within a bucket. (...) If a quantile is located in the highest bucket,
+                        # the upper bound of the second highest bucket is returned. A lower limit of the lowest
+                        # bucket is assumed to be 0 if the upper bound of that bucket is greater than 0. In that
+                        # case, the usual linear interpolation is applied within that bucket. Otherwise, the upper
+                        # bound of the lowest bucket is returned for quantiles located in the lowest bucket."
+                        bucket_midpoints = []
+                        for i, bucket_le in enumerate(histo.buckets):
+                            if i == 0:
+                                if bucket_le > 0:
+                                    bucket_le /= 2.0
+                            elif i == len(histo.buckets) - 1:
+                                bucket_le = histo.buckets[i - 1]
+                            else:
+                                bucket_le = histo.buckets[i - 1] + (bucket_le - histo.buckets[i - 1]) / 2.0
+                            bucket_midpoints.append(bucket_le)
+                        samples[labels].update(
+                            {name: {"counts": counts, "values": bucket_midpoints, "type": "histogram"}}
+                        )
                     if histo.reset_on_collect:
                         histo.reset()
 
@@ -404,6 +427,8 @@ class Histogram(BaseMetric):
     def __init__(self, name=None, reset_on_collect=False, unit=None, buckets=None):
         self._lock = threading.Lock()
         self._buckets = buckets or Histogram.DEFAULT_BUCKETS
+        if self._buckets[-1] != float("inf"):
+            self._buckets.append(float("inf"))
         self._counts = [0] * len(self._buckets)
         self._unit = unit
         super(Histogram, self).__init__(name, reset_on_collect=reset_on_collect)

elasticapm/metrics/sets/prometheus.py

@@ -87,33 +87,24 @@ def _prom_histogram_handler(self, name, samples, unit):
         # After the le-samples, 3 more samples follow, with an overall
         # count, overall sum, and creation timestamp.
         sample_pos = 0
+        prev_val = 0
+        counts = []
+        values = []
         name = self._registry.client.config.prometheus_metrics_prefix + name
-        hist_samples = []
         while sample_pos < len(samples):
             sample = samples[sample_pos]
             if "le" in sample.labels:
-                hist_samples.append((float(sample.labels["le"]), sample))
+                values.append(float(sample.labels["le"]))
+                counts.append(sample.value - prev_val)
+                prev_val = sample.value
                 sample_pos += 1
+
             else:
                 # we reached the end of one set of buckets/values, this is the "count" sample
+                self.histogram(name, unit=unit, buckets=values, **sample.labels).val = counts
+                prev_val = 0
                 counts = []
                 values = []
-                prev_sample = None
-                prev_le = 0
-                for i, (le, hist_sample) in enumerate(hist_samples):
-                    if i == 0:
-                        val = le if le < 0 else le / 2.0
-                    elif le == float("+Inf"):
-                        val = prev_le
-                    else:
-                        val = prev_le + (le - prev_le) / 2.0
-                    values.append(val)
-                    counts.append(int(hist_sample.value - (prev_sample.value if prev_sample else 0)))
-                    prev_le = le
-                    prev_sample = hist_sample
-                self.histogram(name, unit=unit, buckets=values, **sample.labels).val = counts
-
-                hist_samples = []
                 sample_pos += 3  # skip sum/created samples
 
     METRIC_MAP = {

tests/metrics/base_tests.py

@@ -90,7 +90,6 @@ def test_metrics_histogram(elasticapm_client):
     metricset = MetricsSet(MetricsRegistry(elasticapm_client))
     hist = metricset.histogram("x", buckets=[1, 10, 100])
     assert len(hist.buckets) == 4
-    assert hist.buckets[3] == float("inf")
 
     hist.update(0.3)
     hist.update(1)
@@ -103,7 +102,7 @@ def test_metrics_histogram(elasticapm_client):
     assert len(data) == 1
     d = data[0]
     assert d["samples"]["x"]["counts"] == [2, 1, 2, 1]
-    assert d["samples"]["x"]["values"] == [1, 10, 100, float("inf")]
+    assert d["samples"]["x"]["values"] == [0.5, 5.5, 55.0, 100]
 
 
 def test_metrics_labels(elasticapm_client):

tests/metrics/prometheus_tests.py

@@ -45,7 +45,14 @@
     prometheus_client.REGISTRY.unregister(prometheus_client.GC_COLLECTOR)
 
 
-def test_counter(elasticapm_client):
+@pytest.fixture()
+def prometheus():
+    # reset registry
+    prometheus_client.REGISTRY._collector_to_names = {}
+    prometheus_client.REGISTRY._names_to_collectors = {}
+
+
+def test_counter(elasticapm_client, prometheus):
     metricset = PrometheusMetrics(MetricsRegistry(elasticapm_client))
     counter = prometheus_client.Counter("a_bare_counter", "Bare counter")
     counter_with_labels = prometheus_client.Counter(
@@ -64,7 +71,7 @@ def test_counter(elasticapm_client):
     assert data[2]["tags"] == {"alabel": "bar", "anotherlabel": "bazzinga"}
 
 
-def test_gauge(elasticapm_client):
+def test_gauge(elasticapm_client, prometheus):
     metricset = PrometheusMetrics(MetricsRegistry(elasticapm_client))
     gauge = prometheus_client.Gauge("a_bare_gauge", "Bare gauge")
     gauge_with_labels = prometheus_client.Gauge("gauge_with_labels", "Gauge with labels", ["alabel", "anotherlabel"])
@@ -81,7 +88,7 @@ def test_gauge(elasticapm_client):
     assert data[2]["tags"] == {"alabel": "bar", "anotherlabel": "bazzinga"}
 
 
-def test_summary(elasticapm_client):
+def test_summary(elasticapm_client, prometheus):
     metricset = PrometheusMetrics(MetricsRegistry(elasticapm_client))
     summary = prometheus_client.Summary("a_bare_summary", "Bare summary")
     summary_with_labels = prometheus_client.Summary(
@@ -106,11 +113,11 @@ def test_summary(elasticapm_client):
     assert data[2]["tags"] == {"alabel": "bar", "anotherlabel": "bazzinga"}
 
 
-def test_histogram(elasticapm_client):
+def test_histogram(elasticapm_client, prometheus):
     metricset = PrometheusMetrics(MetricsRegistry(elasticapm_client))
-    histo = prometheus_client.Histogram("test", "test histogram", buckets=[1, 10, 100, float("inf")])
+    histo = prometheus_client.Histogram("histo", "test histogram", buckets=[1, 10, 100, float("inf")])
     histo_with_labels = prometheus_client.Histogram(
-        "testwithlabel", "test histogram with labels", ["alabel", "anotherlabel"], buckets=[1, 10, 100, float("inf")]
+        "histowithlabel", "test histogram with labels", ["alabel", "anotherlabel"], buckets=[1, 10, 100, float("inf")]
     )
     histo.observe(0.5)
     histo.observe(0.6)
@@ -124,14 +131,13 @@ def test_histogram(elasticapm_client):
     histo_with_labels.labels(alabel="foo", anotherlabel="baz").observe(100)
     histo_with_labels.labels(alabel="foo", anotherlabel="bazzinga").observe(1000)
     data = list(metricset.collect())
-    assert len(data) == 3, data
-    assert data[0]["samples"]["prometheus.metrics.test"]["values"] == [0.5, 5.5, 55.0, 100.0]
-    assert data[0]["samples"]["prometheus.metrics.test"]["counts"] == [2, 1, 3, 1]
+    assert data[0]["samples"]["prometheus.metrics.histo"]["values"] == [0.5, 5.5, 55.0, 100.0]
+    assert data[0]["samples"]["prometheus.metrics.histo"]["counts"] == [2, 1, 3, 1]
 
-    assert data[1]["samples"]["prometheus.metrics.testwithlabel"]["values"] == [0.5, 5.5, 55.0, 100.0]
-    assert data[1]["samples"]["prometheus.metrics.testwithlabel"]["counts"] == [1, 1, 1, 0]
+    assert data[1]["samples"]["prometheus.metrics.histowithlabel"]["values"] == [0.5, 5.5, 55.0, 100.0]
+    assert data[1]["samples"]["prometheus.metrics.histowithlabel"]["counts"] == [1, 1, 1, 0]
     assert data[1]["tags"] == {"alabel": "foo", "anotherlabel": "baz"}
 
-    assert data[2]["samples"]["prometheus.metrics.testwithlabel"]["values"] == [0.5, 5.5, 55.0, 100.0]
-    assert data[2]["samples"]["prometheus.metrics.testwithlabel"]["counts"] == [0, 0, 0, 1]
+    assert data[2]["samples"]["prometheus.metrics.histowithlabel"]["values"] == [0.5, 5.5, 55.0, 100.0]
+    assert data[2]["samples"]["prometheus.metrics.histowithlabel"]["counts"] == [0, 0, 0, 1]
     assert data[2]["tags"] == {"alabel": "foo", "anotherlabel": "bazzinga"}