refactor(config): Migrate agentURL and traceProtocol by mtoffl01 · Pull Request #4550 · DataDog/dd-trace-go

mtoffl01 · 2026-03-17T00:49:59Z

What does this PR do?

Migrates resolution logic for agentURL and traceProtocol configurations from tracer/option.go into internal/config.

Motivation

Part of config revamp migration.

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
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!

datadog-datadog-prod-us1 · 2026-03-17T00:52:59Z

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
• Patch Coverage: 61.05%
• Overall Coverage: 59.39% (+3.60%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 19c01e7 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!}

codecov · 2026-03-17T00:54:30Z

Codecov Report

❌ Patch coverage is 60.37736% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.14%. Comparing base (e0560ac) to head (19c01e7).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/config/config_helpers.go	43.24%	17 Missing and 4 partials ⚠️
internal/config/config.go	50.00%	16 Missing ⚠️
internal/config/provider/provider.go	0.00%	5 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
ddtrace/tracer/civisibility_transport.go	`60.00% <100.00%> (ø)`
ddtrace/tracer/log.go	`86.81% <100.00%> (ø)`
ddtrace/tracer/option.go	`83.87% <100.00%> (ø)`
ddtrace/tracer/telemetry.go	`89.15% <100.00%> (ø)`
ddtrace/tracer/tracer.go	`88.62% <100.00%> (ø)`
ddtrace/tracer/writer.go	`91.70% <100.00%> (ø)`
internal/config/provider/provider.go	`75.29% <0.00%> (ø)`
internal/config/config.go	`58.11% <50.00%> (+58.11%)`	⬆️
internal/config/config_helpers.go	`29.09% <43.24%> (ø)`

... and 428 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.

pr-commenter · 2026-03-17T01:10:51Z

Benchmarks

Benchmark execution time: 2026-03-18 15:12:46

Comparing candidate commit 19c01e7 in PR branch mtoff/agentURL with baseline commit e0560ac in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 155 metrics, 9 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

kakkoyun

Looking good. Except this one condition.

I have also several nitpicks. However, it was late for me when I did this review. I'll have another look in the morning.

kakkoyun · 2026-03-17T19:57:23Z

 	c.agent.store(af)
