[mar-cf]

WorkerTracer::addSpanUpdate() reads the time from the IoContext, which a tracer driven over RPC doesn't have, so every update it received failed with "expected weakIoContext != nullptr". Add addSpanUpdateInternal(), which takes the timestamp from the caller, like addSpanAttributeInternal().

[clanky-the-clanker]

Superseded by the review of 831af142.

This review, of 1f5cdacf

💬 Approved with comments — A small refactor that copies an existing pattern and keeps current behaviour. The only note is an INFO: the new method has no caller or test in this repository.

Findings: 1 info

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

📋 Code Quality: 💬 1 info

The change copies the addSpanAttribute/addSpanAttributeInternal split correctly. Its one note is that the new method has no caller or test in this repository.

Recommendation: 💬 Optional Nits

• [INFO] No code in this repository calls addSpanUpdateInternal() or tests it, so the fix for the RPC path depends on a caller outside this tree, as addSpanAttributeInternal() already does. A test of that path with an explicit timestamp would guard the fix. (src/workerd/io/tracer.h:175)

View full review

• addSpanUpdate() behaves as before. The pipelineLogLevel == NONE || maybeTailStreamWriter == kj::none early return runs before getTime(), exactly as in addSpanAttribute().
• No code in this repository calls addSpanUpdateInternal() (src/workerd/io/tracer.h:175) yet. The in-tree span submitter (src/workerd/server/factory/worker-factory.c++:521-524) still calls the virtual addSpanUpdate(), which is right for a tracer that runs inside an IoContext. The RPC-driven tracer from the description must therefore call the new method from code outside this tree, as it already does for addSpanAttributeInternal(). Wherever that caller lives, a test that sends a span update with an explicit timestamp over that path would guard the fix.

---

✅ All other checks passed

📝 Documentation: ✅ LGTM

The new header comment matches the one on addSpanAttributeInternal() and describes the method accurately.

---

_{Reviewed commit: 1f5cdacf}

[clanky-the-clanker]

💬 Approved with comments — A small fix that follows an existing pattern. What is left is one INFO (no caller or test for the new method in this repository) and one SUGGESTION about calling getTime() before the log-level check.

Findings: 1 info · 1 suggestion

_{The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.}

---

📋 Code Quality: 💬 1 info · 1 suggestion

The fix is correct. Two optional notes: no test or in-tree caller for the new method yet, and the log-level check now runs after getTime().

Recommendation: 💬 Optional Nits

• [INFO] No code in this repository calls or tests addSpanUpdateInternal(), so the RPC fix depends on a caller outside this tree, as addSpanAttributeInternal() already does. A test with an explicit timestamp would guard it. (src/workerd/io/tracer.h:175)
• [SUGGESTION] addSpanUpdate() now calls getTime() before the pipelineLogLevel == NONE check inside addSpanUpdateInternal(). At log level NONE with a writer present, getTime() runs for nothing and can warn, or assert in test mode, if the IoContext is no longer valid. addSpanAttribute() checks both conditions first. Consider restoring the check in the wrapper for consistency. (src/workerd/io/tracer.c++:279)

View full review

• src/workerd/io/tracer.h:175 (INFO, carried over). Nothing in this repository calls addSpanUpdateInternal() or tests it, so the RPC fix depends on a caller outside this tree. addSpanAttributeInternal() is in the same position. A test that sends a span update with an explicit timestamp would guard this path.

• src/workerd/io/tracer.c++:278-283 (SUGGESTION). @​dom96 pointed out that addSpanUpdateInternal() already checks pipelineLogLevel, and the check was removed from addSpanUpdate(). It is not entirely redundant, though, because the wrapper calls getTime() before the internal check runs. So when pipelineLogLevel == NONE and a tail stream writer exists, getTime() now runs and its result is thrown away. getTime() does KJ_ASSERT_NONNULL(weakIoContext). If the context is no longer valid and completeTime is unset, it logs a periodic warning, or fails an assertion in predictable test mode (tracer.c++:515-534). Before this change, that path returned early. addSpanAttribute() (tracer.c++:629) still checks both conditions before getTime(). The impact is low, since a NONE log level with a writer present is unusual. Restoring pipelineLogLevel == PipelineLogLevel::NONE || in the wrapper would keep the two methods consistent.

---

✅ All other checks passed

📝 Documentation: ✅ LGTM

The new header comment matches the one on addSpanAttributeInternal() and describes the method accurately.

---

_{Reviewed commit: 831af142}