diff --git a/datadog_checks_base/changelog.d/24921.fixed b/datadog_checks_base/changelog.d/24921.fixed new file mode 100644 index 0000000000000..80fb998eacde0 --- /dev/null +++ b/datadog_checks_base/changelog.d/24921.fixed @@ -0,0 +1 @@ +Stop an OpenMetrics V2 instance that configures ``rename_labels`` from silently discarding the renames its check declares in ``get_default_config``. Mapping-valued defaults are now merged with the instance's mapping entry by entry, with the instance's own entries taking precedence, instead of being replaced wholesale. diff --git a/datadog_checks_base/datadog_checks/base/checks/openmetrics/v2/base.py b/datadog_checks_base/datadog_checks/base/checks/openmetrics/v2/base.py index 2fb5062e146bf..59c3e4f41be9c 100644 --- a/datadog_checks_base/datadog_checks/base/checks/openmetrics/v2/base.py +++ b/datadog_checks_base/datadog_checks/base/checks/openmetrics/v2/base.py @@ -4,6 +4,7 @@ from __future__ import annotations from collections import ChainMap +from collections.abc import Mapping from contextlib import contextmanager from pathlib import Path from typing import TYPE_CHECKING @@ -18,8 +19,6 @@ from .scraper import OpenMetricsScraper if TYPE_CHECKING: - from collections.abc import Mapping - from .metrics_mapping import MetricsMapping, _RawMetricsConfig @@ -131,10 +130,25 @@ def get_config_with_defaults(self, config): Subclasses that override this method must call ``super().get_config_with_defaults(config)``; otherwise the YAML mappings declared via ``METRICS_MAP`` (or discovered by convention) are silently skipped. + + The instance config takes precedence over the defaults, option by option. Mapping-valued + options are the exception: they are merged entry by entry rather than replaced wholesale, + since a ``ChainMap`` resolves keys shallowly and an instance that sets such an option at all + would otherwise shadow the whole class default -- silently dropping entries the check + depends on, such as the ``rename_labels`` renames that keep endpoint labels off Datadog's + reserved tag keys. The instance's own entries still win on a per-entry basis. """ defaults = dict(self.get_default_config()) if file_metrics := self._load_file_based_metrics(config): defaults['metrics'] = list(defaults.get('metrics', [])) + file_metrics + + if merged := { + option: {**default, **config[option]} + for option, default in defaults.items() + if isinstance(default, Mapping) and isinstance(config.get(option), Mapping) + }: + config = {**config, **merged} + return ChainMap(config, defaults) def get_default_config(self) -> dict: @@ -143,6 +157,10 @@ def get_default_config(self) -> dict: The returned dict can be mutated by the framework before being wrapped in a ``ChainMap``. Avoid returning a shared or instance-level object to avoid state leakage between check executions. + + Mapping-valued defaults are merged with any mapping the instance configures for the same + option, so a user customizing it adds to these defaults instead of replacing them. Defaults + of any other type are replaced outright by an instance-level value. """ return {} diff --git a/datadog_checks_base/tests/base/checks/openmetrics/test_v2/test_interface.py b/datadog_checks_base/tests/base/checks/openmetrics/test_v2/test_interface.py index d6071cf0226e5..b06c556f7f86c 100644 --- a/datadog_checks_base/tests/base/checks/openmetrics/test_v2/test_interface.py +++ b/datadog_checks_base/tests/base/checks/openmetrics/test_v2/test_interface.py @@ -31,6 +31,96 @@ def get_default_config(self): aggregator.assert_all_metrics_covered() +def test_default_config_mapping_merged_with_instance(aggregator, dd_run_check, mock_http_response): + """ + A mapping-valued default is merged with the instance's mapping for the same option, entry by + entry. The instance config is layered over the defaults in a `ChainMap`, which resolves keys + shallowly, so without the merge an instance that sets `rename_labels` at all would shadow the + class default wholesale and silently lose renames the check depends on. + """ + + class Check(OpenMetricsBaseCheckV2): + __NAMESPACE__ = 'test' + + def get_default_config(self): + return {'metrics': ['.+'], 'rename_labels': {'foo': 'bar'}} + + mock_http_response( + """ + # HELP go_memstats_alloc_bytes Number of bytes allocated and still in use. + # TYPE go_memstats_alloc_bytes gauge + go_memstats_alloc_bytes{foo="baz",qux="quux"} 6.396288e+06 + """ + ) + check = Check('test', {}, [{'openmetrics_endpoint': 'test', 'rename_labels': {'qux': 'corge'}}]) + dd_run_check(check) + + # `bar:baz` is the class default's rename, `corge:quux` the instance's own. + aggregator.assert_metric( + 'test.go_memstats_alloc_bytes', + 6396288, + metric_type=aggregator.GAUGE, + tags=['endpoint:test', 'bar:baz', 'corge:quux'], + ) + + aggregator.assert_all_metrics_covered() + + +def test_default_config_mapping_entry_overridden_by_instance(aggregator, dd_run_check, mock_http_response): + """ + Merging mapping-valued defaults must not cost the ability to override one: an instance entry for + the same key as a default entry still wins. + """ + + class Check(OpenMetricsBaseCheckV2): + __NAMESPACE__ = 'test' + + def get_default_config(self): + return {'metrics': ['.+'], 'rename_labels': {'foo': 'bar'}} + + mock_http_response( + """ + # HELP go_memstats_alloc_bytes Number of bytes allocated and still in use. + # TYPE go_memstats_alloc_bytes gauge + go_memstats_alloc_bytes{foo="baz"} 6.396288e+06 + """ + ) + check = Check('test', {}, [{'openmetrics_endpoint': 'test', 'rename_labels': {'foo': 'corge'}}]) + dd_run_check(check) + + aggregator.assert_metric( + 'test.go_memstats_alloc_bytes', 6396288, metric_type=aggregator.GAUGE, tags=['endpoint:test', 'corge:baz'] + ) + + aggregator.assert_all_metrics_covered() + + +def test_default_config_mapping_not_shared_between_scrapers(aggregator, dd_run_check, mock_http_response): + """ + The merge must not write back into the mapping `get_default_config` returned, or a check with + several scraper configs would accumulate every scraper's custom renames into the shared default. + """ + default_renames = {'foo': 'bar'} + + class Check(OpenMetricsBaseCheckV2): + __NAMESPACE__ = 'test' + + def get_default_config(self): + return {'metrics': ['.+'], 'rename_labels': default_renames} + + mock_http_response( + """ + # HELP go_memstats_alloc_bytes Number of bytes allocated and still in use. + # TYPE go_memstats_alloc_bytes gauge + go_memstats_alloc_bytes{foo="baz"} 6.396288e+06 + """ + ) + check = Check('test', {}, [{'openmetrics_endpoint': 'test', 'rename_labels': {'qux': 'corge'}}]) + dd_run_check(check) + + assert default_renames == {'foo': 'bar'} + + def test_tag_by_endpoint(aggregator, dd_run_check, mock_http_response): mock_http_response( """