feat(nestjs): Filter all HttpExceptions (#13120)

So far we are filtering exceptions on the status code and do not report 4xx errors. However, we actually only want to capture unexpected exceptions but all HttpExceptions are only ever thrown explicitly. Hence, we do not want to capture them and they should be filtered. In `@sentry/nest` we can use the nest `HttpException` directly, since we have `@nest/common` as a dependency. In `@sentry/node` this is not the case, so we filter based on whether a property called status is defined.
getsentry · Jul 31, 2024 · 7150e3f · 7150e3f
1 parent b4f2067
commit 7150e3f
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 30 deletions.
diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.controller.ts
@@ -20,9 +20,14 @@ export class AppController {
     return this.appService.testException(id);
   }
 
-  @Get('test-expected-exception/:id')
-  async testExpectedException(@Param('id') id: string) {
-    return this.appService.testExpectedException(id);
+  @Get('test-expected-400-exception/:id')
+  async testExpected400Exception(@Param('id') id: string) {
+    return this.appService.testExpected400Exception(id);
+  }
+
+  @Get('test-expected-500-exception/:id')
+  async testExpected500Exception(@Param('id') id: string) {
+    return this.appService.testExpected500Exception(id);
   }
 
   @Get('test-span-decorator-async')

diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.service.ts
@@ -30,8 +30,12 @@ export class AppService {
     throw new Error(`This is an exception with id ${id}`);
   }
 
-  testExpectedException(id: string) {
-    throw new HttpException(`This is an expected exception with id ${id}`, HttpStatus.FORBIDDEN);
+  testExpected400Exception(id: string) {
+    throw new HttpException(`This is an expected 400 exception with id ${id}`, HttpStatus.BAD_REQUEST);
+  }
+
+  testExpected500Exception(id: string) {
+    throw new HttpException(`This is an expected 500 exception with id ${id}`, HttpStatus.INTERNAL_SERVER_ERROR);
   }
 
   @SentryTraced('wait and return a string')

diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/errors.test.ts
@@ -29,25 +29,41 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
   });
 });
 
