refactor(config): migrate peer service config to internal/config by funyjane · Pull Request #4483 · DataDog/dd-trace-go

funyjane · 2026-03-02T09:12:30Z

Summary

Add getter/setter methods to internal/config.Config for peerServiceDefaultsEnabled and peerServiceMappings (fields already existed but lacked accessors)
Remove duplicate peerServiceDefaultsEnabled and peerServiceMappings fields from the tracer's local config struct
Wire tracer, telemetry, and tests to use the global config singleton accessors

Test plan

go test ./ddtrace/tracer/... -run "TestTracerOptionsDefaults/peer-service" — config parsing, env vars, programmatic options
go test ./ddtrace/tracer/... -run "TestSpanPeerService" — span tag calculation, mappings, defaults (17 test cases)
go build ./internal/config/... ./ddtrace/tracer/... — compiles cleanly

Move peerServiceDefaultsEnabled and peerServiceMappings from the tracer's local config struct to internal/config.Config. These fields already existed in the global config singleton but lacked getter/setter methods. The tracer now reads/writes peer service configuration through the global config accessors instead of maintaining local copies.

genesor

Nice work! Some modifications are needed to really finish migration into the new struct

ddtrace/tracer/option.go

darccio

LGTM

mtoffl01

fyi: #4487

There are specific optimizations for map fields (and beyond)

ddtrace/tracer/option.go

