From e2668f808a2775ba4bf5105e4881e67bf7d4f28e Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Wed, 31 Jul 2024 13:28:52 +0200 Subject: [PATCH] feat(nestjs): Automatic instrumentation of nestjs guards (#13129) Adds automatic instrumentation to `@sentry/nestjs`. Guards in nest have a `@Injectable` decorator and implement a `canActivate` function. So we can simply extend the existing instrumentation to add a proxy for `canActivate`. Also fixed a mistake with the middleware instrumentation (missing return). --- .../nestjs-basic/src/app.controller.ts | 9 ++- .../nestjs-basic/src/example.guard.ts | 10 +++ .../nestjs-basic/tests/transactions.test.ts | 68 +++++++++++++++++- .../node-nestjs-basic/src/app.controller.ts | 9 ++- .../node-nestjs-basic/src/example.guard.ts | 10 +++ .../tests/transactions.test.ts | 72 ++++++++++++++++++- .../node/src/integrations/tracing/nest.ts | 40 +++++++++-- 7 files changed, 207 insertions(+), 11 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.guard.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.guard.ts 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 c30d8c08ea82..eb0ead5e32d0 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 @@ -1,5 +1,6 @@ -import { Controller, Get, Param } from '@nestjs/common'; +import { Controller, Get, Param, UseGuards } from '@nestjs/common'; import { AppService } from './app.service'; +import { ExampleGuard } from './example.guard'; @Controller() export class AppController { @@ -15,6 +16,12 @@ export class AppController { return this.appService.testMiddleware(); } + @Get('test-guard-instrumentation') + @UseGuards(ExampleGuard) + testGuardInstrumentation() { + return {}; + } + @Get('test-exception/:id') async testException(@Param('id') id: string) { return this.appService.testException(id); diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.guard.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.guard.ts new file mode 100644 index 000000000000..e12bbdc4e994 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.guard.ts @@ -0,0 +1,10 @@ +import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; + +@Injectable() +export class ExampleGuard implements CanActivate { + canActivate(context: ExecutionContext): boolean { + Sentry.startSpan({ name: 'test-guard-span' }, () => {}); + return true; + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts index b7017b72dbf5..ebd8503e1d42 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/tests/transactions.test.ts @@ -132,7 +132,8 @@ test('API route transaction includes nest middleware span. Spans created in and ); }); - await fetch(`${baseURL}/test-middleware-instrumentation`); + const response = await fetch(`${baseURL}/test-middleware-instrumentation`); + expect(response.status).toBe(200); const transactionEvent = await pageloadTransactionEventPromise; @@ -200,3 +201,68 @@ test('API route transaction includes nest middleware span. Spans created in and // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); }); + +test('API route transaction includes nest guard span and span started in guard is nested correctly', async ({ + baseURL, +}) => { + const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-guard-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-guard-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await transactionEventPromise; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleGuard', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + const exampleGuardSpan = transactionEvent.spans.find(span => span.description === 'ExampleGuard'); + const exampleGuardSpanId = exampleGuardSpan?.span_id; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-guard-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + ]), + }), + ); + + // verify correct span parent-child relationships + const testGuardSpan = transactionEvent.spans.find(span => span.description === 'test-guard-span'); + + // 'ExampleGuard' is the parent of 'test-guard-span' + expect(testGuardSpan.parent_span_id).toBe(exampleGuardSpanId); +}); 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 c30d8c08ea82..eb0ead5e32d0 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 @@ -1,5 +1,6 @@ -import { Controller, Get, Param } from '@nestjs/common'; +import { Controller, Get, Param, UseGuards } from '@nestjs/common'; import { AppService } from './app.service'; +import { ExampleGuard } from './example.guard'; @Controller() export class AppController { @@ -15,6 +16,12 @@ export class AppController { return this.appService.testMiddleware(); } + @Get('test-guard-instrumentation') + @UseGuards(ExampleGuard) + testGuardInstrumentation() { + return {}; + } + @Get('test-exception/:id') async testException(@Param('id') id: string) { return this.appService.testException(id); diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.guard.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.guard.ts new file mode 100644 index 000000000000..e12bbdc4e994 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.guard.ts @@ -0,0 +1,10 @@ +import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; + +@Injectable() +export class ExampleGuard implements CanActivate { + canActivate(context: ExecutionContext): boolean { + Sentry.startSpan({ name: 'test-guard-span' }, () => {}); + return true; + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts index b7017b72dbf5..c646ac9aea74 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/tests/transactions.test.ts @@ -125,16 +125,17 @@ test('Sends an API route transaction', async ({ baseURL }) => { test('API route transaction includes nest middleware span. Spans created in and after middleware are nested correctly', async ({ baseURL, }) => { - const pageloadTransactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { return ( transactionEvent?.contexts?.trace?.op === 'http.server' && transactionEvent?.transaction === 'GET /test-middleware-instrumentation' ); }); - await fetch(`${baseURL}/test-middleware-instrumentation`); + const response = await fetch(`${baseURL}/test-middleware-instrumentation`); + expect(response.status).toBe(200); - const transactionEvent = await pageloadTransactionEventPromise; + const transactionEvent = await transactionEventPromise; expect(transactionEvent).toEqual( expect.objectContaining({ @@ -200,3 +201,68 @@ test('API route transaction includes nest middleware span. Spans created in and // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); }); + +test('API route transaction includes nest guard span and span started in guard is nested correctly', async ({ + baseURL, +}) => { + const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-guard-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-guard-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await transactionEventPromise; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleGuard', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + const exampleGuardSpan = transactionEvent.spans.find(span => span.description === 'ExampleGuard'); + const exampleGuardSpanId = exampleGuardSpan?.span_id; + + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-guard-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + ]), + }), + ); + + // verify correct span parent-child relationships + const testGuardSpan = transactionEvent.spans.find(span => span.description === 'test-guard-span'); + + // 'ExampleGuard' is the parent of 'test-guard-span' + expect(testGuardSpan.parent_span_id).toBe(exampleGuardSpanId); +}); diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index b23e4f97da48..dbf3c40ab171 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -17,11 +17,13 @@ import { getDefaultIsolationScope, getIsolationScope, spanToJSON, + startSpan, startSpanManual, withActiveSpan, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; import { addNonEnumerableProperty, logger } from '@sentry/utils'; +import type { Observable } from 'rxjs'; import { generateInstrumentOnce } from '../../otel/instrument'; interface MinimalNestJsExecutionContext { @@ -66,7 +68,10 @@ export interface InjectableTarget { name: string; sentryPatched?: boolean; prototype: { - use?: (req: unknown, res: unknown, next: () => void) => void; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + use?: (req: unknown, res: unknown, next: () => void, ...args: any[]) => void; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + canActivate?: (...args: any[]) => boolean | Promise | Observable; }; } @@ -152,7 +157,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { const [req, res, next, ...args] = argsUse; const prevSpan = getActiveSpan(); - startSpanManual( + return startSpanManual( { name: target.name, attributes: { @@ -167,15 +172,40 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (prevSpan) { withActiveSpan(prevSpan, () => { - Reflect.apply(originalNext, thisArgNext, argsNext); + return Reflect.apply(originalNext, thisArgNext, argsNext); }); } else { - Reflect.apply(originalNext, thisArgNext, argsNext); + return Reflect.apply(originalNext, thisArgNext, argsNext); } }, }); - originalUse.apply(thisArgUse, [req, res, nextProxy, args]); + return originalUse.apply(thisArgUse, [req, res, nextProxy, args]); + }, + ); + }, + }); + } + + // patch guards + if (typeof target.prototype.canActivate === 'function') { + // patch only once + if (isPatched(target)) { + return original(options)(target); + } + + target.prototype.canActivate = new Proxy(target.prototype.canActivate, { + apply: (originalCanActivate, thisArgCanActivate, argsCanActivate) => { + return startSpan( + { + name: target.name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', + }, + }, + () => { + return originalCanActivate.apply(thisArgCanActivate, argsCanActivate); }, ); },