[o11y] Avoid tail stream cancellation warning with empty handler#6560
Conversation
|
The logic is sound. The fix correctly addresses the gap where The change is minimal, correct, and well-commented. There are no backward compatibility concerns (this is fixing a spurious warning, not changing behavior), no security implications, and no API changes. The PR author notes they plan to add a regression test before merge. LGTM |
mar-cf
left a comment
There was a problem hiding this comment.
Let's get this in, but also verify if there's setup that can be avoided with this handler.
We were not marking the TailStreamTarget fulfiller as done if the RPC call with the outcome event did not result in any calls to the tail handler (which can happen if the tail handler is an empty object). This resulted in cumbersome "Streaming tail session canceled" warnings.
013e9a2 to
d8dfd8d
Compare
There certainly is – we could check if the given handler is empty (or more generally does not include a handler for any of the different event types). Before making that optimization, I'll implement some of the remaining functional improvements. |
Merging this PR will degrade performance by 12.83%
Performance Changes
Comparing 
Footnotes
|
We were not marking the TailStreamTarget fulfiller as done if the RPC call with the outcome event did not result in any calls to the tail handler (which can happen if the tail handler is an empty object). This resulted in cumbersome "Streaming tail session canceled" warnings.
Test: When making tailStream() in src/workerd/api/tests/tail-worker-test-invalid.js return {} instead of an invalid value, we no longer get cancellation warnings.