Skip to content

Propagate worker user span context#6515

Merged
mar-cf merged 1 commit intomainfrom
mar/worker-context-2
Apr 22, 2026
Merged

Propagate worker user span context#6515
mar-cf merged 1 commit intomainfrom
mar/worker-context-2

Conversation

@mar-cf
Copy link
Copy Markdown
Contributor

@mar-cf mar-cf commented Apr 7, 2026

No description provided.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 7, 2026

After thorough review, I've checked:

  1. Autogate: Correctly added with enum + string mapping + guarding usage. Follows conventions.
  2. SubrequestMetadata: New userSpanContext field added, all call sites updated consistently.
  3. getUserSpanContext: Autogate-gated, kj::downcast<UserSpanObserver> is safe (only one SpanObserver subclass in codebase, guarded by KJ_IF_SOME so only entered when observer exists).
  4. SpanContext::clone refactoring: Changed from member to static — no remaining callers of the old form. TailEvent::clone() constructs SpanContext directly, doesn't use SpanContext::clone().
  5. TraceContext::getUserSpanParent(): New accessor, consistent with existing getInternalSpanParent().
  6. Onset parentSpanId: Now correctly populated from trigger context's spanId.
  7. server.c++ triggerContext construction: Correctly handles Maybe<SpanId> from SpanContext::getSpanId(), constructs trigger InvocationSpanContext which propagates traceId to child.
  8. UserSpanObserver::getSpanId(): Simple accessor for existing field, fine.
  9. Test rewrite: Well-structured tree-based comparison replacing flat array. Correctly handles both autogate on/off variants. The buildTree logic properly scopes by traceId to avoid false matches.
  10. Memory/thread safety: No new ownership issues. SpanContext is value type, safely passed through SubrequestMetadata. No cross-thread concerns.
  11. Backward compat: Gated behind autogate, so no behavior change without explicit opt-in.

Everything looks clean and well-structured. No actionable issues found.

LGTM

github run

@jasnell
Copy link
Copy Markdown
Collaborator

jasnell commented Apr 7, 2026

this will need to be rebased on main for CI to pass.

@mar-cf mar-cf force-pushed the mar/worker-context-2 branch from 628fde6 to e513522 Compare April 8, 2026 20:16
@mar-cf mar-cf marked this pull request as ready for review April 8, 2026 21:09
@mar-cf mar-cf requested review from a team as code owners April 8, 2026 21:09
@mar-cf
Copy link
Copy Markdown
Contributor Author

mar-cf commented Apr 9, 2026

Confirmed with https://github.com/danlapid/stw-local-dev

Will post when I exercise all the different paths

@mar-cf mar-cf force-pushed the mar/worker-context-2 branch from e513522 to 6a30e5c Compare April 9, 2026 20:52
Comment thread src/workerd/api/actor-state.c++ Outdated
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 14, 2026

I've completed a thorough review of all 15 changed files. Let me summarize my findings:

Autogate: Correctly registered with enum + string mapping in sync. All behavioral changes are gated behind USER_SPAN_CONTEXT_PROPAGATION.

Backward compatibility: Fully gated behind autogate. When disabled, traceId is nullptr, so toSpanContext() returns kj::none, and no trigger context is created - preserving existing behavior exactly.

Memory/thread safety:

  • TraceId is trivially copyable (defaulted copy ctor), so lvalue copies in toSpanContext() and newChild() are safe and don't zero the source field.
  • SpanContext is move-only but the new static clone() correctly constructs a new one from const ref.
  • No cross-thread concerns - all changes stay within the same request flow.

API design: The SpanParent::toSpanContext() virtual dispatch through SpanObserver is clean. The interface mirrors the existing getInternalSpanParent() / getUserSpanParent() pattern.

Test: Well-structured tree-based comparison. buildTree() correctly scopes by traceId to avoid false matches. Both autogate-on and autogate-off paths are tested via the @ and @all-autogates variants. The unsafe_module flag addition is needed for workerd:unsafe import.

danlapid's review comment: Was on an older revision; the current code already uses .userSpanParent = tracing.getUserSpanParent() as suggested.

No actionable issues found.

LGTM

github run

@mar-cf
Copy link
Copy Markdown
Contributor Author

mar-cf commented Apr 14, 2026

Using only public repos, this and https://github.com/danlapid/stw-local-dev:
Screenshot from 2026-04-14 04-43-33

@mar-cf mar-cf force-pushed the mar/worker-context-2 branch from a4c6843 to 6d81884 Compare April 14, 2026 16:44
Comment thread src/workerd/io/trace-stream.c++
@mar-cf mar-cf force-pushed the mar/worker-context-2 branch from 6d81884 to 0221517 Compare April 22, 2026 01:09
@mar-cf mar-cf merged commit 764b4e8 into main Apr 22, 2026
35 of 36 checks passed
@mar-cf mar-cf deleted the mar/worker-context-2 branch April 22, 2026 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants