From ff607b156477dd9c3e0ab4c61d685e79cd3be26a Mon Sep 17 00:00:00 2001 From: Benjamin Wohlwend Date: Mon, 19 Jul 2021 11:54:49 +0200 Subject: [PATCH] move midpoint calculation into base metrics --- elasticapm/metrics/base_metrics.py | 27 +++++++++++++++++++++- elasticapm/metrics/sets/prometheus.py | 27 ++++++++-------------- tests/metrics/base_tests.py | 3 +-- tests/metrics/prometheus_tests.py | 32 ++++++++++++++++----------- 4 files changed, 55 insertions(+), 34 deletions(-) diff --git a/elasticapm/metrics/base_metrics.py b/elasticapm/metrics/base_metrics.py index 2847d9a1c..c3320bd8a 100644 --- a/elasticapm/metrics/base_metrics.py +++ b/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) diff --git a/elasticapm/metrics/sets/prometheus.py b/elasticapm/metrics/sets/prometheus.py index a75abb127..2dffce812 100644 --- a/elasticapm/metrics/sets/prometheus.py +++ b/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 = { diff --git a/tests/metrics/base_tests.py b/tests/metrics/base_tests.py index 00ee3d19a..36c925bec 100644 --- a/tests/metrics/base_tests.py +++ b/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): diff --git a/tests/metrics/prometheus_tests.py b/tests/metrics/prometheus_tests.py index 7ac00cbf2..0308c2cf9 100644 --- a/tests/metrics/prometheus_tests.py +++ b/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"}