Skip to content

Commit 96f0bee

Browse files
committed
autodiscovery: suppress configuration discovery on generic-integration 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>
1 parent daf42da commit 96f0bee

10 files changed

Lines changed: 676 additions & 102 deletions

comp/core/autodiscovery/impl/configmgr.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,10 @@ type reconcilingConfigManager struct {
124124

125125
// staticConfigIndex is a shared name set published to listeners so they
126126
// can deduplicate templates against static configs (see ProcessService).
127+
// It also tracks the namespace root (see listeners.NamespaceRoot) of every
128+
// scheduled static openmetrics/prometheus config, so a configuration-
129+
// discovery template can be suppressed when a host-wide generic-scraper
130+
// config already claims its metric namespace (see filterTemplatesDiscovery).
127131
// May be nil; callers that don't need cross-listener dedup can omit it.
128132
staticConfigIndex *listeners.StaticConfigIndex
129133

@@ -283,6 +287,17 @@ func (cm *reconcilingConfigManager) processNewConfig(config integration.Config)
283287
// duplicate scheduled until something else perturbs the service.
284288
if len(decryptedConfig.Instances) > 0 {
285289
cm.staticConfigIndex.Add(config.Name)
290+
291+
// Also index the namespace root(s) of host-wide static
292+
// openmetrics/prometheus configs under the same set, so discovery
293+
// templates for a dedicated integration can be suppressed when
294+
// such a config is already claiming the same metric namespace
295+
// (see filterTemplatesDiscovery).
296+
if listeners.IsGenericIntegrationCheckName(config.Name) {
297+
for _, root := range listeners.GenericIntegrationNamespaceRoots(decryptedConfig) {
298+
cm.staticConfigIndex.Add(root)
299+
}
300+
}
286301
}
287302
}
288303

@@ -339,6 +354,12 @@ func (cm *reconcilingConfigManager) processDelConfigs(configs []integration.Conf
339354
// Update the cross-listener index.
340355
if len(config.Instances) > 0 {
341356
cm.staticConfigIndex.Remove(config.Name)
357+
358+
if listeners.IsGenericIntegrationCheckName(config.Name) {
359+
for _, root := range listeners.GenericIntegrationNamespaceRoots(config) {
360+
cm.staticConfigIndex.Remove(root)
361+
}
362+
}
342363
}
343364
}
344365

