[o11y] Drop tail stream events when reaching excessive queue size #5637

fhanau · 2025-12-03T21:15:48Z

This will avoid excessive memory overhead in a few edge cases – usually the tail worker will be able to keep up with reporting events but we still need to put a limit to the queue size.

fhanau · 2025-12-03T22:03:17Z

src/workerd/io/tracer.c++

+      tracing::SpanOpen(span.spanId, span.operationName.clone()), span.startTime, spanNameSize);
  // If a span manages to exceed the size limit, truncate it by not providing span attributes.
-  if (span.tags.size() && messageSize <= MAX_TRACE_BYTES) {
+  if (span.tags.size() && spanTagsSize <= MAX_TRACE_BYTES) {


As the watchful reviewer will notice, this is a minor functional change: We're no longer including the size of the span name in this check since it is included in a different tail event, so we're slightly relaxing the size limit for emitting span tags here.

fhanau · 2025-12-03T22:04:19Z

src/workerd/io/trace-stream.c++

+              event.sequence,
+              tracing::Log(event.timestamp, LogLevel::WARN,
+                  kj::str(
+                      "Dropped ", active->droppedEvents, "tail events due to excessive queueing")),


Open to a different message here if requested.

Injecting a synthetic log seems deceptive. Adding droppedEventCount on the Outcome event seems more natural.

@jmorrell-cloudflare any thoughts here? I think adding a log makes more sense since we have precedent for that in BTW and since it is a lot harder to miss than just having a droppedEventCount, but if you disagree it's also easy to change.

fhanau · 2025-12-03T22:06:13Z

src/workerd/io/trace-stream.c++

 // The TailStreamWriterState holds the current client-side state for a collection
 // of streaming tail workers that a worker is reporting events to.
 struct TailStreamWriterState {
+  // The maximum size of the queue, in bytes.


Open to different names for these constants/defining them somewhere else.

Note that testing that these checks take effect at some point will be provided in a downstream PR (to follow). This will also include a discussion of why they are sufficient for almost all use cases.

fhanau · 2025-12-03T22:10:01Z

This is now feature-complete. As noted in a PR comment – tests for this and rationale will be provided in a downstream PR, but I think we can already discuss the merits of the code changes here.

fhanau · 2025-12-03T22:12:00Z

src/workerd/io/trace.h

  Event event;

+  // The approximate size of the event, in bytes.
+  size_t sizeHint;


We could probably restructure this to pass sizeHint as a parameter to reportImpl() instead of storing it in TailEvent directly? Might be cleaner.

fhanau · 2025-12-03T22:47:46Z

src/workerd/io/trace-stream.c++

        auto builder = KJ_ASSERT_NONNULL(current->capability).reportRequest();
        auto eventsBuilder = builder.initEvents(current->queue.size());
        size_t n = 0;
+        // KJ_LOG(WARNING, "queue: sending", current->queue.size(), "events", current->queueSize);


To be removed before merge – still using this while tuning test cases

mar-cf · 2025-12-10T13:58:04Z

Add tests

This will avoid excessive memory overhead in a few edge cases.

mar-cf

My biggest concern is if it's possible to produce inconsistent traces due to dropping.

mar-cf · 2025-12-17T15:33:57Z

src/workerd/io/trace-stream.h

+  // The maximum size of the queue, in bytes.
+  const size_t maxQueueSize = 2 * 1024 * 1024;
+  // The estimated overhead of TailEvent wrapping per message. This does not need to be very
+  // accurate, but should be enough to avoid allocating too much memory/hitting capnp RPC message
+  // size limits when sending many tiny events.
+  const size_t tailSerializationOverhead = 64;
+


Should these limits be configurable rather than hardcoded? Would help to have visibility into drop frequency to validate whether these values are appropriate.

Please see the internal PRs for rationale for having the current limits, for why I think the current approach generally won't result in events being dropped for real-world use cases. We can increase them later if needed

Add a warning log if this is ever hit, and/or coordinate with WOBS to expose a metric.

Please see the internal PRs for rationale for having the current limits

I'd prefer to see some type of empirical evidence, but that works.

mar-cf · 2025-12-17T15:39:54Z

src/workerd/io/tracer.c++

+    tailStreamWriter->report(spanComponentContext, kj::mv(attr), span.startTime, spanTagsSize);
  }
-  tailStreamWriter->report(spanComponentContext, tracing::SpanClose(), span.endTime);
+  tailStreamWriter->report(spanComponentContext, tracing::SpanClose(), span.endTime, 0);


If the queue crosses the limit mid-span, SpanOpen could be delivered without its SpanClose, leaving an unclosed span in the trace.

My rationale here was that the tail producer may drop any event type except for Onset/Return/Outcome, and that the tail consumer is expected to handle any kind of event being omitted – it may choose to report spans as truncated or omit them. I think as user tracing evolves, it will be hard to provide guarantees that specific events are always present without having to worry about excessive queue size in edge cases. SpanClose itself might be easier to support since its size is fixed, but we could end up with the opposite problem too: A SpanClose could be delivered without its SpanOpen if that was dropped earlier, which would result in a span being reported for which only the endTime is known.

mar-cf · 2025-12-17T15:40:33Z

src/workerd/io/trace-stream.c++

+      active->queueSize += tailSerializationOverhead + event.sizeHint;
    } else {
-      active->queue.push(event.clone());
+      active->droppedEvents++;


Consider reporting which event types were dropped rather than just a count.

Does that scale in case we keep adding more event types in the future? If we see a need for it in the future we can always add it later.

Seems like pretty light, I wouldn't be concerned.

fhanau force-pushed the felix/112625-stw-load-shed branch from 3a09407 to 7692d3e Compare December 3, 2025 22:01

fhanau commented Dec 3, 2025

View reviewed changes

fhanau marked this pull request as ready for review December 3, 2025 22:08

fhanau requested review from a team as code owners December 3, 2025 22:08

fhanau added the observability label Dec 3, 2025

fhanau requested a review from mar-cf December 3, 2025 22:08

fhanau commented Dec 3, 2025

View reviewed changes

fhanau force-pushed the felix/112625-stw-load-shed branch from 7692d3e to 70cfa7f Compare December 11, 2025 19:49

[o11y] Drop tail stream events when reaching excessive queue size

51d612b

This will avoid excessive memory overhead in a few edge cases.

fhanau force-pushed the felix/112625-stw-load-shed branch from 70cfa7f to 51d612b Compare December 16, 2025 02:25

mar-cf reviewed Dec 17, 2025

View reviewed changes

[o11y] Drop tail stream events when reaching excessive queue size #5637

Are you sure you want to change the base?

[o11y] Drop tail stream events when reaching excessive queue size #5637

Conversation

fhanau commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhanau Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhanau commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mar-cf commented Dec 10, 2025

Uh oh!

mar-cf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fhanau commented Dec 3, 2025 •

edited

Loading

fhanau Dec 3, 2025 •

edited

Loading