-	if af.v1ProtocolAvailable {
-		c.traceProtocol = traceProtocolV1
+	if !af.v1ProtocolAvailable && c.internalConfig.TraceProtocol() == traceProtocolV1 {


This condition is a little confusing. And maybe not correct.
Could this be?

Suggested change

if !af.v1ProtocolAvailable && c.internalConfig.TraceProtocol() == traceProtocolV1 {

if !af.v1ProtocolAvailable or c.internalConfig.TraceProtocol() == traceProtocolV04 {

The condition is correct as written.

Basically:
The resolution logic for user-configured trace protocol now happens exclusively in internal/config. It does not consider agentFeatures, only the user-supplied DD_TRACE_AGENT_PROTOCOL_VERSION or the default fallback (traceProtocolV04):

Is DD_TRACE_AGENT_PROTOCOL_VERSION set to a valid value? If so, use it.

Else, fallback to traceProtocolV04

So now, on line 464 we got information about agentfeatures; in the case that a user has said "I want trace protocol v1" but agentfeatures shows that v1 is not available, we force the configuration back to traceProtocolV04.

This is aligned with the old logic, it's just that the old logic did it this way:

the "v1ProtocolAvailable" flag on agentFeatures was determined by BOTH /v1/info available on agent AND user-configured DD_TRACE_AGENT_PROTOCOL_VERSION=1.0 flag

therefore, at line 464 of option.go, we were previously only checking af.v1ProtocolAvailable and setting accoridngly; else falling back to v04.

Thanks for the explanation. I was a little tired when I did the review.

That being said, I think we should add a comment here.

kakkoyun · 2026-03-17T20:01:22Z

 }
+
+func validateTraceProtocolVersion(v string) bool {
+	return v == "0.4" || v == "1.0"


nit: We can use some constants here.

The constants for these values are currently defined in the tracer package. I originally moved them over here, into config, as public constants then realized that they are most used by tracer. So this looked like lots of references to internalconfig.TraceProtocolV1 instead of just TraceProtocolV1 across the codebase.

My options are:

re-impliment that

keep things as is

import the tracer constants in config

define a second set of constants in either one of the packages

Any preferences?

I think we should strike for only having a single source of truth for these. For whatever reason if we decide to change these values, we might forget about them in here. So I'm good with anything that would keep them in sync.

kakkoyun · 2026-03-17T20:08:50Z

+			log.Warn("Failed to parse DD_TRACE_AGENT_URL: %s", err.Error())
+		} else {
+			switch u.Scheme {
+			case "unix", "http", "https":


nit: we can use constants here. Especially give we have some repetitions.

kakkoyun · 2026-03-17T20:11:25Z

+	return &u
+}
+
+func (c *Config) SetAgentURL(u *url.URL, origin telemetry.Origin) {


nit: We can consider making the second argument optional here. And make telemetry.OriginCode as default. Most (if not all) of the call-site uses that as argument.

Suggested change

func (c *Config) SetAgentURL(u *url.URL, origin telemetry.Origin) {

func (c *Config) SetAgentURL(u *url.URL, origin ...telemetry.Origin) {

This is a fabulous suggestion 🙇

I originally loved this idea because I dislike having to add a "fake" origin in tests when we don't really care about setting the origin.

But as I went to implement this, I realized making origin variadic was even more displeasing to me... 🤔

First of all, it invites providing multiple values for origin, which is a confusing API, and confusing when the function logic has to "choose" a value from the origin list to use as its source of truth (e.g, origin[0]).

An alternative is to provide secondary APIs like SetAgentURLDefault(u *url.URL) which hardcodes the OriginDefeault.

Or, to leave things as they are.

WDYT?

Yes, unfortunately this is the only optional argument pattern we have in Go. We can explicitly document the behavior here. It's not completely idiomatic but it's very common to use. It's just ergonomics.

That being said, I'm also okay to have methods like SetAgentURLDefault(u *url.URL), I just want to make sure we reduce the possibility of making mistakes here.

I'm leaving the decision to you.

Upon review, I don't see any cal sites that actually need "SetAgentURLDefault." However, in general the Config type would benefit from additional methods like SetXDefault that don't require an origin to be passed in. Because it doesn't directly impact this PR, I'll consider this for a follow-up PR.

Co-authored-by: Kemal Akkoyun <[email protected]>

kakkoyun

LGTM. We only have nits.

kakkoyun · 2026-03-18T11:07:22Z

 	c.agent.store(af)
-	if af.v1ProtocolAvailable {
-		c.traceProtocol = traceProtocolV1
+	if !af.v1ProtocolAvailable && c.internalConfig.TraceProtocol() == traceProtocolV1 {


Thanks for the explanation. I was a little tired when I did the review.

That being said, I think we should add a comment here.

bm1549

Approving on behalf of Capabilities since Ben is out

…`traceProtocol` fields (#4565)  ### What does this PR do? Fixes outdated references to `agentURL` and `traceProtocol` fields on the tracer's `config` struct; these fields were removed from the struct in #4550 and replaced by internalConfig access. ### Motivation Fixes bug on main -- [Monitor Alert](https://app.datadoghq.com/monitors/255603167?q=%40git.repository.id_v2%3Agithub.com%2Fdatadog%2Fdd-trace-go&event_id=AwAAAZ0HFd6YboDOBQAAABhBWjBIRjBWMUFBQlNNTHh6bWp4QUVJWTMAAAAkMDE5ZDA3MWMtMGNhMC00MGZiLTgyMTItNTJhOWMwYWVlYzZkAABXMg&link_event_id=8550971062156716057&link_monitor_id=255603167&link_source=monitor_notif&from_ts=1773939467000&to_ts=1773940667000&live=false). ### 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!

mtoffl01 added 2 commits March 16, 2026 16:02

move agentURL over

9fe1960

migrate agentURL

b518115

rename agentURL/rawAgentURL

ede02a2

mtoffl01 added 2 commits March 16, 2026 21:27

remove calls to internal/AgentURLFromEnv in transport_test

2888799

migrate traceProtocol

2b1674d

mtoffl01 changed the title ~~refactor(config): Migrate agentURL~~ refactor(config): Migrate agentURL and traceProtocol Mar 17, 2026

mtoffl01 added 4 commits March 16, 2026 22:18

add newline to config_helpers to appese linter

f4e153c

address merge conflicts

0915ef6

update old reference to t.config.agentURL

361b04b

fix naming of agentURL vs rawAgentURL

5d47440

mtoffl01 marked this pull request as ready for review March 17, 2026 19:19

mtoffl01 requested review from a team as code owners March 17, 2026 19:19

mtoffl01 requested review from genesor and kakkoyun March 17, 2026 19:19

kakkoyun reviewed Mar 17, 2026

View reviewed changes

kakkoyun requested changes Mar 17, 2026

View reviewed changes

mtoffl01 and others added 4 commits March 17, 2026 16:35

Fix transport_test tests to avoid timeouts

ba38ccf

Update ddtrace/tracer/option_test.go

babe3c0

Co-authored-by: Kemal Akkoyun <[email protected]>

refactor resolveAgentURL for clarity

4e6b799

Fix 'code-full-UDS' test

d57eddc

kakkoyun approved these changes Mar 18, 2026

View reviewed changes

Address github review comments

19c01e7

bm1549 approved these changes Mar 19, 2026

View reviewed changes

rachelyangdog approved these changes Mar 19, 2026

View reviewed changes

mtoffl01 merged commit 0480a77 into main Mar 19, 2026
114 checks passed

mtoffl01 deleted the mtoff/agentURL branch March 19, 2026 16:59

mtoffl01 mentioned this pull request Mar 19, 2026

fix(tracer): Fix outdated references to tracer.config agentURL and traceProtocol fields #4565

Merged

9 tasks

funyjane mentioned this pull request May 8, 2026

refactor(config): migrate dogstatsdAddr #4741

Closed

9 tasks

	if !af.v1ProtocolAvailable && c.internalConfig.TraceProtocol() == traceProtocolV1 {
	if !af.v1ProtocolAvailable or c.internalConfig.TraceProtocol() == traceProtocolV04 {

	func (c Config) SetAgentURL(u url.URL, origin telemetry.Origin) {
	func (c Config) SetAgentURL(u url.URL, origin ...telemetry.Origin) {

Uh oh!

Conversation

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

Uh oh!

What does this PR do?

Motivation

Reviewer's Checklist

Uh oh!

datadog-datadog-prod-us1 Bot commented Mar 17, 2026 • edited by datadog-official Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

pr-commenter Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Explanation

More details about the CI and significant changes

Uh oh!

kakkoyun 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!

Choose a reason for hiding this comment

Uh oh!

mtoffl01 Mar 17, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtoffl01 Mar 17, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

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!

kakkoyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bm1549 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtoffl01 commented Mar 17, 2026 •

edited

Loading

datadog-datadog-prod-us1 Bot commented Mar 17, 2026 •

edited by datadog-official Bot

Loading

codecov Bot commented Mar 17, 2026 •

edited

Loading

pr-commenter Bot commented Mar 17, 2026 •

edited

Loading

kakkoyun left a comment •

edited

Loading

mtoffl01 Mar 17, 2026 •

edited

Loading

mtoffl01 Mar 17, 2026 •

edited

Loading