Repository navigation
[Traces] Add error sampling with opt-in tracer-drop rescue - #169
lucaspimentel wants to merge 44 commits into
Conversation
|
@DataDog review |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Concurrent requests can send times to the shared sampler in reverse order at a bucket limit. This can clear valid counts and permit too many rescued traces.
🤖 Datadog Autotest · Commit f316a2a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@DataDog review |
There was a problem hiding this comment.
A backward system-clock change can move the sampler to an old bucket. The sampler then uses stale counts and applies the TPS limit incorrectly.
🤖 Datadog Autotest · Commit e5fcd5e · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
e5fcd5e to
bfbe164
Compare
lucaspimentel
changed the title
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. |
![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: bfbe16481b
ℹ️ 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".
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in sampler test equivalence, mutex contention, and integration-test error propagation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds shared error sampling to rescue eligible errored P0 trace chunks while preserving priorities and stats behavior.
Changes:
- Integrates configurable, shared error sampling.
- Stamps
_dd.errors_sron rescued root spans. - Adds configuration, dependency wiring, and unit/integration coverage.
| File | Review summary |
|---|---|
crates/datadog-trace-agent/tests/integration_test.rs |
Moderate findings (1 vote each): propagate mini-agent startup and shutdown/task errors. |
crates/datadog-trace-agent/src/trace_processor.rs |
Moderate findings: align test environments in sampler oracles (2 votes each); avoid holding a blocking mutex during the full payload walk (1 vote). Nit: document root-resolution fallback behavior (2 votes). |
crates/datadog-trace-agent/src/stats_processor.rs |
Test configuration fixtures updated; no findings. |
crates/datadog-trace-agent/src/config.rs |
Sampler configuration parsing updated; no findings. |
crates/datadog-trace-agent/Cargo.toml |
Adds the sampler dependency; no findings. |
crates/datadog-serverless-compat/src/main.rs |
Initializes the shared sampler; no findings. |
Cargo.lock |
Records dependency updates; no findings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@DataDog review |
There was a problem hiding this comment.
lucaspimentel
changed the title
|
@DataDog review |
There was a problem hiding this comment.
3788c42 to
0346af6
Compare
f01c4f5 to
23fc576
Compare
lym953
left a comment
lym953
left a comment
There was a problem hiding this comment.
[Written by Claude, an AI assistant, for Yiming]
I reviewed the production code and the tests. The PR adds about 310 lines of production code and about 1,490 lines of tests. The inline comments have the details.
Production code
- The Lambda extension configures the same sampler with different environment variables.
- The timestamp floor can live in the sampler mutex.
- Two questions: why the stats setting controls rescue, and why priority -128 is never eligible.
Tests
I estimate that you can remove about 400 to 450 test lines and keep coverage of each behavior:
- The eligibility table test covers six single-case tests.
- Two tests do the same check.
- Some tests check the sampler crate again.
- The config tests repeat the same setup block.
I suggest that you keep these tests, because no other test covers them:
- the eligibility table test
- decision-maker promotion
- the initialization warning
- the resolved-root test
- the test that healthy chunks use no budget
- the disabled-TPS test
- the timestamp clamp
- the stats-off no-op test
- the test that stats see chunks before stamping
- the two integration tests
I measured the line counts. The saving is an estimate, because I did not write the shorter version or run a coverage tool.
|
@DataDog review |
|
@codex review |
There was a problem hiding this comment.
More details
The diff reflects the discussed design revisions: no-priority error chunks qualify, and rescue runs independently of stats computation. Static review identified no reportable defect.
🤖 Bits Code Review · Commit 391cbfe
74b4dff to
c91d837
Compare
Chunks without a sampling priority are kept by the backend, so they no longer use the shared error sampling budget. This matches the Go agent, where only chunks dropped by the no-priority sampler reach the error sampler.
An empty or whitespace-only value now uses the default without logging an invalid-value warning, as the Go agent does.
Drop the AlwaysKeep disabled case and the lock-ordering test, and assert literal keep decisions instead of replaying a standalone sampler.
ServerlessTraceProcessor::new now takes the config and owns its error sampler, instead of exposing the shared lock in its public signature. Clones still share one budget.
Disabled rescue no longer takes the shared lock, and span views are built before locking, so large payloads hold it for less time.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6f521f16
ℹ️ 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".
| let eligible = chunk.priority == 0 | ||
| || (tracer_drop_enabled | ||
| && chunk.priority < 0 | ||
| && chunk.priority != i32::from(i8::MIN) |
There was a problem hiding this comment.
For traces whose tracer omitted sampling priority, the collector represents the priority as -128; the PR description says these errored chunks are eligible by default, but this condition excludes them even when the tracer-drop feature is enabled, so they can never receive _dd.errors_sr. Either restore the advertised eligibility or update the PR description and QA expectations to document the intentional exclusion.
AGENTS.md reference: AGENTS.md:L64-L65
Useful? React with 👍 / 👎.
| fn test_rate_limited_under_sustained_load_keeps_and_rejects() { | ||
| for (sample_rate, expected_keeps) in [(None, 83_usize), (Some(0.25_f64), 9)] { |
There was a problem hiding this comment.
This test asserts exact keep counts (83 and 9) produced by the shared sampler's rate algorithm, duplicating behavior already tested in datadog-agent-trace-sampler and coupling the trace-agent suite to its internals rather than merely checking the adapter call. The change adds about 272 core lines but 955 test lines; replacing this 100-chunk algorithm test with a small adapter-focused case would reduce that disproportion without losing integration coverage.
AGENTS.md reference: AGENTS.md:L45-L47
Useful? React with 👍 / 👎.
What does this PR do?
Integrate the shared error sampler from #141 into the SCL trace processor, giving dropped error chunks another sampling opportunity.
error != 0. Priority0is eligible by default, regardless of decision maker.DD_APM_FEATURES=error_rare_sample_tracer_drop. Exclude exact manual drops (priority == -1and chunk_dd.p.dm == "-4"); positive priorities never qualify.-128) are never eligible. The backend keeps them anyway, and in the Go agent they reach the error sampler only after its no-priority sampler drops them, so rescuing them would only spend the shared budget. The Lambda extension also treats them as kept._dd.p.dmfrom the first span carrying it to chunk tags before enrichment, independently of rescue settings. Existing chunk values take precedence, including empty strings; span metadata remains intact._dd.errors_sron the resolved root when the sampler keeps a chunk. This enables backend retention without changing sampling priorities or decision makers. Forward every chunk, including those not rescued.Motivation
APMSVLS-472: retain a representative sample of errors that would otherwise be dropped, with a shared adaptive sampling budget. The opt-in feature widens eligibility only; it does not enable a disabled sampler.
Additional Notes
10error traces per second per instance, matching the Go agent'sErrorTPSdefault. SetDD_APM_ERROR_TPS=0to turn it off.RateLimitedmode with extra sample rate1.DD_APM_ERROR_TPS <= 0disables rescue; blank values count as unset; malformed/non-finite values warn and fall back to10.DD_APM_ERROR_SAMPLER_MODEis not read.DD_SERVERLESS_ERROR_SAMPLER_ENABLED(off by default, keeps every candidate) and only warns onDD_APM_ERROR_TPS. This PR follows the Go agent setting instead; the extension can be aligned separately.Describe how to test/QA your changes