Cover the ETP/v1 ↔ Client-Side Stats intersection in both directions - #7444
Draft
darccio wants to merge 5 commits into
Draft
Cover the ETP/v1 ↔ Client-Side Stats intersection in both directions#7444darccio wants to merge 5 commits into
darccio wants to merge 5 commits into
Conversation
Contributor
|
|
|
CSS and the ETP/v1 trace protocol were only ever exercised separately. No scenario configured both, so the combination was reached solely by accident, via whichever protocol a tracer happened to default to. That is how #7419 happened: dd-trace-java started defaulting to /v1.0/traces and Test_Client_Drop_P0s broke, because it only looked at legacy trace endpoints. The two features do interact. CSS drops P0s client-side, changing which traces are emitted, while ETP/v1 changes how they are encoded. Both act on the same trace-emission path, so the intersection deserves explicit coverage rather than incidental coverage. Add TRACE_STATS_COMPUTATION_V1: identical to TRACE_STATS_COMPUTATION apart from the trace protocol, so a failure there points at a CSS/ETP interaction and not at a sampling or obfuscation difference. Run the format-sensitive stats classes in it: Test_Client_Stats (hit and top-level-hit counting plus IsTraceRoot, which depend on top-level span detection that v1 relocates into `attributes`), Test_Peer_Tags (peer tag extraction from span structure) and Test_Transport_Headers (stats request headers). Test_Time_Bucketing is left out as timing-sensitive, and Test_Agent_Info_Endpoint as format-independent. Also fix the endpoint probing in Test_Client_Drop_P0s. The sequential `if len(...) == 0` chain stopped at the first non-empty endpoint, so a tracer emitting on both v0.4 and v1.0 only had v0.4 header-checked. get_data accepts a list of path filters and matches any of them, so all trace payloads are now collected and every one is verified. Co-Authored-By: Claude <noreply@anthropic.com>
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>
darccio
force-pushed
the
dario.castane/xenodochial-nobel-5379e0
branch
from
August 7, 2026 10:01
0391205 to
8dfba0d
Compare
The dd-lib-java-init-test-app weblog runs on Alpine, where the Datadog Java profiler fails to start (missing libgcc_s.so.1), same root cause already tracked for the VM auto-inject multialpine variant. Co-authored-by: Claude <noreply@anthropic.com>
…tation_v1 trace_stats_computation_v1 exercises the same nodejs /v0.6/stats endpoint affected by the pre-existing APMLP-1498 bug (missing Service and IsTraceRoot fields), but the schema known-bug exclusions only matched trace_stats_computation_client_drop_p0s_false, causing test_library to fail for all nodejs weblog jobs in CI. Co-Authored-By: Claude <noreply@anthropic.com>
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
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.
Motivation
CSS (Client-Side Stats) and the ETP/v1 trace protocol were only ever exercised separately. No scenario configured both, so the combination was reached solely by accident — via whichever protocol a tracer happened to default to.
The two features genuinely interact. CSS drops P0s client-side, changing which traces are emitted; ETP/v1 changes how they are encoded. Both act on the same trace-emission path — which is also why several parametric tests have to set
DD_TRACE_STATS_COMPUTATION_ENABLED=falseto observe P0s at all (see the comment attests/parametric/test_span_sampling.py:862).And the interaction was not hypothetical. Released dd-trace-go gates v1 on CSS capability and silently downgrades to
/v0.4/tracesthe moment stats computation is disabled; DataDog/dd-trace-go#5122 removes that gate. system-tests never reported it, because the one place it would have surfaced —Test_V1PayloadByDefault, which runs in the DEFAULT scenario whereDD_TRACE_STATS_COMPUTATION_ENABLED=false— is declaredmissing_feature, i.e. a non-strict xfail that cannot fail. Worse, its stated reason (not implemented by default yet) is inaccurate: Go had implemented v1-by-default; the test failed because the scenario disables CSS. The declaration hid the bug and misattributed its cause.This PR makes the intersection explicit in both directions.
Changes
1.
TRACE_STATS_COMPUTATION_V1— CSS on, v1 protocolIdentical to
TRACE_STATS_COMPUTATIONapart from two lines: weblogDD_TRACE_AGENT_PROTOCOL_VERSION=1.0and agentDD_APM_ENABLE_V1_TRACE_ENDPOINT=true. Keeping everything else byte-for-byte identical is deliberate: a failure here points at a CSS/ETP interaction, not at a sampling or obfuscation difference.The format-sensitive stats classes now run in it too:
Test_Client_StatsIsTraceRootdepend on top-level span detection, which v1 relocates intoattributesTest_Peer_TagsTest_Transport_HeadersDeliberately excluded:
Test_Time_Bucketing(timing-sensitive — would double an existing flake surface) andTest_Agent_Info_Endpoint(asserts/infocapabilities, format-independent).2.
APM_TRACING_EFFICIENT_PAYLOAD_STATS_DISABLED— CSS off, v1 protocolScenario 1 alone is not a regression test for the coupling: CSS-on + v1 worked even while v1 was gated on CSS, so it cannot discriminate. The discriminating case is the mirror — v1 pinned while CSS is off — and no existing scenario did that. Only two scenarios pin the protocol, and neither disabled CSS.
APM_TRACING_EFFICIENT_PAYLOADplusDD_TRACE_STATS_COMPUTATION_ENABLED=false, withTest_V1PayloadWithStatsDisabledasserting every trace still goes to/v1.0/tracesin v1 format. It first asserts that no/v0.6/statspayloads exist, so it fails loudly rather than passing vacuously if CSS were somehow active.This also settles something the suite currently cannot answer: whether the old code honoured an explicit protocol pin or overrode it — the difference between a default-selection bug and a config-precedence bug.
3. Fixed endpoint probing in
Test_Client_Drop_P0sThe sequential
if len(...) == 0chain stopped at the first non-empty endpoint, so a tracer emitting on bothv0.4andv1.0only ever hadv0.4header-checked.get_dataaccepts a list of path filters and matches any of them, so all trace payloads are now collected and every one is verified.Manifests
Test_V1PayloadWithStatsDisabledis declaredmissing_featurefor: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 new test is meaningful rather than vacuous.Plumbing
Both scenarios registered in
.github/workflows/run-end-to-end.yml; affected entries updated intests/test_the_test/scenarios.json.Longer term: this becomes the permanent guard
Once DataDog/dd-trace-go#5122 ships,
Test_V1PayloadByDefaultin the DEFAULT scenario becomes a real regression guard for the decoupling, at no extra cost — DEFAULT already disables CSS and already asserts v1, which is precisely the property #5122 establishes.That gives a concrete acceptance criterion, in this order:
v2.10.0-rc.1— verified withgit merge-base --is-ancestor— so it lands inv2.11.0unless backported.Test_V1PayloadWithStatsDisabledfor golang frommissing_featureto that release version.Test_V1PayloadByDefaultfor golang frommissing_feature (not implemented by default yet)to the same version, replacing the inaccurate reason.v2.10.0-rc.1. That tag predates the decoupling, so CSS-off still downgrades to v0.4 and both tests must fail onnet-http/net-http-orchestrion. Until #5122 is released,missing_featureis the correct declaration — just not for the reason currently written down.Worth noting for whoever does step 3: because
missing_featureis a non-strict xfail, these tests will XPASS silently the moment Go starts passing them. Nothing will tell you it is time to flip them. That is the same gap that let the original coupling go unreported, and it is being tracked separately.Notes for the reviewer
This needs R&P review on two counts — scenarios are added, and files outside
tests//manifests/are touched (utils/_context/,.github/workflows/).One open decision. Python, .NET and PHP have CSS coverage but no ETP/v1 support, so
DD_TRACE_AGENT_PROTOCOL_VERSION=1.0will likely be ignored and they will emit legacy payloads — makingTRACE_STATS_COMPUTATION_V1a duplicate run ofTRACE_STATS_COMPUTATIONfor them. It passes, it just costs CI time. Left ungated on purpose: when those tracers gain ETP/v1 the coverage activates by itself with no manifest edit, and it surfaces the day a tracer starts honouring the flag. Happy to addmissing_featuregating for those three if you would rather pay less CI.Verification done — the full
./format.sh --checkequivalent is green locally:mypy2.1.0 (as pinned):Success: no issues found in 528 source filesruff format --check: 612 files already formattedruff check: all checks passedutils/manifest/_internal/parser.load), including the quoted#in the golang reason stringTest_V1PayloadWithStatsDisabledresolves to exactly one scenario marker,APM_TRACING_EFFICIENT_PAYLOAD_STATS_DISABLEDtest_disablecorrectly still maps toDEFAULT— its method-level@scenarios.defaultoverrides the class markers, perconftest.py:344Not verified: neither scenario has been executed against a real tracer, so they are unproven end-to-end. CI is the first real proof, and the manifest declarations above are my best reading of each tracer's current state — expect to adjust them.
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present🤖 Generated with Claude Code