feat(api, worker, application-generic): Add exhaustive error handling for bridge requests #6715
Conversation
| raw = { | ||
| url: statelessBridgeUrl, | ||
| statusCode: error.response?.statusCode, | ||
| message: error.response?.statusMessage, | ||
| ...(error.response?.retryCount ? { retryCount: error.response?.retryCount } : {}), | ||
| ...(error?.response?.body?.length > 0 ? { raw: JSON.parse(error?.response?.body) } : {}), | ||
| ...(error?.response?.body?.length > 0 ? { raw: rawMessage } : {}), |
There was a problem hiding this comment.
I believe this lack of error handling during body parsing was contributing to execution detail logs not surfacing due to an inability to parse the error body. This was observed with errors such as "BSON circular value cannot be converted..."
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| environmentBridgeUrl, | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Throw an exception here explicitly if trying to request to an external Bridge URL when it doesn't exist on the environment.
| } catch (error) { | ||
| if (error instanceof HttpException) { | ||
| throw new BadRequestException(error.message); | ||
| } |
There was a problem hiding this comment.
Simplifying the error thrown here.
Let non-HTTP exceptions bubble up to the API exception handler.
| GetDecryptedSecretKeyCommand.create({ | ||
| environmentId: command.environmentId, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Refactoring many of these to discrete methods to make the execute method easier to read.
| @@ -152,75 +147,38 @@ export class ExecuteBridgeRequest { | |||
| headers, | |||
| }).json(); | |||
| } catch (error) { | |||
| if (error instanceof RequestError) { | |||
There was a problem hiding this comment.
Moved to a discrete handler for errors.
| @@ -272,4 +241,135 @@ export class ExecuteBridgeRequest { | |||
|
|
|||
| return apiUrl; | |||
| } | |||
|
|
|||
| private handleResponseError(error: unknown, url: string) { | |||
There was a problem hiding this comment.
Exhaustive error handling added for all of:
gotpackage error codes (it does a good job of handling these with explicit classes per error type- All Node.js error code (ref)
There was a problem hiding this comment.
All thrown error messages now include the URL to support client debugging.
| } from '@nestjs/common'; | ||
| import got, { OptionsOfTextResponseBody, RequestError } from 'got'; | ||
| import got, { |
There was a problem hiding this comment.
In the era of fetch, do we need to rely on got? Got by default gives us retries which I am not very sure if it's a great idea.
There was a problem hiding this comment.
We will tackle this offline.
There was a problem hiding this comment.
Elaborating a little, is the thinking that we should handle retries elsewhere? At the queue layer, for example?
I tend to agree, the value we're getting from retries in practice is minimal, especially considering the total retry duration after incorporating backoff is less than 1 minute. It's unlikely an external bridge endpoint would recover from a total network fault within that timespan.
libs/application-generic/src/usecases/execute-bridge-request/execute-bridge-request.usecase.ts
Outdated
Show resolved
Hide resolved
rifont
deleted the
nv-4502-add-exhaustive-error-handling-for-bridge-requests
branch
What changed? Why was the change needed?
Add exhaustive error handling during Bridge requests for all of:
gotpackage error codes (it does a good job of handling these with explicit classes per error typeScreenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer