Use structured error types in frontend RPC service #6570
Conversation
bduffany
changed the title
bduffany
force-pushed
the
rpc-structured-errors
branch
2 times, most recently
from
7d2cdab
to
c5bb892
Compare
bduffany
force-pushed
the
rpc-structured-errors
branch
6 times, most recently
from
5632e7b
to
a7071c3
Compare
| } catch (e) { | ||
| message = `unknown (failed to read response body: ${e})`; | ||
| if (structuredErrors) { | ||
| // If we failed to read the response body then ignore the status code |
There was a problem hiding this comment.
Not sure how I feel about this precedence--I guess we still have the network tab to tell the difference, I just figure if a 500 error yielded an unparseable response then saying "500" is more useful than "failed to read".
There was a problem hiding this comment.
I was thinking that this approach would play more nicely with retry middleware (on the frontend) if we return a generic network error here. That way if we return an error status but fail to read the response body, the client can at least retry for a chance to get a more informative error message that could maybe help them fix the error. I guess if the client got permanently disconnected though, they will be stuck with the "failed to read" error, but I think that's not too much worse than 500: <failed to read body>
|
|
||
| /** | ||
| * High-level error class representing any error from the BuildBuddy server, | ||
| * either GRPC or HTTP. |
There was a problem hiding this comment.
The class hierarchy is a little weird here based on the documentation--do you actually intend to flip the code on its head at some point (HTTPStatusError extends BBError) or more cleanly compose things so that HTTPStatusError/FetchError etc are held by BBError?
There was a problem hiding this comment.
Yeah, thanks for pointing this out - I think what I would like to do is:
- Get rid of
BuildBuddyErrorentirely - it's sort of poorly defined - In the places where we're doing stuff like
catch (e) { error_service.handleError(BuildBuddyError.parse(e)); }we would just passeinstead of trying to parse the error. It's kind of ugly that we "parse" errors at all and I would rather have the errors be nicely structured at a lower level to begin with. - The error_service could then do nicer stuff depending on the type of error that it's rendering, rather than receiving a generic
BuildBuddyErrorwhich is more "watered down". e.g. forFetchErrorwe could show a nice "network broken" icon or something like that (if we really wanted to, haha)
There was a problem hiding this comment.
Sure, that works, how about plopping something in the comment here about what the plan is?
|
Gah I accidentally merged #6578 into this PR. Fixing up the commits now... |
bduffany
force-pushed
the
rpc-structured-errors
branch
from
7982a4d
to
73c7e7d
Compare
| } catch (e) { | ||
| message = `unknown (failed to read response body: ${e})`; | ||
| if (structuredErrors) { | ||
| // If we failed to read the response body then ignore the status code |
|
|
||
| /** | ||
| * High-level error class representing any error from the BuildBuddy server, | ||
| * either GRPC or HTTP. |
There was a problem hiding this comment.
Sure, that works, how about plopping something in the comment here about what the plan is?
Done |
(This is all behind the
streaming_http_enabledflag, which is only enabled in dev currently.)This PR is preparing for making server-streaming RPCs retryable on the client. Ideally we'd always retry these, otherwise when the apps roll out, the live-streaming would suddenly stop.
We currently throw a somewhat haphazard mixture of
stringandTypeErrorfrom the RPC service methods when a fetch fails. This PR updates things to be more structured, so that we can more easily distinguish between gRPC status errors, other HTTP errors (with status code + body, but missing gRPC error message), or generic network errors (e.g. can't reach the app, app restarted, etc.).Related issues: N/A