From c1052ab86a37b6d46f7a54d90f665cc26bdfd0fe Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Mon, 5 Aug 2024 16:29:36 +0200 Subject: [PATCH] feat(nestjs): Filter RPC exceptions (#13227) `RpcExceptions` are 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/nestjs` we can simply use `instanceof RpcExceptions` to achieve this. In `@sentry/node` we do not have access to this class, so we need to check based on a property. [ref](https://github.com/getsentry/sentry-javascript/issues/13190) --- .../nestjs-basic/package.json | 1 + .../nestjs-basic/src/app.controller.ts | 5 ++++ .../nestjs-basic/src/app.service.ts | 5 ++++ .../nestjs-basic/tests/errors.test.ts | 25 +++++++++++++++++++ .../node-nestjs-basic/package.json | 1 + .../node-nestjs-basic/src/app.controller.ts | 5 ++++ .../node-nestjs-basic/src/app.service.ts | 5 ++++ .../node-nestjs-basic/tests/errors.test.ts | 25 +++++++++++++++++++ packages/nestjs/package.json | 6 +++-- packages/nestjs/src/setup.ts | 3 ++- .../src/integrations/tracing/nest/nest.ts | 14 ++++++++--- yarn.lock | 8 ++++++ 12 files changed, 96 insertions(+), 7 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/package.json b/dev-packages/e2e-tests/test-applications/nestjs-basic/package.json index f4c44ff7cef3..f7a0da2fc403 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/package.json +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/package.json @@ -17,6 +17,7 @@ "dependencies": { "@nestjs/common": "^10.0.0", "@nestjs/core": "^10.0.0", + "@nestjs/microservices": "^10.0.0", "@nestjs/schedule": "^4.1.0", "@nestjs/platform-express": "^10.0.0", "@sentry/nestjs": "latest || *", 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 index c04fd5613e95..5becddbc05e0 100644 --- 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 @@ -49,6 +49,11 @@ export class AppController { return this.appService.testExpected500Exception(id); } + @Get('test-expected-rpc-exception/:id') + async testExpectedRpcException(@Param('id') id: string) { + return this.appService.testExpectedRpcException(id); + } + @Get('test-span-decorator-async') async testSpanDecoratorAsync() { return { result: await this.appService.testSpanDecoratorAsync() }; 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 index b2dadbb0a269..f1c935257013 100644 --- 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 @@ -1,4 +1,5 @@ import { HttpException, HttpStatus, Injectable } from '@nestjs/common'; +import { RpcException } from '@nestjs/microservices'; import { Cron, SchedulerRegistry } from '@nestjs/schedule'; import * as Sentry from '@sentry/nestjs'; import { SentryCron, SentryTraced } from '@sentry/nestjs'; @@ -38,6 +39,10 @@ export class AppService { throw new HttpException(`This is an expected 500 exception with id ${id}`, HttpStatus.INTERNAL_SERVER_ERROR); } + testExpectedRpcException(id: string) { + throw new RpcException(`This is an expected RPC exception with id ${id}`); + } + @SentryTraced('wait and return a string') async wait() { await new Promise(resolve => setTimeout(resolve, 500)); 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 index cffc5f4946a3..ccc919cdd025 100644 --- 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 @@ -69,3 +69,28 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { expect(errorEventOccurred).toBe(false); }); + +test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError('nestjs-basic', event => { + if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected RPC exception with id 123') { + errorEventOccurred = true; + } + + return event?.transaction === 'GET /test-expected-rpc-exception/:id'; + }); + + const transactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { + return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/:id'; + }); + + const response = await fetch(`${baseURL}/test-expected-rpc-exception/123`); + expect(response.status).toBe(500); + + await transactionEventPromise; + + await new Promise(resolve => setTimeout(resolve, 10000)); + + expect(errorEventOccurred).toBe(false); +}); diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/package.json b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/package.json index ec6510ac03ff..b7334026d18f 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/package.json +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/package.json @@ -17,6 +17,7 @@ "dependencies": { "@nestjs/common": "^10.0.0", "@nestjs/core": "^10.0.0", + "@nestjs/microservices": "^10.0.0", "@nestjs/schedule": "^4.1.0", "@nestjs/platform-express": "^10.0.0", "@sentry/nestjs": "latest || *", 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 index c04fd5613e95..5becddbc05e0 100644 --- 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 @@ -49,6 +49,11 @@ export class AppController { return this.appService.testExpected500Exception(id); } + @Get('test-expected-rpc-exception/:id') + async testExpectedRpcException(@Param('id') id: string) { + return this.appService.testExpectedRpcException(id); + } + @Get('test-span-decorator-async') async testSpanDecoratorAsync() { return { result: await this.appService.testSpanDecoratorAsync() }; 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 index b2dadbb0a269..f1c935257013 100644 --- 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 @@ -1,4 +1,5 @@ import { HttpException, HttpStatus, Injectable } from '@nestjs/common'; +import { RpcException } from '@nestjs/microservices'; import { Cron, SchedulerRegistry } from '@nestjs/schedule'; import * as Sentry from '@sentry/nestjs'; import { SentryCron, SentryTraced } from '@sentry/nestjs'; @@ -38,6 +39,10 @@ export class AppService { throw new HttpException(`This is an expected 500 exception with id ${id}`, HttpStatus.INTERNAL_SERVER_ERROR); } + testExpectedRpcException(id: string) { + throw new RpcException(`This is an expected RPC exception with id ${id}`); + } + @SentryTraced('wait and return a string') async wait() { await new Promise(resolve => setTimeout(resolve, 500)); 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 index 11eafc38f430..171f42920487 100644 --- 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 @@ -69,3 +69,28 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { expect(errorEventOccurred).toBe(false); }); + +test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError('node-nestjs-basic', event => { + if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected RPC exception with id 123') { + errorEventOccurred = true; + } + + return event?.transaction === 'GET /test-expected-rpc-exception/:id'; + }); + + const transactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { + return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/:id'; + }); + + const response = await fetch(`${baseURL}/test-expected-rpc-exception/123`); + expect(response.status).toBe(500); + + await transactionEventPromise; + + await new Promise(resolve => setTimeout(resolve, 10000)); + + expect(errorEventOccurred).toBe(false); +}); diff --git a/packages/nestjs/package.json b/packages/nestjs/package.json index cc35e0c33a5c..d218c0be07c2 100644 --- a/packages/nestjs/package.json +++ b/packages/nestjs/package.json @@ -51,11 +51,13 @@ }, "devDependencies": { "@nestjs/common": "^8.0.0 || ^9.0.0 || ^10.0.0", - "@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0" + "@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0", + "@nestjs/microservices": "^8.0.0 || ^9.0.0 || ^10.0.0" }, "peerDependencies": { "@nestjs/common": "^8.0.0 || ^9.0.0 || ^10.0.0", - "@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0" + "@nestjs/core": "^8.0.0 || ^9.0.0 || ^10.0.0", + "@nestjs/microservices": "^8.0.0 || ^9.0.0 || ^10.0.0" }, "scripts": { "build": "run-p build:transpile build:types", diff --git a/packages/nestjs/src/setup.ts b/packages/nestjs/src/setup.ts index f788ccb9b67c..b068ed052a91 100644 --- a/packages/nestjs/src/setup.ts +++ b/packages/nestjs/src/setup.ts @@ -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'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, @@ -68,7 +69,7 @@ class SentryGlobalFilter extends BaseExceptionFilter { */ public catch(exception: unknown, host: ArgumentsHost): void { // don't report expected errors - if (exception instanceof HttpException) { + if (exception instanceof HttpException || exception instanceof RpcException) { return super.catch(exception, host); } diff --git a/packages/node/src/integrations/tracing/nest/nest.ts b/packages/node/src/integrations/tracing/nest/nest.ts index 4f7f7a1f59d3..4f8d88fa8f86 100644 --- a/packages/node/src/integrations/tracing/nest/nest.ts +++ b/packages/node/src/integrations/tracing/nest/nest.ts @@ -87,10 +87,16 @@ export function setupNestErrorHandler(app: MinimalNestJsApp, baseFilter: NestJsE const originalCatch = Reflect.get(target, prop, receiver); return (exception: unknown, host: unknown) => { - const status_code = (exception as { status?: number }).status; - - // don't report expected errors - if (status_code !== undefined) { + 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; + + /* + Don't report expected NestJS control flow errors + - `HttpException` errors will have a `status` property + - `RpcException` errors will have an `error` property + */ + if (exceptionStatusCode !== null || exceptionErrorProperty !== null) { return originalCatch.apply(target, [exception, host]); } diff --git a/yarn.lock b/yarn.lock index 953f0eee5f3a..660c53cce594 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6135,6 +6135,14 @@ path-to-regexp "3.2.0" tslib "2.6.3" +"@nestjs/microservices@^8.0.0 || ^9.0.0 || ^10.0.0": + version "10.3.10" + resolved "https://registry.yarnpkg.com/@nestjs/microservices/-/microservices-10.3.10.tgz#e00957e0c22b0cc8b041242a40538e2d862255fb" + integrity sha512-zZrilhZmXU2Ik5Usrcy4qEX262Uhvz0/9XlIdX6SRn8I39ns1EE9tAhEBmmkMwh7lsEikRFa4aaa05loi8Gsow== + dependencies: + iterare "1.2.1" + tslib "2.6.3" + "@nestjs/platform-express@^10.3.3": version "10.3.3" resolved "https://registry.yarnpkg.com/@nestjs/platform-express/-/platform-express-10.3.3.tgz#c1484d30d1e7666c4c8d0d7cde31cfc0b9d166d7"