[o11y] Additional hardening against missing outcome event, STW optimizations #5527

fhanau · 2025-11-13T23:23:45Z

Introduce a new WorkerTracer parameter indicating whether any BTWs are
present. In a follow-up, this will be used to optimize memory management,
but for now it helps us assert that if we have a tracer with logLevel none,
we have BTWs (otherwise the tracer would be redundant, indicating waste).
Log an error if a WorkerTracer is destructed without getting the Outcome event
even when logLevel == none

src/workerd/io/tracer.h

fhanau · 2025-11-20T03:25:24Z

src/workerd/io/tracer.c++

-  auto& tailStreamWriter = KJ_UNWRAP_OR_RETURN(maybeTailStreamWriter);
+  // This is where we'll actually encode the span. This function should never be invoked if STW is
+  // inactive as span tracing is only used in STW.
+  auto& tailStreamWriter = KJ_ASSERT_NONNULL(maybeTailStreamWriter);


this is intended as additional hardening, using KJ_UNWRAP_OR_RETURN could cause us to miss out on span tracing being set up when it is not being used – I'm highly confident that we're already avoiding this though.

fhanau · 2025-11-20T03:26:02Z

src/workerd/io/tracer.c++


  KJ_IF_SOME(writer, maybeTailStreamWriter) {
-    auto& spanContext = KJ_UNWRAP_OR_RETURN(topLevelInvocationSpanContext);
+    auto& spanContext = KJ_ASSERT_NONNULL(topLevelInvocationSpanContext);


This serves as additional hardening too, I'm very confident that we always provide the Onset event when doing tracing.

fhanau · 2025-11-20T03:26:28Z

Downstream PR is still WIP, but this is ready for review.

github-actions · 2025-11-24T02:17:38Z

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

mar-cf

There are changes here that shouldn't be bundled into the same commit.

hasBufferedTailWorkers flag (currently buffered). Used for an optimization to skip buffering when no BTWs exist, and to confirm efficiently creating tracers with valid consumers.
Onset detection + stricter assertions. How do the stricter assertion manifest? If we detect a tracing misconfiguration, we can report that but still function degraded.

It's not from this change, but its not clear that topLevelInvocationSpanContext is being used for two purposes, one of which is as an implicit flag if the onset was reported.

src/workerd/io/tracer.h

src/workerd/io/tracer.c++

src/workerd/io/tracer.h

mar-cf · 2025-12-02T09:03:04Z

src/workerd/io/tracer.c++

+    // WorkerInterfaces and then ends up not using it due to an error/incorrect parameters; such
+    // error checking should be done beforehand to avoid unused allocations). Report such cases.
+    LOG_ERROR_PERIODICALLY(
+        "destructed WorkerTracer with STW without reporting Onset event", kj::getStackTrace());


this is now "destructed WorkerTracer without reporting Onset event"

good catch, will update this

jasnell · 2025-12-02T16:52:49Z

src/workerd/io/tracer.h

-      kj::Maybe<kj::Own<tracing::TailStreamWriter>> maybeTailStreamWriter);
-  explicit WorkerTracer(PipelineLogLevel pipelineLogLevel, ExecutionModel executionModel);
+      kj::Maybe<kj::Own<tracing::TailStreamWriter>> maybeTailStreamWriter,
+      bool buffered);


Nit: not a fan of bool arguments. Can this be a strong-bool or an enum?

Please do this.

fhanau · 2025-12-02T17:26:13Z

There are changes here that shouldn't be bundled into the same commit.

hasBufferedTailWorkers flag (currently buffered). Used for an optimization to skip buffering when no BTWs exist, and to confirm efficiently creating tracers with valid consumers.

Onset detection + stricter assertions. How do the stricter assertion manifest? If we detect a tracing misconfiguration, we can report that but still function degraded.

It's not from this change, but its not clear that topLevelInvocationSpanContext is being used for two purposes, one of which is as an implicit flag if the onset was reported.

