Only warn about trace buffer overflow when a kept trace is lost - #12348
Only warn about trace buffer overflow when a kept trace is lost#12348dougqh wants to merge 2 commits into
Conversation
The RemoteWriter logged "Dropped due to a buffer overflow" at WARN for every overflow, including traces that sampling had already decided to drop. Those traces were never going to reach the UI, so the warning alarms users about a non-event; it has driven several support tickets. Split the overflow result so the prioritization strategy reports which queue overflowed - it already knows. Overflow of a kept trace keeps the warning, now naming the queue size setting so the message is actionable. Overflow of an already sampled-out trace drops to debug. Classifying at the source rather than re-deriving from sampling priority at the log site also keeps force-keep traces correct: they go to the primary queue regardless of their priority byte, so losing one is real data loss even though its priority reads as drop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RemoteWriterLoggingTest pins the user-visible behaviour: a kept trace lost to overflow warns and names dd.writer.queue.size, a sampled-out one stays off the warning path, and a flood of the benign case cannot consume the rate limiter budget that the real warning needs. Both new assertions were checked against the pre-fix behaviour to confirm they fail without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dougqh
added
tag: ai generated
|
🎯 Code Coverage (details) 🔗 Commit SHA: a321351 | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![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: a3213514c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return spanSampling.offer(trace) | ||
| ? PublishResult.ENQUEUED_FOR_SINGLE_SPAN_SAMPLING | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW; | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW_SAMPLED_OUT; |
There was a problem hiding this comment.
When single-span sampling rules are configured, this queue exists specifically so SpanSamplingWorker can evaluate a dropped trace and forward matching spans to the primary queue. If this offer fails, those potentially sampled spans are lost before evaluation, yet the new result makes RemoteWriter log only at DEBUG. Return the kept-data overflow result for this queue, or introduce a distinct result that still warns about the loss of single-span-sampling candidates.
Useful? React with 👍 / 👎.
| return secondary.offer(trace) | ||
| ? PublishResult.ENQUEUED_FOR_SERIALIZATION | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW; | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW_SAMPLED_OUT; |
There was a problem hiding this comment.
When ENSURE_TRACE is selected and CoreTracer force-keeps an error trace while its sampling priority remains SAMPLER_DROP or USER_DROP, the tracer still calls writer.write, but this strategy does not check root.isForceKeep() as FastLaneStrategy does. If the secondary queue is full, this return therefore labels an intended export as sampled out and suppresses the warning; handle force-keep before the priority switch or return the kept-overflow result in this case.
Useful? React with 👍 / 👎.
| "Dropped a kept trace due to a buffer overflow: [{} spans]." | ||
| + " Traces are being produced faster than they can be sent to the agent;" | ||
| + " consider raising dd.writer.queue.size", |
There was a problem hiding this comment.
The warning tells users to raise dd.writer.queue.size, but a repo-wide lookup finds no code that reads this setting; the standard WriterFactory path leaves each writer builder's hard-coded BUFFER_SIZE unchanged. Consequently, users encountering real kept-trace loss cannot act on this remediation because setting the advertised property has no effect; either wire a supported setting through Config and WriterFactory or recommend an option that actually controls this queue.
Useful? React with 👍 / 👎.
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A full single-span sampling queue can drop spans that sampling rules can keep, but the change hides this warning. The kept-trace warning also names a setting that the tracer does not read.
🤖 Datadog Autotest · Commit a321351 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| return spanSampling.offer(trace) | ||
| ? PublishResult.ENQUEUED_FOR_SINGLE_SPAN_SAMPLING | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW; | ||
| : PublishResult.DROPPED_BUFFER_OVERFLOW_SAMPLED_OUT; |
There was a problem hiding this comment.
Users receive no warning when queue overflow drops spans that single-span sampling can keep.
Assertion details
- Input: Enable single-span sampling, submit a trace with SAMPLER_DROP or USER_DROP, and fill the single-span sampling queue.
- Expected:
A full single-span sampling queue must return DROPPED_BUFFER_OVERFLOW. The warning must remain because single-span sampling can keep spans from this trace. - Actual:
Both prioritization strategies return DROPPED_BUFFER_OVERFLOW_SAMPLED_OUT when the single-span sampling queue is full. RemoteWriter then writes only a debug log.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| rlLog.warn( | ||
| "Dropped a kept trace due to a buffer overflow: [{} spans]." | ||
| + " Traces are being produced faster than they can be sent to the agent;" | ||
| + " consider raising dd.writer.queue.size", |
There was a problem hiding this comment.
Users can set this property and still lose kept traces because the queue size does not change.
Assertion details
- Input: A kept trace reaches a full writer queue and causes the warning.
- Expected:
The warning must name a supported action or omit the setting. - Actual:
The warning tells users to raise dd.writer.queue.size, but the tracer does not read this setting.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
1 participant
What Does This Do
Splits the trace-writer buffer-overflow log so that only genuine data loss reaches the user as a warning.
RemoteWriterloggedDropped due to a buffer overflow: [N spans]at WARN for every overflow, regardless of whether the lost trace was one the tracer intended to send.Prioritizationroutes sampled-out traces (SAMPLER_DROP / USER_DROP) to a separate queue from kept traces, so it already knows which case occurred — but that distinction was discarded by the time it reached the log site.DROPPED_BUFFER_OVERFLOW— a kept trace was lost. Still WARN, and the message now namesdd.writer.queue.sizeso it is actionable.DROPPED_BUFFER_OVERFLOW_SAMPLED_OUT(new) — an already sampled-out trace was lost. Now DEBUG.Health metrics and drop accounting are unchanged: both cases still go through
handleDroppedTrace, sosamplerDropDroppedTraces/samplerKeepDroppedTracescontinue to report exactly as before.Motivation
This warning has generated repeated support tickets and escalations for what is usually a non-event. Investigating one of them took five tracer flares to establish that no trace data was being lost at all — the overflow was landing entirely on traffic that trace sampling had already discarded.
The signal that distinguishes the two cases (
samplerKeepDroppedTracesbeing zero or not) is not discoverable from the log line, which prints only a span count. Users reasonably read "Dropped" as "you are losing traces," and the WARN level endorses that reading.Two details worth noting for review:
Classification happens at the source, not at the log site. Re-deriving severity from the sampling priority in
RemoteWriterwould misclassify force-keep traces:FastLaneStrategy.publishsends them to the primary queue regardless of their priority byte, so losing one is real data loss even though its priority reads as drop. Returning the distinction frompublishavoids that.The rate limiter has one shared budget.
RatelimitedLoggerholds a singlenextLogNanosper instance. Had both cases stayed on the warning path, a flood of benign overflow could consume the budget and suppress the warning that matters for up to a minute — inverting the priority. Routing the benign case tolog.debugavoids needing a second limiter. There is a test for this.Trade-off
A service with a genuinely undersized queue under heavy sampled-out load loses an early warning signal. That seems right: the health metrics still carry it, and the current behaviour costs real support time for a non-event. But "no warnings" will no longer imply "queue is comfortably sized."
Additional Notes
The message text change means anyone alerting on the exact old string will need to update. Given the string was the source of the confusion, that seems acceptable, but flagging it.
Contributor Checklist
./gradlew spotlessApply:dd-trace-core:test --tests "datadog.trace.common.writer.*"passesJira ticket: APMS-20414
🤖 Generated with Claude Code