From d7a892092aa949239eb960a4454832556b398cab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20Herv=C3=A9?= Date: Thu, 2 Apr 2020 11:02:19 +0200 Subject: [PATCH] Update prometheus_client (#6200) This updates prometheus client library to the latest available version, by removing the change in text_fd_to_metric_families which is breaking our compatibility. --- .../base/checks/libs/prometheus.py | 91 +++++++++++++++++++ .../base/checks/openmetrics/mixins.py | 5 +- .../base/checks/prometheus/mixins.py | 2 +- .../base/data/agent_requirements.in | 2 +- datadog_checks_base/requirements.in | 2 +- datadog_checks_base/tests/test_openmetrics.py | 75 ++++++++------- kubelet/tests/test_kubelet.py | 12 +-- openmetrics/tests/test_openmetrics.py | 8 +- prometheus/tests/conftest.py | 2 +- prometheus/tests/test_prometheus.py | 6 +- 10 files changed, 153 insertions(+), 52 deletions(-) create mode 100644 datadog_checks_base/datadog_checks/base/checks/libs/prometheus.py diff --git a/datadog_checks_base/datadog_checks/base/checks/libs/prometheus.py b/datadog_checks_base/datadog_checks/base/checks/libs/prometheus.py new file mode 100644 index 0000000000000..7c5c68776c6fa --- /dev/null +++ b/datadog_checks_base/datadog_checks/base/checks/libs/prometheus.py @@ -0,0 +1,91 @@ +# (C) Datadog, Inc. 2020-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) + +from prometheus_client.metrics_core import Metric +from prometheus_client.parser import _parse_sample, _replace_help_escaping + + +# This copies most of the code from upstream at that version: +# https://github.com/prometheus/client_python/blob/049744296d216e6be65dc8f3d44650310f39c384/prometheus_client/parser.py#L144 +# but reverting the behavior to a compatible version, which doesn't change counters to have a total suffix. See +# https://github.com/prometheus/client_python/commit/a4dd93bcc6a0422e10cfa585048d1813909c6786#diff-0adf47ea7f99c66d4866ccb4e557a865L158 +def text_fd_to_metric_families(fd): + """Parse Prometheus text format from a file descriptor. + + This is a laxer parser than the main Go parser, so successful parsing does + not imply that the parsed text meets the specification. + + Yields Metric's. + """ + name = '' + documentation = '' + typ = 'untyped' + samples = [] + allowed_names = [] + + def build_metric(name, documentation, typ, samples): + # This is where the change is happening: we don't munge counters as upstream does. + metric = Metric(name, documentation, typ) + metric.samples = samples + return metric + + for line in fd: + line = line.strip() + + if line.startswith('#'): + parts = line.split(None, 3) + if len(parts) < 2: + continue + if parts[1] == 'HELP': + if parts[2] != name: + if name != '': + yield build_metric(name, documentation, typ, samples) + # New metric + name = parts[2] + typ = 'untyped' + samples = [] + allowed_names = [parts[2]] + if len(parts) == 4: + documentation = _replace_help_escaping(parts[3]) + else: + documentation = '' + elif parts[1] == 'TYPE': + if parts[2] != name: + if name != '': + yield build_metric(name, documentation, typ, samples) + # New metric + name = parts[2] + documentation = '' + samples = [] + typ = parts[3] + allowed_names = { + 'counter': [''], + 'gauge': [''], + 'summary': ['_count', '_sum', ''], + 'histogram': ['_count', '_sum', '_bucket'], + }.get(typ, ['']) + allowed_names = [name + n for n in allowed_names] + else: + # Ignore other comment tokens + pass + elif line == '': + # Ignore blank lines + pass + else: + sample = _parse_sample(line) + if sample.name not in allowed_names: + if name != '': + yield build_metric(name, documentation, typ, samples) + # New metric, yield immediately as untyped singleton + name = '' + documentation = '' + typ = 'untyped' + samples = [] + allowed_names = [] + yield build_metric(sample[0], documentation, typ, [sample]) + else: + samples.append(sample) + + if name != '': + yield build_metric(name, documentation, typ, samples) diff --git a/datadog_checks_base/datadog_checks/base/checks/openmetrics/mixins.py b/datadog_checks_base/datadog_checks/base/checks/openmetrics/mixins.py index 509d42126b571..a7cb923d7b5eb 100644 --- a/datadog_checks_base/datadog_checks/base/checks/openmetrics/mixins.py +++ b/datadog_checks_base/datadog_checks/base/checks/openmetrics/mixins.py @@ -9,7 +9,7 @@ from re import compile import requests -from prometheus_client.parser import text_fd_to_metric_families +from prometheus_client.samples import Sample from six import PY3, iteritems, itervalues, string_types from ...config import is_affirmative @@ -17,6 +17,7 @@ from ...utils.common import to_native_string from ...utils.http import RequestsWrapper from .. import AgentCheck +from ..libs.prometheus import text_fd_to_metric_families if PY3: long = int @@ -841,7 +842,7 @@ def _decumulate_histogram_buckets(self, metric): ] # Replacing the sample tuple sample[self.SAMPLE_LABELS]["lower_bound"] = str(matching_bucket_tuple[0]) - metric.samples[i] = (sample[self.SAMPLE_NAME], sample[self.SAMPLE_LABELS], matching_bucket_tuple[2]) + metric.samples[i] = Sample(sample[self.SAMPLE_NAME], sample[self.SAMPLE_LABELS], matching_bucket_tuple[2]) def _submit_sample_histogram_buckets(self, metric_name, sample, scraper_config, hostname=None): if "lower_bound" not in sample[self.SAMPLE_LABELS] or "le" not in sample[self.SAMPLE_LABELS]: diff --git a/datadog_checks_base/datadog_checks/base/checks/prometheus/mixins.py b/datadog_checks_base/datadog_checks/base/checks/prometheus/mixins.py index 9c202c191e562..3a4d08ea077e1 100644 --- a/datadog_checks_base/datadog_checks/base/checks/prometheus/mixins.py +++ b/datadog_checks_base/datadog_checks/base/checks/prometheus/mixins.py @@ -9,13 +9,13 @@ import requests from google.protobuf.internal.decoder import _DecodeVarint32 # pylint: disable=E0611,E0401 -from prometheus_client.parser import text_fd_to_metric_families from six import PY3, iteritems, itervalues, string_types from urllib3.exceptions import InsecureRequestWarning from ...utils.prometheus import metrics_pb2 from ...utils.warnings_util import disable_warnings_ctx from .. import AgentCheck +from ..libs.prometheus import text_fd_to_metric_families if PY3: long = int diff --git a/datadog_checks_base/datadog_checks/base/data/agent_requirements.in b/datadog_checks_base/datadog_checks/base/data/agent_requirements.in index f5e685ca7e979..743dff351e7a3 100644 --- a/datadog_checks_base/datadog_checks/base/data/agent_requirements.in +++ b/datadog_checks_base/datadog_checks/base/data/agent_requirements.in @@ -34,7 +34,7 @@ openstacksdk==0.24.0 orjson==2.6.1; python_version > '3.0' paramiko==2.6.0 ply==3.10 -prometheus-client==0.3.0 +prometheus-client==0.7.1 protobuf==3.7.0 psutil==5.6.7 psycopg2-binary==2.8.4 diff --git a/datadog_checks_base/requirements.in b/datadog_checks_base/requirements.in index c497b42f7de81..935c8ad74d64e 100644 --- a/datadog_checks_base/requirements.in +++ b/datadog_checks_base/requirements.in @@ -7,7 +7,7 @@ enum34==1.1.6; python_version < '3.0' ipaddress==1.0.22; python_version < '3.0' kubernetes==8.0.1 orjson==2.6.1; python_version > '3.0' -prometheus-client==0.3.0 +prometheus-client==0.7.1 protobuf==3.7.0 pysocks==1.7.0 pytz==2019.3 diff --git a/datadog_checks_base/tests/test_openmetrics.py b/datadog_checks_base/tests/test_openmetrics.py index 3d3de56e0dcd7..5d55a30c253d2 100644 --- a/datadog_checks_base/tests/test_openmetrics.py +++ b/datadog_checks_base/tests/test_openmetrics.py @@ -13,6 +13,7 @@ import pytest import requests from prometheus_client.core import CounterMetricFamily, GaugeMetricFamily, HistogramMetricFamily, SummaryMetricFamily +from prometheus_client.samples import Sample from six import iteritems from urllib3.exceptions import InsecureRequestWarning @@ -632,6 +633,8 @@ def test_parse_one_counter(p_check, mocked_prometheus_scraper_config): expected_etcd_metric = CounterMetricFamily('go_memstats_mallocs_total', 'Total number of mallocs.') expected_etcd_metric.add_metric([], 18713) + # Fix up the _total change + expected_etcd_metric.name = 'go_memstats_mallocs_total' # Iter on the generator to get all metrics response = MockResponse(text_data, text_content_type) @@ -976,67 +979,67 @@ def test_decumulate_histogram_buckets(p_check, mocked_prometheus_scraper_config) 'rest_client_request_latency_seconds_bucket', 'Request latency in seconds. Broken down by verb and URL.' ) expected_metric.samples = [ - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.004', 'lower_bound': '0.002', 'verb': 'GET'}, 81.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.001', 'lower_bound': '0', 'verb': 'GET'}, 254.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.002', 'lower_bound': '0.001', 'verb': 'GET'}, 367.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.008', 'lower_bound': '0.004', 'verb': 'GET'}, 25.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.016', 'lower_bound': '0.008', 'verb': 'GET'}, 11.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.032', 'lower_bound': '0.016', 'verb': 'GET'}, 6.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.064', 'lower_bound': '0.032', 'verb': 'GET'}, 4.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.128', 'lower_bound': '0.064', 'verb': 'GET'}, 6.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.256', 'lower_bound': '0.128', 'verb': 'GET'}, 1.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '0.512', 'lower_bound': '0.256', 'verb': 'GET'}, 0.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '+Inf', 'lower_bound': '0.512', 'verb': 'GET'}, 0.0, ), - ( + Sample( 'rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 2.185820220000001, ), - ('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), + Sample('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), ] current_metric = metrics[0] @@ -1065,17 +1068,17 @@ def test_decumulate_histogram_buckets_single_bucket(p_check, mocked_prometheus_s 'rest_client_request_latency_seconds_bucket', 'Request latency in seconds. Broken down by verb and URL.' ) expected_metric.samples = [ - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '+Inf', 'lower_bound': '0', 'verb': 'GET'}, 755.0, ), - ( + Sample( 'rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 2.185820220000001, ), - ('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), + Sample('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), ] current_metric = metrics[0] @@ -1129,40 +1132,42 @@ def test_decumulate_histogram_buckets_multiple_contexts(p_check, mocked_promethe ) expected_metric.samples = [ - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '1', 'lower_bound': '0', 'verb': 'GET'}, 100.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '2', 'lower_bound': '1.0', 'verb': 'GET'}, 100.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '+Inf', 'lower_bound': '2.0', 'verb': 'GET'}, 100.0, ), - ('rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 256.0), - ('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 300.0), - ( + Sample('rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 256.0), + Sample('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 300.0), + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '1', 'lower_bound': '0', 'verb': 'POST'}, 50.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '2', 'lower_bound': '1.0', 'verb': 'POST'}, 50.0, ), - ( + Sample( 'rest_client_request_latency_seconds_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '+Inf', 'lower_bound': '2.0', 'verb': 'POST'}, 50.0, ), - ('rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'POST'}, 200.0), - ('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'POST'}, 150.0), + Sample('rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'POST'}, 200.0), + Sample( + 'rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'POST'}, 150.0 + ), ] current_metric = metrics[0] @@ -1192,33 +1197,33 @@ def test_decumulate_histogram_buckets_negative_buckets(p_check, mocked_prometheu expected_metric = HistogramMetricFamily('random_histogram_bucket', 'Nonsense histogram.') expected_metric.samples = [ - ( + Sample( 'random_histogram_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '-Inf', 'lower_bound': '-inf', 'verb': 'GET'}, 0.0, ), - ( + Sample( 'random_histogram_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '-10.0', 'lower_bound': '-inf', 'verb': 'GET'}, 50.0, ), - ( + Sample( 'random_histogram_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '-2.0', 'lower_bound': '-10.0', 'verb': 'GET'}, 5.0, ), - ( + Sample( 'random_histogram_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '15.0', 'lower_bound': '-2.0', 'verb': 'GET'}, 10.0, ), - ( + Sample( 'random_histogram_bucket', {'url': 'http://127.0.0.1:8080/api', 'le': '+Inf', 'lower_bound': '15.0', 'verb': 'GET'}, 5.0, ), - ('random_histogram_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 3.14), - ('random_histogram_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 70.0), + Sample('random_histogram_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 3.14), + Sample('random_histogram_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 70.0), ] current_metric = metrics[0] @@ -1246,12 +1251,12 @@ def test_decumulate_histogram_buckets_no_buckets(p_check, mocked_prometheus_scra 'random_histogram_bucket', 'Request latency in seconds. Broken down by verb and URL.' ) expected_metric.samples = [ - ( + Sample( 'rest_client_request_latency_seconds_sum', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 2.185820220000001, ), - ('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), + Sample('rest_client_request_latency_seconds_count', {'url': 'http://127.0.0.1:8080/api', 'verb': 'GET'}, 755.0), ] current_metric = metrics[0] diff --git a/kubelet/tests/test_kubelet.py b/kubelet/tests/test_kubelet.py index 6a28157b1a00c..5cf7e5544a606 100644 --- a/kubelet/tests/test_kubelet.py +++ b/kubelet/tests/test_kubelet.py @@ -482,9 +482,9 @@ def test_prometheus_filtering(monkeypatch, aggregator): mock_method.assert_called_once() metric = mock_method.call_args[0][0] assert len(metric.samples) == 27 - for name, labels, _ in metric.samples: - assert name == "container_cpu_usage_seconds_total" - assert labels["pod"] != "" + for sample in metric.samples: + assert sample.name == "container_cpu_usage_seconds_total" + assert sample.labels["pod"] != "" with mock.patch(method_name) as mock_method: # k8s < 1.16 @@ -494,9 +494,9 @@ def test_prometheus_filtering(monkeypatch, aggregator): mock_method.assert_called_once() metric = mock_method.call_args[0][0] assert len(metric.samples) == 12 - for name, labels, _ in metric.samples: - assert name == "container_cpu_usage_seconds_total" - assert labels["pod_name"] != "" + for sample in metric.samples: + assert sample.name == "container_cpu_usage_seconds_total" + assert sample.labels["pod_name"] != "" def test_kubelet_check_instance_config(monkeypatch): diff --git a/openmetrics/tests/test_openmetrics.py b/openmetrics/tests/test_openmetrics.py index a840978abdf5b..7bd1af5119908 100644 --- a/openmetrics/tests/test_openmetrics.py +++ b/openmetrics/tests/test_openmetrics.py @@ -13,7 +13,7 @@ instance = { 'prometheus_url': 'http://localhost:10249/metrics', 'namespace': 'openmetrics', - 'metrics': [{'metric1': 'renamed.metric1'}, 'metric2', 'counter1'], + 'metrics': [{'metric1': 'renamed.metric1'}, 'metric2', 'counter1_total'], 'send_histograms_buckets': True, 'send_monotonic_counter': True, } @@ -32,7 +32,9 @@ def test_openmetrics_check(aggregator): tags=['timestamp:123', 'node:host2', 'matched_label:foobar'], metric_type=aggregator.GAUGE, ) - aggregator.assert_metric(CHECK_NAME + '.counter1', tags=['node:host2'], metric_type=aggregator.MONOTONIC_COUNT) + aggregator.assert_metric( + CHECK_NAME + '.counter1_total', tags=['node:host2'], metric_type=aggregator.MONOTONIC_COUNT + ) aggregator.assert_all_metrics_covered() @@ -50,7 +52,7 @@ def test_openmetrics_check_counter_gauge(aggregator): tags=['timestamp:123', 'node:host2', 'matched_label:foobar'], metric_type=aggregator.GAUGE, ) - aggregator.assert_metric(CHECK_NAME + '.counter1', tags=['node:host2'], metric_type=aggregator.GAUGE) + aggregator.assert_metric(CHECK_NAME + '.counter1_total', tags=['node:host2'], metric_type=aggregator.GAUGE) aggregator.assert_all_metrics_covered() diff --git a/prometheus/tests/conftest.py b/prometheus/tests/conftest.py index c6ebba65c0a43..897617b7c34f1 100644 --- a/prometheus/tests/conftest.py +++ b/prometheus/tests/conftest.py @@ -20,7 +20,7 @@ INSTANCE_UNIT = { 'prometheus_url': 'http://localhost:10249/metrics', 'namespace': 'prometheus', - 'metrics': [{'metric1': 'renamed.metric1'}, 'metric2', 'counter1'], + 'metrics': [{'metric1': 'renamed.metric1'}, 'metric2', 'counter1_total'], 'send_histograms_buckets': True, 'send_monotonic_counter': True, } diff --git a/prometheus/tests/test_prometheus.py b/prometheus/tests/test_prometheus.py index 2687235a36f2c..e2a2a01c1664c 100644 --- a/prometheus/tests/test_prometheus.py +++ b/prometheus/tests/test_prometheus.py @@ -28,7 +28,9 @@ def test_prometheus_check(aggregator, instance, poll_mock): tags=['timestamp:123', 'node:host2', 'matched_label:foobar'], metric_type=aggregator.GAUGE, ) - aggregator.assert_metric(CHECK_NAME + '.counter1', tags=['node:host2'], metric_type=aggregator.MONOTONIC_COUNT) + aggregator.assert_metric( + CHECK_NAME + '.counter1_total', tags=['node:host2'], metric_type=aggregator.MONOTONIC_COUNT + ) assert aggregator.metrics_asserted_pct == 100.0 @@ -50,7 +52,7 @@ def test_prometheus_check_counter_gauge(aggregator, instance, poll_mock): tags=['timestamp:123', 'node:host2', 'matched_label:foobar'], metric_type=aggregator.GAUGE, ) - aggregator.assert_metric(CHECK_NAME + '.counter1', tags=['node:host2'], metric_type=aggregator.GAUGE) + aggregator.assert_metric(CHECK_NAME + '.counter1_total', tags=['node:host2'], metric_type=aggregator.GAUGE) assert aggregator.metrics_asserted_pct == 100.0