feat(nestjs): Filter RPC exceptions #13227
Conversation
size-limit report 📦
|
nicohrubec
requested review from
a team,
mydea and
lforst
and removed request for
a team
| const status_code = (exception as ExpectedException).status; | ||
| const error_property = (exception as ExpectedException).error; | ||
|
|
||
| // don't report expected errors | ||
| if (status_code !== undefined) { | ||
| if (status_code !== undefined || error_property !== undefined) { | ||
| return originalCatch.apply(target, [exception, host]); | ||
| } | ||
|
|
There was a problem hiding this comment.
h: Let's go for something like the following here:
const exceptionIsObject = typeof exception === 'object' && exception !== null;
const exceptionStatusCode = exceptionIsObject && 'status' in exception ? exception.status : null;
const exceptionErrorProperty = exceptionIsObject && 'error' in exception ? exception.error : null;
// Only report expected errors
// - ExpectedNestJS control flow errors that with status codes like NestJS' `HttpException` errors will have a `status` property.
// - NestJS' expected `RpcExceptions` will have an `error` property
if (exceptionStatusCode === null && exceptionErrorProperty === null) {
captureException(exception, {
mechanism: {
handled: false,
},
});
}
return originalCatch.apply(target, [exception, host]);There was a problem hiding this comment.
Why? It's not immediately obvious to me how that improves the current implementation
There was a problem hiding this comment.
The current implementation will throw if the error is nullish, which can always happen. We should avoid doing any type casts (as) as much as possible. The change I suggested does runtime checks not to throw when nullish values are passed.
Also, we should add documentation. Somebody who comes across the code is basically in the dark what it means to look on the status or error props.
There was a problem hiding this comment.
Ok sure, thanks for clarifying. Will update
| - `HttpException` errors will have a `status` property | ||
| - `RpcException` errors will have an `error` property | ||
| */ | ||
| if (exceptionStatusCode !== null || exceptionErrorProperty !== null) { |
There was a problem hiding this comment.
l: I would flip the condition: We report the error if this and this condition is true. To me the early return is a weird pattern here but I'll leave it up to you.
| @@ -11,6 +11,7 @@ import { Catch } from '@nestjs/common'; | |||
| import { Injectable } from '@nestjs/common'; | |||
| import { Global, Module } from '@nestjs/common'; | |||
| import { APP_FILTER, APP_INTERCEPTOR, BaseExceptionFilter } from '@nestjs/core'; | |||
| import { RpcException } from '@nestjs/microservices'; | |||
There was a problem hiding this comment.
This breaks NestJS projects which don't use @nestjs/microservices.
I've tried creating an issue, but couldn't submit it because GH kept showing "Something went wrong" popup.
There was a problem hiding this comment.
Thanks for writing in! I am currently working on a fix for the nest SDK and will remove the dependency again as part of that PR. ref
RpcExceptionsare always explicitly thrown in nest. Therefore, they are expected for users and should not be sent to Sentry.This PR filters these exceptions. In
@sentry/nestjswe can simply useinstanceof RpcExceptionsto achieve this. In@sentry/nodewe do not have access to this class, so we need to check based on a property.ref