comp/core/autodiscovery/listeners/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ go_library(
7373
"//pkg/util/log",
7474
"//pkg/util/option",
7575
"@com_github_gosnmp_gosnmp//:gosnmp",
76+
"@in_yaml_go_yaml_v2//:yaml",
7677
"@io_k8s_api//core/v1:core",
7778
"@io_k8s_api//discovery/v1:discovery",
7879
"@io_k8s_apimachinery//pkg/api/equality",
@@ -92,6 +93,7 @@ dd_agent_go_test(
9293
name = "listeners_test",
9394
srcs = [
9495
"cloudfoundry_test.go",
96+
"common_filter_test.go",
9597
"common_test.go",
9698
"container_test.go",
9799
"dbm_aurora_test.go",

comp/core/autodiscovery/listeners/common_filter.go

Lines changed: 140 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66
package listeners
77

88
import (
9+
"slices"
10+
"strings"
11+
12+
yaml "go.yaml.in/yaml/v2"
13+
914
"github.com/DataDog/datadog-agent/comp/core/autodiscovery/integration"
1015
workloadfilter "github.com/DataDog/datadog-agent/comp/core/workloadfilter/def"
1116
"github.com/DataDog/datadog-agent/pkg/util/log"
@@ -29,24 +34,138 @@ func filterTemplatesMatched(svc FilterableService, configs map[string]integratio
2934
}
3035
}
3136

32-
// genericIntegrationNames are check names for generic metric-scraping
33-
// integrations that customers commonly point at any service, potentially
34-
// under a different check name than the one discovery would configure. Their
35-
// presence for a service (or host) is treated as covering every integration,
36-
// since we can't tell whether they already scrape the same metrics.
37-
var genericIntegrationNames = map[string]struct{}{
37+
// genericIntegrationCheckNames are check names whose entire configuration —
38+
// including the metric namespace — is supplied directly by the user, rather
39+
// than being intrinsic to a dedicated integration. These are commonly used as
40+
// a fallback to collect metrics from services that don't (yet) have a
41+
// dedicated Datadog integration. A configuration-discovery template is
42+
// suppressed when one of these already claims the same metric namespace the
43+
// discovery-driven integration would use, since that's a strong, specific
44+
// signal the user is already covering it manually.
45+
var genericIntegrationCheckNames = map[string]struct{}{
3846
"openmetrics": {},
3947
"prometheus": {},
4048
}
4149

50+
// IsGenericIntegrationCheckName reports whether name is a "generic"
51+
// integration check name (openmetrics, prometheus) — see
52+
// genericIntegrationCheckNames. Exported so the config manager can use the
53+
// same check to decide which scheduled static configs to also track by
54+
// namespace root in StaticConfigIndex.
55+
func IsGenericIntegrationCheckName(name string) bool {
56+
_, ok := genericIntegrationCheckNames[name]
57+
return ok
58+
}
59+
60+
// NamespaceRoot returns the portion of namespace before the first '.', or the
61+
// whole string if there is none — e.g. "krakend.api" roots to "krakend". A
62+
// discovery-driven integration's check name is assumed to equal its own
63+
// metric namespace's root (true for the vast majority of integrations; a
64+
// small, currently-accepted set of exceptions diverge — e.g. zk's own
65+
// namespace is "zookeeper", not "zk"), so comparing a generic-scraper
66+
// namespace's root against a discovery template's check name directly is
67+
// enough to detect a conflict without a hand-maintained map.
68+
func NamespaceRoot(namespace string) string {
69+
if i := strings.IndexByte(namespace, '.'); i >= 0 {
70+
return namespace[:i]
71+
}
72+
return namespace
73+
}
74+
75+
// GenericIntegrationNamespaceRoots returns, for each instance in cfg, the
76+
// metric-namespace root (see NamespaceRoot) it would submit metrics under:
77+
// - if the instance sets an explicit `namespace:`, that field's root, or
78+
// - otherwise, the root of each explicit metric rename target in the
79+
// instance's `metrics`/`extra_metrics` field (see
80+
// instanceMetricRenameTargets).
81+
//
82+
// The metrics-rename fallback only matters when namespace is unset: a
83+
// generic openmetrics/prometheus check submits `namespace.metric_name`, but
84+
// when namespace is empty the metric name is submitted completely
85+
// unprefixed (verified in datadog_checks_base's AgentCheck._format_namespace)
86+
// — so a rename target that's already a fully-qualified dotted name (e.g.
87+
// `envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active`)
88+
// collides with the native integration's own metric, and there's no
89+
// `namespace:` value to catch it. When namespace *is* set, it's prepended on
90+
// top of the rename target regardless, so the rename can't itself collide —
91+
// hence checking metrics only in the no-namespace case.
92+
//
93+
// Instances with neither an explicit namespace nor a qualifying rename
94+
// contribute nothing: with no signal to compare, assuming a match would risk
95+
// suppressing discovery unnecessarily. Exported so the config manager can use
96+
// the same logic to populate StaticConfigIndex with namespace roots from
97+
// scheduled static (non-template) generic-scraper configs.
98+
func GenericIntegrationNamespaceRoots(cfg integration.Config) []string {
99+
var roots []string
100+
for _, inst := range cfg.Instances {
101+
var common integration.CommonInstanceConfig
102+
if err := yaml.Unmarshal(inst, &common); err != nil {
103+
continue
104+
}
105+
if common.Namespace != "" {
106+
roots = append(roots, NamespaceRoot(common.Namespace))
107+
continue
108+
}
109+
for _, target := range instanceMetricRenameTargets(inst) {
110+
roots = append(roots, NamespaceRoot(target))
111+
}
112+
}
113+
return roots
114+
}
115+
116+
// instanceMetricRenameTargets returns the explicit rename target of each
117+
// entry in inst's `metrics`/`extra_metrics` field that renames a raw metric
118+
// to a different name, mirroring the shapes accepted by
119+
// MetricTransformer.normalize_metric_config (openmetrics v2) and the legacy
120+
// metrics_mapper loops (openmetrics v1, prometheus) in datadog_checks_base:
121+
// each list entry is either
122+
// - a plain string: pass-through, not a rename, skipped;
123+
// - a single-key map to a string: the string is the rename target; or
124+
// - a single-key map to a nested map with a `name` key: that key's value is
125+
// the rename target (no `name` key means the raw metric name is kept,
126+
// i.e. still not a rename, skipped).
127+
func instanceMetricRenameTargets(inst integration.Data) []string {
128+
var raw struct {
129+
Metrics []interface{} `yaml:"metrics"`
130+
ExtraMetrics []interface{} `yaml:"extra_metrics"`
131+
}
132+
if err := yaml.Unmarshal(inst, &raw); err != nil {
133+
return nil
134+
}
135+
var targets []string
136+
for _, entry := range slices.Concat(raw.Metrics, raw.ExtraMetrics) {
137+
m, ok := entry.(map[interface{}]interface{})
138+
if !ok {
139+
continue // plain string (or any other scalar): pass-through, no rename
140+
}
141+
for _, value := range m {
142+
switch v := value.(type) {
143+
case string:
144+
targets = append(targets, v)
145+
case map[interface{}]interface{}:
146+
if name, ok := v["name"].(string); ok {
147+
targets = append(targets, name)
148+
}
149+
}
150+
}
151+
}
152+
return targets
153+
}
154+
42155
// filterTemplatesDiscovery drops configuration-discovery templates that are
43-
// redundant with another config source for the same integration. Dropped when:
156+
// redundant with another config source for the same integration, or with a
157+
// generic scraper (openmetrics/prometheus) config that's already claiming the
158+
// same metric namespace. Dropped when:
44159
// 1. another check template (Instances > 0) for the same integration Name has
45160
// matched this same service (present in configs), or
46-
// 2. a scheduled non-template (static) config exists for the same Name
47-
// (tracked in staticIdx), or
48-
// 3. a generic integration (openmetrics/prometheus) config matched this
49-
// service or is scheduled host-wide, regardless of its Name.
161+
// 2. a sibling generic-scraper (openmetrics/prometheus) template matched to
162+
// this same service configures a namespace whose root matches this
163+
// integration's check name, or
164+
// 3. a scheduled non-template (static) config exists for the same Name, or a
165+
// scheduled non-template generic-scraper config anywhere on the host
166+
// configures a namespace whose root matches this integration's check name
167+
// (both tracked, by check name and by namespace root respectively, in the
168+
// same staticIdx — see configmgr.go).
50169
//
51170
// Logs-only sibling templates (no Instances) are ignored — discovery covers
52171
// metric-check configuration and shouldn't be suppressed by an integration's
@@ -56,28 +175,25 @@ func filterTemplatesDiscovery(staticIdx *StaticConfigIndex, configs map[string]i
56175
return
57176
}
58177
nonDiscoveryNames := map[string]struct{}{}
59-
hasGenericSibling := false
178+
siblingGenericNamespaceRoots := map[string]struct{}{}
60179
for _, cfg := range configs {
61-
if !cfg.IsDiscovery() && len(cfg.Instances) > 0 {
62-
nonDiscoveryNames[cfg.Name] = struct{}{}
63-
if _, ok := genericIntegrationNames[cfg.Name]; ok {
64-
hasGenericSibling = true
65-
}
180+
if cfg.IsDiscovery() || len(cfg.Instances) == 0 {
181+
continue
66182
}
67-
}
68-
hasGenericStatic := false
69-
for name := range genericIntegrationNames {
70-
if staticIdx.Has(name) {
71-
hasGenericStatic = true
72-
break
183+
nonDiscoveryNames[cfg.Name] = struct{}{}
184+
if IsGenericIntegrationCheckName(cfg.Name) {
185+
for _, root := range GenericIntegrationNamespaceRoots(cfg) {
186+
siblingGenericNamespaceRoots[root] = struct{}{}
187+
}
73188
}
74189
}
75190
for digest, cfg := range configs {
76191
if !cfg.IsDiscovery() {
77192
continue
78193
}
79194
_, hasSibling := nonDiscoveryNames[cfg.Name]
80-
if hasGenericSibling || hasGenericStatic || hasSibling || staticIdx.Has(cfg.Name) {
195+
_, hasNamespaceConflict := siblingGenericNamespaceRoots[cfg.Name]
196+
if hasSibling || hasNamespaceConflict || staticIdx.Has(cfg.Name) {
81197
log.Debugf("Ignoring discovery template %s from %s: another config source already covers this integration",
82198
cfg.Name, cfg.Source)
83199
delete(configs, digest)
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed
2+
// under the Apache License Version 2.0.
3+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
4+
// Copyright 2017-present Datadog, Inc.
5+
6+
package listeners
7+
8+
import (
9+
"testing"
10+
11+
"github.com/stretchr/testify/assert"
12+
13+
"github.com/DataDog/datadog-agent/comp/core/autodiscovery/integration"
14+
)
15+
16+
func TestNamespaceRoot(t *testing.T) {
17+
cases := []struct {
18+
name string
19+
namespace string
20+
want string
21+
}{
22+
{"no dot", "haproxy", "haproxy"},
23+
{"rooted namespace", "krakend.api", "krakend"},
24+
{"multiple dots roots at the first one", "a.b.c", "a"},
25+
{"empty", "", ""},
26+
}
27+
for _, tc := range cases {
28+
t.Run(tc.name, func(t *testing.T) {
29+
assert.Equal(t, tc.want, NamespaceRoot(tc.namespace))
30+
})
31+
}
32+
}
33+
34+
func TestIsGenericIntegrationCheckName(t *testing.T) {
35+
assert.True(t, IsGenericIntegrationCheckName("openmetrics"))
36+
assert.True(t, IsGenericIntegrationCheckName("prometheus"))
37+
assert.False(t, IsGenericIntegrationCheckName("krakend"))
38+
assert.False(t, IsGenericIntegrationCheckName(""))
39+
}
40+
41+
func TestGenericIntegrationNamespaceRoots(t *testing.T) {
42+
cases := []struct {
43+
name string
44+
yaml string
45+
want []string
46+
}{
47+
{
48+
"explicit namespace",
49+
"namespace: krakend.api\nopenmetrics_endpoint: http://1.2.3.4:9091/metrics",
50+
[]string{"krakend"},
51+
},
52+
{
53+
"no namespace, no metrics",
54+
"openmetrics_endpoint: http://1.2.3.4:9092/metrics",
55+
nil,
56+
},
57+
{
58+
"no namespace, plain-string metrics entries are pass-through, not renames",
59+
"metrics:\n - envoy_cluster_http2_streams_active\n - envoy_.*",
60+
nil,
61+
},
62+
{
63+
"no namespace, single-key map to string is a rename",
64+
"metrics:\n - envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active",
65+
[]string{"envoy"},
66+
},
67+
{
68+
"no namespace, single-key map to nested map with name is a rename",
69+
"metrics:\n - envoy_cluster_http2_streams_active:\n name: envoy.cluster.http2.streams_active\n type: rate",
70+
[]string{"envoy"},
71+
},
72+
{
73+
"no namespace, single-key map to nested map without name keeps the raw name, not a rename",
74+
"metrics:\n - envoy_cluster_http2_streams_active:\n type: rate",
75+
nil,
76+
},
77+
{
78+
"no namespace, extra_metrics handled the same as metrics",
79+
"extra_metrics:\n - envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active",
80+
[]string{"envoy"},
81+
},
82+
{
83+
"namespace set: metrics renames are ignored, since namespace is prepended regardless",
84+
"namespace: myapp\nmetrics:\n - envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active",
85+
[]string{"myapp"},
86+
},
87+
{
88+
"malformed yaml is skipped gracefully",
89+
"not: valid: yaml: [",
90+
nil,
91+
},
92+
}
93+
for _, tc := range cases {
94+
t.Run(tc.name, func(t *testing.T) {
95+
cfg := integration.Config{
96+
Name: "openmetrics",
97+
Instances: []integration.Data{[]byte(tc.yaml)},
98+
}
99+
assert.Equal(t, tc.want, GenericIntegrationNamespaceRoots(cfg))
100+
})
101+
}
102+
}

0 commit comments

Comments
 (0)