Skip to content

Use new "response stream disconnected" status for disconnections during proxying#2503

Merged
jclee merged 2 commits intomainfrom
jlee/response-stream-disconnected
Aug 15, 2024
Merged

Use new "response stream disconnected" status for disconnections during proxying#2503
jclee merged 2 commits intomainfrom
jlee/response-stream-disconnected

Conversation

@jclee
Copy link
Copy Markdown
Contributor

@jclee jclee commented Aug 9, 2024

Add new responseStreamDisconnected request status, to be used when an exception is thrown due to disconnection during deferred proxying, which is common when the client or server hangs up early, particularly when proxying websockets. Should allow us to differentiate these cases from those where JS is still running, such that a thrown exception could still be caught and handled by the worker.

Also, add instrumentation to report failure during deferred proxying.

@jclee jclee requested review from a team as code owners August 9, 2024 07:55
Copy link
Copy Markdown
Contributor

@justin-mp justin-mp left a comment

Choose a reason for hiding this comment

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

The code changes look good to me (in conjunction to the internal PRs to the schema as needed by outcome.capnp), but I'm surprised that there's no test change for workerd. Is this because RequestObserver::reportFailure in workerd is an empty implementation, and, as such, doesn't change with this code?

@jclee jclee force-pushed the jlee/response-stream-disconnected branch from 906b7f5 to 1f27176 Compare August 13, 2024 22:12
@jclee
Copy link
Copy Markdown
Contributor Author

jclee commented Aug 14, 2024

The code changes look good to me (in conjunction to the internal PRs to the schema as needed by outcome.capnp), but I'm surprised that there's no test change for workerd. Is this because RequestObserver::reportFailure in workerd is an empty implementation, and, as such, doesn't change with this code?

Yeah, as far as I can tell, we don't really have test coverage in workerd for RequestObserver method invocation, yet (and similarly for a few other interfaces that only have internal implementations).

Most workerd tests seem to be .wd-test integration tests that don't yet have a way to introspect RequestObserver calls. It might be possible to add some sort of test-only API to allow .wd-tests to detect the calls -- or to add unit or integration tests in *-test.c++ files? But for this task, I ended up just augmenting the existing internal integration tests, which were already instrumented detect the behavior changes.

jclee added 2 commits August 15, 2024 11:20
...to be used when an exception is thrown due to disconnection during deferred
proxying, which is common when the client or server hangs up early,
particularly when proxying websockets.  Should allow us to differentiate these
cases from those where JS is still running, such that a thrown exception could
still be caught and handled by the worker.

Also, add instrumentation to report failure during deferred proxying.
@jclee jclee force-pushed the jlee/response-stream-disconnected branch from 1f27176 to 3fc25de Compare August 15, 2024 19:34
@jclee jclee merged commit 7aa2d5f into main Aug 15, 2024
@jclee jclee deleted the jlee/response-stream-disconnected branch August 15, 2024 22:02
ToriLindsay added a commit to cloudflare/cloudflare-docs that referenced this pull request Feb 24, 2025
* [worker] Document response stream disconnected error

This change documents the `responseStreamDisconnected` error `outcome`
seen in CloudFlare worker logs and error metrics.

Previously, this outcome doesn't appear to be documented, but presents
as an error in metrics and presents as a non-error entry in the
CloudFlare Worker Logs. This change aims to reduce confusion going forward.

Changes introducing `responseStreamDisconnected`: cloudflare/workerd#2503

* Update src/content/docs/workers/observability/errors.mdx

* Moved content under metrics heading

---------

Co-authored-by: ToriLindsay <tgalatro@cloudflare.com>
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