…e defaults - Fix gofmt: remove extra blank line left from field removal - Keep schema-dependent peerServiceDefaultsEnabled logic in the tracer since spanAttributeSchemaVersion uses namingschema (understands "v1") while loadConfig uses getInt (can't parse "v1"). Add TODO to move once spanAttributeSchemaVersion is fully migrated.

SetPeerServiceMappings and SetPeerServiceMapping have non-standard signatures (map param / three value params) that the reflective test helper cannot auto-generate values for. Register them as special cases.

Address PR review comments: move the peer service defaults enabled logic (schema-dependent calculation) from tracer's newConfig() to internal config's loadConfig(). Add parseSpanAttributeSchema parser that handles "v0"/"v1" string values (matching namingschema behavior), replacing the previous getInt call that couldn't parse these formats.

mtoffl01 · 2026-03-03T15:37:26Z

internal/config/config.go

 	cfg.profilerEndpoints = provider.getBool("DD_PROFILING_ENDPOINT_COLLECTION_ENABLED", true)
-	cfg.spanAttributeSchemaVersion = provider.getInt("DD_TRACE_SPAN_ATTRIBUTE_SCHEMA", 0)
-	cfg.peerServiceDefaultsEnabled = provider.getBool("DD_TRACE_PEER_SERVICE_DEFAULTS_ENABLED", false)
+	cfg.spanAttributeSchemaVersion = get(provider, "DD_TRACE_SPAN_ATTRIBUTE_SCHEMA", 0, parseSpanAttributeSchema)


get is a nested helper function and should not be called directly outside of configprovider. Instead, you can create a new getBoolWithValidator function (see globalSampleRate and getFloatWithValidator for example).

Changed it to using provider.getString to read the raw value, then parsing it with parseSpanAttributeSchema in the derived evaluations section below. Looked it up on featureFlags and logToStdout.

mtoffl01 · 2026-03-03T15:38:19Z

internal/config/config.go

+	// peer.service tag default calculation is enabled by default if using attribute schema >= 1.
+	// When schema >= v1 the env var default is overridden to true.


These lines express the same thing; use only one or the other.

mtoffl01 · 2026-03-03T15:39:49Z

internal/config/config.go

+	if cfg.spanAttributeSchemaVersion >= 1 {
+		cfg.peerServiceDefaultsEnabled = true
+	} else {
+		cfg.peerServiceDefaultsEnabled = provider.getBool("DD_TRACE_PEER_SERVICE_DEFAULTS_ENABLED", false)
+	}


The status quo right now is to keep all the evaluations that are provider.getX based together and, down below, perform the evaluations that rely on other configs.

That is -- move this to the line 158+ section.

Moved the schema-dependent override to the derived evaluations section below, after the feature flags parsing. All provider.getX calls are now grouped together.

mtoffl01 · 2026-03-03T17:22:27Z

ddtrace/tracer/tracer.go

-		PeerServiceDefaults:  t.config.peerServiceDefaultsEnabled,
-		PeerServiceMappings:  t.config.peerServiceMappings,
+		PeerServiceDefaults:  t.config.internalConfig.PeerServiceDefaultsEnabled(),
+		PeerServiceMappings:  t.config.internalConfig.PeerServiceMappings(),


This is a bit of a problem.

TracerConf() is called inside of setPeerService, which is invoked for every single span we create. This means -- if t.config.internalConfig.PeerServiceMappings() is not empty -- that we are acquiring the lock and iterating over and copying internalconfig's PeerServiceMappings map on every single span, just to ultimately query the map by a key value. This creates lock contention and unnecessary memory allocation.

I don't have a solution off the top of my head, but perhaps you can run this^ concern, along with the guidelines of the new readme, through cursor/whatever AI dev tool you may be using and have it help you come up with an alternate solution. Perhaps function signatures may have to change.

This one is slightly more complicated, I replaced PeerServiceMappings map[string]string in TracerConf with PeerServiceMapping func(string) (string, bool). Do you think a single-key lookup that avoids copying the map and acquiring the lock on every span is a good solution here? As you can see from these changes setPeerService now calls this function directly.

mtoffl01 · 2026-03-03T18:32:43Z

internal/config/config.go

+// PeerServiceMapping performs a single mapping lookup without copying the underlying map.
+// This is better than PeerServiceMappings() for hot paths to avoid per-call allocations.
+func (c *Config) PeerServiceMapping(from string) (to string, ok bool) {
+	c.mu.RLock()
+	m := c.peerServiceMappings
+	if m == nil {
+		c.mu.RUnlock()
+		return "", false
+	}
+	to, ok = m[from]
+	c.mu.RUnlock()
+	return to, ok
+}


No need to introduce APIs that are as of yet unused. So, depending on the outcome of this, you can potentially delete this, as well as SetPeerServiceMapping.

…okup - Add getIntWithParser to configprovider instead of calling get() directly - Keep all provider.getX calls together in loadConfig; move the schema-dependent peer service defaults override to the derived evaluations section below - Replace PeerServiceMappings map in TracerConf with a PeerServiceMapping func for single-key lookups, avoiding lock contention and per-call map copies on the hot path (setPeerService is called for every span) - Update setPeerService to use the function-based lookup with nil guard - Fix CiVisibility TracerConf test to compare fields individually since functions cannot be compared with reflect.DeepEqual

…al mutation Made-with: Cursor

Follow the existing configprovider pattern: use getString to read the raw DD_TRACE_SPAN_ATTRIBUTE_SCHEMA value and parse it in the derived evaluations section with parseSpanAttributeSchema, matching how featureFlags and logToStdout are handled. Remove the getIntWithParser method that was just a passthrough to get().

codecov · 2026-03-04T14:37:08Z

Codecov Report

❌ Patch coverage is 49.29577% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.93%. Comparing base (87126fd) to head (55bf4cf).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/config/config.go	47.05%	25 Missing and 2 partials ⚠️
internal/config/config_helpers.go	0.00%	9 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
ddtrace/tracer/option.go	`83.36% <100.00%> (-2.96%)`	⬇️
ddtrace/tracer/spancontext.go	`88.47% <100.00%> (-1.83%)`	⬇️
ddtrace/tracer/telemetry.go	`89.02% <100.00%> (-3.66%)`	⬇️
ddtrace/tracer/tracer.go	`87.64% <100.00%> (-3.79%)`	⬇️
internal/config/config_helpers.go	`0.00% <0.00%> (ø)`
internal/config/config.go	`57.39% <47.05%> (+0.17%)`	⬆️

... and 263 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mtoffl01

Background
The goal is for internal/config to become the single source of truth for configuration values.

That means, our end state will not capture and store locally any configuration values (unless there is a specific reason for capturing a "snapshot" of config at a certain time, but that's not the case here).

TracerConf is a bandaid for getting access to configuration values that were previously only available inside the tracer.config object. Now that we have global access to configuration values via internal/config, it is no longer necessary.

Recommendation
So I would recommend deleting PeerServiceMapping from TracerConf entirely, changing the function signature of setPeerService to just setPeerService(s *Span), call the config singleton inside of setPeerService and use its peer service value:

	if to, ok := internalconfig.Get().PeerServiceMapping(ps); ok {
		s.setMetaLocked(keyPeerServiceRemappedFrom, ps)
		s.setMetaLocked(ext.PeerService, to)
	}

Concerns
The thing is -- this approach presents lots of potential for lock contention. We will be acquiring the lock in internalconfig.Get() in a very hot path (setPeerService is called for every span). I'd suggest you make this change and observe any issues in existing tests or benchmarks. We may need to replace the internalconfig.Get() logic to not use a lock.

cc @genesor

…r own packages (#4494)  ### What does this PR do? Migrates `configprovider` and `telemetry` logic in the `internal/config` package into their own respective packages. "Internalize" logic that is just an implementation detail. ### Motivation Inspired by early commits in: #4483 The API contract for the configprovider and the telemetry logic was not clear. I migrated these to their own packages to make their API usage clearer. ### Reviewer's Checklist  - [ ] 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! Co-authored-by: kemal.akkoyun <[email protected]>

genesor · 2026-03-10T10:28:23Z

That means, our end state will not capture and store locally any configuration values (unless there is a specific reason for capturing a "snapshot" of config at a certain time, but that's not the case here).

TracerConf is a bandaid for getting access to configuration values that were previously only available inside the tracer.config object. Now that we have global access to configuration values via internal/config, it is no longer necessary.

Recommendation So I would recommend deleting PeerServiceMapping from TracerConf entirely, changing the function signature of setPeerService to just setPeerService(s *Span), call the config singleton inside of setPeerService and use its peer service value:

Concerns The thing is -- this approach presents lots of potential for lock contention. We will be acquiring the lock in internalconfig.Get() in a very hot path (setPeerService is called for every span). I'd suggest you make this change and observe any issues in existing tests or benchmarks. We may need to replace the internalconfig.Get() logic to not use a lock.

I agree with you @mtoffl01, we should look into removing the band-aid in the future but currently TracerConf is already depending on multiple calls to .internalConfig.

My 2cts on this topic would be to keep the migration simple and tackle the TracerConf later on in another PR.

funyjane requested review from a team as code owners March 2, 2026 09:12

genesor reviewed Mar 2, 2026

View reviewed changes

ddtrace/tracer/option.go Outdated Show resolved Hide resolved

ddtrace/tracer/option.go Outdated Show resolved Hide resolved

darccio approved these changes Mar 2, 2026

View reviewed changes

mtoffl01 requested changes Mar 2, 2026

View reviewed changes

ddtrace/tracer/option.go Outdated Show resolved Hide resolved

ddtrace/tracer/option.go Outdated Show resolved Hide resolved

funyjane and others added 4 commits March 3, 2026 00:22

Merge branch 'main' into config-revamp/peer-service

f11ae64

fix: add peer service map setters to specialCaseSetters in test

4712800

SetPeerServiceMappings and SetPeerServiceMapping have non-standard signatures (map param / three value params) that the reflective test helper cannot auto-generate values for. Register them as special cases.

funyjane requested review from genesor and mtoffl01 March 3, 2026 11:19

mtoffl01 requested changes Mar 3, 2026

View reviewed changes

funyjane added 3 commits March 4, 2026 14:54

fix: defensively copy map in SetPeerServiceMappings to prevent extern…

49922cf

…al mutation Made-with: Cursor

funyjane requested a review from mtoffl01 March 4, 2026 14:56

fix: regenerate api.txt with apiextractor tool

55bf4cf

mtoffl01 mentioned this pull request Mar 9, 2026

refactor(internal/config): put configprovider and telemetry into their own packages #4494

Merged

9 tasks

mtoffl01 requested changes Mar 9, 2026

View reviewed changes

		// peer.service tag default calculation is enabled by default if using attribute schema >= 1.
		// When schema >= v1 the env var default is overridden to true.

Conversation

funyjane commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

genesor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

darccio left a comment

Choose a reason for hiding this comment

Uh oh!

mtoffl01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mtoffl01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genesor commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

funyjane commented Mar 2, 2026 •

edited

Loading

codecov bot commented Mar 4, 2026 •

edited

Loading

mtoffl01 left a comment •

edited

Loading