Skip to content

Commit 0b134fc

Browse files
authored
feat(autodiscovery): tag configuration-discovery instances to mitigate duplicate metrics risk (#54660)
### What does this PR do? Adds a `dd_config_discovery:true` tag to every check instance scheduled via the Autodiscovery configuration-discovery mechanism (i.e. any `auto_conf.yaml` template with `discovery: {}`, resolved through `comp/core/autodiscovery/impl/configmgr_discovery.go`'s `applyDiscoveredConfigsLocked`). The tag used is `dd_config_discovery:true`, a plain `dd_`-prefixed key, following the precedent of other agent-added, customer-visible marker/provenance tags already in the codebase: - `dd_remote_config_id` / `dd_remote_config_rev` (`comp/core/tagger/tags/tags.go`) - `dd_enable_check_intake` (`pkg/collector/worker/worker.go`) ### Motivation [DSCVR-651](https://datadoghq.atlassian.net/browse/DSCVR-651): there is a risk that an agent on host A is monitoring a service on host B with a manually-configured check (e.g. a generic `openmetrics` check), while the agent running locally on host B also autodiscovers and schedules a dedicated integration for the same service via configuration discovery. Neither agent's local anti-duplication logic can see the other's config, so both submit metrics for the same underlying data. This tag doesn't prevent the duplication, but lets users identify and, if needed, exclude the autodiscovered side of it (e.g. `metric{!dd_config_discovery:true}`), both for the cross-host case above and for any single-host case the automatic suppression doesn't catch. ### Describe how you validated your changes Unit and E2E tests. --- 🤖 This PR description and implementation were generated with assistance from [Claude Code](https://claude.com/claude-code). [DSCVR-651]: https://datadoghq.atlassian.net/browse/DSCVR-651?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Co-authored-by: vincent.whitchurch <vincent.whitchurch@datadoghq.com>
1 parent d4251bf commit 0b134fc

7 files changed

Lines changed: 228 additions & 4 deletions

File tree

comp/core/autodiscovery/impl/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ dd_agent_go_test(
120120
"@com_github_stretchr_testify//assert",
121121
"@com_github_stretchr_testify//require",
122122
"@com_github_stretchr_testify//suite",
123+
"@in_yaml_go_yaml_v2//:yaml",
123124
"@org_uber_go_atomic//:atomic",
124125
"@org_uber_go_fx//:fx",
125126
],

comp/core/autodiscovery/impl/common_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ type dummyService struct {
2121
Ports []workloadmeta.ContainerPort
2222
Pid int
2323
Hostname string
24+
Tags []string
2425
filterTemplates func(map[string]integration.Config)
2526
}
2627

@@ -51,7 +52,7 @@ func (s *dummyService) GetPorts() ([]workloadmeta.ContainerPort, error) {
5152

5253
// GetTags returns the tags for this service
5354
func (s *dummyService) GetTags() ([]string, error) {
54-
return nil, nil
55+
return s.Tags, nil
5556
}
5657

5758
// GetTagsWithCardinality returns the tags for this service

comp/core/autodiscovery/impl/configmgr_discovery.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,23 @@ type discoveryState struct {
3535
// of completions without blocking the worker goroutine on a busy scheduler.
3636
const discoveredChangesBuffer = 128
3737

38+
// configDiscoveryTag is added to every instance of a check scheduled via
39+
// configuration discovery. Configuration discovery has no way of knowing
40+
// about a manually-configured, differently-named check (e.g. a generic
41+
// `openmetrics` check) that a user has pointed at the same service from
42+
// elsewhere, so the two can end up scraping the same target and duplicating
43+
// (or, for additive metric types, doubling) submitted metrics. This tag lets
44+
// users spot and exclude the autodiscovered side of such a duplication.
45+
//
46+
// Uses a plain `dd_`-prefixed key (not `dd.internal.*`, which is reserved for
47+
// tags consumed and stripped internally before reaching the backend, e.g.
48+
// dd.internal.resource in pkg/metrics/series.go) so it survives to the
49+
// backend and stays queryable, following the precedent of other
50+
// agent-added, customer-visible marker tags such as dd_remote_config_id /
51+
// dd_remote_config_rev (comp/core/tagger/tags/tags.go) and
52+
// dd_enable_check_intake (pkg/collector/worker/worker.go).
53+
const configDiscoveryTag = "dd_config_discovery:true"
54+
3855
// initDiscoveryWorker wires the workqueue-backed discovery worker into cm.
3956
func initDiscoveryWorker(cm *reconcilingConfigManager, disco discoverer.ConfigDiscoverer) {
4057
cm.discoveredCh = make(chan integration.ConfigChanges, discoveredChangesBuffer)
@@ -152,6 +169,11 @@ func (cm *reconcilingConfigManager) applyDiscoveredConfigsLocked(svcID, tplDiges
152169
return changes
153170
}
154171
resolved.Source = rewriteSource(resolved.Source, svcAndADIDs.svc)
172+
for i := range resolved.Instances {
173+
if err := resolved.Instances[i].MergeAdditionalTags([]string{configDiscoveryTag}); err != nil {
174+
log.Errorf("error adding configuration-discovery tag to config %s for service %s: %v", resolved.Name, svcID, err)
175+
}
176+
}
155177
decrypted, err := decryptConfig(resolved, cm.secretResolver, tplDigest)
156178
if err != nil {
157179
log.Errorf("error decrypting discovered config %s for service %s: %v", resolved.Name, svcID, err)

comp/core/autodiscovery/impl/configmgr_discovery_test.go

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"github.com/stretchr/testify/assert"
1717
"github.com/stretchr/testify/require"
1818
"go.uber.org/atomic"
19+
yaml "go.yaml.in/yaml/v2"
1920

2021
"github.com/DataDog/datadog-agent/comp/core/autodiscovery/discoverer"
2122
"github.com/DataDog/datadog-agent/comp/core/autodiscovery/integration"
@@ -154,6 +155,8 @@ func TestConfigMgr_DiscoveryTemplate_RoutesThroughDiscoverer(t *testing.T) {
154155
"discovered instance config should have %%host%% resolved via the configresolver path")
155156
assert.Equal(t, tc.wantSource, discovered.Schedule[0].Source,
156157
"discovered config's Source should be tagged with the discovery provider")
158+
assert.Contains(t, string(discovered.Schedule[0].Instances[0]), configDiscoveryTag,
159+
"discovered instance config should carry the configuration-discovery marker tag")
157160
case <-time.After(2 * time.Second):
158161
t.Fatalf("timed out waiting for discovered changes")
159162
}
@@ -163,6 +166,145 @@ func TestConfigMgr_DiscoveryTemplate_RoutesThroughDiscoverer(t *testing.T) {
163166
}
164167
}
165168

169+
// countTag returns how many times tag appears in tags.
170+
func countTag(tags []string, tag string) int {
171+
n := 0
172+
for _, t := range tags {
173+
if t == tag {
174+
n++
175+
}
176+
}
177+
return n
178+
}
179+
180+
// instanceTags unmarshals the `tags` field of a resolved instance for
181+
// precise assertions (exact membership/count), rather than substring
182+
// matching on the raw YAML.
183+
func instanceTags(t *testing.T, instance integration.Data) []string {
184+
t.Helper()
185+
var parsed struct {
186+
Tags []string `yaml:"tags"`
187+
}
188+
require.NoError(t, yaml.Unmarshal(instance, &parsed))
189+
return parsed.Tags
190+
}
191+
192+
// TestConfigMgr_DiscoveryTemplate_TagsAllInstances verifies that the
193+
// configuration-discovery marker tag is added exactly once to every instance of
194+
// a discovered config, is merged alongside any tags the discovered instance
195+
// already carries rather than replacing them, and is still added even when the
196+
// discovered config opts out of ordinary autodiscovery service tags via
197+
// ignore_autodiscovery_tags (which must still suppress the service's own tags,
198+
// proving the marker tag is applied independently of that mechanism).
199+
func TestConfigMgr_DiscoveryTemplate_TagsAllInstances(t *testing.T) {
200+
mockResolver := MockSecretResolver{}
201+
disco := newStubDiscoverer(func(_, _ string) (string, error) {
202+
return `[{
203+
"instances": [
204+
{"openmetrics_endpoint": "http://%%host%%:8080/metrics", "tags": ["custom:tag"]},
205+
{"openmetrics_endpoint": "http://%%host%%:8081/metrics"}
206+
],
207+
"ignore_autodiscovery_tags": true
208+
}]`, nil
209+
})
210+
cm := newReconcilingConfigManager(&mockResolver, nil, nil, disco, nil).(*reconcilingConfigManager)
211+
cm.start()
212+
defer cm.stop()
213+
214+
tpl := integration.Config{
215+
Name: "krakend",
216+
ADIdentifiers: []string{"krakend"},
217+
Discovery: &integration.DiscoveryConfig{},
218+
Source: "file:/etc/datadog-agent/conf.d/krakend.d/auto_conf.yaml",
219+
Provider: names.File,
220+
}
221+
svc := &dummyService{
222+
ID: "docker://k1",
223+
ADIdentifiers: []string{"krakend"},
224+
Hosts: map[string]string{"main": "10.0.0.1"},
225+
Tags: []string{"service:tag"},
226+
}
227+
228+
_, _ = cm.processNewConfig(tpl)
229+
changes := cm.processNewService(svc)
230+
assertConfigsMatch(t, changes.Schedule)
231+
assertConfigsMatch(t, changes.Unschedule)
232+
233+
ch := cm.discoveredChanges()
234+
require.NotNil(t, ch)
235+
select {
236+
case discovered := <-ch:
237+
require.Len(t, discovered.Schedule, 1)
238+
require.Len(t, discovered.Schedule[0].Instances, 2)
239+
240+
firstTags := instanceTags(t, discovered.Schedule[0].Instances[0])
241+
assert.Equal(t, 1, countTag(firstTags, configDiscoveryTag),
242+
"marker tag should appear exactly once on the first instance, got %v", firstTags)
243+
assert.Contains(t, firstTags, "custom:tag",
244+
"first instance's own tag should be preserved alongside the marker tag")
245+
assert.NotContains(t, firstTags, "service:tag",
246+
"ignore_autodiscovery_tags should still suppress ordinary service tags")
247+
248+
secondTags := instanceTags(t, discovered.Schedule[0].Instances[1])
249+
assert.Equal(t, 1, countTag(secondTags, configDiscoveryTag),
250+
"marker tag should appear exactly once on the second instance, got %v", secondTags)
251+
case <-time.After(2 * time.Second):
252+
t.Fatalf("timed out waiting for discovered changes")
253+
}
254+
}
255+
256+
// TestConfigMgr_DiscoveryTemplate_TagPersistsAcrossRediscovery verifies that
257+
// a second discovery result for the same service+template (e.g. the
258+
// integration's discover_config returning updated instance data on a later
259+
// probe) still carries exactly one configDiscoveryTag on the replacement
260+
// config, rather than accumulating duplicates or losing the tag across
261+
// re-resolution.
262+
func TestConfigMgr_DiscoveryTemplate_TagPersistsAcrossRediscovery(t *testing.T) {
263+
mockResolver := MockSecretResolver{}
264+
tpl := integration.Config{
265+
Name: "krakend",
266+
ADIdentifiers: []string{"krakend"},
267+
Discovery: &integration.DiscoveryConfig{},
268+
Source: "file:/etc/datadog-agent/conf.d/krakend.d/auto_conf.yaml",
269+
Provider: names.File,
270+
}
271+
svc := &dummyService{
272+
ID: "docker://k1",
273+
ADIdentifiers: []string{"krakend"},
274+
Hosts: map[string]string{"main": "10.0.0.1"},
275+
}
276+
277+
// The discovery worker is never started: this test drives
278+
// applyDiscoveredConfigsLocked directly (as the worker's callback would),
279+
// so no discoverer or running goroutine is needed.
280+
cm := newReconcilingConfigManager(&mockResolver, nil, nil, nil, nil).(*reconcilingConfigManager)
281+
_, _ = cm.processNewConfig(tpl)
282+
_ = cm.processNewService(svc)
283+
284+
tplDigest := tpl.Digest()
285+
firstConfigs := []integration.Config{{
286+
Instances: []integration.Data{integration.Data(`openmetrics_endpoint: http://%%host%%:8080/metrics`)},
287+
}}
288+
cm.m.Lock()
289+
changes := cm.applyDiscoveredConfigsLocked(svc.ID, tplDigest, firstConfigs)
290+
cm.m.Unlock()
291+
require.Len(t, changes.Schedule, 1)
292+
firstTags := instanceTags(t, changes.Schedule[0].Instances[0])
293+
require.Equal(t, 1, countTag(firstTags, configDiscoveryTag))
294+
295+
secondConfigs := []integration.Config{{
296+
Instances: []integration.Data{integration.Data(`openmetrics_endpoint: http://%%host%%:9090/metrics`)},
297+
}}
298+
cm.m.Lock()
299+
changes = cm.applyDiscoveredConfigsLocked(svc.ID, tplDigest, secondConfigs)
300+
cm.m.Unlock()
301+
require.Len(t, changes.Schedule, 1, "rediscovery should schedule the replacement config")
302+
require.Len(t, changes.Unschedule, 1, "rediscovery should unschedule the previous config")
303+
secondTags := instanceTags(t, changes.Schedule[0].Instances[0])
304+
assert.Equal(t, 1, countTag(secondTags, configDiscoveryTag),
305+
"marker tag should appear exactly once on the re-discovered config, got %v", secondTags)
306+
}
307+
166308
// TestConfigMgr_DiscoveryTemplate_ServiceDeletionCancels confirms that
167309
// deleting the service forgets in-flight probes so the worker stops retrying.
168310
func TestConfigMgr_DiscoveryTemplate_ServiceDeletionCancels(t *testing.T) {
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# Each section from every release note are combined when the
2+
# CHANGELOG.rst is rendered. So the text needs to be worded so that
3+
# it does not depend on any information only available in another
4+
# section. This may mean repeating some details, but each section
5+
# must be readable independently of the other.
6+
#
7+
# Each section note must be formatted as reStructuredText.
8+
---
9+
enhancements:
10+
- |
11+
Check instances scheduled via configuration discovery now carry the
12+
``dd_config_discovery:true`` tag. This can be used to identify,
13+
and if needed exclude, metrics submitted by an autodiscovered check that
14+
duplicates a check configured manually elsewhere for the same service.

test/new-e2e/tests/discovery/BUILD.bazel

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,5 +83,13 @@ dd_agent_go_test(
8383
"@com_github_stretchr_testify//require",
8484
"@io_k8s_api//core/v1:core",
8585
"@io_k8s_apimachinery//pkg/apis/meta/v1:meta",
86-
],
86+
] + select({
87+
"@rules_go//go/platform:android": [
88+
"//test/fakeintake/client",
89+
],
90+
"@rules_go//go/platform:linux": [
91+
"//test/fakeintake/client",
92+
],
93+
"//conditions:default": [],
94+
}),
8795
)

test/new-e2e/tests/discovery/config_discovery_linux_test.go

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,17 @@ import (
1212
"testing"
1313
"time"
1414

15-
"github.com/DataDog/datadog-agent/test/e2e-framework/components/datadog/dockeragentparams"
16-
scendocker "github.com/DataDog/datadog-agent/test/e2e-framework/scenarios/aws/ec2docker"
1715
"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
1816
"github.com/stretchr/testify/assert"
1917

18+
"github.com/DataDog/datadog-agent/test/e2e-framework/components/datadog/dockeragentparams"
19+
scendocker "github.com/DataDog/datadog-agent/test/e2e-framework/scenarios/aws/ec2docker"
20+
2021
"github.com/DataDog/datadog-agent/test/e2e-framework/testing/e2e"
2122
"github.com/DataDog/datadog-agent/test/e2e-framework/testing/environments"
2223
awsdocker "github.com/DataDog/datadog-agent/test/e2e-framework/testing/provisioners/aws/docker"
24+
"github.com/DataDog/datadog-agent/test/fakeintake/aggregator"
25+
fakeintakeclient "github.com/DataDog/datadog-agent/test/fakeintake/client"
2326
)
2427

2528
//go:embed testdata/compose/docker-compose.fake-krakend.yaml
@@ -77,6 +80,10 @@ func (s *configDiscoverySuite) verifyKrakendConfigDiscovery(c *assert.CollectT)
7780
t.Logf("configcheck output: %s", configCheckOutput)
7881
return
7982
}
83+
if !assert.True(c, strings.Contains(configCheckOutput, configDiscoveryTag), "krakend config resolved via configuration discovery should carry the %s marker tag", configDiscoveryTag) {
84+
t.Logf("configcheck output: %s", configCheckOutput)
85+
return
86+
}
8087

8188
statusOutput := s.Env().Docker.Client.ExecuteCommand(s.Env().Agent.ContainerName, "agent", "status", "collector", "--json")
8289
var status collectorStatus
@@ -105,6 +112,26 @@ func (s *configDiscoverySuite) verifyKrakendConfigDiscovery(c *assert.CollectT)
105112
// config came from the configuration-discovery path (via the Docker
106113
// listener), not a plain file provider.
107114
s.verifyKrakendCheckProvider(c)
115+
116+
// Verify the metric actually submitted by the discovered krakend check
117+
// carries the configuration-discovery marker tag, not just the resolved
118+
// config (checked above via configcheck).
119+
s.verifyKrakendMetricHasConfigDiscoveryTag(c)
120+
}
121+
122+
// verifyKrakendMetricHasConfigDiscoveryTag checks, via fakeintake, that a
123+
// metric submitted by the discovered krakend check (krakend.api.go.goroutines,
124+
// from the fake container's go_goroutines gauge) carries the
125+
// configDiscoveryTag marker tag end to end, not just in the resolved config.
126+
func (s *configDiscoverySuite) verifyKrakendMetricHasConfigDiscoveryTag(c *assert.CollectT) {
127+
const metricName = "krakend.api.go.goroutines"
128+
129+
metrics, err := s.Env().FakeIntake.Client().FilterMetrics(metricName,
130+
fakeintakeclient.WithTags[*aggregator.MetricSeries]([]string{configDiscoveryTag}))
131+
if !assert.NoError(c, err, "failed to query fakeintake for %s", metricName) {
132+
return
133+
}
134+
assert.NotEmpty(c, metrics, "expected at least one %s series tagged with %s", metricName, configDiscoveryTag)
108135
}
109136

110137
// adContainerDiscoveryProvider mirrors names.ADContainerDiscovery in
@@ -115,6 +142,15 @@ func (s *configDiscoverySuite) verifyKrakendConfigDiscovery(c *assert.CollectT)
115142
// (e.g. containers, discovered via the Docker listener here).
116143
const adContainerDiscoveryProvider = "ad-container-discovery+file"
117144

145+
// configDiscoveryTag mirrors configDiscoveryTag in
146+
// comp/core/autodiscovery/impl/configmgr_discovery.go (not importable here:
147+
// it lives in the root module, which test/new-e2e does not depend on). It is
148+
// the marker tag configuration discovery adds to every instance it
149+
// schedules, so users can identify (and, if needed, exclude) metrics
150+
// submitted by an autodiscovered check that duplicates a manually-configured
151+
// one pointed at the same service from elsewhere.
152+
const configDiscoveryTag = "dd_config_discovery:true"
153+
118154
// verifyKrakendCheckProvider checks that the krakend check has
119155
// config.provider = adContainerDiscoveryProvider in the inventory-checks
120156
// metadata, confirming it was resolved via configuration discovery against

0 commit comments

Comments
 (0)