From b429c863ae01a9359a9f65d9bfc554fd0e39f546 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 9 Apr 2024 17:32:13 +0200 Subject: [PATCH] feat(nestjs): Update scope transaction name with parameterized route (#11510) This PR adds scope transactionName updating to our NestJS instrumentation. Similarly to Hapi and Fastify, we can use a framework-native component, an Interceptor, to assign the parameterized route. ref #10846 --- .../node-nestjs-app/src/app.controller.ts | 6 +- .../node-nestjs-app/src/app.service.ts | 4 +- .../node-nestjs-app/tests/errors.test.ts | 10 ++-- .../nestjs-errors-no-express/scenario.ts | 58 +++++++++++++++++++ .../tracing/nestjs-errors-no-express/test.ts | 39 +++++++++++++ .../nestjs-errors-no-express/tsconfig.json | 9 +++ .../suites/tracing/nestjs-errors/scenario.ts | 56 ++++++++++++++++++ .../suites/tracing/nestjs-errors/test.ts | 39 +++++++++++++ .../tracing/nestjs-errors/tsconfig.json | 9 +++ .../tracing/nestjs-no-express/scenario.ts | 58 +++++++++++++++++++ .../suites/tracing/nestjs-no-express/test.ts | 40 +++++++++++++ .../tracing/nestjs-no-express/tsconfig.json | 9 +++ .../node/src/integrations/tracing/nest.ts | 42 ++++++++++++-- 13 files changed, 365 insertions(+), 14 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/tsconfig.json create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-errors/tsconfig.json create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/tsconfig.json diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.controller.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.controller.ts index 5dda4845d392..6350cb49f1c5 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.controller.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.controller.ts @@ -40,9 +40,9 @@ export class AppController1 { return this.appService.testError(); } - @Get('test-exception') - async testException() { - return this.appService.testException(); + @Get('test-exception/:id') + async testException(@Param('id') id: string) { + return this.appService.testException(id); } @Get('test-outgoing-fetch-external-allowed') diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.service.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.service.ts index 387668889c24..79b01f26f51c 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.service.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-app/src/app.service.ts @@ -48,8 +48,8 @@ export class AppService1 { return { exceptionId }; } - testException() { - throw new Error('This is an exception'); + testException(id: string) { + throw new Error(`This is an exception with id ${id}`); } async testOutgoingFetchExternalAllowed() { diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-app/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-app/tests/errors.test.ts index 8d478c063472..ba7b0cf1849b 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-app/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-app/tests/errors.test.ts @@ -43,11 +43,11 @@ test('Sends captured error to Sentry', async ({ baseURL }) => { test('Sends exception to Sentry', async ({ baseURL }) => { const errorEventPromise = waitForError('node-nestjs-app', event => { - return !event.type && event.exception?.values?.[0]?.value === 'This is an exception'; + return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 123'; }); try { - axios.get(`${baseURL}/test-exception`); + axios.get(`${baseURL}/test-exception/123`); } catch { // this results in an error, but we don't care - we want to check the error event } @@ -55,16 +55,16 @@ test('Sends exception to Sentry', async ({ baseURL }) => { const errorEvent = await errorEventPromise; expect(errorEvent.exception?.values).toHaveLength(1); - expect(errorEvent.exception?.values?.[0]?.value).toBe('This is an exception'); + expect(errorEvent.exception?.values?.[0]?.value).toBe('This is an exception with id 123'); expect(errorEvent.request).toEqual({ method: 'GET', cookies: {}, headers: expect.any(Object), - url: 'http://localhost:3030/test-exception', + url: 'http://localhost:3030/test-exception/123', }); - expect(errorEvent.transaction).toEqual('GET /test-exception'); + expect(errorEvent.transaction).toEqual('GET /test-exception/:id'); expect(errorEvent.contexts?.trace).toEqual({ trace_id: expect.any(String), diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/scenario.ts new file mode 100644 index 000000000000..bcf4c3adb432 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/scenario.ts @@ -0,0 +1,58 @@ +// eslint-disable-next-line @typescript-eslint/ban-ts-comment +// @ts-nocheck These are only tests +/* eslint-disable @typescript-eslint/naming-convention */ +/* eslint-disable @typescript-eslint/explicit-member-accessibility */ +import { loggingTransport, sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 0, + transport: loggingTransport, + integrations: integrations => integrations.filter(i => i.name !== 'Express'), + debug: true, +}); + +import { Controller, Get, Injectable, Module, Param } from '@nestjs/common'; +import { NestFactory } from '@nestjs/core'; + +const port = 3480; + +// Stop the process from exiting before the transaction is sent +// eslint-disable-next-line @typescript-eslint/no-empty-function +setInterval(() => {}, 1000); + +@Injectable() +class AppService { + getHello(): string { + return 'Hello World!'; + } +} + +@Controller() +class AppController { + constructor(private readonly appService: AppService) {} + + @Get('test-exception/:id') + async testException(@Param('id') id: string): void { + Sentry.captureException(new Error(`error with id ${id}`)); + } +} + +@Module({ + imports: [], + controllers: [AppController], + providers: [AppService], +}) +class AppModule {} + +async function init(): Promise { + const app = await NestFactory.create(AppModule); + Sentry.setupNestErrorHandler(app); + await app.listen(port); + sendPortToRunner(port); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +init(); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/test.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/test.ts new file mode 100644 index 000000000000..f38550469446 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/test.ts @@ -0,0 +1,39 @@ +import { conditionalTest } from '../../../utils'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +jest.setTimeout(20000); + +const { TS_VERSION } = process.env; +const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.'); + +// This is required to run the test with ts-node and decorators +process.env.TS_NODE_PROJECT = `${__dirname}/tsconfig.json`; + +conditionalTest({ min: 16 })('nestjs auto instrumentation', () => { + afterAll(async () => { + cleanupChildProcesses(); + }); + + test("should assign scope's transactionName if spans are not sampled and express integration is disabled", done => { + if (isOldTS) { + // Skipping test on old TypeScript + return done(); + } + + createRunner(__dirname, 'scenario.ts') + .expect({ + event: { + exception: { + values: [ + { + value: 'error with id 456', + }, + ], + }, + transaction: 'GET /test-exception/:id', + }, + }) + .start(done) + .makeRequest('get', '/test-exception/456'); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/tsconfig.json b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/tsconfig.json new file mode 100644 index 000000000000..84b8f8d6c44e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors-no-express/tsconfig.json @@ -0,0 +1,9 @@ +{ + "include": ["scenario.ts"], + "compilerOptions": { + "module": "commonjs", + "emitDecoratorMetadata": true, + "experimentalDecorators": true, + "target": "ES2021", + } +} diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/scenario.ts new file mode 100644 index 000000000000..8a00c25fab7a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/scenario.ts @@ -0,0 +1,56 @@ +// eslint-disable-next-line @typescript-eslint/ban-ts-comment +// @ts-nocheck These are only tests +/* eslint-disable @typescript-eslint/naming-convention */ +/* eslint-disable @typescript-eslint/explicit-member-accessibility */ +import { loggingTransport, sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 0, + transport: loggingTransport, +}); + +import { Controller, Get, Injectable, Module, Param } from '@nestjs/common'; +import { NestFactory } from '@nestjs/core'; + +const port = 3460; + +// Stop the process from exiting before the transaction is sent +// eslint-disable-next-line @typescript-eslint/no-empty-function +setInterval(() => {}, 1000); + +@Injectable() +class AppService { + getHello(): string { + return 'Hello World!'; + } +} + +@Controller() +class AppController { + constructor(private readonly appService: AppService) {} + + @Get('test-exception/:id') + async testException(@Param('id') id: string): void { + Sentry.captureException(new Error(`error with id ${id}`)); + } +} + +@Module({ + imports: [], + controllers: [AppController], + providers: [AppService], +}) +class AppModule {} + +async function init(): Promise { + const app = await NestFactory.create(AppModule); + Sentry.setupNestErrorHandler(app); + await app.listen(port); + sendPortToRunner(port); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +init(); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/test.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/test.ts new file mode 100644 index 000000000000..264cbe1482cc --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/test.ts @@ -0,0 +1,39 @@ +import { conditionalTest } from '../../../utils'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +jest.setTimeout(20000); + +const { TS_VERSION } = process.env; +const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.'); + +// This is required to run the test with ts-node and decorators +process.env.TS_NODE_PROJECT = `${__dirname}/tsconfig.json`; + +conditionalTest({ min: 16 })('nestjs auto instrumentation', () => { + afterAll(async () => { + cleanupChildProcesses(); + }); + + test("should assign scope's transactionName if spans are not sampled", done => { + if (isOldTS) { + // Skipping test on old TypeScript + return done(); + } + + createRunner(__dirname, 'scenario.ts') + .expect({ + event: { + exception: { + values: [ + { + value: 'error with id 123', + }, + ], + }, + transaction: 'GET /test-exception/:id', + }, + }) + .start(done) + .makeRequest('get', '/test-exception/123'); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/tsconfig.json b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/tsconfig.json new file mode 100644 index 000000000000..84b8f8d6c44e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-errors/tsconfig.json @@ -0,0 +1,9 @@ +{ + "include": ["scenario.ts"], + "compilerOptions": { + "module": "commonjs", + "emitDecoratorMetadata": true, + "experimentalDecorators": true, + "target": "ES2021", + } +} diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/scenario.ts new file mode 100644 index 000000000000..cffc8de263d2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/scenario.ts @@ -0,0 +1,58 @@ +// eslint-disable-next-line @typescript-eslint/ban-ts-comment +// @ts-nocheck These are only tests +/* eslint-disable @typescript-eslint/naming-convention */ +/* eslint-disable @typescript-eslint/explicit-member-accessibility */ +import { loggingTransport, sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import * as Sentry from '@sentry/node'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1, + transport: loggingTransport, + integrations: integrations => integrations.filter(i => i.name !== 'Express'), + debug: true, +}); + +import { Controller, Get, Injectable, Module, Param } from '@nestjs/common'; +import { NestFactory } from '@nestjs/core'; + +const port = 3470; + +// Stop the process from exiting before the transaction is sent +// eslint-disable-next-line @typescript-eslint/no-empty-function +setInterval(() => {}, 1000); + +@Injectable() +class AppService { + getHello(): string { + return 'Hello World!'; + } +} + +@Controller() +class AppController { + constructor(private readonly appService: AppService) {} + + @Get('test-exception/:id') + async testException(@Param('id') id: string): void { + Sentry.captureException(new Error(`error with id ${id}`)); + } +} + +@Module({ + imports: [], + controllers: [AppController], + providers: [AppService], +}) +class AppModule {} + +async function init(): Promise { + const app = await NestFactory.create(AppModule); + Sentry.setupNestErrorHandler(app); + await app.listen(port); + sendPortToRunner(port); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +init(); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/test.ts b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/test.ts new file mode 100644 index 000000000000..70eb9e9aaa26 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/test.ts @@ -0,0 +1,40 @@ +import { conditionalTest } from '../../../utils'; +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +jest.setTimeout(20000); + +const { TS_VERSION } = process.env; +const isOldTS = TS_VERSION && TS_VERSION.startsWith('3.'); + +// This is required to run the test with ts-node and decorators +process.env.TS_NODE_PROJECT = `${__dirname}/tsconfig.json`; + +conditionalTest({ min: 16 })('nestjs auto instrumentation', () => { + afterAll(async () => { + cleanupChildProcesses(); + }); + + test("should assign scope's transactionName if express integration is disabled", done => { + if (isOldTS) { + // Skipping test on old TypeScript + return done(); + } + + createRunner(__dirname, 'scenario.ts') + .ignore('transaction') + .expect({ + event: { + exception: { + values: [ + { + value: 'error with id 456', + }, + ], + }, + transaction: 'GET /test-exception/:id', + }, + }) + .start(done) + .makeRequest('get', '/test-exception/456'); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/tsconfig.json b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/tsconfig.json new file mode 100644 index 000000000000..84b8f8d6c44e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/nestjs-no-express/tsconfig.json @@ -0,0 +1,9 @@ +{ + "include": ["scenario.ts"], + "compilerOptions": { + "module": "commonjs", + "emitDecoratorMetadata": true, + "experimentalDecorators": true, + "target": "ES2021", + } +} diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index fb9a211b2a1e..2371c4e93e35 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -1,7 +1,28 @@ import { registerInstrumentations } from '@opentelemetry/instrumentation'; import { NestInstrumentation } from '@opentelemetry/instrumentation-nestjs-core'; -import { captureException, defineIntegration } from '@sentry/core'; +import { captureException, defineIntegration, getDefaultIsolationScope, getIsolationScope } from '@sentry/core'; import type { IntegrationFn } from '@sentry/types'; +import { logger } from '@sentry/utils'; + +interface MinimalNestJsExecutionContext { + switchToHttp: () => { + // minimal request object + // according to official types, all properties are required but + // let's play it safe and assume they're optional + getRequest: () => { + route?: { + path?: string; + }; + method?: string; + }; + }; +} +interface MinimalNestJsApp { + useGlobalFilters: (arg0: { catch(exception: unknown): void }) => void; + useGlobalInterceptors: (interceptor: { + intercept: (context: MinimalNestJsExecutionContext, next: { handle: () => void }) => void; + }) => void; +} const _nestIntegration = (() => { return { @@ -30,8 +51,21 @@ const SentryNestExceptionFilter = { /** * Setup an error handler for Nest. */ -export function setupNestErrorHandler(app: { - useGlobalFilters: (arg0: { catch(exception: unknown): void }) => void; -}): void { +export function setupNestErrorHandler(app: MinimalNestJsApp): void { + app.useGlobalInterceptors({ + intercept(context, next) { + if (getIsolationScope() === getDefaultIsolationScope()) { + logger.warn('Isolation scope is still the default isolation scope, skipping setting transactionName.'); + return next.handle(); + } + + const req = context.switchToHttp().getRequest(); + if (req.route) { + getIsolationScope().setTransactionName(`${req.method?.toUpperCase() || 'GET'} ${req.route.path}`); + } + return next.handle(); + }, + }); + app.useGlobalFilters(SentryNestExceptionFilter); }