From 545ea049895163fd2bc01f13a7f4571daedae1c4 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Fri, 26 Jul 2024 10:31:13 +0200 Subject: [PATCH 01/27] First non-working implementation of injectable patch --- .../node/src/integrations/tracing/nest.ts | 97 ++++++++++++++++++- 1 file changed, 96 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index ab6a66fdb895..8eb71b0e8187 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -12,6 +12,12 @@ import { import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../otel/instrument'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { + InstrumentationBase, + InstrumentationNodeModuleDefinition, InstrumentationNodeModuleFile +} from '@opentelemetry/instrumentation'; +import { isWrapped } from '@opentelemetry/core'; interface MinimalNestJsExecutionContext { getType: () => string; @@ -44,7 +50,96 @@ interface MinimalNestJsApp { const INTEGRATION_NAME = 'Nest'; -export const instrumentNest = generateInstrumentOnce(INTEGRATION_NAME, () => new NestInstrumentation()); +const supportedVersions = ['>=8.0.0 <11']; + +/** + * + */ +export class SentryNestInstrumentation extends InstrumentationBase { + public static readonly COMPONENT = '@nestjs/common'; + public static readonly COMMON_ATTRIBUTES = { + component: SentryNestInstrumentation.COMPONENT, + }; + + public constructor(config: InstrumentationConfig = {}) { + super('sentry-nestjs', '1.0.0', config); + } + + /** + * + */ + public init(): void { + logger.log('init!'); + const module = new InstrumentationNodeModuleDefinition( + SentryNestInstrumentation.COMPONENT, + supportedVersions + ) + + module.files.push( + this._getInjectableFileInstrumentation(supportedVersions) + ) + } + + /** + * + */ + private _getInjectableFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { + logger.log('create instrumentation node module file'); + return new InstrumentationNodeModuleFile( + '@nestjs/common/decorators/core/injectable.decorator.js', + versions, + (moduleExports: any) => { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (isWrapped(moduleExports.Injectable)) { + this._unwrap(moduleExports, 'Injectable'); + } + this._wrap(moduleExports, 'Injectable', this._createWrapInjectable()); + return moduleExports; + }, + (moduleExports: any) => { + this._unwrap(moduleExports, 'Injectable'); + } + ) + } + + /** + * + */ + private _createWrapInjectable() { + return function wrapInjectable(original: any) { + return function wrappedInjectable(options?: any) { + return function (target: any) { + logger.log('Injectable target:', target); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (typeof target.prototype.use === 'function') { + logger.log('middleware!'); + } + + return original(options)(target); + }; + }; + }; + } +} + +const instrumentNestCore = generateInstrumentOnce('Nest-Core', () => { + logger.log('init nest core instrumentation'); + return new NestInstrumentation(); +}); + +const instrumentMiddleware = generateInstrumentOnce('Nest-Middleware', () => { + logger.log('init nest middleware instrumentation'); + return new SentryNestInstrumentation(); +}); + +export const instrumentNest = Object.assign( + (): void => { + instrumentNestCore(); + instrumentMiddleware(); + }, + { id: INTEGRATION_NAME }, +); const _nestIntegration = (() => { return { From 5aade8a93bd71c71ff1cf2b97c55bfbb01b94667 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Fri, 26 Jul 2024 11:19:42 +0200 Subject: [PATCH 02/27] Add global class middleware in nest application for testing --- .../nestjs-basic/src/app.module.ts | 12 ++++++++++-- .../nestjs-basic/src/logger.middleware.ts | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts index f4c5ceb0cc5a..c733a2786590 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts @@ -1,12 +1,20 @@ -import { Module } from '@nestjs/common'; +import {MiddlewareConsumer, Module} from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { SentryModule } from '@sentry/nestjs/setup'; import { AppController } from './app.controller'; import { AppService } from './app.service'; +import { LoggerMiddleware } from "./logger.middleware"; @Module({ imports: [SentryModule.forRoot(), ScheduleModule.forRoot()], controllers: [AppController], providers: [AppService], }) -export class AppModule {} +export class AppModule { + configure(consumer: MiddlewareConsumer): void { + console.log('Add class middleware.'); + consumer + .apply(LoggerMiddleware) + .forRoutes(AppController); + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts new file mode 100644 index 000000000000..44b8b6f3c8fc --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts @@ -0,0 +1,18 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import { Request, Response, NextFunction } from 'express'; + +function delay(ms) { + const start = Date.now(); + while (Date.now() - start < ms) { + // Do nothing + } +} + +@Injectable() +export class LoggerMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + console.log('Class middleware...'); + delay(500); + next(); + } +} From 6e35eb19aee6713c84f1f4ec9e384f69d46002f8 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 26 Jul 2024 12:09:21 +0000 Subject: [PATCH 03/27] make work lol --- .../nestjs-basic/src/app.module.ts | 8 ++-- .../node/src/integrations/tracing/nest.ts | 48 +++++++++---------- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts index c733a2786590..11ad3fd03fae 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts @@ -1,9 +1,9 @@ -import {MiddlewareConsumer, Module} from '@nestjs/common'; +import { MiddlewareConsumer, Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { SentryModule } from '@sentry/nestjs/setup'; import { AppController } from './app.controller'; import { AppService } from './app.service'; -import { LoggerMiddleware } from "./logger.middleware"; +import { LoggerMiddleware } from './logger.middleware'; @Module({ imports: [SentryModule.forRoot(), ScheduleModule.forRoot()], @@ -13,8 +13,6 @@ import { LoggerMiddleware } from "./logger.middleware"; export class AppModule { configure(consumer: MiddlewareConsumer): void { console.log('Add class middleware.'); - consumer - .apply(LoggerMiddleware) - .forRoutes(AppController); + consumer.apply(LoggerMiddleware).forRoutes(AppController); } } diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 8eb71b0e8187..8f57f9285ebc 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -1,3 +1,10 @@ +import { isWrapped } from '@opentelemetry/core'; +import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; +import { + InstrumentationBase, + InstrumentationNodeModuleDefinition, + InstrumentationNodeModuleFile, +} from '@opentelemetry/instrumentation'; import { NestInstrumentation } from '@opentelemetry/instrumentation-nestjs-core'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, @@ -12,12 +19,6 @@ import { import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../otel/instrument'; -import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; -import { - InstrumentationBase, - InstrumentationNodeModuleDefinition, InstrumentationNodeModuleFile -} from '@opentelemetry/instrumentation'; -import { isWrapped } from '@opentelemetry/core'; interface MinimalNestJsExecutionContext { getType: () => string; @@ -68,23 +69,17 @@ export class SentryNestInstrumentation extends InstrumentationBase { /** * */ - public init(): void { - logger.log('init!'); - const module = new InstrumentationNodeModuleDefinition( - SentryNestInstrumentation.COMPONENT, - supportedVersions - ) - - module.files.push( - this._getInjectableFileInstrumentation(supportedVersions) - ) + public init(): InstrumentationNodeModuleDefinition { + const moduleDef = new InstrumentationNodeModuleDefinition(SentryNestInstrumentation.COMPONENT, supportedVersions); + + moduleDef.files.push(this._getInjectableFileInstrumentation(supportedVersions)); + return moduleDef; } /** * */ private _getInjectableFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { - logger.log('create instrumentation node module file'); return new InstrumentationNodeModuleFile( '@nestjs/common/decorators/core/injectable.decorator.js', versions, @@ -98,8 +93,8 @@ export class SentryNestInstrumentation extends InstrumentationBase { }, (moduleExports: any) => { this._unwrap(moduleExports, 'Injectable'); - } - ) + }, + ); } /** @@ -109,11 +104,18 @@ export class SentryNestInstrumentation extends InstrumentationBase { return function wrapInjectable(original: any) { return function wrappedInjectable(options?: any) { return function (target: any) { - logger.log('Injectable target:', target); - + // TODO: Check if the class was already patched à la + // if (target[sentryPatchedSymbol]) { + // return original(options)(target); + // } else { + // addNonEnumerableProperty(target, sentryPatchedSymbol, true); + // } + + // TODO: proper typing // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if (typeof target.prototype.use === 'function') { - logger.log('middleware!'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-console + console.log('middleware!'); } return original(options)(target); @@ -124,12 +126,10 @@ export class SentryNestInstrumentation extends InstrumentationBase { } const instrumentNestCore = generateInstrumentOnce('Nest-Core', () => { - logger.log('init nest core instrumentation'); return new NestInstrumentation(); }); const instrumentMiddleware = generateInstrumentOnce('Nest-Middleware', () => { - logger.log('init nest middleware instrumentation'); return new SentryNestInstrumentation(); }); From 3a68e897c160768bc7026480a1241b18ec5ce7c8 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 11:29:44 +0200 Subject: [PATCH 04/27] Patching use works --- .../node/src/integrations/tracing/nest.ts | 25 +++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 8f57f9285ebc..a14ac6aa6543 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -14,7 +14,7 @@ import { getClient, getDefaultIsolationScope, getIsolationScope, - spanToJSON, + spanToJSON, startSpanManual, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -114,7 +114,28 @@ export class SentryNestInstrumentation extends InstrumentationBase { // TODO: proper typing // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if (typeof target.prototype.use === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-console + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const originalUse = target.prototype.use; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + target.prototype.use = function (req: any, res: any, next: (error?: Error | any) => void) { + startSpanManual( + { + op: 'middleware.nestjs', + name: '', // TODO: set class name as name + attributes: {} + }, + (span: Span) => { + const wrappedNext = (error?: Error | any): void => { + span.end(); + next(error); + }; + console.log('use!'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + return originalUse.apply(this, [req, res, wrappedNext]) + } + ) + } console.log('middleware!'); } From 283a7a0b1b9ee6003355e1d68d6e1a496905bc3e Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 11:35:00 +0200 Subject: [PATCH 05/27] . --- packages/node/src/integrations/tracing/nest.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index a14ac6aa6543..809ea178aeb9 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -118,7 +118,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { const originalUse = target.prototype.use; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - target.prototype.use = function (req: any, res: any, next: (error?: Error | any) => void) { + target.prototype.use = function (req: any, res: any, next: any) { startSpanManual( { op: 'middleware.nestjs', @@ -126,6 +126,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { attributes: {} }, (span: Span) => { + // patch next to end span before next middleware is being called const wrappedNext = (error?: Error | any): void => { span.end(); next(error); From 131caf097923ffc4851c1b5c78e0d6a9638b5740 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:00:14 +0200 Subject: [PATCH 06/27] Improve span attributes --- packages/node/src/integrations/tracing/nest.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 809ea178aeb9..59b9a3c87ee9 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -122,8 +122,10 @@ export class SentryNestInstrumentation extends InstrumentationBase { startSpanManual( { op: 'middleware.nestjs', - name: '', // TODO: set class name as name - attributes: {} + name: 'middleware', // TODO: set class name as name + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs' + } }, (span: Span) => { // patch next to end span before next middleware is being called From f1e82187544ec42dc5ad2f664f2ff9b499ca5bc4 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:06:24 +0200 Subject: [PATCH 07/27] Update description of span --- packages/node/src/integrations/tracing/nest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 59b9a3c87ee9..45e2ab9db81a 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -122,7 +122,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { startSpanManual( { op: 'middleware.nestjs', - name: 'middleware', // TODO: set class name as name + name: target.name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs' } From 4f20e341aa3280f1b4ccc48b90bf985f58f783ce Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:25:35 +0200 Subject: [PATCH 08/27] Add test in nestjs-basic to check if middleware span exists --- .../nestjs-basic/src/app.controller.ts | 5 +++ .../nestjs-basic/src/app.module.ts | 3 +- .../nestjs-basic/src/logger.middleware.ts | 1 - .../nestjs-basic/tests/transactions.test.ts | 36 +++++++++++++++++++ .../node/src/integrations/tracing/nest.ts | 2 +- 5 files changed, 43 insertions(+), 4 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 index b54604d999cb..02f6bde6f80c 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 @@ -10,6 +10,11 @@ export class AppController { return this.appService.testTransaction(); } + @Get('test-middleware-instrumentation') + testMiddlewareInstrumentation() { + 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/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts index 11ad3fd03fae..5f15d7f7993d 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts @@ -12,7 +12,6 @@ import { LoggerMiddleware } from './logger.middleware'; }) export class AppModule { configure(consumer: MiddlewareConsumer): void { - console.log('Add class middleware.'); - consumer.apply(LoggerMiddleware).forRoutes(AppController); + consumer.apply(LoggerMiddleware).forRoutes('test-middleware-instrumentation'); } } diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts index 44b8b6f3c8fc..0f88dd86c174 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts @@ -11,7 +11,6 @@ function delay(ms) { @Injectable() export class LoggerMiddleware implements NestMiddleware { use(req: Request, res: Response, next: NextFunction) { - console.log('Class middleware...'); delay(500); next(); } 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 ae5d8b63b16d..7b6709b847e6 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 @@ -121,3 +121,39 @@ test('Sends an API route transaction', async ({ baseURL }) => { }), ); }); + +test('API route transaction includes nest middleware span', async ({ baseURL }) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-middleware-instrumentation' + ); + }); + + await fetch(`${baseURL}/test-middleware-instrumentation`); + + const transactionEvent = await pageloadTransactionEventPromise; + + console.log(transactionEvent); + 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: 'LoggerMiddleware', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs' + }, + ]), + }), + ); +}); diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 45e2ab9db81a..ed4fa6c5f771 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -121,9 +121,9 @@ export class SentryNestInstrumentation extends InstrumentationBase { target.prototype.use = function (req: any, res: any, next: any) { startSpanManual( { - op: 'middleware.nestjs', name: target.name, attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs' } }, From 42c77317f41051e96a7618bef2eb86b7d7ebb838 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:30:06 +0200 Subject: [PATCH 09/27] Add test for node-nestjs-basic --- .../nestjs-basic/src/logger.middleware.ts | 1 + .../node-nestjs-basic/src/app.controller.ts | 5 +++ .../node-nestjs-basic/src/app.module.ts | 9 +++-- .../src/logger.middleware.ts | 18 ++++++++++ .../tests/transactions.test.ts | 36 +++++++++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts index 0f88dd86c174..bcedc82e3c7f 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts @@ -11,6 +11,7 @@ function delay(ms) { @Injectable() export class LoggerMiddleware implements NestMiddleware { use(req: Request, res: Response, next: NextFunction) { + console.log('Logger middleware!'); delay(500); next(); } 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 b54604d999cb..02f6bde6f80c 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 @@ -10,6 +10,11 @@ export class AppController { return this.appService.testTransaction(); } + @Get('test-middleware-instrumentation') + testMiddlewareInstrumentation() { + 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/app.module.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts index ceb7199a99cf..316c836f779b 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts @@ -1,11 +1,16 @@ -import { Module } from '@nestjs/common'; +import {MiddlewareConsumer, Module} from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { AppController } from './app.controller'; import { AppService } from './app.service'; +import {LoggerMiddleware} from "./logger.middleware"; @Module({ imports: [ScheduleModule.forRoot()], controllers: [AppController], providers: [AppService], }) -export class AppModule {} +export class AppModule { + configure(consumer: MiddlewareConsumer): void { + consumer.apply(LoggerMiddleware).forRoutes('test-middleware-instrumentation'); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts new file mode 100644 index 000000000000..bcedc82e3c7f --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts @@ -0,0 +1,18 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import { Request, Response, NextFunction } from 'express'; + +function delay(ms) { + const start = Date.now(); + while (Date.now() - start < ms) { + // Do nothing + } +} + +@Injectable() +export class LoggerMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + console.log('Logger middleware!'); + delay(500); + next(); + } +} 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 ae5d8b63b16d..7b6709b847e6 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 @@ -121,3 +121,39 @@ test('Sends an API route transaction', async ({ baseURL }) => { }), ); }); + +test('API route transaction includes nest middleware span', async ({ baseURL }) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-middleware-instrumentation' + ); + }); + + await fetch(`${baseURL}/test-middleware-instrumentation`); + + const transactionEvent = await pageloadTransactionEventPromise; + + console.log(transactionEvent); + 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: 'LoggerMiddleware', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs' + }, + ]), + }), + ); +}); From 7f8ccf0487d0c8c0b8611fece35a3ba4b44c72ab Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:31:35 +0200 Subject: [PATCH 10/27] Cleaning logs --- .../test-applications/nestjs-basic/tests/transactions.test.ts | 1 - .../node-nestjs-basic/tests/transactions.test.ts | 1 - packages/node/src/integrations/tracing/nest.ts | 3 +-- 3 files changed, 1 insertion(+), 4 deletions(-) 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 7b6709b847e6..61580298d5d7 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 @@ -134,7 +134,6 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) const transactionEvent = await pageloadTransactionEventPromise; - console.log(transactionEvent); expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ 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 7b6709b847e6..61580298d5d7 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 @@ -134,7 +134,6 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) const transactionEvent = await pageloadTransactionEventPromise; - console.log(transactionEvent); expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index ed4fa6c5f771..7541d96a5a83 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -133,13 +133,12 @@ export class SentryNestInstrumentation extends InstrumentationBase { span.end(); next(error); }; - console.log('use!'); + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access return originalUse.apply(this, [req, res, wrappedNext]) } ) } - console.log('middleware!'); } return original(options)(target); From d320894c1ec7d91339e48c45f4a28330cd095f92 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:43:17 +0200 Subject: [PATCH 11/27] Add docstrings --- packages/node/src/integrations/tracing/nest.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 7541d96a5a83..674da1c302fb 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -54,7 +54,9 @@ const INTEGRATION_NAME = 'Nest'; const supportedVersions = ['>=8.0.0 <11']; /** + * Custom instrumentation for nestjs. * + * This hooks into the @Injectable decorator, which is applied on class middleware, interceptors and guards. */ export class SentryNestInstrumentation extends InstrumentationBase { public static readonly COMPONENT = '@nestjs/common'; @@ -63,11 +65,11 @@ export class SentryNestInstrumentation extends InstrumentationBase { }; public constructor(config: InstrumentationConfig = {}) { - super('sentry-nestjs', '1.0.0', config); + super('sentry-nestjs', '1.0.0', config); // TODO: instrumentationVersion } /** - * + * Initializes the instrumentation by defining the modules to be patched. */ public init(): InstrumentationNodeModuleDefinition { const moduleDef = new InstrumentationNodeModuleDefinition(SentryNestInstrumentation.COMPONENT, supportedVersions); @@ -77,7 +79,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { } /** - * + * Wraps the @Injectable decorator. */ private _getInjectableFileInstrumentation(versions: string[]): InstrumentationNodeModuleFile { return new InstrumentationNodeModuleFile( @@ -98,7 +100,9 @@ export class SentryNestInstrumentation extends InstrumentationBase { } /** + * Creates a wrapper function for the @Injectable decorator. * + * Wraps the use method to instrument nest class middleware. */ private _createWrapInjectable() { return function wrapInjectable(original: any) { From 9957d5bae36dcf73652ed87190692d12a058c38b Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 13:57:50 +0200 Subject: [PATCH 12/27] Improve typing --- .../node/src/integrations/tracing/nest.ts | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 674da1c302fb..64a047276d01 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -53,6 +53,16 @@ const INTEGRATION_NAME = 'Nest'; const supportedVersions = ['>=8.0.0 <11']; +/** + * Represents an injectable target class in NestJS. + */ +interface InjectableTarget { + name: string, + prototype: { + use?: (req: unknown, res: unknown, next: (error?: Error | unknown) => void) => void; + }; +} + /** * Custom instrumentation for nestjs. * @@ -85,15 +95,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { return new InstrumentationNodeModuleFile( '@nestjs/common/decorators/core/injectable.decorator.js', versions, - (moduleExports: any) => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (moduleExports: { Injectable: unknown }) => { if (isWrapped(moduleExports.Injectable)) { this._unwrap(moduleExports, 'Injectable'); } this._wrap(moduleExports, 'Injectable', this._createWrapInjectable()); return moduleExports; }, - (moduleExports: any) => { + (moduleExports: { Injectable: unknown }) => { this._unwrap(moduleExports, 'Injectable'); }, ); @@ -105,9 +114,9 @@ export class SentryNestInstrumentation extends InstrumentationBase { * Wraps the use method to instrument nest class middleware. */ private _createWrapInjectable() { - return function wrapInjectable(original: any) { - return function wrappedInjectable(options?: any) { - return function (target: any) { + return function wrapInjectable(original: any) { // TODO: improve type here + return function wrappedInjectable(options?: unknown) { + return function (target: InjectableTarget) { // TODO: Check if the class was already patched à la // if (target[sentryPatchedSymbol]) { // return original(options)(target); @@ -115,13 +124,9 @@ export class SentryNestInstrumentation extends InstrumentationBase { // addNonEnumerableProperty(target, sentryPatchedSymbol, true); // } - // TODO: proper typing - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if (typeof target.prototype.use === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access const originalUse = target.prototype.use; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access target.prototype.use = function (req: any, res: any, next: any) { startSpanManual( { @@ -133,12 +138,11 @@ export class SentryNestInstrumentation extends InstrumentationBase { }, (span: Span) => { // patch next to end span before next middleware is being called - const wrappedNext = (error?: Error | any): void => { + const wrappedNext = (error?: Error | unknown): void => { span.end(); next(error); }; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access return originalUse.apply(this, [req, res, wrappedNext]) } ) From ef1d0d09e37162e140ecad22472216e721f9bf7e Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 14:06:24 +0200 Subject: [PATCH 13/27] Linting --- .../nestjs-basic/src/logger.middleware.ts | 2 +- .../nestjs-basic/tests/transactions.test.ts | 2 +- .../node-nestjs-basic/src/app.module.ts | 4 +-- .../src/logger.middleware.ts | 2 +- .../tests/transactions.test.ts | 2 +- .../node/src/integrations/tracing/nest.ts | 25 ++++++++++--------- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts index bcedc82e3c7f..606a0ddd30b1 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts @@ -1,5 +1,5 @@ import { Injectable, NestMiddleware } from '@nestjs/common'; -import { Request, Response, NextFunction } from 'express'; +import { NextFunction, Request, Response } from 'express'; function delay(ms) { const start = Date.now(); 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 61580298d5d7..be1cd47b6833 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 @@ -150,7 +150,7 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) timestamp: expect.any(Number), status: 'ok', op: 'middleware.nestjs', - origin: 'auto.middleware.nestjs' + origin: 'auto.middleware.nestjs', }, ]), }), diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts index 316c836f779b..bc0a37000e47 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts @@ -1,8 +1,8 @@ -import {MiddlewareConsumer, Module} from '@nestjs/common'; +import { MiddlewareConsumer, Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { AppController } from './app.controller'; import { AppService } from './app.service'; -import {LoggerMiddleware} from "./logger.middleware"; +import { LoggerMiddleware } from './logger.middleware'; @Module({ imports: [ScheduleModule.forRoot()], diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts index bcedc82e3c7f..606a0ddd30b1 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts @@ -1,5 +1,5 @@ import { Injectable, NestMiddleware } from '@nestjs/common'; -import { Request, Response, NextFunction } from 'express'; +import { NextFunction, Request, Response } from 'express'; function delay(ms) { const start = Date.now(); 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 61580298d5d7..be1cd47b6833 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 @@ -150,7 +150,7 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) timestamp: expect.any(Number), status: 'ok', op: 'middleware.nestjs', - origin: 'auto.middleware.nestjs' + origin: 'auto.middleware.nestjs', }, ]), }), diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 64a047276d01..0da6eba1a97c 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -14,7 +14,8 @@ import { getClient, getDefaultIsolationScope, getIsolationScope, - spanToJSON, startSpanManual, + spanToJSON, + startSpanManual, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -57,7 +58,7 @@ const supportedVersions = ['>=8.0.0 <11']; * Represents an injectable target class in NestJS. */ interface InjectableTarget { - name: string, + name: string; prototype: { use?: (req: unknown, res: unknown, next: (error?: Error | unknown) => void) => void; }; @@ -95,14 +96,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { return new InstrumentationNodeModuleFile( '@nestjs/common/decorators/core/injectable.decorator.js', versions, - (moduleExports: { Injectable: unknown }) => { + (moduleExports: { Injectable: InjectableTarget }) => { if (isWrapped(moduleExports.Injectable)) { this._unwrap(moduleExports, 'Injectable'); } this._wrap(moduleExports, 'Injectable', this._createWrapInjectable()); return moduleExports; }, - (moduleExports: { Injectable: unknown }) => { + (moduleExports: { Injectable: InjectableTarget }) => { this._unwrap(moduleExports, 'Injectable'); }, ); @@ -114,7 +115,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { * Wraps the use method to instrument nest class middleware. */ private _createWrapInjectable() { - return function wrapInjectable(original: any) { // TODO: improve type here + return function wrapInjectable(original: any) { return function wrappedInjectable(options?: unknown) { return function (target: InjectableTarget) { // TODO: Check if the class was already patched à la @@ -127,14 +128,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (typeof target.prototype.use === 'function') { const originalUse = target.prototype.use; - target.prototype.use = function (req: any, res: any, next: any) { + target.prototype.use = function (req: unknown, res: unknown, next: (error?: unknown) => void): void { startSpanManual( { name: target.name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs' - } + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', + }, }, (span: Span) => { // patch next to end span before next middleware is being called @@ -143,10 +144,10 @@ export class SentryNestInstrumentation extends InstrumentationBase { next(error); }; - return originalUse.apply(this, [req, res, wrappedNext]) - } - ) - } + return originalUse.apply(this, [req, res, wrappedNext]); + }, + ); + }; } return original(options)(target); From 3fdcfef42181e30009a5f70c74cf20016aa62989 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 14:07:56 +0200 Subject: [PATCH 14/27] . --- packages/node/src/integrations/tracing/nest.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 0da6eba1a97c..8d8f194ab9cc 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -115,6 +115,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { * Wraps the use method to instrument nest class middleware. */ private _createWrapInjectable() { + // eslint-disable-next-line @typescript-eslint/no-explicit-any return function wrapInjectable(original: any) { return function wrappedInjectable(options?: unknown) { return function (target: InjectableTarget) { From 2392da774326bd28f692555d1b395a8c76ca1c56 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 15:55:07 +0200 Subject: [PATCH 15/27] Do not nest created spans --- packages/node/src/integrations/tracing/nest.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 8d8f194ab9cc..02c0b1329ceb 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -11,11 +11,13 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, captureException, defineIntegration, + getActiveSpan, getClient, getDefaultIsolationScope, getIsolationScope, spanToJSON, startSpanManual, + withActiveSpan, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; import { logger } from '@sentry/utils'; @@ -130,6 +132,8 @@ export class SentryNestInstrumentation extends InstrumentationBase { const originalUse = target.prototype.use; target.prototype.use = function (req: unknown, res: unknown, next: (error?: unknown) => void): void { + const prevSpan = getActiveSpan(); + startSpanManual( { name: target.name, @@ -142,7 +146,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { // patch next to end span before next middleware is being called const wrappedNext = (error?: Error | unknown): void => { span.end(); - next(error); + + if (prevSpan) { + withActiveSpan(prevSpan, () => { + next(error); + }); + } else { + next(error); + } }; return originalUse.apply(this, [req, res, wrappedNext]); From b28601d0c8b7508d190df5fa24e17be75fe25818 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 16:19:30 +0200 Subject: [PATCH 16/27] Check for proper nesting in node-nestjs-basic middleware test --- .../node-nestjs-basic/src/app.controller.ts | 2 +- .../node-nestjs-basic/src/app.module.ts | 4 +- .../node-nestjs-basic/src/app.service.ts | 5 +++ .../src/example.middleware.ts | 12 +++++ .../src/logger.middleware.ts | 18 -------- .../tests/transactions.test.ts | 44 ++++++++++++++++++- 6 files changed, 63 insertions(+), 22 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts delete mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts 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 02f6bde6f80c..2a4f14cae541 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 @@ -12,7 +12,7 @@ export class AppController { @Get('test-middleware-instrumentation') testMiddlewareInstrumentation() { - return {}; + return this.appService.testMiddleware(); } @Get('test-exception/:id') diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts index bc0a37000e47..567dbefeadb7 100644 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/app.module.ts @@ -2,7 +2,7 @@ import { MiddlewareConsumer, Module } from '@nestjs/common'; import { ScheduleModule } from '@nestjs/schedule'; import { AppController } from './app.controller'; import { AppService } from './app.service'; -import { LoggerMiddleware } from './logger.middleware'; +import { ExampleMiddleware } from './example.middleware'; @Module({ imports: [ScheduleModule.forRoot()], @@ -11,6 +11,6 @@ import { LoggerMiddleware } from './logger.middleware'; }) export class AppModule { configure(consumer: MiddlewareConsumer): void { - consumer.apply(LoggerMiddleware).forRoutes('test-middleware-instrumentation'); + consumer.apply(ExampleMiddleware).forRoutes('test-middleware-instrumentation'); } } 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 3afb7b5147bd..9a47f0e08e7a 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 @@ -21,6 +21,11 @@ export class AppService { }); } + testMiddleware() { + // span that should not be a child span of the middleware span + Sentry.startSpan({ name: 'test-controller-span' }, () => {}); + } + testException(id: string) { throw new Error(`This is an exception with id ${id}`); } diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts new file mode 100644 index 000000000000..31d15c9372ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.middleware.ts @@ -0,0 +1,12 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { NextFunction, Request, Response } from 'express'; + +@Injectable() +export class ExampleMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + // span that should be a child span of the middleware span + Sentry.startSpan({ name: 'test-middleware-span' }, () => {}); + next(); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts deleted file mode 100644 index 606a0ddd30b1..000000000000 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/logger.middleware.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { Injectable, NestMiddleware } from '@nestjs/common'; -import { NextFunction, Request, Response } from 'express'; - -function delay(ms) { - const start = Date.now(); - while (Date.now() - start < ms) { - // Do nothing - } -} - -@Injectable() -export class LoggerMiddleware implements NestMiddleware { - use(req: Request, res: Response, next: NextFunction) { - console.log('Logger middleware!'); - delay(500); - next(); - } -} 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 be1cd47b6833..e899ebfec22b 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 @@ -144,7 +144,7 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'LoggerMiddleware', + description: 'ExampleMiddleware', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -155,4 +155,46 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) ]), }), ); + + const exampleMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'ExampleMiddleware'); + const exampleMiddlewareSpanId = exampleMiddlewareSpan?.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-controller-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-middleware-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 testMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'test-middleware-span'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleMiddleware' is the parent of 'test-middleware-span' + expect(testMiddlewareSpan.parent_span_id).toBe(exampleMiddlewareSpanId); + + // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); }); From 453a3fd5fa5fb0cd369c51bbd7341893d8ca21e3 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 16:26:08 +0200 Subject: [PATCH 17/27] Tests --- .../nestjs-basic/src/app.controller.ts | 2 +- .../nestjs-basic/src/app.module.ts | 4 +- .../nestjs-basic/src/app.service.ts | 5 ++ .../nestjs-basic/src/example.middleware.ts | 12 +++++ .../nestjs-basic/src/logger.middleware.ts | 18 ------- .../nestjs-basic/tests/transactions.test.ts | 48 ++++++++++++++++++- .../tests/transactions.test.ts | 4 +- 7 files changed, 69 insertions(+), 24 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts delete mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.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 02f6bde6f80c..2a4f14cae541 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 @@ -12,7 +12,7 @@ export class AppController { @Get('test-middleware-instrumentation') testMiddlewareInstrumentation() { - return {}; + return this.appService.testMiddleware(); } @Get('test-exception/:id') diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts index 5f15d7f7993d..b2aad014c745 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/app.module.ts @@ -3,7 +3,7 @@ import { ScheduleModule } from '@nestjs/schedule'; import { SentryModule } from '@sentry/nestjs/setup'; import { AppController } from './app.controller'; import { AppService } from './app.service'; -import { LoggerMiddleware } from './logger.middleware'; +import { ExampleMiddleware } from './example.middleware'; @Module({ imports: [SentryModule.forRoot(), ScheduleModule.forRoot()], @@ -12,6 +12,6 @@ import { LoggerMiddleware } from './logger.middleware'; }) export class AppModule { configure(consumer: MiddlewareConsumer): void { - consumer.apply(LoggerMiddleware).forRoutes('test-middleware-instrumentation'); + consumer.apply(ExampleMiddleware).forRoutes('test-middleware-instrumentation'); } } 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 3afb7b5147bd..9a47f0e08e7a 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 @@ -21,6 +21,11 @@ export class AppService { }); } + testMiddleware() { + // span that should not be a child span of the middleware span + Sentry.startSpan({ name: 'test-controller-span' }, () => {}); + } + testException(id: string) { throw new Error(`This is an exception with id ${id}`); } diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts new file mode 100644 index 000000000000..31d15c9372ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.middleware.ts @@ -0,0 +1,12 @@ +import { Injectable, NestMiddleware } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { NextFunction, Request, Response } from 'express'; + +@Injectable() +export class ExampleMiddleware implements NestMiddleware { + use(req: Request, res: Response, next: NextFunction) { + // span that should be a child span of the middleware span + Sentry.startSpan({ name: 'test-middleware-span' }, () => {}); + next(); + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts deleted file mode 100644 index 606a0ddd30b1..000000000000 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/logger.middleware.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { Injectable, NestMiddleware } from '@nestjs/common'; -import { NextFunction, Request, Response } from 'express'; - -function delay(ms) { - const start = Date.now(); - while (Date.now() - start < ms) { - // Do nothing - } -} - -@Injectable() -export class LoggerMiddleware implements NestMiddleware { - use(req: Request, res: Response, next: NextFunction) { - console.log('Logger middleware!'); - delay(500); - next(); - } -} 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 be1cd47b6833..b7017b72dbf5 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 @@ -122,7 +122,9 @@ test('Sends an API route transaction', async ({ baseURL }) => { ); }); -test('API route transaction includes nest middleware span', 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 => { return ( transactionEvent?.contexts?.trace?.op === 'http.server' && @@ -144,7 +146,7 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'LoggerMiddleware', + description: 'ExampleMiddleware', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -155,4 +157,46 @@ test('API route transaction includes nest middleware span', async ({ baseURL }) ]), }), ); + + const exampleMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'ExampleMiddleware'); + const exampleMiddlewareSpanId = exampleMiddlewareSpan?.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-controller-span', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + origin: 'manual', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-middleware-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 testMiddlewareSpan = transactionEvent.spans.find(span => span.description === 'test-middleware-span'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleMiddleware' is the parent of 'test-middleware-span' + expect(testMiddlewareSpan.parent_span_id).toBe(exampleMiddlewareSpanId); + + // 'ExampleMiddleware' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleMiddlewareSpanId); }); 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 e899ebfec22b..b7017b72dbf5 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 @@ -122,7 +122,9 @@ test('Sends an API route transaction', async ({ baseURL }) => { ); }); -test('API route transaction includes nest middleware span', 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 => { return ( transactionEvent?.contexts?.trace?.op === 'http.server' && From f70c606ab98a691dbe81b83d38a07363d1968c46 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Mon, 29 Jul 2024 16:57:05 +0200 Subject: [PATCH 18/27] Check that class is not patched multiple times --- packages/node/src/integrations/tracing/nest.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 02c0b1329ceb..21ce45eec90a 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -56,11 +56,14 @@ const INTEGRATION_NAME = 'Nest'; const supportedVersions = ['>=8.0.0 <11']; +const sentryPatched = Symbol('sentryPatched'); + /** * Represents an injectable target class in NestJS. */ interface InjectableTarget { name: string; + sentryPatched?: symbol; prototype: { use?: (req: unknown, res: unknown, next: (error?: Error | unknown) => void) => void; }; @@ -121,13 +124,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { return function wrapInjectable(original: any) { return function wrappedInjectable(options?: unknown) { return function (target: InjectableTarget) { - // TODO: Check if the class was already patched à la - // if (target[sentryPatchedSymbol]) { - // return original(options)(target); - // } else { - // addNonEnumerableProperty(target, sentryPatchedSymbol, true); - // } + // ensure class has not been patched before + if (target.sentryPatched) { + return original(options)(target); + } else { + Object.defineProperty(target, sentryPatched, { value: true }); + } + // patch middleware if (typeof target.prototype.use === 'function') { const originalUse = target.prototype.use; From 30b97910192747fb3cb4829828be69e9de4e9d00 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 09:44:54 +0200 Subject: [PATCH 19/27] Address pr comments --- .../node/src/integrations/tracing/nest.ts | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 21ce45eec90a..524a4093ab07 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -20,7 +20,7 @@ import { withActiveSpan, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; -import { logger } from '@sentry/utils'; +import {addNonEnumerableProperty, logger} from '@sentry/utils'; import { generateInstrumentOnce } from '../../otel/instrument'; interface MinimalNestJsExecutionContext { @@ -56,19 +56,31 @@ const INTEGRATION_NAME = 'Nest'; const supportedVersions = ['>=8.0.0 <11']; -const sentryPatched = Symbol('sentryPatched'); +const sentryPatched = 'sentryPatched'; /** * Represents an injectable target class in NestJS. */ interface InjectableTarget { name: string; - sentryPatched?: symbol; + sentryPatched?: boolean; prototype: { use?: (req: unknown, res: unknown, next: (error?: Error | unknown) => void) => void; }; } +/** + * Helper checking if a target is already patched. + */ +function isPatched(target: InjectableTarget): boolean { + if (target.sentryPatched) { + return true; + } + + addNonEnumerableProperty(target, sentryPatched, true); + return false; +} + /** * Custom instrumentation for nestjs. * @@ -124,15 +136,13 @@ export class SentryNestInstrumentation extends InstrumentationBase { return function wrapInjectable(original: any) { return function wrappedInjectable(options?: unknown) { return function (target: InjectableTarget) { - // ensure class has not been patched before - if (target.sentryPatched) { - return original(options)(target); - } else { - Object.defineProperty(target, sentryPatched, { value: true }); - } - // patch middleware if (typeof target.prototype.use === 'function') { + // patch only once + if (isPatched(target)) { + return original(options)(target); + } + const originalUse = target.prototype.use; target.prototype.use = function (req: unknown, res: unknown, next: (error?: unknown) => void): void { @@ -177,14 +187,14 @@ const instrumentNestCore = generateInstrumentOnce('Nest-Core', () => { return new NestInstrumentation(); }); -const instrumentMiddleware = generateInstrumentOnce('Nest-Middleware', () => { +const instrumentNestCommon = generateInstrumentOnce('Nest-Common', () => { return new SentryNestInstrumentation(); }); export const instrumentNest = Object.assign( (): void => { instrumentNestCore(); - instrumentMiddleware(); + instrumentNestCommon(); }, { id: INTEGRATION_NAME }, ); From 02a62ee99929cf9280cfbfa8143f50be6457db7a Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 09:54:58 +0200 Subject: [PATCH 20/27] Lint --- packages/node/src/integrations/tracing/nest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 524a4093ab07..256ee8081116 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -20,7 +20,7 @@ import { withActiveSpan, } from '@sentry/core'; import type { IntegrationFn, Span } from '@sentry/types'; -import {addNonEnumerableProperty, logger} from '@sentry/utils'; +import { addNonEnumerableProperty, logger } from '@sentry/utils'; import { generateInstrumentOnce } from '../../otel/instrument'; interface MinimalNestJsExecutionContext { From eaac26d2b581f0fb6371e47576e929d2887b0027 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 10:09:45 +0200 Subject: [PATCH 21/27] Simplify generation of nest instrumentatino --- packages/node/src/integrations/tracing/nest.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 256ee8081116..3be964ae2ff1 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -191,13 +191,11 @@ const instrumentNestCommon = generateInstrumentOnce('Nest-Common', () => { return new SentryNestInstrumentation(); }); -export const instrumentNest = Object.assign( - (): void => { - instrumentNestCore(); - instrumentNestCommon(); - }, - { id: INTEGRATION_NAME }, -); +export const instrumentNest = (): void => { + instrumentNestCore(); + instrumentNestCommon(); +}; +instrumentNest.id = INTEGRATION_NAME; const _nestIntegration = (() => { return { From c47590c284b9cd538d7bc4e251b6508e8d294d67 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 11:53:11 +0200 Subject: [PATCH 22/27] Wrap next in proxy --- .../node/src/integrations/tracing/nest.ts | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 3be964ae2ff1..a06139e933a5 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -157,20 +157,21 @@ export class SentryNestInstrumentation extends InstrumentationBase { }, }, (span: Span) => { - // patch next to end span before next middleware is being called - const wrappedNext = (error?: Error | unknown): void => { - span.end(); - - if (prevSpan) { - withActiveSpan(prevSpan, () => { - next(error); - }); - } else { - next(error); - } - }; - - return originalUse.apply(this, [req, res, wrappedNext]); + const nextProxy = new Proxy(next, { + apply: (originalFunction, thisArg, args) => { + span.end(); + + if (prevSpan) { + withActiveSpan(prevSpan, () => { + originalFunction.apply(thisArg, args); + }); + } else { + originalFunction.apply(thisArg, args); + } + }, + }); + + return originalUse.apply(this, [req, res, nextProxy]); }, ); }; From c3ed4ca4962df7ee6f99c0264a5d097f81659789 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 12:02:35 +0200 Subject: [PATCH 23/27] Revert to Object.assign --- packages/node/src/integrations/tracing/nest.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index a06139e933a5..ae1441c93408 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -192,11 +192,13 @@ const instrumentNestCommon = generateInstrumentOnce('Nest-Common', () => { return new SentryNestInstrumentation(); }); -export const instrumentNest = (): void => { - instrumentNestCore(); - instrumentNestCommon(); -}; -instrumentNest.id = INTEGRATION_NAME; +export const instrumentNest = Object.assign( + (): void => { + instrumentNestCore(); + instrumentNestCommon(); + }, + { id: INTEGRATION_NAME }, +); const _nestIntegration = (() => { return { From c0c3195a3dc3ae1aeecd59bc4e6ed69db45f2e5d Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 13:43:46 +0200 Subject: [PATCH 24/27] Update use to be proxy --- .../node/src/integrations/tracing/nest.ts | 65 ++++++++++--------- 1 file changed, 33 insertions(+), 32 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index ae1441c93408..6bfc49704d49 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -65,7 +65,7 @@ interface InjectableTarget { name: string; sentryPatched?: boolean; prototype: { - use?: (req: unknown, res: unknown, next: (error?: Error | unknown) => void) => void; + use?: (req: unknown, res: unknown, next: () => void) => void; }; } @@ -143,38 +143,39 @@ export class SentryNestInstrumentation extends InstrumentationBase { return original(options)(target); } - const originalUse = target.prototype.use; - - target.prototype.use = function (req: unknown, res: unknown, next: (error?: unknown) => void): void { - const prevSpan = getActiveSpan(); - - startSpanManual( - { - name: target.name, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', - }, - }, - (span: Span) => { - const nextProxy = new Proxy(next, { - apply: (originalFunction, thisArg, args) => { - span.end(); - - if (prevSpan) { - withActiveSpan(prevSpan, () => { - originalFunction.apply(thisArg, args); - }); - } else { - originalFunction.apply(thisArg, args); - } + target.prototype.use = new Proxy(target.prototype.use, { + apply: (originalUse, thisArgUse, argsUse) => { + const [req, res, next] = argsUse; + const prevSpan = getActiveSpan(); + + startSpanManual( + { + name: target.name, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', }, - }); - - return originalUse.apply(this, [req, res, nextProxy]); - }, - ); - }; + }, + (span: Span) => { + const nextProxy = new Proxy(next, { + apply: (originalNext, thisArgNext, argsNext) => { + span.end(); + + if (prevSpan) { + withActiveSpan(prevSpan, () => { + Reflect.apply(originalNext, thisArgNext, argsNext); + }); + } else { + Reflect.apply(originalNext, thisArgNext, argsNext); + } + }, + }); + + originalUse.apply(thisArgUse, [req, res, nextProxy]); + }, + ); + }, + }); } return original(options)(target); From 7fdc40275805ca9aadc4ee3122567f9df5ca46d6 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 14:11:22 +0200 Subject: [PATCH 25/27] Pass all arguments to use --- packages/node/src/integrations/tracing/nest.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 6bfc49704d49..1b39f74fa6c6 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -70,7 +70,10 @@ interface InjectableTarget { } /** - * Helper checking if a target is already patched. + * Helper checking if a concrete target class is already patched. + * + * We already guard duplicate patching with isWrapped. However, isWrapped checks whether a file has been patched, whereas we use this check for concrete target classes. + * This check might not be necessary, but better to play it safe. */ function isPatched(target: InjectableTarget): boolean { if (target.sentryPatched) { @@ -145,7 +148,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { target.prototype.use = new Proxy(target.prototype.use, { apply: (originalUse, thisArgUse, argsUse) => { - const [req, res, next] = argsUse; + const [req, res, next, ...args] = argsUse; const prevSpan = getActiveSpan(); startSpanManual( @@ -171,7 +174,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { }, }); - originalUse.apply(thisArgUse, [req, res, nextProxy]); + originalUse.apply(thisArgUse, [req, res, nextProxy, args]); }, ); }, From 72a90683c1b0ea07dd0be684dca8a375f13803eb Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 15:33:17 +0200 Subject: [PATCH 26/27] Add unit tests for isPatched --- packages/node/src/integrations/tracing/nest.ts | 4 ++-- .../node/test/integrations/tracing/nest.test.ts | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 packages/node/test/integrations/tracing/nest.test.ts diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index 1b39f74fa6c6..a7129dc10cd5 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -61,7 +61,7 @@ const sentryPatched = 'sentryPatched'; /** * Represents an injectable target class in NestJS. */ -interface InjectableTarget { +export interface InjectableTarget { name: string; sentryPatched?: boolean; prototype: { @@ -75,7 +75,7 @@ interface InjectableTarget { * We already guard duplicate patching with isWrapped. However, isWrapped checks whether a file has been patched, whereas we use this check for concrete target classes. * This check might not be necessary, but better to play it safe. */ -function isPatched(target: InjectableTarget): boolean { +export function isPatched(target: InjectableTarget): boolean { if (target.sentryPatched) { return true; } diff --git a/packages/node/test/integrations/tracing/nest.test.ts b/packages/node/test/integrations/tracing/nest.test.ts new file mode 100644 index 000000000000..82e1783b4359 --- /dev/null +++ b/packages/node/test/integrations/tracing/nest.test.ts @@ -0,0 +1,17 @@ +import type { InjectableTarget } from '../../../src/integrations/tracing/nest'; +import { isPatched } from '../../../src/integrations/tracing/nest'; + +describe ('Nest', () => { + describe('isPatched', () => { + it('should return true if target is already patched', () => { + const target = { name: 'TestTarget', sentryPatched: true, prototype: {} }; + expect(isPatched(target)).toBe(true); + }); + + it('should add the sentryPatched property and return false if target is not patched', () => { + const target: InjectableTarget = { name: 'TestTarget', prototype: {} }; + expect(isPatched(target)).toBe(false); + expect(target.sentryPatched).toBe(true); + }); + }); +}) From 0648265f4a5702b8f8dee4de07e5e9a5b17ad42a Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 30 Jul 2024 15:37:39 +0200 Subject: [PATCH 27/27] . --- packages/node/src/integrations/tracing/nest.ts | 3 ++- packages/node/test/integrations/tracing/nest.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest.ts b/packages/node/src/integrations/tracing/nest.ts index a7129dc10cd5..2ec5fa840387 100644 --- a/packages/node/src/integrations/tracing/nest.ts +++ b/packages/node/src/integrations/tracing/nest.ts @@ -7,6 +7,7 @@ import { } from '@opentelemetry/instrumentation'; import { NestInstrumentation } from '@opentelemetry/instrumentation-nestjs-core'; import { + SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, captureException, @@ -96,7 +97,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { }; public constructor(config: InstrumentationConfig = {}) { - super('sentry-nestjs', '1.0.0', config); // TODO: instrumentationVersion + super('sentry-nestjs', SDK_VERSION, config); } /** diff --git a/packages/node/test/integrations/tracing/nest.test.ts b/packages/node/test/integrations/tracing/nest.test.ts index 82e1783b4359..3dc321f28008 100644 --- a/packages/node/test/integrations/tracing/nest.test.ts +++ b/packages/node/test/integrations/tracing/nest.test.ts @@ -1,7 +1,7 @@ import type { InjectableTarget } from '../../../src/integrations/tracing/nest'; import { isPatched } from '../../../src/integrations/tracing/nest'; -describe ('Nest', () => { +describe('Nest', () => { describe('isPatched', () => { it('should return true if target is already patched', () => { const target = { name: 'TestTarget', sentryPatched: true, prototype: {} }; @@ -14,4 +14,4 @@ describe ('Nest', () => { expect(target.sentryPatched).toBe(true); }); }); -}) +});