Repository navigation
feat(traces): wire in the error sampler to rescue errored p0 trace chunks - #169
lucaspimentel wants to merge 23 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
Wire the shared datadog-agent-trace-sampler into the SCL trace processor so error chunks with automatic-drop priority (0) get a second look before being forwarded. On a keep, the sampler's rate is stamped as a positive `_dd.errors_sr` metric on the chunk's root span, which the APM backend treats as an `error` ingestion reason and retains without any priority promotion. Chunks are never dropped locally: unrescued chunks are forwarded unchanged for the backend to discard. - Default to rate-limited rescue at 10 error TPS per process, with a shared budget across requests and processor clones - Configure via DD_APM_ERROR_SAMPLER_MODE (rate_limited | always_keep) and DD_APM_ERROR_TPS (<= 0 disables rescue in either mode); invalid values warn and fall back to defaults - Gate rescue on agent stats computation and submit all chunks to the stats concentrator before any rescue stamping
Extracts the "prefer tracer payload env, fall back to agent config env" rule into one function reused by stats flushing and error-rescue sampling, and removes duplicated sampler lock/disabled-check and SpanView-construction code in the error sampler and its tests.
The clock read and timestamp clamp were happening before the sampler lock was acquired, letting concurrent requests deliver timestamps out of lock-acquisition order and undercount TPS in the rolling window. 🤖
Extract shared fixtures for rescue processor setup, errored P0 requests, and keep counts; replace redundant assertions and fully-qualified paths. No behavior change.
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
|
Codex Review: Didn't find any major issues. Delightful! 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". |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved contract mismatches permit no-priority sentinel sampling and stats-disabled rescue, while production configuration ignores the advertised AlwaysKeep mode.
3 open issues
3 resolved since last review
Review effort: Balanced
| /// 1.0. Startup stays operational when the optional TPS setting is malformed. | ||
| fn parse_error_sampler_config(raw: Option<&str>) -> ErrorSamplerConfig { | ||
| let mut config = ErrorSamplerConfig { | ||
| mode: ErrorSamplerMode::RateLimited, |
| tracer_drop_enabled: bool, | ||
| ) { | ||
| let eligible = chunk.priority == 0 | ||
| || chunk.priority == i32::from(i8::MIN) |
| // conditional stats submission so the concentrator observes chunks before | ||
| // rescue stamping, and before payload splitting so the inserted metric is | ||
| // included in the recomputed outbound size. | ||
| self.apply_error_rescue(&mut payload, &config); |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f436a7174
ℹ️ 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".
| // conditional stats submission so the concentrator observes chunks before | ||
| // rescue stamping, and before payload splitting so the inserted metric is | ||
| // included in the recomputed outbound size. | ||
| self.apply_error_rescue(&mut payload, &config); |
There was a problem hiding this comment.
When DD_AGENT_STATS_COMPUTATION_ENABLED=false, info_handler still sets client_drop_p0s=true (mini_agent.rs:755-759), so compliant tracers discard priority-0 traces before sending them to this processor. Consequently, this supposedly stats-independent rescue pass never sees the errored P0 chunks it is intended to rescue; the info response should also disable client-side P0 dropping whenever the error sampler is enabled.
Useful? React with 👍 / 👎.
What does this PR do?
Wires the shared error sampler (
datadog-agent-trace-sampler, introduced in #141) into the SCL trace processor, with opt-in tracer-drop error rescue viaDD_APM_FEATURES=error_rare_sample_tracer_drop.error != 0; HTTP status or error metadata alone does not qualify. Priority0is eligible with or without the feature, regardless of decision maker.-128) and exact manual drops: priority-1with chunk_dd.p.dm == "-4". Absent, empty, and other decision makers at-1qualify; other negative priorities, such as-2, qualify even with"-4". Positive priorities never qualify._dd.p.dmfrom the first span carrying the key in vector order to chunk tags. Existing chunk values, including empty strings, take precedence; an empty first span value is also preserved. Original span metadata stays intact. This happens independently of the feature, sampler, and stats settings._dd.errors_sron the resolved root. Preserve chunk priorities, span_sampling_priority_v1, decision-maker values, origins, and span order. All chunks remain forwarded, including unrescued negatives; local sampled-out dropping is deferred.Motivation
APMSVLS-472: give dropped error traces another sampling opportunity while preserving existing sampler enablement and budgets. The opt-in feature widens eligibility only; it does not select a sampler mode or enable a disabled sampler.
Additional Notes
RateLimited, target TPS10, and extra sample rate1.AlwaysKeepremains available throughDD_APM_ERROR_SAMPLER_MODE.DD_APM_ERROR_TPS <= 0disables rescue in either mode. Invalid/non-finite TPS values warn and fall back to10.agent_stats_computation_enabled, after all chunks are submitted to stats and before payload splitting. Stats observe chunks before rescue stamping, including chunks not rescued.DD_APM_FEATURESfollows the grammar in #186: if any comma exists, split only on commas; otherwise split only on literal ASCII spaces. Trim entries, ignore empties, and match the entire feature name case-sensitively. Interior tabs are not delimiters. Absent/unreadable values disable the feature; unrelated features are ignored. No YAML/shared-loader changes are included: the trace agent already builds its own config, and adopting the shared loader solely for this flag would add broader loader coupling.10does not emit this disabled-sampler warning."-9"remain unchanged, without a retention workaround.Describe how to test/QA your changes
Automated coverage includes feature grammar and configuration independence, unconditional decision-maker promotion and precedence, the feature-on/off eligibility matrix, child errors and root fallback, disabled TPS in both modes, initialization warning capture, shared-budget decisions compared with a standalone sampler, the production stats gate, and pre-rescue concentrator snapshots. Paired real-flusher tests decode outbound
AgentPayloadand stats, checking promotion in both feature states, unchanged priorities and span metadata, rescue stamping, forwarding, and stats inclusion for every input chunk.Local working-tree validation passes:
cargo test -p datadog-trace-agent --lib config::testscargo test -p datadog-trace-agent --lib trace_processor::testscargo test -p datadog-trace-agent --test integration_test error_rescue -- --test-threads=1cargo nextest run --workspace --no-fail-fastcargo test --workspace --doccargo fmt --all -- --checkcargo check --workspacecargo clippy --workspace --all-features -- -D warningsValidation was rerun after behavior-preserving test simplifications, using
CARGO_PROFILE_DEV_DEBUG=0,CARGO_PROFILE_TEST_DEBUG=0, andCARGO_INCREMENTAL=0. The paired wire tests use distinct listener ports to avoid competing under nextest's process isolation.