ericjeker / nestjs-sentry-example

Explanations and example project on how to setup Sentry in a Nest.js project
MIT No Attribution
83 stars 16 forks source link

Does not report errors in middleware and guards #1

Open kembala opened 2 years ago

kembala commented 2 years ago

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 commented 2 years ago

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

ericjeker commented 2 years ago

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

vixeven commented 2 years ago

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();
        }),
      );
  }
}
ericjeker commented 2 years ago

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

KiwiKilian commented 11 months ago

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.

ericjeker commented 11 months ago

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 commented 8 months ago

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.

KiwiKilian commented 8 months ago

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.

ericjeker commented 8 months ago

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