From 0faa46761b25a7a318ce0a018717a547668c5cd0 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 10:43:33 +0200 Subject: [PATCH 1/6] Add nestjs microservices dependency --- packages/nestjs/package.json | 6 ++++-- yarn.lock | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/nestjs/package.json b/packages/nestjs/package.json index ed5412cb5f0f..280a85c9d11f 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/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" From cebcd44a4f182e4b97db47735dffbe1ee69b88c3 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 11:16:32 +0200 Subject: [PATCH 2/6] Filter rpc exceptions in sentry/nest + make tests go brrr --- .../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 | 28 ++++++++++++++++++- packages/nestjs/src/setup.ts | 3 +- 5 files changed, 40 insertions(+), 2 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 dad5d391bdde..d788365e6b6a 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 @@ -1,5 +1,6 @@ import { expect, test } from '@playwright/test'; import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; +import { flush } from '@sentry/core'; test('Sends exception to Sentry', async ({ baseURL }) => { const errorEventPromise = waitForError('nestjs', event => { @@ -65,7 +66,32 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { await transactionEventPromise400; await transactionEventPromise500; - await new Promise(resolve => setTimeout(resolve, 10000)); + flush(); + + expect(errorEventOccurred).toBe(false); +}); + +test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { + let errorEventOccurred = false; + + waitForError('nestjs', 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', 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; + + flush(); expect(errorEventOccurred).toBe(false); }); 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); } From 1db607fb4fa818442da6341f88d6160d8c4ff71d Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 11:21:42 +0200 Subject: [PATCH 3/6] Make tests slow again --- .../test-applications/nestjs-basic/tests/errors.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) 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 d788365e6b6a..3956bd60bd01 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 @@ -1,6 +1,5 @@ import { expect, test } from '@playwright/test'; import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; -import { flush } from '@sentry/core'; test('Sends exception to Sentry', async ({ baseURL }) => { const errorEventPromise = waitForError('nestjs', event => { @@ -66,7 +65,7 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { await transactionEventPromise400; await transactionEventPromise500; - flush(); + await new Promise(resolve => setTimeout(resolve, 10000)); expect(errorEventOccurred).toBe(false); }); @@ -91,7 +90,7 @@ test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { await transactionEventPromise; - flush(); + await new Promise(resolve => setTimeout(resolve, 10000)); expect(errorEventOccurred).toBe(false); }); From da797a7075d374cca3d093952cd050e7b8a6903d Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 11:37:39 +0200 Subject: [PATCH 4/6] Filter rpc exceptions in sentry node --- .../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 +++++++++++++++++++ .../src/integrations/tracing/nest/nest.ts | 7 +++--- .../src/integrations/tracing/nest/types.ts | 10 ++++++++ 6 files changed, 50 insertions(+), 3 deletions(-) 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 dad5d391bdde..3956bd60bd01 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('nestjs', 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', 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/node/src/integrations/tracing/nest/nest.ts b/packages/node/src/integrations/tracing/nest/nest.ts index 4f7f7a1f59d3..96fa5ca1a07a 100644 --- a/packages/node/src/integrations/tracing/nest/nest.ts +++ b/packages/node/src/integrations/tracing/nest/nest.ts @@ -13,7 +13,7 @@ import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../../otel/instrument'; import { SentryNestInstrumentation } from './sentry-nest-instrumentation'; -import type { MinimalNestJsApp, NestJsErrorFilter } from './types'; +import type { ExpectedException, MinimalNestJsApp, NestJsErrorFilter } from './types'; const INTEGRATION_NAME = 'Nest'; @@ -87,10 +87,11 @@ 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; + 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]); } diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 2cdd1b6aefaf..7ce477657b63 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -27,6 +27,16 @@ export interface MinimalNestJsApp { }) => void; } +/** + * Interface for an expected exception in nest. + * + * Expected exceptions in nest are HttpExceptions (have a status property) or RpcExceptions (have an error property). + */ +export interface ExpectedException { + status?: number; + error?: string | object; +} + /** * A minimal interface for an Observable. */ From 979d7e73ccf93088ec283be3be0df0319ec005b9 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 12:52:38 +0200 Subject: [PATCH 5/6] Fix tests? --- .../test-applications/nestjs-basic/tests/errors.test.ts | 4 ++-- .../test-applications/node-nestjs-basic/tests/errors.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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 c83be8ad7f0d..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 @@ -73,7 +73,7 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { let errorEventOccurred = false; - waitForError('nestjs', event => { + waitForError('nestjs-basic', event => { if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected RPC exception with id 123') { errorEventOccurred = true; } @@ -81,7 +81,7 @@ test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { return event?.transaction === 'GET /test-expected-rpc-exception/:id'; }); - const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + const transactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/:id'; }); 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 8073153db513..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 @@ -73,7 +73,7 @@ test('Does not send HttpExceptions to Sentry', async ({ baseURL }) => { test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { let errorEventOccurred = false; - waitForError('nestjs', event => { + waitForError('node-nestjs-basic', event => { if (!event.type && event.exception?.values?.[0]?.value === 'This is an expected RPC exception with id 123') { errorEventOccurred = true; } @@ -81,7 +81,7 @@ test('Does not send RpcExceptions to Sentry', async ({ baseURL }) => { return event?.transaction === 'GET /test-expected-rpc-exception/:id'; }); - const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + const transactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { return transactionEvent?.transaction === 'GET /test-expected-rpc-exception/:id'; }); From 619e8fe6ca80cddca046f00d0255d6c48870eed5 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 5 Aug 2024 15:59:17 +0200 Subject: [PATCH 6/6] Address pr comments --- .../node/src/integrations/tracing/nest/nest.ts | 17 +++++++++++------ .../node/src/integrations/tracing/nest/types.ts | 10 ---------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/nest.ts b/packages/node/src/integrations/tracing/nest/nest.ts index 96fa5ca1a07a..4f8d88fa8f86 100644 --- a/packages/node/src/integrations/tracing/nest/nest.ts +++ b/packages/node/src/integrations/tracing/nest/nest.ts @@ -13,7 +13,7 @@ import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../../otel/instrument'; import { SentryNestInstrumentation } from './sentry-nest-instrumentation'; -import type { ExpectedException, MinimalNestJsApp, NestJsErrorFilter } from './types'; +import type { MinimalNestJsApp, NestJsErrorFilter } from './types'; const INTEGRATION_NAME = 'Nest'; @@ -87,11 +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 ExpectedException).status; - const error_property = (exception as ExpectedException).error; - - // don't report expected errors - if (status_code !== undefined || error_property !== 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/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 7ce477657b63..2cdd1b6aefaf 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -27,16 +27,6 @@ export interface MinimalNestJsApp { }) => void; } -/** - * Interface for an expected exception in nest. - * - * Expected exceptions in nest are HttpExceptions (have a status property) or RpcExceptions (have an error property). - */ -export interface ExpectedException { - status?: number; - error?: string | object; -} - /** * A minimal interface for an Observable. */