Skip to content

[o11y] Fix tail worker test with unstable and incomplete outcome#6517

Merged
fhanau merged 1 commit intomainfrom
felix/040726-stw-fix-test
Apr 8, 2026
Merged

[o11y] Fix tail worker test with unstable and incomplete outcome#6517
fhanau merged 1 commit intomainfrom
felix/040726-stw-fix-test

Conversation

@fhanau
Copy link
Copy Markdown
Contributor

@fhanau fhanau commented Apr 7, 2026

images-api-instrumentation-test uses traceId as its span identification key, which does not uniquely identify spans (unlike spanId). This led to us missing out on spans reported under the same traceId, having span tags be assigned to the wrong span, and changed the test results when reporting span tags earlier, falsely creating the impression that #6051 would introduce span reporting bugs. This can be avoided by using the same tail handler that similar tests already use.

This test seriously discombobulated me, was really confusing while working on #6051. By fixing this we avoid future confusion.

images-api-instrumentation-test uses traceId as its span identification key,
which does not uniquely identify spans (unlike spanId). This led to us missing
out on spans reported under the same traceId, having span tags be assigned to
the wrong span, and changed the test results when reporting span tags earlier,
falsely creating the impression that #6051 would introduce span reporting bugs.
This can be avoided by using the same tail handler that similar tests already
use.
@fhanau fhanau requested a review from mar-cf April 7, 2026 21:15
@fhanau fhanau requested review from a team as code owners April 7, 2026 21:15
@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 7, 2026

The images test now follows the exact same pattern as d1. The change is straightforward and correct:

  1. Replaces a buggy custom tailStream handler that keyed spans by traceId (not unique per span) with the shared createTailStreamHandler that keys by invocationId#spanId (unique).
  2. Updates expectedSpans to include all the spans that were previously lost due to the traceId key collision.
  3. Removes the TODO comment about refactoring.

The new expected spans list is much larger because the shared handler correctly captures every span individually, whereas the old handler was silently overwriting spans that shared the same traceId.

This is a test-only change with no production code impact, no backward compatibility concerns, and no security implications. The fix is well-motivated by the PR description.

LGTM

github run

@fhanau fhanau merged commit 6a9e483 into main Apr 8, 2026
25 checks passed
@fhanau fhanau deleted the felix/040726-stw-fix-test branch April 8, 2026 15:01
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.

2 participants