feat(pipeline): send Datadog-Client-Computed-Stats when enabled#164
Conversation
Overall package sizeSelf size: 30.07 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------|🤖 This report was automatically generated by heaviest-objects-in-the-universe |
![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: 872ba6b50b
ℹ️ 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".
| if client_computed_stats { | ||
| builder.set_client_computed_stats(); |
There was a problem hiding this comment.
When a caller enables the existing stats_enabled path but does not also pass the new client_computed_stats flag, this constructor still builds the StatsCollector and sends client-side stats, but the trace exporter is left without Datadog-Client-Computed-Stats. In that configuration the agent can still compute its own APM stats/sampling for the same traces, which is the scenario this change is meant to prevent; the condition should include stats_enabled as well, or the caller boundary needs to make the two flags impossible to diverge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@bengl what's the relationship between stats_enabled and client_computed_stats?
There was a problem hiding this comment.
stats_enabled = we compute client-side stats (StatsCollector → /v0.6/stats). client_computed_stats = the Datadog-Client-Computed-Stats header telling the agent to skip its own APM stats/sampling.
Computing stats client-side requires the header (else the agent double-counts), so stats_enabled always implies it. APM-standalone (apmTracingEnabled=false) sets the header without client stats. So the header = client_computed_stats || stats_enabled — now OR'd in the binding rather than trusting the caller (fixes Codex's P2). 0a5ee27b41.
There was a problem hiding this comment.
For context, apmTracingEnabled=false is a mode where we still emit traces, but only as a transport for other products (like Application Security Monitoring).
No tracing related data is supposed to end up in customer accounts so that they are not billed for APM.
So we set client_computed_stats to true even if we don't actually compute stats so that the agent does not create trace stat metrics them for the transport traces
Add a `client_computed_stats` parameter to `WasmSpanState::new` and set `builder.set_client_computed_stats()` so the exporter sends the `Datadog-Client-Computed-Stats: true` header. The tracer needs this when it runs in APM-standalone (apmTracingEnabled=false) so the agent skips its own APM stats/sampling. The header is also sent whenever `stats_enabled` is set: computing stats client-side requires telling the agent to skip its own, otherwise the same traces are counted twice. Enabling stats therefore always implies the header, so the flags are OR'd in the binding rather than relying on the caller to keep them in sync. libdatadog's TraceExporterBuilder already supports the flag; the WASM binding just never called it. Adds pipeline tests asserting the header is present when client_computed_stats OR stats_enabled is set, and absent when both are off.
bengl
force-pushed
the
bengl/client-computed-stats
branch
from
872ba6b to
0a5ee27
Compare
Add a `client_computed_stats` parameter to `WasmSpanState::new` and set `builder.set_client_computed_stats()` so the exporter sends the `Datadog-Client-Computed-Stats: true` header. The tracer needs this when it runs in APM-standalone (apmTracingEnabled=false) so the agent skips its own APM stats/sampling. The header is also sent whenever `stats_enabled` is set: computing stats client-side requires telling the agent to skip its own, otherwise the same traces are counted twice. Enabling stats therefore always implies the header, so the flags are OR'd in the binding rather than relying on the caller to keep them in sync. libdatadog's TraceExporterBuilder already supports the flag; the WASM binding just never called it. Adds pipeline tests asserting the header is present when client_computed_stats OR stats_enabled is set, and absent when both are off.
3 participants
What
Add a
client_computed_stats: boolparameter toWasmSpanState::newand callbuilder.set_client_computed_stats()when set, so the exporter emits theDatadog-Client-Computed-Stats: trueheader.Why
The tracer needs this header when it computes stats client-side or runs in APM-standalone (
apmTracingEnabled=false), so the agent skips its own APM stats/sampling for those traces. dd-trace-js's native-spans branch (DataDog/dd-trace-js#9139) has ~46 ASM-standalone system-test failures asserting this header, plus it's a prerequisite for client-computed stats to be honored (test_resource_renaming).libdatadog's
TraceExporterBuilder::set_client_computed_stats()already exists; the WASM binding just never called it.Changes
crates/pipeline/src/lib.rs: newclient_computed_statsparam onWasmSpanState::new; conditionalbuilder.set_client_computed_stats().test/pipeline.js: thread the new arg through the test harness; add tests asserting the header istruewhen enabled and absent when disabled (both confirm the send reached/v0.4/traces).Consumer bump
This changes the
WasmSpanStateconstructor arity (ABI break), so it ships as 0.13.0 and dd-trace-js bumps in lockstep (it pins an exact version and owns both sides), wiringclientComputedStats = stats.DD_TRACE_STATS_COMPUTATION_ENABLED || apmTracingEnabled === falsethroughtracer.js/native_spans.js.Test plan
cargo check -p pipeline --target wasm32-unknown-unknown✓yarn build-wasm pipeline✓node --test test/pipeline.js→ 48 pass / 0 fail (incl. 2 new header tests)