[APMSVLS-469] fix(traces): filter sampled-out trace chunks when tracer or extension computes stats - #1358
Conversation
…xtension computes stats Trace intake now drops sampled-out chunks whenever stats are computed before the backend, by either the tracer (Datadog-Client-Computed-Stats) or the extension, instead of only when extension-side stats are enabled. The backend still receives them when it owns stats computation, since it needs them to compute stats. The error sampler now rescues eligible errored drops on both non-backend paths.
|
🔗 Commit SHA: 0cf9309 | Docs | View more details | Give us feedback! |
lucaspimentel
left a comment
lucaspimentel
left a comment
There was a problem hiding this comment.
Found one high-severity trace-loss issue.
Review by Claude Code
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, aligns behavior with the documented stats-ownership matrix, and is backed by expanded unit + E2E test coverage for both extension-owned and tracer-owned paths.
Pull request overview
This PR updates Bottlecap’s trace intake filtering to drop sampled-out trace chunks whenever trace stats are computed before the backend—whether stats are computed by the extension or by the tracer—while preserving backend-owned behavior (keeping all chunks so the backend can compute stats). This aligns the filtering gate with the existing stats-ownership precedence (client_computed_stats overrides extension-side stats).
Changes:
- Resolve stats ownership via
StatsComputedBy::resolve(...)and filter sampled-out chunks whenever the backend does not own stats. - Extend unit tests and fake-intake E2E tests to cover tracer-owned stats paths (including error sampler behavior) in addition to extension-owned paths.
- Update config/docs comments to reflect that error sampling/filtering applies on both extension-owned and tracer-owned stats paths.
File summaries
| File | Description |
|---|---|
| bottlecap/src/traces/trace_processor.rs | Filters sampled-out chunks based on resolved stats owner (backend vs extension vs tracer) and expands unit tests to cover all ownership combinations. |
| bottlecap/tests/apm_integration_test.rs | Extends fake-intake E2E coverage to validate filtering + error rescue behavior for both extension-owned and tracer-owned stats paths. |
| bottlecap/src/config/mod.rs | Updates DD_SERVERLESS_ERROR_SAMPLER_ENABLED documentation to reflect applicability beyond extension-owned stats. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: b57d663bc5
ℹ️ 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".
Overview
Follow-up to #1320. Filters sampled-out trace chunks from trace intake whenever trace stats are computed before the backend, by either the tracer or the extension. Previously the filtering gate only checked
lambda_extension_compute_stats, so tracers that compute stats themselves and reportDatadog-Client-Computed-Stats: true(for example,dd-trace-javav1.63.0 withDD_TRACE_STATS_COMPUTATION_ENABLEDon by default) still had their rejected chunks forwarded to intake.lambda_extension_compute_stats=false,client_computed_stats=false)lambda_extension_compute_stats=true)client_computed_stats=true)Behavior notes:
client_computed_stats=truemeans the tracer owns stats even when extension-side stats are also configured.process_traces, stats ownership is resolved once and reused for both_dd.compute_statsstamping and sampled-out filtering, keeping those paths aligned.AutoDropchunks; explicit/user drops (negative priority) remain dropped, and_dd.errors_sris stamped on rescued root spans.Testing
trace_processor.rs: table-driven over all four ownership combinations, asserting filtering vs. no-filtering, the pre-filter stats payload contents, and error rescue on both non-backend paths. The tracer-owned case was confirmed failing before the fix.apm_integration_test.rs:e2e_sampled_out_chunks_filtered_by_stats_owner(backend keeps all chunks; extension and tracer paths filter to the kept trace only) ande2e_error_sampler_rescues_only_errored_p0_tracesextended to both non-backend owners.cargo nextest run --workspace: 678 passed, 0 failed.cargo fmt --all -- --check,cargo check --workspace, and both clippy passes (default and fips features) pass.Found while investigating excess trace volume in the
serverless-e2e-testsjava17-samplingscenario: 20 traces observed at a 0.2 sample rate over 50 invocations where roughly 10 were expected.