openmetrics v2: merge mapping-valued defaults instead of replacing them - #24921
openmetrics v2: merge mapping-valued defaults instead of replacing them#24921Kyle-Neale wants to merge 2 commits into
Conversation
`OpenMetricsBaseCheckV2.get_config_with_defaults` layers the instance config over `get_default_config()` in a `ChainMap`, which resolves keys shallowly. An instance that sets a mapping-valued option at all therefore shadows the class default for that option wholesale rather than overriding individual entries. The option this bites is `rename_labels`, which nine integrations use to keep endpoint labels off Datadog's reserved tag keys (`host`, `version`, ...). A user adding one unrelated rename silently drops every required rename, and the raw reserved label keys start being submitted as tags. Nothing warns: the forbidden-tag guard (`check_tag_names`) lives only in the aggregator test stub, so this is loud in tests and silent in production. Merge mapping-valued defaults with the instance's mapping entry by entry, with the instance's own entries still taking precedence, so customizing such an option adds to the defaults instead of replacing them. Options whose defaults are of any other type -- notably the list-valued `metrics` -- keep their existing replace semantics. Replace semantics for this case were neither tested nor documented, so no integration can have been relying on them deliberately. Two integrations (`kueue`, `avi_vantage`) already hand-roll this exact merge in their own `__init__`; those workarounds are now redundant and can be removed separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kyle-Neale
added
the
qa/required
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🔄 Datadog auto-retried 3 jobs - 3 passed on retry 🎯 Code Coverage (details) 🔗 Commit SHA: c2455ae | Docs | View more details | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6ee494ed4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if merged := { | ||
| option: {**default, **config[option]} | ||
| for option, default in defaults.items() | ||
| if isinstance(default, Mapping) and isinstance(config.get(option), Mapping) |
There was a problem hiding this comment.
When a Kuma instance explicitly supplies share_labels: {} or configures only a different source metric, this generic mapping merge also retains Kuma's default cp_info entry from kuma/datadog_checks/kuma/check.py:42. Previously the instance mapping replaced that default, but after this change LabelAggregator again propagates instance_id and version to the other metrics, unexpectedly changing their tags and leaving no way to disable that sharing. Restrict this merge to rename_labels, or define an explicit opt-out for other mapping-valued options.
Useful? React with 👍 / 👎.
| check = Check('test', {}, [{'openmetrics_endpoint': 'test', 'rename_labels': {'qux': 'corge'}}]) | ||
| dd_run_check(check) | ||
|
|
||
| assert default_renames == {'foo': 'bar'} |
There was a problem hiding this comment.
This assertion only verifies that the caller-owned defaults dictionary was not mutated; it never creates a second scraper configuration or checks its effective renames, so it does not verify the stated behavior that one scraper's custom renames cannot leak into another. An implementation that caches accumulated merges on the check while leaving default_renames untouched would still pass, so the test should exercise two distinct configurations and assert their observable tag output.
AGENTS.md reference: AGENTS.md:L177-L180
Useful? React with 👍 / 👎.
Validation ReportAll 21 validations passed. Show details
|
1 participant
What does this PR do?
OpenMetricsBaseCheckV2.get_config_with_defaultslayers the instance config overget_default_config()in aChainMap, which resolves keys shallowly — so an instance that sets a mapping-valued option at all shadows the class default for that option wholesale instead of overriding individual entries.Mapping-valued defaults are now merged entry by entry, with the instance's entries taking precedence. Defaults of any other type, notably the list-valued
metrics, keep their existing replace semantics.Motivation
Nine integrations declare
rename_labelsdefaults this way to keep endpoint labels off Datadog's reserved tag keys. Adding one unrelated rename drops all of them and the raw reserved keys start being submitted as tags.Verified on a real Agent scraping InterSystems IRIS: adding only
rename_labels: {namespace: iris_namespace}replaced 24interop_host:tags and 1iris_version:tag with rawhost:/version:tags. Metric count and health service check were unchanged, so the tag keys are the only signal —check_tag_nameswould have flagged it, but it exists only in the aggregator test stub.Replace semantics for this case were untested and undocumented.
kueueandavi_vantagealready hand-roll this merge in their own code; both keep working unchanged, and their workarounds can be removed separately.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged🤖 Generated with Claude Code