Repository navigation
fix(stats): send trace stats with their traces in on-demand mode - #1489
lucaspimentel wants to merge 7 commits into
Conversation
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 3 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 6c5a247 | Docs | View more details | Give us feedback! |
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. |
|
@DataDog review |
There was a problem hiding this comment.
🟡 Changes recommended
A critical forced-stats flush issue can keep accumulating batches while spans arrive, delaying stats delivery and blocking flush completion until ingestion leaves a gap.
1 open issue
What changed in this PR
This PR makes on-demand Lambda flushes send extension-computed trace stats without holding back recent buckets, reducing delays when the sandbox freezes. Managed Instance mode retains its existing behavior.
Changes:
- Adds opt-in forced stats flushing across blocking, non-blocking, and retry paths.
- Enables forced stats flushing for on-demand execution.
- Adds fake-intake tests for forced delivery and default hold-back behavior.
| File | Description |
|---|---|
| bottlecap/tests/apm_integration_test.rs | Tests current-bucket stats delivery through both flush paths. |
| bottlecap/src/flushing/service.rs | Adds and propagates the forced-stats option. |
| bottlecap/src/bin/bottlecap/main.rs | Enables forced stats for on-demand mode. |
Review effort: Balanced
![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: 7d006a4d7a
ℹ️ 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".
| dsm_processor.clone(), | ||
| ); | ||
| ) | ||
| .with_forced_stats(); |
There was a problem hiding this comment.
For high-throughput functions whose invocations repeatedly end within the same 10-second stats bucket, forcing every on-demand flush removes that bucket and lets later spans recreate it with fresh per-bucket cardinality state. Each invocation can therefore admit another full set of resource, endpoint, peer-tag, and additional-tag values, so the combined payloads for one logical bucket can far exceed the configured limits that normally collapse excess groups and bound stats cardinality. Preserve the limiter state across partial flushes, or avoid splitting a live bucket in a way that resets its limits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted tradeoff: partial flushes can admit more distinct values across payloads for the same logical bucket, while each in-memory bucket remains bounded. Holding those stats back would reintroduce the delivery delay this PR fixes. Preserving cardinality tracking across partial flushes would need support in libdatadog rather than duplicate tracking in the extension.
![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.
More details
The on-demand setting reaches blocking, non-blocking, and retry stats flushes while preserving Managed Instance bucket hold-back. Static review identified no actionable defect; test execution was blocked by toolchain and dependency availability.
🤖 Bits Code Review · Commit 7d006a4
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea687e2801
ℹ️ 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 mut force_pull = force_flush; | ||
| loop { | ||
| let stats = guard.get_batch(force_flush).await; | ||
| let stats = guard.get_batch(force_pull).await; | ||
| force_pull = false; |
There was a problem hiding this comment.
This first-pull-only behavior also applies to flush_blocking_final(), where there is no next flush to collect data arriving after the initial forced concentrator command. During shutdown, trace/OTLP handlers and their forwarding tasks are canceled but not awaited before the final flush, so an Add can be queued after the first forced pull; the next pull is now non-forced, holds that span in a current bucket, returns empty, and lets shutdown discard it. Establish an ingestion barrier before the final forced snapshot, or otherwise retain final-flush semantics that cannot leave concurrently queued current buckets behind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The missing shutdown ingestion barrier is already tracked in APMSVLS-679. That work coordinates listener completion and accepted-ingest draining before the final flush, with a bounded shutdown wait. This specific late-addition case belongs in its regression coverage. Keeping that broader shutdown fix separate from this PR.
eab7a02 to
87112eb
Compare
On-demand flushes kept the newest two 10s stats buckets back until a later flush. After an end-of-invocation flush the sandbox can freeze until the next invocation or shutdown, so the stats for spans that were already sent arrived minutes later, or were lost if the final flush failed. Stats are the only telemetry held back this way. On-demand mode now forces the stats flush on every flush, so stats leave together with their traces. A span that arrives after its bucket was flushed is still counted, in the oldest open bucket. Managed Instance mode keeps the hold-back because its timer keeps flushing.
A forced concentrator pull returns every span added since the previous pull, so a flush that forced every pull kept going, one stats request per pull, for as long as spans kept arriving.
The on-demand flush tests now route the trace through the trace aggregator the FlushingService flushes, and assert that the same flush sends the trace.
Cover the two trade-offs of forcing stats: a second flush within one bucket sends that bucket again as a partial payload, and a span older than every open bucket still counts in the next flush.
87112eb to
6c5a247
Compare
Overview
In on-demand mode, Bottlecap sends traces on every flush but holds back the newest ~20s of trace stats until a later flush. In Lambda, that later flush can be minutes or hours away (the next invocation or shutdown), and the stats are lost if the shutdown flush fails. This PR sends stats in the same flush as the traces they describe.
Why it matters. Stats drive RED metrics, service pages and monitors, and they are computed from 100% of spans while traces are sampled. Today the sampled data arrives on time and the complete data arrives late, or not at all. Sparse or bursty functions are hit hardest: the stats for their last burst wait for shutdown, the riskiest flush (AWS allows 2s before killing the extension).
Root cause. Every on-demand flush called the stats flusher with
force = false, so libdatadog'sSpanConcentratorkept its newest two buckets (10s each in bottlecap) for late spans; only the shutdown flush forced them out. That trade-off suits a long-running agent whose timer flushes again in seconds, but in Lambda the sandbox can freeze right after a flush. Stats are the only telemetry type held back this way: logs, metrics and traces are flushed in full every time.Evidence. In
serverless-e2e-testslambda-features, thestatsv2scenario readstrace.aws.lambda.hitsshort of the span count on most first reads, typically by 1,000 to 2,000 of 50,000, while the span count is already complete. Over 14 days of CI Visibility data (Oct 19 to Nov 2), about 80% of non-outlier first reads were short, evenly across runtime families. A deficit of that size matches the last ~20s of each function's traffic.Change.
FlushingService::with_forced_stats()makes the blocking and non-blocking flushes force the stats flusher. The default is unchanged.FlushingServiceinmain.rsenables it, covering theEnd,PeriodicandContinuousflush decisions, including the interval ticks of theend,<ms>strategy.aws.lambdaspan is in the concentrator before the End flush:on_platform_runtime_doneis awaited end to end,send_processed_tracesenqueues eachAddsynchronously, andAddandFlushshare one FIFO channel.bottlecap-test-modeis unchanged.Trade-offs.
endstrategy: an unforced flush sends nothing until a bucket ages out (about one request per 10s bucket), while a forced End flush sends one per invocation. The request runs concurrently with the trace request in the same flush; its effect on post-runtime duration at high volume is being measured. Underdefaultat high traffic, flushes are every 20s, so the change is negligible.Testing
tests/apm_integration_test.rscover forced blocking and non-blocking flushes sending current stats alongside their traces, the hold-back that Managed Instance keeps, and the partial and late-bucket trade-offs above.stats_flusher::tests::forced_flush_ends_while_spans_keep_arrivingpins that a forced flush sends one stats request while spans keep arriving.cargo nextest run --workspacepasses, as docargo fmt --checkandcargo clippy --workspace --all-targets --features defaultwith-D warnings.lambda-featureswith first-read hits equal to spans in all 18 runtimes, but that read came 11.6 minutes after the invocations ended, late enough that the unfixed hold-back would also have converged. A fresh deployment read at the usual ~2 minute timing is still needed.