Here's the intent of this PR, why I think this PR contains the right amount of changes:
1.) We are not actually doing such an optimization in this PR (as you also mention in #5527 (comment)), that would be a larger change. However, we do use the buffered variable to perform optimizations: Not constructing a WorkerTracer in some cases, please see the downstream PR for that. Based on that, we are adding the variable and making use of it for optimizations in the same change.
2) Note that these assertions do not catch a misconfiguration in which we could still provide some valid trace data, but serve to verify that the optimization that we use the buffered variable for is being applied correctly. If an assertion is violated, that indicates that we constructed a WorkerTracer or tail stream writer that will be wholly unused:
If logLevel == None, we do not perform any STW tracing. Therefore, any tail stream writer being present would be unused, and if we have buffered == false we won't have BTW tracing either and won't be doing tracing at all – we should have not constructed a WorkerTracer in the first place.

As for changes that are not needed here, I think we can get rid of hasBuffered() and the buffered class variable – at this stage, it is only used in the constructor. I'll update the PR accordingly.

codspeed-hq · 2025-12-02T19:57:49Z

CodSpeed Performance Report

Merging #5527 will not alter performance

_{Comparing felix/110925-stw-2 (60ea4d1) with main (04fb23c)}

Summary

✅ 140 untouched
⏩ 38 skipped¹

38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

mar-cf · 2025-12-05T17:04:04Z

Here's the intent of this PR, why I think this PR contains the right amount of changes:

My point was this PR looks to be two mostly independent changes in one. See the comment for how I would split this up.

we are adding the variable

It should be possible to do without adding this with a query that will check parents. I'm not extremely opposed to adding it, but maybe you could (or already have) consider it and provide a reason why this is better?

…zations - Introduce a new WorkerTracer parameter indicating whether any BTWs are present. In a follow-up, this will be used to optimize memory management, but for now it helps us assert that if we have a tracer with logLevel none, we have BTWs (otherwise the tracer would be redundant, indicating waste). - Log an error if a WorkerTracer is destructed without getting the Outcome event even when logLevel == none

fhanau added the observability label Nov 13, 2025

fhanau mentioned this pull request Nov 17, 2025

[o11y] Make STW unused tracer warning log periodically for now #5529

Merged

fhanau force-pushed the felix/110925-stw-2 branch from d8a2a9c to 3d6898e Compare November 20, 2025 03:21

fhanau changed the title ~~[DRAFT][o11y] Additional hardening against missing outcome event, STW optimizations~~ [o11y] Additional hardening against missing outcome event, STW optimizations Nov 20, 2025

fhanau marked this pull request as ready for review November 20, 2025 03:22

fhanau requested review from a team as code owners November 20, 2025 03:22

fhanau commented Nov 20, 2025

View reviewed changes

src/workerd/io/tracer.h Outdated Show resolved Hide resolved

fhanau commented Nov 20, 2025

View reviewed changes

fhanau requested a review from mar-cf November 20, 2025 03:26

fhanau force-pushed the felix/110925-stw-2 branch from 3d6898e to 5dc1403 Compare November 24, 2025 02:06

fhanau force-pushed the felix/110925-stw-2 branch from 5dc1403 to b329282 Compare November 24, 2025 02:20

mar-cf reviewed Dec 2, 2025

View reviewed changes

jasnell reviewed Dec 2, 2025

View reviewed changes

fhanau force-pushed the felix/110925-stw-2 branch from b329282 to baef58c Compare December 2, 2025 19:31

fhanau force-pushed the felix/110925-stw-2 branch 2 times, most recently from 2d6e151 to bda6a55 Compare December 11, 2025 16:43

fhanau force-pushed the felix/110925-stw-2 branch from bda6a55 to 60ea4d1 Compare December 30, 2025 04:11

[o11y] Additional hardening against missing outcome event, STW optimizations #5527

Are you sure you want to change the base?

[o11y] Additional hardening against missing outcome event, STW optimizations #5527

Conversation

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

Uh oh!

Uh oh!

Uh oh!

fhanau Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

fhanau Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

fhanau commented Nov 20, 2025

Uh oh!

github-actions bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mar-cf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mar-cf Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

fhanau Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

jasnell Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

mar-cf Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

fhanau commented Dec 2, 2025

Uh oh!

codspeed-hq bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #5527 will not alter performance

Summary

Footnotes

Uh oh!

mar-cf commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fhanau commented Nov 13, 2025 •

edited

Loading

github-actions bot commented Nov 24, 2025 •

edited

Loading

codspeed-hq bot commented Dec 2, 2025 •

edited

Loading

mar-cf commented Dec 5, 2025 •

edited

Loading