Skip to content

Commit fbd4e3d

Browse files
authored
autodiscovery: detect metric-rename collisions when namespace is omitted
DSCVR-626-followup: a generic openmetrics/prometheus config that omits `namespace:` but explicitly renames a raw metric to a fully-qualified dotted name (e.g. `envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active`) submits that name completely unprefixed, colliding with the native integration's own metric exactly the same way a matching `namespace:` field would. Verified against datadog_checks_base (AgentCheck._format_namespace, MetricTransformer.normalize_metric_config, and the legacy metrics_mapper loops in openmetrics/prometheus mixins) in integrations-core. See https://datadoghq.atlassian.net/browse/DSCVR-626?focusedCommentId=3493570 InstanceNamespaces is replaced by GenericIntegrationNamespaceRoots, which returns a namespace root per instance either from `namespace:` or, when that's unset, from the root of any explicit `metrics`/`extra_metrics` rename target -- feeding into the exact same StaticConfigIndex/set-based matching already used for the namespace case, no new suppression codepath. Environment: Datadog workspace Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Vincent Whitchurch <vincent.whitchurch@datadoghq.com>
1 parent 02dbfc6 commit fbd4e3d

6 files changed

Lines changed: 246 additions & 26 deletions

File tree

comp/core/autodiscovery/impl/configmgr.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,8 +312,8 @@ func (cm *reconcilingConfigManager) processNewConfig(config integration.Config)
312312
// such a config is already claiming the same metric namespace
313313
// (see filterTemplatesDiscovery).
314314
if listeners.IsGenericIntegrationCheckName(config.Name) {
315-
for _, ns := range listeners.InstanceNamespaces(decryptedConfig) {
316-
cm.staticConfigIndex.Add(listeners.NamespaceRoot(ns))
315+
for _, root := range listeners.GenericIntegrationNamespaceRoots(decryptedConfig) {
316+
cm.staticConfigIndex.Add(root)
317317
}
318318
}
319319
}
@@ -374,8 +374,8 @@ func (cm *reconcilingConfigManager) processDelConfigs(configs []integration.Conf
374374
cm.staticConfigIndex.Remove(config.Name)
375375

376376
if listeners.IsGenericIntegrationCheckName(config.Name) {
377-
for _, ns := range listeners.InstanceNamespaces(config) {
378-
cm.staticConfigIndex.Remove(listeners.NamespaceRoot(ns))
377+
for _, root := range listeners.GenericIntegrationNamespaceRoots(config) {
378+
cm.staticConfigIndex.Remove(root)
379379
}
380380
}
381381
}

comp/core/autodiscovery/impl/configmgr_discovery.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,8 @@ func (cm *reconcilingConfigManager) conflictingGenericIntegrationLocked(svcID, t
217217
if !found || !listeners.IsGenericIntegrationCheckName(otherCfg.Name) {
218218
continue
219219
}
220-
for _, ns := range listeners.InstanceNamespaces(otherCfg) {
221-
if listeners.NamespaceRoot(ns) == checkName {
220+
for _, root := range listeners.GenericIntegrationNamespaceRoots(otherCfg) {
221+
if root == checkName {
222222
return otherCfg.Name, true
223223
}
224224
}

comp/core/autodiscovery/impl/configmgr_discovery_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -766,6 +766,65 @@ func TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved(t *testing.T) {
766766
assert.Contains(t, warnings, "krakend")
767767
}
768768

769+
// TestConfigMgr_Discovery_SuppressedBySiblingMetricRename mirrors
770+
// TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved, but the sibling
771+
// has no namespace: field at all -- instead it renames a metric directly to
772+
// krakend's own fully-qualified namespace via metrics:, which (per
773+
// https://datadoghq.atlassian.net/browse/DSCVR-626?focusedCommentId=3493570)
774+
// is submitted completely unprefixed and collides just the same.
775+
func TestConfigMgr_Discovery_SuppressedBySiblingMetricRename(t *testing.T) {
776+
errorStats = newAcErrorStats() // isolate from other tests' warnings
777+
778+
mockResolver := MockSecretResolver{}
779+
disco := newStubDiscoverer(func(_, _ string) (string, error) {
780+
return `[{"instances":[{"openmetrics_endpoint":"http://%%host%%:9091/metrics"}]}]`, nil
781+
})
782+
cm := newReconcilingConfigManager(&mockResolver, nil, nil, disco, nil).(*reconcilingConfigManager)
783+
cm.start()
784+
t.Cleanup(cm.stop)
785+
786+
svc := &dummyService{
787+
ID: "docker://conflict-2",
788+
ADIdentifiers: []string{"krakend", "docker://conflict-2"},
789+
Hosts: map[string]string{"main": "10.0.0.1"},
790+
}
791+
792+
sibling := integration.Config{
793+
Name: "openmetrics",
794+
ADIdentifiers: []string{"docker://conflict-2"},
795+
Instances: []integration.Data{[]byte(
796+
"openmetrics_endpoint: http://10.0.0.1:9091/metrics\n" +
797+
"metrics:\n - krakend_requests_total: krakend.api.requests_total",
798+
)},
799+
}
800+
_ = cm.processNewService(svc)
801+
changes, _ := cm.processNewConfig(sibling)
802+
assertConfigsMatch(t, changes.Schedule, matchName("openmetrics"))
803+
require.Len(t, changes.Schedule, 1, "the openmetrics sibling should resolve synchronously")
804+
805+
tpl := integration.Config{
806+
Name: "krakend",
807+
ADIdentifiers: []string{"krakend"},
808+
Discovery: &integration.DiscoveryConfig{},
809+
}
810+
_, _ = cm.processNewConfig(tpl)
811+
812+
select {
813+
case discovered := <-cm.discoveredChanges():
814+
t.Fatalf("krakend discovery should have been suppressed, got: %+v", discovered)
815+
case <-time.After(500 * time.Millisecond):
816+
}
817+
818+
cm.m.Lock()
819+
resolvedDigest, found := cm.serviceResolutions[svc.ID][tpl.Digest()]
820+
cm.m.Unlock()
821+
require.True(t, found, "the suppressed digest should still be recorded so reconcileService doesn't reprobe")
822+
assert.Equal(t, discoverySuppressedDigest, resolvedDigest)
823+
824+
warnings := GetResolveWarnings()
825+
assert.Contains(t, warnings, "krakend")
826+
}
827+
769828
// TestConfigMgr_Discovery_SuppressedByHostWideStaticNamespace mirrors the
770829
// above but for the non-service-scoped case: a static (non-template)
771830
// openmetrics config claiming a matching namespace anywhere on the host,

comp/core/autodiscovery/listeners/common_filter.go

Lines changed: 74 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package listeners
77

88
import (
9+
"slices"
910
"strings"
1011

1112
yaml "go.yaml.in/yaml/v2"
@@ -72,24 +73,85 @@ func NamespaceRoot(namespace string) string {
7273
return namespace
7374
}
7475

75-
// InstanceNamespaces returns the configured `namespace` of each instance in
76-
// cfg.Instances that sets one explicitly. Instances with no namespace
77-
// configured are skipped: with nothing to compare, assuming a match would
78-
// risk suppressing discovery unnecessarily. Exported so the config manager
79-
// can use the same logic to populate StaticConfigIndex with namespace roots
80-
// from scheduled static (non-template) generic-scraper configs.
81-
func InstanceNamespaces(cfg integration.Config) []string {
82-
var namespaces []string
76+
// GenericIntegrationNamespaceRoots returns, for each instance in cfg, the
77+
// metric-namespace root (see NamespaceRoot) it would submit metrics under:
78+
// - if the instance sets an explicit `namespace:`, that field's root, or
79+
// - otherwise, the root of each explicit metric rename target in the
80+
// instance's `metrics`/`extra_metrics` field (see
81+
// instanceMetricRenameTargets).
82+
//
83+
// The metrics-rename fallback only matters when namespace is unset: a
84+
// generic openmetrics/prometheus check submits `namespace.metric_name`, but
85+
// when namespace is empty the metric name is submitted completely
86+
// unprefixed (verified in datadog_checks_base's AgentCheck._format_namespace)
87+
// — so a rename target that's already a fully-qualified dotted name (e.g.
88+
// `envoy_cluster_http2_streams_active: envoy.cluster.http2.streams_active`)
89+
// collides with the native integration's own metric, and there's no
90+
// `namespace:` value to catch it. When namespace *is* set, it's prepended on
91+
// top of the rename target regardless, so the rename can't itself collide —
92+
// hence checking metrics only in the no-namespace case. See
93+
// https://datadoghq.atlassian.net/browse/DSCVR-626?focusedCommentId=3493570
94+
//
95+
// Instances with neither an explicit namespace nor a qualifying rename
96+
// contribute nothing: with no signal to compare, assuming a match would risk
97+
// suppressing discovery unnecessarily. Exported so the config manager can use
98+
// the same logic to populate StaticConfigIndex with namespace roots from
99+
// scheduled static (non-template) generic-scraper configs.
100+
func GenericIntegrationNamespaceRoots(cfg integration.Config) []string {
101+
var roots []string
83102
for _, inst := range cfg.Instances {
84103
var common integration.CommonInstanceConfig
85104
if err := yaml.Unmarshal(inst, &common); err != nil {
86105
continue
87106
}
88107
if common.Namespace != "" {
89-
namespaces = append(namespaces, common.Namespace)
108+
roots = append(roots, NamespaceRoot(common.Namespace))
109+
continue
110+
}
111+
for _, target := range instanceMetricRenameTargets(inst) {
112+
roots = append(roots, NamespaceRoot(target))
113+
}
114+
}
115+
return roots
116+
}
117+
118+
// instanceMetricRenameTargets returns the explicit rename target of each
119+
// entry in inst's `metrics`/`extra_metrics` field that renames a raw metric
120+
// to a different name, mirroring the shapes accepted by
121+
// MetricTransformer.normalize_metric_config (openmetrics v2) and the legacy
122+
// metrics_mapper loops (openmetrics v1, prometheus) in datadog_checks_base:
123+
// each list entry is either
124+
// - a plain string: pass-through, not a rename, skipped;
125+
// - a single-key map to a string: the string is the rename target; or
126+
// - a single-key map to a nested map with a `name` key: that key's value is
127+
// the rename target (no `name` key means the raw metric name is kept,
128+
// i.e. still not a rename, skipped).
129+
func instanceMetricRenameTargets(inst integration.Data) []string {
130+
var raw struct {
131+
Metrics []interface{} `yaml:"metrics"`
132+
ExtraMetrics []interface{} `yaml:"extra_metrics"`
133+
}
134+
if err := yaml.Unmarshal(inst, &raw); err != nil {
135+
return nil
136+
}
137+
var targets []string
138+
for _, entry := range slices.Concat(raw.Metrics, raw.ExtraMetrics) {
139+
m, ok := entry.(map[interface{}]interface{})
140+
if !ok {
141+
continue // plain string (or any other scalar): pass-through, no rename
142+
}
143+
for _, value := range m {
144+
switch v := value.(type) {
145+
case string:
146+
targets = append(targets, v)
147+
case map[interface{}]interface{}:
148+
if name, ok := v["name"].(string); ok {
149+
targets = append(targets, name)
150+
}
151+
}
90152
}
91153
}
92-
return namespaces
154+
return targets
93155
}
94156

95157
// filterTemplatesDiscovery drops configuration-discovery templates that are
@@ -122,8 +184,8 @@ func filterTemplatesDiscovery(staticIdx *StaticConfigIndex, configs map[string]i
122184
}
123185
nonDiscoveryNames[cfg.Name] = struct{}{}
124186
if IsGenericIntegrationCheckName(cfg.Name) {
125-
for _, ns := range InstanceNamespaces(cfg) {
126-
siblingGenericNamespaceRoots[NamespaceRoot(ns)] = struct{}{}
187+
for _, root := range GenericIntegrationNamespaceRoots(cfg) {
188+
siblingGenericNamespaceRoots[root] = struct{}{}
127189
}
128190
}
129191
}

comp/core/autodiscovery/listeners/common_filter_test.go

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,14 +38,65 @@ func TestIsGenericIntegrationCheckName(t *testing.T) {
3838
assert.False(t, IsGenericIntegrationCheckName(""))
3939
}
4040

41-
func TestInstanceNamespaces(t *testing.T) {
42-
cfg := integration.Config{
43-
Name: "openmetrics",
44-
Instances: []integration.Data{
45-
[]byte("namespace: krakend.api\nopenmetrics_endpoint: http://1.2.3.4:9091/metrics"),
46-
[]byte("openmetrics_endpoint: http://1.2.3.4:9092/metrics"), // no namespace set
47-
[]byte("not: valid: yaml: ["), // malformed, should be skipped
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,
4876
},
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+
})
49101
}
50-
assert.Equal(t, []string{"krakend.api"}, InstanceNamespaces(cfg))
51102
}

comp/core/autodiscovery/listeners/service_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,54 @@ func TestServiceFilterTemplatesDiscovery(t *testing.T) {
421421
"discovery template should be kept when the sibling has no explicit namespace to compare")
422422
})
423423

424+
t.Run("discovery dropped when sibling has no namespace but renames a metric to the fully-qualified name", func(t *testing.T) {
425+
// See https://datadoghq.atlassian.net/browse/DSCVR-626?focusedCommentId=3493570:
426+
// with no namespace set, an explicit metrics: rename target is
427+
// submitted completely unprefixed, so a rename to
428+
// "krakend.api...." collides with krakend's own metrics exactly the
429+
// same way a matching namespace: field would.
430+
sibling := integration.Config{
431+
Name: "openmetrics",
432+
Provider: names.KubeContainer,
433+
ADIdentifiers: []string{"container-1"},
434+
Instances: []integration.Data{[]byte(
435+
"openmetrics_endpoint: http://1.2.3.4:9091/metrics\n" +
436+
"metrics:\n - krakend_requests_total: krakend.api.requests_total",
437+
)},
438+
Source: "container:docker://container-1",
439+
}
440+
configs := map[string]integration.Config{
441+
krakendDiscoveryTpl.Digest(): krakendDiscoveryTpl,
442+
sibling.Digest(): sibling,
443+
}
444+
mkSvc(NewStaticConfigIndex()).FilterTemplates(configs)
445+
assert.NotContains(t, configs, krakendDiscoveryTpl.Digest(),
446+
"discovery template should be dropped when a namespace-less sibling renames a metric to krakend's own namespace")
447+
})
448+
449+
t.Run("discovery kept when sibling has a namespace and also renames a metric to a matching name", func(t *testing.T) {
450+
// The rename target only matters when namespace is unset -- when
451+
// namespace is set, it's prepended on top of the rename target
452+
// regardless, so this isn't a real collision.
453+
sibling := integration.Config{
454+
Name: "openmetrics",
455+
Provider: names.KubeContainer,
456+
ADIdentifiers: []string{"container-1"},
457+
Instances: []integration.Data{[]byte(
458+
"namespace: myapp\nopenmetrics_endpoint: http://1.2.3.4:9091/metrics\n" +
459+
"metrics:\n - krakend_requests_total: krakend.api.requests_total",
460+
)},
461+
Source: "container:docker://container-1",
462+
}
463+
configs := map[string]integration.Config{
464+
krakendDiscoveryTpl.Digest(): krakendDiscoveryTpl,
465+
sibling.Digest(): sibling,
466+
}
467+
mkSvc(NewStaticConfigIndex()).FilterTemplates(configs)
468+
assert.Contains(t, configs, krakendDiscoveryTpl.Digest(),
469+
"discovery template should be kept: sibling's namespace is prepended regardless of the rename target")
470+
})
471+
424472
t.Run("discovery dropped when static config index has a rooted-in namespace match", func(t *testing.T) {
425473
// The config manager adds the *root* of a static generic-integration
426474
// config's namespace (see listeners.NamespaceRoot) to the same

0 commit comments

Comments
 (0)