Skip to content

Commit f9776f2

Browse files
authored
refactor(config): add cross-product gate to prevent conflicting programmatic overrides (#4640)
<!-- * New contributors are highly encouraged to read our [CONTRIBUTING](/CONTRIBUTING.md) documentation. * Commit and PR titles should be prefixed with the general area of the pull request's change. --> ### What does this PR do? Adds a cross-product gate to internal/config that prevents different products (tracer, profiler, etc.) from setting conflicting values for the same configuration field via programmatic APIs. When a conflict is attempted, a new telemetry metric `config.product_conflict` is emitted. Every `Set*` method now accepts an optional `...Product` parameter. When a product passes its identity (e.g. `ProductTracer`), the gate records that claim. If a different product later tries to set the same field, the call is rejected (first-in-wins) and a warning is logged. Env vars, defaults, and non-programmatic origins bypass the gate entirely. Tests and integrations omit the product parameter and are unaffected. ### Motivation Previously, each product (tracer, profiler, etc) loaded configuration independently from all sources and stored its own local copy. Programmatic APIs like `tracer.WithService` or `profiler.WithEnv` would overwrite only that product's local copy. As we consolidate into a single shared Config instance in internal/config, programmatic APIs from different products now write to the same fields. The gate ensures these writes don't silently conflict: the first product to claim a field via programmatic API owns it, and attempts from other products are rejected with a warning. ### Reviewer's Checklist <!-- * Authors can use this list as a reference to ensure that there are no problems during the review but the signing off is to be done by the reviewer(s). --> - [ ] Changed code has unit tests for its functionality at or near 100% coverage. - [ ] [System-Tests](https://github.com/DataDog/system-tests/) covering this feature have been added and enabled with the va.b.c-dev version tag. - [ ] There is a benchmark for any new code, or changes to existing code. - [ ] If this interacts with the agent in a new way, a system test has been added. - [ ] New code is free of linting errors. You can check this by running `make lint` locally. - [ ] New code doesn't break existing tests. You can check this by running `make test` locally. - [ ] Add an appropriate team label so this PR gets put in the right place for the release notes. - [ ] All generated files are up to date. You can check this by running `make generate` locally. - [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild. Make sure all nested modules are up to date by running `make fix-modules` locally. Unsure? Have a question? Request a review!
1 parent 0029919 commit f9776f2

File tree

4 files changed

+373
-61
lines changed

4 files changed

+373
-61
lines changed

ddtrace/tracer/option.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -376,27 +376,27 @@ func newConfig(opts ...StartOption) (*config, error) {
376376
if c.internalConfig.Env() == "" {
377377
if v, ok := globalTags["env"]; ok {
378378
if e, ok := v.(string); ok {
379-
c.internalConfig.SetEnv(e, c.globalTags.Origin())
379+
c.internalConfig.SetEnv(e, c.globalTags.Origin(), internalconfig.ProductTracer)
380380
}
381381
}
382382
}
383383
if c.internalConfig.Version() == "" {
384384
if v, ok := globalTags["version"]; ok {
385385
if ver, ok := v.(string); ok {
386-
c.internalConfig.SetVersion(ver, c.globalTags.Origin())
386+
c.internalConfig.SetVersion(ver, c.globalTags.Origin(), internalconfig.ProductTracer)
387387
}
388388
}
389389
}
390390
if c.internalConfig.ServiceName() == "" {
391391
if v, ok := globalTags["service"]; ok {
392392
if s, ok := v.(string); ok {
393-
c.internalConfig.SetServiceName(s, c.globalTags.Origin())
393+
c.internalConfig.SetServiceName(s, c.globalTags.Origin(), internalconfig.ProductTracer)
394394
globalconfig.SetServiceName(s)
395395
}
396396
} else {
397397
// There is not an explicit service set, default to binary name.
398398
// In this case, don't set a global service name so the contribs continue using their defaults.
399-
c.internalConfig.SetServiceName(filepath.Base(os.Args[0]), internalconfig.OriginDefault)
399+
c.internalConfig.SetServiceName(filepath.Base(os.Args[0]), internalconfig.OriginDefault, internalconfig.ProductTracer)
400400
}
401401
} else {
402402
globalconfig.SetServiceName(c.internalConfig.ServiceName())
@@ -960,7 +960,7 @@ func WithPropagator(p Propagator) StartOption {
960960
// WithService sets the default service name for the program.
961961
func WithService(name string) StartOption {
962962
return func(c *config) {
963-
c.internalConfig.SetServiceName(name, internalconfig.OriginCode)
963+
c.internalConfig.SetServiceName(name, internalconfig.OriginCode, internalconfig.ProductTracer)
964964
globalconfig.SetServiceName(name)
965965
}
966966
}
@@ -1031,7 +1031,7 @@ func WithAgentTimeout(timeout int) StartOption {
10311031
// The default value is the environment variable DD_ENV, if it is set.
10321032
func WithEnv(env string) StartOption {
10331033
return func(c *config) {
1034-
c.internalConfig.SetEnv(env, telemetry.OriginCode)
1034+
c.internalConfig.SetEnv(env, telemetry.OriginCode, internalconfig.ProductTracer)
10351035
}
10361036
}
10371037

@@ -1182,7 +1182,7 @@ func WithSamplingRules(rules []SamplingRule) StartOption {
11821182
// span service name and config service name match. Do NOT use with WithUniversalVersion.
11831183
func WithServiceVersion(version string) StartOption {
11841184
return func(cfg *config) {
1185-
cfg.internalConfig.SetVersion(version, telemetry.OriginCode)
1185+
cfg.internalConfig.SetVersion(version, telemetry.OriginCode, internalconfig.ProductTracer)
11861186
cfg.universalVersion = false
11871187
}
11881188
}
@@ -1192,7 +1192,7 @@ func WithServiceVersion(version string) StartOption {
11921192
// See: WithService, WithServiceVersion. Do NOT use with WithServiceVersion.
11931193
func WithUniversalVersion(version string) StartOption {
11941194
return func(c *config) {
1195-
c.internalConfig.SetVersion(version, telemetry.OriginCode)
1195+
c.internalConfig.SetVersion(version, telemetry.OriginCode, internalconfig.ProductTracer)
11961196
c.universalVersion = true
11971197
}
11981198
}

internal/config/README.md

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,29 @@ When migrating a configuration value from another package (e.g. `ddtrace/tracer`
99
- **Define the field on `Config`**: add a private field on `internal/config.Config`.
1010
- **Initialize it in `loadConfig()`**: read from the config provider, which iterates over the following sources, in order, returning the default if no valid value found: local declarative config file, OTEL env vars, env vars, managed declarative config file
1111
- **Expose an accessor**: add a getter (and a setter if the value is updated at runtime).
12-
- **Report telemetry in setters**: setters should call `reportTelemetry(...)` with the correct origin.
12+
- **Report telemetry in setters**: setters should call `configtelemetry.Report(...)` with the correct origin.
13+
- **Add the cross-product gate**: every setter must call `c.checkProductConflict(...)` as its first action after acquiring the lock (see below).
1314
- **Update callers**: replace reads/writes on local "config" structs with calls to the singleton (`internal/config.Get()`).
1415
- **Delete old state**: remove the migrated field from any legacy config structs once no longer referenced.
1516
- **Update tests**: tests should call the singleton setter/getter (or set env vars) rather than mutating legacy fields.
1617

1718
Sample migration PR: https://github.com/DataDog/dd-trace-go/pull/4214
1819

20+
## Cross-product gate
21+
22+
Every `Set*` method accepts an optional trailing `...Product` parameter. When a product (tracer, profiler, etc.) sets a field via its programmatic API, it passes its `Product` identity:
23+
24+
```go
25+
c.internalConfig.SetServiceName(name, internalconfig.OriginCode, internalconfig.ProductTracer)
26+
```
27+
28+
The gate enforces **first-in-wins**: if a different product already claimed the field via programmatic API, the call is silently rejected and a warning is logged. This prevents conflicting overrides like `tracer.WithService("A")` + `profiler.WithService("B")`.
29+
30+
Key rules:
31+
- **Env vars, defaults, and RC always pass through** — the gate only activates for `OriginCode`.
32+
- **Tests and integrations omit the product** — they call `SetServiceName(name, origin)` without a product, bypassing the gate entirely.
33+
- **Same product can call a setter multiple times** — repeated calls from the same product just update the value.
34+
1935
## Hot paths & performance guidelines
2036

2137
Some configuration accessors may be called in hot paths (e.g., span start/finish, partial flush logic).

0 commit comments

Comments
 (0)