autodiscovery: suppress configuration discovery on generic-integration namespace conflicts - #54254
autodiscovery: suppress configuration discovery on generic-integration namespace conflicts#54254vitkyrka wants to merge 3 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d17cf684d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 380b584 | Docs | Datadog PR Page | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 2f78841f: Results for datadog-agent_7.84.0~devel.git.199.380b584.pipeline.131011016-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.199.380b584.pipeline.131011016-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
11 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 2f78841 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +3.05 | [+2.14, +3.95] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.40 | [+0.25, +0.54] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.30 | [+0.18, +0.42] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.09 | [-0.01, +0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.07 | [+0.03, +0.11] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.02 | [-0.09, +0.06] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.36 | [-0.48, -0.25] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.91 | [-1.15, -0.67] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 173.34MiB ≤ 178MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 745.57KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 519.93MiB ≤ 538MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.14MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 206.84MiB ≤ 229MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.21MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 349.95 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 418.56MiB ≤ 439MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.12MiB ≤ 76MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 26.53 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 329.28MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 61.57 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 304.64MiB ≤ 314MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.92 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 309.28MiB ≤ 343MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
0399e12 to
14ac1b2
Compare
14ac1b2 to
9022da1
Compare
9022da1 to
d71a9da
Compare
d71a9da to
96f0bee
Compare
…n namespace conflicts
Configuration discovery (`discovery: {}` in a shipped `auto_conf.yaml`, e.g.
krakend, haproxy) could schedule a duplicate check on a container or host
that already has a manually-configured generic `openmetrics`/`prometheus`
check claiming the same (or a rooted-in) metric namespace. Both checks would
then scrape the same target and submit to the same final metric name; for
counter-derived metrics this doubles the reported value, since Datadog sums
same-context contributions within a flush interval. See DSCVR-626 and
DSCVR-626-investigation.md for the empirical repro (krakend and haproxy,
real customer-style configs) and
https://datadoghq.atlassian.net/wiki/spaces/DSCVR/pages/7031522288/Conflict+with+generic+integrations
for the design.
`filterTemplatesDiscovery` now also drops a discovery template when:
- a sibling `openmetrics`/`prometheus` template matched to the same service
configures a namespace matching/rooted-in the integration's own
(per-integration namespace overrides only needed for zk/gearmand, whose
namespace diverges from their check name; every other integration
defaults to its own name), or
- a scheduled static (non-template, host-wide) `openmetrics`/`prometheus`
config claims such a namespace, tracked via a new
GenericIntegrationNamespaceIndex (mirrors StaticConfigIndex, but keyed by
namespace with rooted-in matching instead of by exact integration name).
Adds unit test coverage in listeners/service_test.go and a new
generic_integration_namespace_index_test.go, and extends the krakend e2e
discovery suite with a second fake container
(docker-compose.fake-krakend-conflict.yaml) carrying a conflicting manual
openmetrics config, asserting discovery is suppressed there while the
original krakend discovery test is unaffected.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
…tection
The namespace-based conflict detection assumed a discovery-driven
integration's metric namespace root equals its own check name -- wrong for
the small set of integrations that diverge (e.g. gearmand's own metrics use
namespace "gearman", not "gearmandd"). We'd deliberately dropped a
hand-maintained override map for this (layering violation, maintenance
burden) and accepted the gap.
integrations-core#24861 closes it declaratively: gearmand, krakend, and
prefect each add a `metrics_prefix` field to their auto_conf.yaml's
`discovery:` block (e.g. `discovery: {metrics_prefix: gearman}`), parsed
automatically via the file provider's existing generic YAML unmarshal into
the new integration.DiscoveryConfig.MetricsPrefix field (yaml-tagged only --
verified no JSON-sourced config path (container labels/pod annotations) can
ever carry a discovery: block, so a json tag would be dead code).
New ExpectedNamespaceRoot(cfg) returns the root of MetricsPrefix when set, or
the check name otherwise, and filterTemplatesDiscovery now compares against
it instead of cfg.Name for namespace-conflict checks specifically (the
separate same-check-name-sibling rule stays keyed by cfg.Name, since it's an
orthogonal collision case). Only the root is used even when metrics_prefix is
itself multi-segment (e.g. krakend's "krakend.api"), consistent with how the
generic-scraper side of the comparison already only ever compares roots.
Also fixes a latent digest bug caught during review: IntDigest/FastDigest
only hashed the presence of Discovery, never MetricsPrefix's actual value --
an auto_conf.yaml update that only changes metrics_prefix would have hashed
identically to the old config and been silently treated as already-tracked.
Environment: Datadog workspace
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
54f7d3b to
1fa2914
Compare
The existing e2e suite only covered the "sibling" suppression path (a manual openmetrics config attached to the same container as the discovered integration, via AD labels). Add coverage for the other, independent suppression path: a manually-authored, host-level conf.d/openmetrics.d/conf.yaml (no ad_identifiers at all) claiming a namespace that matches a different container's discoverable integration, tracked host-wide via StaticConfigIndex. Uses haproxy (fake-haproxy) as the conflicting integration so krakend's existing tests double as the non-conflict control, and extends the shared fake metrics server to emulate both integrations. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
What does this PR do?
Fixes DSCVR-626: Autodiscovery's configuration-discovery feature (
discovery: {}in a shippedauto_conf.yaml, e.g. krakend, haproxy, gearmand) could schedule a duplicate check on a container or host that already has a manually-configured genericopenmetrics/prometheuscheck claiming the same (or a rooted-in) metric namespace. Both checks would then scrape the same target and submit to the same final metric name; for counter-derived metrics this doubles the reported value, since Datadog sums same-context contributions within a flush interval.filterTemplatesDiscoverynow also drops a discovery template when:openmetrics/prometheustemplate matched to the same service configures a namespace whose root matches the integration's own expected namespace root, oropenmetrics/prometheusconfig claims such a namespace anywhere on the host (both tracked in the existingStaticConfigIndex, which now also indexes namespace roots — no separate index needed).An integration's "expected namespace root" is its own check name by default, or the root of its
auto_conf.yaml'sdiscovery.metrics_prefixfield when declared — see integrations-core#24861, which addsmetrics_prefixfor the three integrations (gearmand,krakend,prefect) whose real namespace diverges from their check name. This replaces an earlier hard-coded override map forzk/gearmand(dropped as a layering violation / maintenance burden) with a declarative, per-integration field. Only the root ofmetrics_prefixis used even when it's itself multi-segment (e.g. krakend'skrakend.api), since a generic scraper's own namespace/rename could independently collide at a shorter prefix and there's no way to tell from the raw instance config alone.Also detects the case where a generic config omits
namespace:entirely but explicitly renames a raw metric to an already fully-qualified name (e.g.metrics: [{envoy_requests_total: envoy.requests_total}]) — with no namespace set, the check submits that name completely unprefixed, colliding with the native integration's own metric just the same.Known gap, out of scope here
A separate race exists: an async configuration-discovery probe can take several retry cycles to complete, and a conflicting generic-integration config that appears while the probe is in flight can still get scheduled once the probe finally returns, since the synchronous check above only runs before the probe is enqueued. This is a pre-existing issue (not introduced here) that #54368 fixes generally, for every conflict rule. This PR deliberately leaves that race unfixed to avoid duplicating/competing with that fix.
Motivation
Confirmed empirically (real Docker containers, real agent build, real Datadog org) that a manual
openmetrics/prometheusconfig and a later-discovered dedicated integration both targeting the same container/host can double-count metrics for counter-derived metrics — e.g. a synthetic counter with a known ground-truth rate of 600/min was reported at exactly 1200/min with both checks active.Describe how you validated your changes
comp/core/autodiscovery/listeners/{common_filter,service}_test.goandcomp/core/autodiscovery/integration/config_test.go: per-service sibling namespace match/mismatch, no-namespace-set sibling (no false positive), global static-config namespace match/mismatch, the metrics-rename-without-namespace case,metrics_prefix-driven detection (including the gearmand check-name/namespace divergence case, both via the sibling and the static-index path), and a digest regression test (aDiscovery.MetricsPrefix-only change must change the config digest, or anauto_conf.yamlupdate that only adds/changes it would be silently treated as an already-tracked config).dda inv test --targets=./comp/core/autodiscovery/...passes locally (965 tests).dda inv linter.go --targets=./comp/core/autodiscovery/...passes locally.go-code-reviewer; applied all confirmed findings, including the digest bug above and a redundant lock acquisition in the common (no-metrics_prefix) path.test/new-e2e/tests/discovery/config_discovery_linux_test.go) with a second fake container (docker-compose.fake-krakend-conflict.yaml.tmpl) carrying a conflicting manualopenmetricsconfig (namespace: krakend.api, rooted in krakend's own__NAMESPACE__), asserting discovery is suppressed there while the original krakend discovery test is unaffected. The two compose fixtures' previously copy-pasted fake metrics server is now a single shared file (fake-krakend-server.py), spliced into both viatext/template(the same pattern already used elsewhere in this test package for Helm values). Validated via CI (new-e2e-discovery), not locally (requires real AWS infra).Additional Notes
No changes needed to the discoverer package or any config-flag plumbing. The
StaticConfigIndexshared between listeners and the config manager does double duty: it's keyed by check name for the pre-existing same-name-sibling rule, and now also by namespace root for the new namespace-conflict rule — no new data structure required.PR description updated by Claude Code.