-test('Does not send expected exception to Sentry', async ({ baseURL }) => {
+test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => {
   let errorEventOccurred = false;
 
   waitForError('nestjs', event => {
-    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected exception with id 123') {
+    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected 400 exception with id 123') {
       errorEventOccurred = true;
     }
 
-    return event?.transaction === 'GET /test-expected-exception/:id';
+    return event?.transaction === 'GET /test-expected-400-exception/:id';
   });
 
-  const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => {
-    return transactionEvent?.transaction === 'GET /test-expected-exception/:id';
+  waitForError('nestjs', event => {
+    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected 500 exception with id 123') {
+      errorEventOccurred = true;
+    }
+
+    return event?.transaction === 'GET /test-expected-500-exception/:id';
   });
 
-  const response = await fetch(`${baseURL}/test-expected-exception/123`);
-  expect(response.status).toBe(403);
+  const transactionEventPromise400 = waitForTransaction('nestjs', transactionEvent => {
+    return transactionEvent?.transaction === 'GET /test-expected-400-exception/:id';
+  });
+
+  const transactionEventPromise500 = waitForTransaction('nestjs', transactionEvent => {
+    return transactionEvent?.transaction === 'GET /test-expected-500-exception/:id';
+  });
+
+  const response400 = await fetch(`${baseURL}/test-expected-400-exception/123`);
+  expect(response400.status).toBe(400);
+
+  const response500 = await fetch(`${baseURL}/test-expected-500-exception/123`);
+  expect(response500.status).toBe(500);
 
-  await transactionEventPromise;
+  await transactionEventPromise400;
+  await transactionEventPromise500;
 
   await new Promise(resolve => setTimeout(resolve, 10000));
 

diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.controller.ts
@@ -20,9 +20,14 @@ export class AppController {
     return this.appService.testException(id);
   }
 
-  @Get('test-expected-exception/:id')
-  async testExpectedException(@Param('id') id: string) {
-    return this.appService.testExpectedException(id);
+  @Get('test-expected-400-exception/:id')
+  async testExpected400Exception(@Param('id') id: string) {
+    return this.appService.testExpected400Exception(id);
+  }
+
+  @Get('test-expected-500-exception/:id')
+  async testExpected500Exception(@Param('id') id: string) {
+    return this.appService.testExpected500Exception(id);
   }
 
   @Get('test-span-decorator-async')

diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.service.ts
@@ -30,8 +30,12 @@ export class AppService {
     throw new Error(`This is an exception with id ${id}`);
   }
 
-  testExpectedException(id: string) {
-    throw new HttpException(`This is an expected exception with id ${id}`, HttpStatus.FORBIDDEN);
+  testExpected400Exception(id: string) {
+    throw new HttpException(`This is an expected 400 exception with id ${id}`, HttpStatus.BAD_REQUEST);
+  }
+
+  testExpected500Exception(id: string) {
+    throw new HttpException(`This is an expected 500 exception with id ${id}`, HttpStatus.INTERNAL_SERVER_ERROR);
   }
 
   @SentryTraced('wait and return a string')

diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/errors.test.ts
@@ -29,25 +29,41 @@ test('Sends exception to Sentry', async ({ baseURL }) => {
   });
 });
 
-test('Does not send expected exception to Sentry', async ({ baseURL }) => {
+test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => {
   let errorEventOccurred = false;
 
   waitForError('nestjs', event => {
-    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected exception with id 123') {
+    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected 400 exception with id 123') {
       errorEventOccurred = true;
     }
 
-    return event?.transaction === 'GET /test-expected-exception/:id';
+    return event?.transaction === 'GET /test-expected-400-exception/:id';
   });
 
-  const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => {
-    return transactionEvent?.transaction === 'GET /test-expected-exception/:id';
+  waitForError('nestjs', event => {
+    if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected 500 exception with id 123') {
+      errorEventOccurred = true;
+    }
+
+    return event?.transaction === 'GET /test-expected-500-exception/:id';
   });
 
-  const response = await fetch(`${baseURL}/test-expected-exception/123`);
-  expect(response.status).toBe(403);
+  const transactionEventPromise400 = waitForTransaction('nestjs', transactionEvent => {
+    return transactionEvent?.transaction === 'GET /test-expected-400-exception/:id';
+  });
+
+  const transactionEventPromise500 = waitForTransaction('nestjs', transactionEvent => {
+    return transactionEvent?.transaction === 'GET /test-expected-500-exception/:id';
+  });
+
+  const response400 = await fetch(`${baseURL}/test-expected-400-exception/123`);
+  expect(response400.status).toBe(400);
+
+  const response500 = await fetch(`${baseURL}/test-expected-500-exception/123`);
+  expect(response500.status).toBe(500);
 
-  await transactionEventPromise;
+  await transactionEventPromise400;
+  await transactionEventPromise500;
 
   await new Promise(resolve => setTimeout(resolve, 10000));
 

diff --git a/packages/nestjs/src/setup.ts b/packages/nestjs/src/setup.ts
@@ -6,6 +6,7 @@ import type {
   NestInterceptor,
   OnModuleInit,
 } from '@nestjs/common';
+import { HttpException } from '@nestjs/common';
 import { Catch } from '@nestjs/common';
 import { Injectable } from '@nestjs/common';
 import { Global, Module } from '@nestjs/common';
@@ -63,10 +64,8 @@ class SentryGlobalFilter extends BaseExceptionFilter {
    * Catches exceptions and reports them to Sentry unless they are expected errors.
    */
   public catch(exception: unknown, host: ArgumentsHost): void {
-    const status_code = (exception as { status?: number }).status;
-
     // don't report expected errors
-    if (status_code !== undefined && status_code >= 400 && status_code < 500) {
+    if (exception instanceof HttpException) {
       return super.catch(exception, host);
     }
 

diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts
@@ -262,7 +262,7 @@ export function setupNestErrorHandler(app: MinimalNestJsApp, baseFilter: NestJsE
           const status_code = (exception as { status?: number }).status;
 
           // don't report expected errors
-          if (status_code !== undefined && status_code >= 400 && status_code < 500) {
+          if (status_code !== undefined) {
             return originalCatch.apply(target, [exception, host]);
           }