fix(ddtrace/tracer): decouple trace protocol v1.0 from client-side stats - #5122
Closed
darccio wants to merge 6 commits into
Closed
fix(ddtrace/tracer): decouple trace protocol v1.0 from client-side stats#5122darccio wants to merge 6 commits into
darccio wants to merge 6 commits into
Conversation
Contributor
Config Audit |
Member
Author
|
@codex review |
This comment has been minimized.
This comment has been minimized.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 487f15c88e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BenchmarksBenchmark execution time: 2026-08-07 17:17:14 Comparing candidate commit 5e7ee42 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 326 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.
|
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
darccio
added
performance/ignore-performance-regression
AI Generated
darccio
force-pushed
the
dario.castane/nostalgic-fermat-150b6e
branch
from
August 4, 2026 14:34
f0aea3e to
a23bd62
Compare
darccio
added a commit
to DataDog/system-tests
that referenced
this pull request
Aug 4, 2026
The previous commit covers CSS with the v1 protocol, but that pairing worked even while the two were coupled, so it cannot detect the coupling. The discriminating case is the mirror: v1 pinned while CSS is off. Released dd-trace-go gates v1 on CSS capability and silently downgrades to /v0.4/traces the moment stats computation is disabled. CSS is negotiated entirely out of band -- a Datadog-Client-Computed-Stats header and a separate /v0.6/stats endpoint, both handled identically by the Agent on either protocol -- so disabling it has no bearing on the trace wire format. DataDog/dd-trace-go#5122 removes the gate. Add APM_TRACING_EFFICIENT_PAYLOAD_STATS_DISABLED, which is APM_TRACING_EFFICIENT_PAYLOAD plus DD_TRACE_STATS_COMPUTATION_ENABLED =false, and Test_V1PayloadWithStatsDisabled asserting every trace still goes to /v1.0/traces in v1 format. The test first asserts no /v0.6/stats payloads exist, so it fails loudly rather than passing vacuously if CSS were somehow active. Declared missing_feature for golang until #5122 ships, and for nodejs and ruby to match every other class in this file (neither has working v1). Java is deliberately left inheriting the file-level >=1.62.0: it supports v1 when the protocol is pinned and has no CSS coupling, so it is the language that proves the test is meaningful rather than vacuous. Co-Authored-By: Claude <noreply@anthropic.com>
Draft
5 tasks
darccio
marked this pull request as ready for review
August 4, 2026 15:04
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a23bd62442
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kakkoyun
reviewed
Aug 5, 2026
kakkoyun
left a comment
kakkoyun
left a comment
Member
There was a problem hiding this comment.
This is a lot of code changes and some of them are refactoring the code. It would have been nicer to isolate those into their own commits.
| // traceProtocol is the Datadog trace protocol version the user requested | ||
| // (TraceProtocolV04 or TraceProtocolV1). This is independent of whether the | ||
| // trace-agent actually supports it — see RequestedTraceProtocol's doc. | ||
| traceProtocol float64 |
Member
There was a problem hiding this comment.
We should have had a dedicated type for protocol. If we don't have a lot of conversion points. We support limited numbers here.
Member
Author
There was a problem hiding this comment.
Let me address this in a follow up.
darccio
added a commit
that referenced
this pull request
Aug 5, 2026
refreshAgentFeatures treated a 404 on /info the same as any other fetch error: keep the last-known-good snapshot untouched. But an agent that returns 404 on /info doesn't support /info at all, and /v1.0/traces support postdates /info support, so that response IS positive evidence v1 is gone. Retaining v1ProtocolAvailable=true in that case meant a v1-capable agent rolled back to a pre-/info version left the tracer posting to /v1.0/traces indefinitely, with the agent rejecting every payload. errAgentFeaturesNotSupported now resets the v1 upgrade streak and clears v1ProtocolAvailable immediately, matching the existing downgrade-is-immediate hysteresis, while every other dynamic field stays at its last-known-good value since a 404 carries no fresh snapshot to refresh them from. A generic fetch error (network failure, timeout, decode failure) still returns early and leaves everything untouched, since that's never evidence v1 became unavailable. Addresses a Codex review comment on #5122: #5122 (comment)
Member
Author
Agreed. I'm juggling this with all the code reviews and dropped the bar 😓 |
…gurable
The mock agent always advertised exactly the patterns registered via
HandleTraces, which left tests unable to express "the agent serves this
endpoint but does not advertise it" — the shape agent-capability
negotiation actually has to cope with.
Add SetInfoEndpoints to decouple what /info advertises from what is
registered, and use it to pin the ddtrace/tracer inspectable-tracer
harness to v0.4 on purpose. Those suites already run on v0.4, but only as
a side effect: the mock advertises no /v0.6/stats, so client-side stats
are unavailable and the tracer's protocol gate downgrades v1.0 -> v0.4.
Making the pin explicit is a no-op today and keeps the harness on a
deterministic wire format the next time that gate is touched.
Fix handleV1Traces while here. It built a bare &payloadV1{buf: body},
leaving header nil, so decodeBuffer panicked with an index-out-of-range
inside updateHeader. The handler has been registered but unreachable since
it was written, because the gate always selected v0.4 for this mock, so
the bug never surfaced. Add TestHandleV1TracesDecodesEncodedPayload, which
reproduces the panic without the fix.
Also guard the endpoints slice with the existing mutex on both sides.
HandleTraces appended to it unlocked while handleInfo read it; harmless
in practice because all registration precedes Start, but it is the kind
of thing SetInfoEndpoints invites tests to do at a less careful moment.
…d's protocol httpTransport held a single mutable traceURL that had to be kept in step with the configured protocol. Keeping them in step was the caller's job: newConfig patched traceURL by string-comparing it against the v1 path whenever it downgraded the protocol, which only worked as long as every future protocol decision remembered to do the same. Hold one immutable URL per protocol instead, computed once in newHTTPTransport, and have send() select between them from the payload's own protocol(). A payload is encoded for exactly one protocol and carries that with it, so wire format and destination can no longer diverge — not because a lock protects them, but because there is no shared mutable state left to desynchronize. newConfig's URL patch-up goes away, and resolveTraceURL with it. This also fixes reportAPIErrorsMetric, which passed a hardcoded tracesAPIPath: a failed v1.0 send was reported to datadog.tracer.api.errors tagged endpoint:/v0.4/traces. endpoint() gains a protocol argument for the startup diagnostics probe. It is documented as diagnostics-only — an actual send never consults it, so it cannot reintroduce the mismatch. No behaviour change beyond the metric tag: the protocol downgrade still happens in config, and the writer still reads it when creating a payload.
…ocol
The name read like "the protocol in use", but the value is only what was
asked for — it says nothing about whether the trace-agent can actually
serve it. Callers have to combine it with agent capability to get the
protocol on the wire, and the old name gave no hint of that, which is how
the gate in newConfig ended up being the only place that knew the
difference.
Rename to RequestedTraceProtocol and document the distinction. No
behaviour change: the OTLP-span-metrics override stays exactly as it was.
Also add TraceProtocolVersionString, the inverse of resolveTraceProtocol,
and use it in SetTraceProtocol. The env-var path reports
DD_TRACE_AGENT_PROTOCOL_VERSION through the provider as the raw string
("1.0"), while the setter reported a float64 — the same telemetry key
arrived with two different types depending on which source set it last.
The v1.0 protocol was gated on client-side stats being available
(!canComputeStats(), commented "v1 requires CSS"), but that isn't true:
- The Datadog Agent has accepted v1.0 payloads without CSS since the
format's first release; its handler computes stats server-side when
ClientComputedStats is false, exactly as it does for v0.4.
- CSS is negotiated out-of-band, via the Datadog-Client-Computed-Stats
header and the separate /v0.6/stats endpoint. The trace-send path is
otherwise protocol-blind.
- The v1 payload encoder reads no config and no agent capability state.
- The original ETP RFC never mentions stats or CSS; its only gating rule
is agent endpoint availability.
No commit, PR or doc states a rationale for the coupling — it arrived as
an unexplained one-line addition to a test-enablement PR. The silent
consequence: DD_TRACE_STATS_COMPUTATION_ENABLED=false (or disabling the
Agent's client_drop_p0s, e.g. via its probabilistic sampler) downgraded
the wire protocol with no log, even though 1.0 is the tracer's default.
Replace the startup gate with (*config).effectiveTraceProtocol, a pure
function of (requested protocol, agent v1 availability) evaluated at point
of use. Nothing is downgraded in config and nothing is baked into a
component, so the requested value stays inspectable and the effective one
can't drift from it. Config telemetry now reports the effective protocol
via ReportEffectiveTraceProtocol, which dedupes so repeated evaluation
cannot inflate seqIDs.
TestNewConfigKeepsV1WhenCSSDisabled pins this end-to-end at the wire
level: with CSS off and the agent advertising /v1.0/traces, the request
must land on /v1.0/traces with a msgpack map body. Before this change it
went to /v0.4/traces with an array body (0x91).
This was referenced Aug 5, 2026
darccio
force-pushed
the
dario.castane/nostalgic-fermat-150b6e
branch
from
August 5, 2026 15:50
b2a6b56 to
543a96c
Compare
This was referenced Aug 5, 2026
kakkoyun
approved these changes
Aug 6, 2026
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
…gurable (#5146) ## What does this PR do? First in a split of [#5122](#5122) into a reviewable stack of smaller PRs (see that PR for the full sequence and rationale). This PR is standalone against `main`. The mock agent always advertised exactly the patterns registered via `HandleTraces`, which left tests unable to express "the agent serves this endpoint but does not advertise it" — the shape agent-capability negotiation actually has to cope with. Adds `SetInfoEndpoints` to decouple what `/info` advertises from what is registered, and uses it to pin the `ddtrace/tracer` inspectable-tracer harness to v0.4 on purpose. Those suites already run on v0.4, but only as a side effect: the mock advertises no `/v0.6/stats`, so client-side stats are unavailable and the tracer's protocol gate downgrades v1.0 -> v0.4. Making the pin explicit is a no-op today and keeps the harness on a deterministic wire format the next time that gate is touched (removed later in this stack). Also fixes `handleV1Traces`, found while testing this stack: it built a bare `&payloadV1{buf: body}`, leaving `header` nil, so `decodeBuffer` panicked with an index-out-of-range inside `updateHeader`. The handler had been registered but unreachable since it was written, because the gate always selected v0.4 for this mock, so the bug never surfaced. `TestHandleV1TracesDecodesEncodedPayload` reproduces the panic without the fix. Also guards the `endpoints` slice with the existing mutex on both sides — `HandleTraces` appended to it unlocked while `handleInfo` read it. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] All generated files are up to date. You can check this by running `make generate` locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: dario.castane <dario.castane@datadoghq.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
…d's protocol (#5147) ## What does this PR do? Second in a split of [#5122](#5122) into a reviewable stack (see that PR for the full sequence). Stacked on [#5146](#5146) — this PR's diff is scoped to just the change below; merge #5146 first. `httpTransport` held a single mutable `traceURL` that had to be kept in step with the configured protocol. Keeping them in step was the caller's job: `newConfig` patched `traceURL` by string-comparing it against the v1 path whenever it downgraded the protocol, which only worked as long as every future protocol decision remembered to do the same. Holds one immutable URL per protocol instead, computed once in `newHTTPTransport`, and has `send()` select between them from the payload's own `protocol()`. A payload is encoded for exactly one protocol and carries that with it, so wire format and destination can no longer diverge — not because a lock protects them, but because there is no shared mutable state left to desynchronize. `newConfig`'s URL patch-up goes away, and `resolveTraceURL` with it. This also fixes `reportAPIErrorsMetric`, which passed a hardcoded `tracesAPIPath`: a failed v1.0 send was reported to `datadog.tracer.api.errors` tagged `endpoint:/v0.4/traces`. `endpoint()` gains a protocol argument for the startup diagnostics probe. It is documented as diagnostics-only — an actual send never consults it, so it cannot reintroduce the mismatch. No behaviour change beyond the metric tag: the protocol downgrade still happens in config, and the writer still reads it when creating a payload. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] All generated files are up to date. You can check this by running `make generate` locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: dario.castane <dario.castane@datadoghq.com>
darccio
force-pushed
the
dario.castane/tp-3-config-rename
branch
from
August 6, 2026 14:09
09ee283 to
d906b29
Compare
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 6, 2026
…ocol (#5148) ## What does this PR do? Third in a split of [#5122](#5122) into a reviewable stack (see that PR for the full sequence). Stacked on [#5147](#5147) — this PR's diff is scoped to just the change below; merge #5147 first. The name `TraceProtocol` read like "the protocol in use", but the value is only what was asked for — it says nothing about whether the trace-agent can actually serve it. Callers have to combine it with agent capability to get the protocol on the wire, and the old name gave no hint of that, which is how the gate in `newConfig` ended up being the only place that knew the difference. Renames to `RequestedTraceProtocol` and documents the distinction. No behaviour change: the OTLP-span-metrics override stays exactly as it was (removed later in this stack, in a sibling PR). Also adds `TraceProtocolVersionString`, the inverse of `resolveTraceProtocol`, and uses it in `SetTraceProtocol`. The env-var path reports `DD_TRACE_AGENT_PROTOCOL_VERSION` through the provider as the raw string (`"1.0"`), while the setter reported a `float64` — the same telemetry key arrived with two different types depending on which source set it last. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] All generated files are up to date. You can check this by running `make generate` locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: dario.castane <dario.castane@datadoghq.com>
darccio
added a commit
to DataDog/system-tests
that referenced
this pull request
Aug 7, 2026
The previous commit covers CSS with the v1 protocol, but that pairing worked even while the two were coupled, so it cannot detect the coupling. The discriminating case is the mirror: v1 pinned while CSS is off. Released dd-trace-go gates v1 on CSS capability and silently downgrades to /v0.4/traces the moment stats computation is disabled. CSS is negotiated entirely out of band -- a Datadog-Client-Computed-Stats header and a separate /v0.6/stats endpoint, both handled identically by the Agent on either protocol -- so disabling it has no bearing on the trace wire format. DataDog/dd-trace-go#5122 removes the gate. Add APM_TRACING_EFFICIENT_PAYLOAD_STATS_DISABLED, which is APM_TRACING_EFFICIENT_PAYLOAD plus DD_TRACE_STATS_COMPUTATION_ENABLED =false, and Test_V1PayloadWithStatsDisabled asserting every trace still goes to /v1.0/traces in v1 format. The test first asserts no /v0.6/stats payloads exist, so it fails loudly rather than passing vacuously if CSS were somehow active. Declared missing_feature for golang until #5122 ships, and for nodejs and ruby to match every other class in this file (neither has working v1). Java is deliberately left inheriting the file-level >=1.62.0: it supports v1 when the protocol is pinned and has no CSS coupling, so it is the language that proves the test is meaningful rather than vacuous. Co-Authored-By: Claude <noreply@anthropic.com>
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
…metry (#5141) ## What does this PR do? Part of a split of [#5122](#5122) into a reviewable stack (see that PR for the full sequence and rationale for the split). `OTEL_TRACES_SPAN_METRICS_ENABLED=false` disables native stats computation when `DD_TRACE_STATS_COMPUTATION_ENABLED` was left at its default. That was done by writing the struct field directly, which skipped `configtelemetry.Report` — so telemetry kept reporting `DD_TRACE_STATS_COMPUTATION_ENABLED` as its default (`true`) while the tracer behaved as `false`, and the discrepancy was invisible from the outside. Routes it through `SetStatsComputationEnabled` with `OriginCalculated`, which is what every other derived value in `loadConfig` does. Safe to call here: the config being built is still local, so taking its mutex cannot contend. ### Motivation Unrelated telemetry-accuracy fix that surfaced while working on the trace-protocol/CSS decoupling in #5122; split out because it's independent of that change. ### Reviewer's Checklist - [x] Changed code has unit tests for its functionality at or near 100% coverage. - [x] New code doesn't break existing tests. You can check this by running `make test` locally. - [x] New code is free of linting errors. You can check this by running `make lint` locally. - [x] All generated files are up to date. You can check this by running `make generate` locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: dario.castane <dario.castane@datadoghq.com>
darccio
added a commit
that referenced
this pull request
Aug 7, 2026
…abled TraceProtocol special-cased OTLPSpanMetricsEnabled and returned v0.4 unconditionally, on the rationale that the trace transport "must stay on v0.4 where the Datadog Agent can see the Datadog-Client-Computed-Stats header". That rationale doesn't hold: the header is a request header the Agent reads on either protocol, and OTLP span metrics are aggregated by their own concentrator and shipped over the separate /v0.6/stats endpoint. Neither depends on the /vX/traces wire format. This is the same disproven reasoning as the CSS gate removed by #5122, applied to a second knob, so it goes the same way. The cross-repo prerequisite this depended on is already satisfied: DataDog/system-tests#7403 (merged 2026-07-29) added /v1.0/traces to the trace-request filter tuple in tests/parametric/test_otlp_trace_metrics.py, which is exactly what this change needed upstream before it could land without breaking that test's span-metrics configuration.
darccio
added a commit
that referenced
this pull request
Aug 7, 2026
… fix The comment above the v1->v0.4 downgrade check still described RequestedTraceProtocol() as forcing v0.4 for OTLP span metrics, which this PR's config fix removes. The CSS-required gate is unrelated and still applies (tracked separately by #5122).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fourth in a split of this PR into a reviewable stack, following feedback that the original was too large to review as one unit. See the sequence below. Stacked on #5148 — this PR's diff is scoped to just the decoupling itself; merge #5148 first.
The v1.0 protocol was gated on
!canComputeStats(), commented "v1 requires CSS", but that's not actually true:ClientComputedStatsis false, exactly like it does for v0.4.Datadog-Client-Computed-Statsheader and the separate/v0.6/statsendpoint. The trace-send path is otherwise protocol-blind.No commit, PR, or doc anywhere states a rationale for the coupling — it was introduced as an unexplained one-line addition to a test-enablement PR. The practical, silent consequence:
DD_TRACE_STATS_COMPUTATION_ENABLED=false(or disabling the Agent'sclient_drop_p0s, e.g. via its probabilistic sampler) would downgrade the wire protocol with no log, even though1.0is the tracer's default.Replaces the startup gate with
(*config).effectiveTraceProtocol, a pure function of (requested protocol, agent v1 availability) evaluated at point of use. Nothing is downgraded in config and nothing is baked into a component, so the requested value stays inspectable and the effective one can't drift from it. Config telemetry now reports the effective protocol viaReportEffectiveTraceProtocol, which dedupes so repeated evaluation cannot inflate seqIDs.TestNewConfigKeepsV1WhenCSSDisabledpins this end-to-end at the wire level: with CSS off and the agent advertising/v1.0/traces, the request must land on/v1.0/traceswith a msgpack map body. Before this change it went to/v0.4/traceswith an array body (0x91).The stack
This PR was originally one 1215-line diff across 23 files (kakkoyun's feedback). It's now split as follows — ✅ merged into main, 🔵 open:
test(ddtrace/x/agenttest): configurable mock-agent/inforesponse — basemainrefactor(ddtrace/tracer): transport derives URL from payload protocol — stacked on test(ddtrace/x/agenttest): make the mock agent's /info response configurable #5146refactor(internal/config): renameTraceProtocol→RequestedTraceProtocol— stacked on refactor(ddtrace/tracer): derive the trace-intake URL from the payload's protocol #5147fix(ddtrace/tracer): runtime re-evaluation of agent v1 support — stacked on this PRfeat(ddtrace/tracer): trace protocol observability (startup log + downgrade diagnostic) — stacked on Matt2 #5, aboveIndependent siblings, not part of this chain:
fix(internal/config): report derived stats-computation to config telemetry — basemainfix(internal/config): stop forcing v0.4 when OTLP span metrics are enabled — stacked on fix(internal/config): report derived stats-computation to config telemetry #5141Notes for reviewers
tests/parametric/test_otlp_trace_metrics.py:264inDataDog/system-tests) is already resolved: DataDog/system-tests#7403, merged 2026-07-29, added/v1.0/tracesto the trace-request filter tuple. That's a prerequisite for fix(internal/config): stop forcing v0.4 when OTLP span metrics are enabled #5143 above, not for this PR.make generateand the config-inverter check produce zero diff.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.🤖 Generated with Claude Code