Skip to content

feat(WO-1143): add SpanKind to SpanOpen and Onset STW events#6471

Open
repository wants to merge 2 commits intomainfrom
lngo/spankind-1
Open

feat(WO-1143): add SpanKind to SpanOpen and Onset STW events#6471
repository wants to merge 2 commits intomainfrom
lngo/spankind-1

Conversation

@repository
Copy link
Copy Markdown
Member

@repository repository commented Mar 31, 2026

We currently determine SpanKind in our streaming tail worker based on the names/attributes of the spans, but we'd like this kind of determination to be made directly in the runtime when spans are crated.

This adds the field SpanKind to the Onset and SpanOpen for the STW, which can be "one of Client, Server, Internal, Producer, or Consumer" as per OTel spec.

If not specified, spans created via UserSpanObserver::reportStart will default to Internal.

This is the first of three changes. Please also see:

Exposing SpanKind on Onset to JS, and determining it by the onset info type
#6476

Exposing SpanKind on SpanOpen to JS, adding methods to set it, and setting it at various call sites
#6477

Internal MR:
https://gitlab.cfdata.org/cloudflare/ew/edgeworker/-/merge_requests/12863

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm Bonk, and I've done a quick review of your PR.

This PR adds SpanKind (matching the OTel spec) to SpanOpen, Onset, and related data structures, threading it through serialization and the span submission/reporting interfaces.

Findings (ranked by severity):

  1. [MEDIUM] Tests don't verify spanKind round-trips through serialization/clone. The existing Read/Write SpanOpen works and Read/Write Onset works tests (src/workerd/io/trace-test.c++) only exercise the default SpanKind::INTERNAL and never assert the field value. A serialization bug (e.g. copyTo or the reader constructor silently dropping the field) would not be caught. I'd recommend adding test cases that construct SpanOpen and Onset with a non-default SpanKind (e.g., CLIENT or SERVER) and assert it survives:

    • copyTo → reader → constructor round-trip
    • clone()

    For example, for SpanOpen:

    KJ_TEST("Read/Write SpanOpen preserves SpanKind") {
      capnp::MallocMessageBuilder builder;
      auto infoBuilder = builder.initRoot<rpc::Trace::SpanOpen>();
    
      SpanOpen info(0x2a2a2a2a2a2a2a2a, "foo"_kjc, SpanKind::CLIENT);
      KJ_ASSERT(info.spanKind == SpanKind::CLIENT);
      info.copyTo(infoBuilder);
    
      auto reader = infoBuilder.asReader();
      SpanOpen info2(reader);
      KJ_ASSERT(info2.spanKind == SpanKind::CLIENT);
    
      SpanOpen info3 = info.clone();
      KJ_ASSERT(info3.spanKind == SpanKind::CLIENT);
    }

    And similarly for Onset with SpanKind::SERVER.

  2. [LOW] CompleteSpan has spanKind field but UserSpanData capnp does not. This is fine because CompleteSpan/UserSpanData is legacy (marked obsolete26) and the CompleteSpan is decomposed into SpanOpenData + SpanEndData before transmission. Just noting for awareness — no action needed.

Everything else looks correct: cap'n proto changes are purely additive (wire-compatible), serialization/deserialization is consistent across all paths, defaults are sensible, and the SpanKind enum values match the OTel spec.


This review was generated by an AI assistant and may contain inaccuracies.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 31, 2026

Review posted successfully on PR #6471. The main finding is a medium-severity gap in test coverage -- the existing serialization round-trip tests for SpanOpen and Onset don't verify that spanKind survives serialization/deserialization/clone, since they only use the default value. I provided concrete test code suggestions. The rest of the PR looks correct: cap'n proto changes are wire-compatible, all serialization paths are consistent, and the enum values match the OTel spec.

github run

@repository repository marked this pull request as ready for review April 1, 2026 15:25
@repository repository requested review from a team as code owners April 1, 2026 15:25
@repository repository changed the title feat(WO-1143): add SpanKind to SpanOpen and Outcome STW events feat(WO-1143): add SpanKind to SpanOpen and Onset STW events Apr 1, 2026
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.

1 participant