fix(types): add streamDiagEvent to TailStream.TailEventHandlerObject by PedroRosalba · Pull Request #7848 · cloudflare/workerd · GitHub
Skip to content

fix(types): add streamDiagEvent to TailStream.TailEventHandlerObject - #7848

Open
PedroRosalba wants to merge 1 commit into
cloudflare:mainfrom
PedroRosalba:fix/tail-stream-diag-handler-key
Open

PedroRosalba wants to merge 1 commit into
cloudflare:mainfrom
PedroRosalba:fix/tail-stream-diag-handler-key

Conversation

@PedroRosalba

Copy link
Copy Markdown

Problem

TailStream.TailEventHandlerObject, the object form a tailStream() handler may return, has no key for StreamDiagnostic events, although StreamDiagnostic is a member of TailStream.EventType. The runtime dispatches those events to the handler-object key streamDiagEvent (getHandlerName in src/workerd/io/trace-stream.c++), so in typed code

return { streamDiagEvent(ev) { ... } }

fails with TS2353 ('streamDiagEvent' does not exist in type 'TailEventHandlerType') while workerd calls exactly that key with an event of type "streamDiagnostic" whose diagnostic is { diagnosticsType: "droppedEvents", count }. A typed object handler cannot observe dropped tail events. The function form of the handler was already correct through the EventType default.

Origin: 23c0d98 (#5637) added the StreamDiagnostic interface, the EventType member and the C++ dispatch key together. In that PR's review the Event suffix was dropped from the TS interface and the type string (StreamDiagnosticsEvent → StreamDiagnostic), but the C++ handler key kept it and the handler-object key was never added.

Fix

Declare streamDiagEvent?: TailEventHandler<StreamDiagnostic> in types/defines/trace.d.ts and the four snapshot files, with a type test. Additive, types only; the key name follows the shipping runtime.

Alternative

streamDiagEvent is the one handler key that does not equal its event type (every other key does). If you consider the object-form keys not yet public, the cleaner fix is a C++ plus types rename of the key to streamDiagnostic, as 91f6875 did for attributes. I did not do that here because it would change behaviour for any handler already using streamDiagEvent; happy to switch if you prefer.

Tests

  • bazel test //types:test/types/tail-stream-diag (new; asserts the key's handler type and that the handler parameter is TailEvent<StreamDiagnostic>; fails on the previous snapshot)
  • bazel test //src/workerd/api/tests:tail-worker-test@ (runtime unchanged)
  • just generate-types reproduces the snapshot edits byte for byte.
  • Verified on the built binary with a .wd-test that the object handler's streamDiagEvent is called with diagnosticsType === "droppedEvents" and that a streamDiagnostic key is never called.

Related: #7846 touches the same trace.d.ts (different members); the two apply in either order.

Tested on macOS (Apple Silicon) only.

The object form of a tailStream() handler had no key for StreamDiagnostic
events, although StreamDiagnostic is a member of TailStream.EventType. The
runtime dispatches those events to the handler-object key "streamDiagEvent"
(getHandlerName in src/workerd/io/trace-stream.c++), so in typed code

  return { streamDiagEvent(ev) { ... } }

failed to compile (TS2353: 'streamDiagEvent' does not exist in type
'TailEventHandlerType') while workerd calls exactly that key with an event
whose type is "streamDiagnostic" and whose diagnostic is
{ diagnosticsType: "droppedEvents", count }. A typed object handler could
not observe dropped tail events.

23c0d98 added the StreamDiagnostic interface, the EventType member and the
C++ dispatch key together but never added the handler-object key. Declare
streamDiagEvent?: TailEventHandler<StreamDiagnostic> in
types/defines/trace.d.ts and the generated snapshots. The key name follows
the runtime; it is not renamed here because that would change behaviour for
existing handlers.

Test: types/test/types/tail-stream-diag.ts (bazel test
//types:test/types/tail-stream-diag) asserts the key's handler type and that
the handler parameter is TailEvent<StreamDiagnostic>; it fails to compile
without the define change.
@github-actions

Copy link
Copy Markdown

@clanky-the-clanker

Copy link
Copy Markdown

This pull request was not reviewed automatically because its author does not have write access to this repository. A maintainer with write access can start a review by commenting clanky review (or clanker review or @clanky-the-clanker review), optionally followed by instructions (e.g. clanky review focus on the retry logic).

@PedroRosalba

Copy link
Copy Markdown
Author

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