nestjs-sentry-example icon indicating copy to clipboard operation
nestjs-sentry-example copied to clipboard

Does not report errors in middleware and guards

Open kembala opened this issue 2 years ago • 9 comments

Awesome work, thanks for sharing!

Just wanted to flag that due to the nature of NestJS request lifecycle this implementation will not report errors happening in your middlewares or guards.

This could be useful even with a stable implementation so that the performance monitoring tracks BadRequestExceptions for example.

kembala avatar Jun 20 '22 12:06 kembala

This could be solved in theory by adding an extra layer with Exception filters

kembala avatar Jun 20 '22 12:06 kembala

Hi @kemenesbalazs, thanks for your message, I will have a look at that.

ericjeker avatar Jun 24 '22 08:06 ericjeker

Thank you for sharing this setup!

I used tap to capture exceptions — in this way the error is not touched. I can't confirm if this solves the problem with middleware and guards, but it fixed the problem where the error response was different.

@Injectable({ scope: Scope.REQUEST })
export class SentryInterceptor implements NestInterceptor {
  constructor(private sentryService: SentryService) { }

  intercept(_context: ExecutionContext, next: CallHandler): Observable<any> {
    const span = this.sentryService.startChild({ op: 'http' });

    return next
      .handle()
      .pipe(
        tap(() => { }, (exception) => {
          Sentry.captureException(
            exception,
            this.sentryService.span.getTraceContext()
          );
        }),
        finalize(() => {
          span.finish();
          this.sentryService.span.finish();
        }),
      );
  }
}

vixeven avatar Aug 02 '22 08:08 vixeven

Thanks @vixeven , sorry for the very late reply, I don't check this repo very often. I will have a look at your setup.

ericjeker avatar Aug 22 '22 14:08 ericjeker

I was generally thinking, if Exception Filter is the better solution? The current approach submits all errors and I'm not looking forward setting up a black/whitelist of errors I (don't) wan't reported. Implementing BaseExceptionFilter.handleUnknownError would be another approach but I'm no sure where else it would differ in behavior against the Interceptor of this example? I think it's more difficult to integrate with the tracing span, because finalize runs before BaseExceptionFilter.handleUnknownError.

KiwiKilian avatar Sep 27 '23 13:09 KiwiKilian

Hi @KiwiKilian, usually my approach is to handle the exceptions I don't want reported. Everything that is falling through should appear on Sentry. If I then see something on Sentry that shouldn't be reported I add a clean handler for it, using a try..catch or something else.

ericjeker avatar Sep 29 '23 12:09 ericjeker

Hi @KiwiKilian, in version 2.0 I added an Exception Filter that is catching everything. Let me know what you think. Obviously, there is a need to add some filtering here as we don't want to have all the 40x error in Sentry.

ericjeker avatar Dec 24 '23 11:12 ericjeker

Thanks @ericjeker for taking a thought about this twice! I've yet not made up my mind, what's the best approach. Actually there are some rare cases I even want the 4xx reported, but maybe this is misuse on my side of sentry 😅. And with that comes a lot of noise. I will have a look at your new approach after the holidays.

KiwiKilian avatar Dec 24 '23 11:12 KiwiKilian

@KiwiKilian, there are many ways to filter events I guess. It's just a matter of only logging what you want really:

  • I added the beforeSend hook as an example too. You can read more about it here.
  • Exception Filters
  • Catching only exception that fall through the cracks

ericjeker avatar Dec 25 '23 00:12 ericjeker