From 5123d6db4e3e5c13d152d956b1c981b531c77866 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 7 Aug 2024 15:54:27 +0200 Subject: [PATCH 01/17] Progress --- .../nest/sentry-nest-instrumentation.ts | 34 +++++++++++++++++-- .../src/integrations/tracing/nest/types.ts | 13 ++++++- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 52c3a4ad6b40..dc69795ad391 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -9,7 +9,7 @@ import { getActiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sent import type { Span } from '@sentry/types'; import { SDK_VERSION } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; -import type { InjectableTarget } from './types'; +import type {InjectableTarget, Observable} from './types'; const supportedVersions = ['>=8.0.0 <11']; @@ -154,9 +154,39 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (prevSpan) { return withActiveSpan(prevSpan, () => { - return Reflect.apply(originalHandle, thisArgNext, args); + console.log('active span!'); + const returnedObservable: Observable = Reflect.apply(originalHandle, thisArgNext, args); + + return new Proxy(returnedObservable, { + get(observableTarget, observableProperty, observableReceiver) { + if (observableProperty === 'subscribe') { + return new Proxy(observableTarget.subscribe, { + apply(originalSubscribe, thisArgSubscribe, argsSubscribe) { + return startSpanManual(getMiddlewareSpanOptions(target), (afterSpan: Span) => { + console.log('Observable subscribed'); + const subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + subscription.add(() => { + console.log('Observable completed'); + afterSpan.end(); + }) + + return subscription; + }) + + + + } + }); + } + + return Reflect.get(observableTarget, observableProperty, observableReceiver); + } + }) }); } else { + console.log('no active span!'); return Reflect.apply(originalHandle, thisArgNext, args); } }; diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 2cdd1b6aefaf..f4965d6d1cee 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -27,11 +27,22 @@ export interface MinimalNestJsApp { }) => void; } +export interface Observer { + next(value: T): void; + error?(err: any): void; + complete?(): void; +} + /** * A minimal interface for an Observable. */ export interface Observable { - subscribe(observer: (value: T) => void): void; + pipe: (...args: any[]) => Observable; + subscribe( + next: (value: T) => void, + error?: (err: any) => void, + complete?: () => void + ): any; } /** From e73d83f3979ec803905e2c24644f85eeae6c6d46 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Thu, 8 Aug 2024 09:35:24 +0200 Subject: [PATCH 02/17] Linting --- .../nest/sentry-nest-instrumentation.ts | 19 ++++++++++--------- .../src/integrations/tracing/nest/types.ts | 6 +----- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 8ec8397ad76a..2a2f2fddcef7 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -176,7 +176,11 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (prevSpan) { return withActiveSpan(prevSpan, () => { console.log('active span!'); - const returnedObservable: Observable = Reflect.apply(originalHandle, thisArgNext, args); + const returnedObservable: Observable = Reflect.apply( + originalHandle, + thisArgNext, + args, + ); return new Proxy(returnedObservable, { get(observableTarget, observableProperty, observableReceiver) { @@ -191,20 +195,17 @@ export class SentryNestInstrumentation extends InstrumentationBase { subscription.add(() => { console.log('Observable completed'); afterSpan.end(); - }) + }); return subscription; - }) - - - - } + }); + }, }); } return Reflect.get(observableTarget, observableProperty, observableReceiver); - } - }) + }, + }); }); } else { console.log('no active span!'); diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index b42cbbabd7a9..1c1c2a39581e 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -38,11 +38,7 @@ export interface Observer { */ export interface Observable { pipe: (...args: any[]) => Observable; - subscribe( - next: (value: T) => void, - error?: (err: any) => void, - complete?: () => void - ): any; + subscribe(next: (value: T) => void, error?: (err: any) => void, complete?: () => void): any; } /** From 2081e1ade591836678d05ac2c57fe133cb3b37de Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Thu, 8 Aug 2024 11:45:22 +0200 Subject: [PATCH 03/17] Stuff works --- .../nest/sentry-nest-instrumentation.ts | 84 ++++++++----------- .../src/integrations/tracing/nest/types.ts | 6 +- 2 files changed, 38 insertions(+), 52 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 2a2f2fddcef7..60bfd8a584ca 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -5,11 +5,11 @@ import { InstrumentationNodeModuleDefinition, InstrumentationNodeModuleFile, } from '@opentelemetry/instrumentation'; -import { getActiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core'; +import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core'; import type { Span } from '@sentry/types'; import { SDK_VERSION } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; -import type { CatchTarget, InjectableTarget, Observable } from './types'; +import type { CallHandler, CatchTarget, InjectableTarget, Observable, Subscription } from './types'; const supportedVersions = ['>=8.0.0 <11']; @@ -161,64 +161,46 @@ export class SentryNestInstrumentation extends InstrumentationBase { target.prototype.intercept = new Proxy(target.prototype.intercept, { apply: (originalIntercept, thisArgIntercept, argsIntercept) => { - const [executionContext, next, args] = argsIntercept; + const next: CallHandler = argsIntercept[1]; const prevSpan = getActiveSpan(); + let afterSpan: Span; return startSpanManual(getMiddlewareSpanOptions(target), (span: Span) => { - const nextProxy = new Proxy(next, { - get: (thisArgNext, property, receiver) => { - if (property === 'handle') { - const originalHandle = Reflect.get(thisArgNext, property, receiver); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (...args: any[]) => { - span.end(); - - if (prevSpan) { - return withActiveSpan(prevSpan, () => { - console.log('active span!'); - const returnedObservable: Observable = Reflect.apply( - originalHandle, - thisArgNext, - args, - ); - - return new Proxy(returnedObservable, { - get(observableTarget, observableProperty, observableReceiver) { - if (observableProperty === 'subscribe') { - return new Proxy(observableTarget.subscribe, { - apply(originalSubscribe, thisArgSubscribe, argsSubscribe) { - return startSpanManual(getMiddlewareSpanOptions(target), (afterSpan: Span) => { - console.log('Observable subscribed'); - const subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); - - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - subscription.add(() => { - console.log('Observable completed'); - afterSpan.end(); - }); - - return subscription; - }); - }, - }); - } + // eslint-disable-next-line @typescript-eslint/unbound-method + next.handle = new Proxy(next.handle, { + apply: (originalHandle, thisArgHandle, argsHandle) => { + span.end(); - return Reflect.get(observableTarget, observableProperty, observableReceiver); - }, - }); - }); - } else { - console.log('no active span!'); - return Reflect.apply(originalHandle, thisArgNext, args); - } - }; + if (prevSpan) { + return withActiveSpan(prevSpan, () => { + const handleReturn = Reflect.apply(originalHandle, thisArgHandle, argsHandle); + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target)); + return handleReturn; + }); + } else { + const handleReturn = Reflect.apply(originalHandle, thisArgHandle, argsHandle); + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target)); + return handleReturn; } + }, + }); - return Reflect.get(target, property, receiver); + // TODO: maybe promise + const returnedObservableIntercept: Observable = originalIntercept.apply( + thisArgIntercept, + argsIntercept, + ); + + // eslint-disable-next-line @typescript-eslint/unbound-method + returnedObservableIntercept.subscribe = new Proxy(returnedObservableIntercept.subscribe, { + apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { + const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); + subscription.add(() => afterSpan.end()); + return subscription; }, }); - return originalIntercept.apply(thisArgIntercept, [executionContext, nextProxy, args]); + return returnedObservableIntercept; }); }, }); diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 1c1c2a39581e..fac6a775fc83 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -33,12 +33,16 @@ export interface Observer { complete?(): void; } +export interface Subscription { + add(...args: any[]): void; +} + /** * A minimal interface for an Observable. */ export interface Observable { pipe: (...args: any[]) => Observable; - subscribe(next: (value: T) => void, error?: (err: any) => void, complete?: () => void): any; + subscribe(next?: (value: T) => void, error?: (err: any) => void, complete?: () => void): Subscription; } /** From 96ad0a226355538d7b2d2ab3c087ef7dd4fd7f1c Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 15:33:28 +0200 Subject: [PATCH 04/17] Seems to work? --- .../src/integrations/tracing/nest/helpers.ts | 15 ++++-- .../nest/sentry-nest-instrumentation.ts | 54 +++++++++++++++---- .../src/integrations/tracing/nest/types.ts | 4 +- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts index babf80022c1f..3aa8a9b2660b 100644 --- a/packages/node/src/integrations/tracing/nest/helpers.ts +++ b/packages/node/src/integrations/tracing/nest/helpers.ts @@ -1,6 +1,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { addNonEnumerableProperty } from '@sentry/utils'; -import type { CatchTarget, InjectableTarget } from './types'; +import type { CallHandler, CatchTarget, InjectableTarget, Observable } from './types'; const sentryPatched = 'sentryPatched'; @@ -23,9 +23,18 @@ export function isPatched(target: InjectableTarget | CatchTarget): boolean { * Returns span options for nest middleware spans. */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type -export function getMiddlewareSpanOptions(target: InjectableTarget | CatchTarget) { +export function getMiddlewareSpanOptions(target: InjectableTarget | CatchTarget, name: string | undefined = undefined) { + let span_name; + + // default to class name if name is not provided + if (name) { + span_name = name; + } else { + span_name = target.name; + } + return { - name: target.name, + name: span_name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'middleware.nestjs', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.middleware.nestjs', diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index c15ddc07dcc2..e0de727dadc1 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -9,10 +9,19 @@ import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiv import type { Span } from '@sentry/types'; import { SDK_VERSION } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; -import type { CallHandler, CatchTarget, InjectableTarget, Observable, Subscription } from './types'; +import type { + CallHandler, + CatchTarget, + InjectableTarget, + MinimalNestJsExecutionContext, + Observable, + Subscription, +} from './types'; const supportedVersions = ['>=8.0.0 <11']; +const AFTER_ROUTE_SPAN_KEY = 'AFTER_ROUTE_SPAN_KEY'; + /** * Custom instrumentation for nestjs. * @@ -163,7 +172,10 @@ export class SentryNestInstrumentation extends InstrumentationBase { target.prototype.intercept = new Proxy(target.prototype.intercept, { apply: (originalIntercept, thisArgIntercept, argsIntercept) => { + const context: MinimalNestJsExecutionContext = argsIntercept[0]; const next: CallHandler = argsIntercept[1]; + const request = context.switchToHttp().getRequest(); + const prevSpan = getActiveSpan(); let afterSpan: Span; @@ -175,14 +187,25 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (prevSpan) { return withActiveSpan(prevSpan, () => { - const handleReturn = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target)); - return handleReturn; + const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); + if (request.AFTER_ROUTE_SPAN_KEY) { + return handleReturnObservable; + } + + console.log('start span a!'); + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); + return handleReturnObservable; }); } else { - const handleReturn = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target)); - return handleReturn; + const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); + + if (request.AFTER_ROUTE_SPAN_KEY) { + return handleReturnObservable; + } + + console.log('start span a!'); + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); + return handleReturnObservable; } }, }); @@ -193,15 +216,26 @@ export class SentryNestInstrumentation extends InstrumentationBase { argsIntercept, ); + console.log(request.AFTER_ROUTE_SPAN_KEY); + if (request.AFTER_ROUTE_SPAN_KEY) { + return returnedObservableIntercept; + } + // eslint-disable-next-line @typescript-eslint/unbound-method returnedObservableIntercept.subscribe = new Proxy(returnedObservableIntercept.subscribe, { apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { - const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); - subscription.add(() => afterSpan.end()); - return subscription; + return withActiveSpan(afterSpan ?? prevSpan, () => { + const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); + + + + subscription.add(() => { afterSpan.end(); console.log('end span!'); }); + return subscription; + }); }, }); + request.AFTER_ROUTE_SPAN_KEY = true; return returnedObservableIntercept; }); }, diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index fac6a775fc83..b11327baf891 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -interface MinimalNestJsExecutionContext { +export interface MinimalNestJsExecutionContext { getType: () => string; switchToHttp: () => { @@ -12,6 +12,7 @@ interface MinimalNestJsExecutionContext { path?: string; }; method?: string; + AFTER_ROUTE_SPAN_KEY?: boolean }; }; } @@ -41,7 +42,6 @@ export interface Subscription { * A minimal interface for an Observable. */ export interface Observable { - pipe: (...args: any[]) => Observable; subscribe(next?: (value: T) => void, error?: (err: any) => void, complete?: () => void): Subscription; } From 02131cb30bf2aee1481e5b2296859269c090b239 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 16:11:59 +0200 Subject: [PATCH 05/17] Cleanup --- packages/node/src/integrations/tracing/nest/helpers.ts | 2 +- .../tracing/nest/sentry-nest-instrumentation.ts | 10 +--------- packages/node/src/integrations/tracing/nest/types.ts | 2 +- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts index 3aa8a9b2660b..1fd74591d7c2 100644 --- a/packages/node/src/integrations/tracing/nest/helpers.ts +++ b/packages/node/src/integrations/tracing/nest/helpers.ts @@ -1,6 +1,6 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import { addNonEnumerableProperty } from '@sentry/utils'; -import type { CallHandler, CatchTarget, InjectableTarget, Observable } from './types'; +import type { CatchTarget, InjectableTarget } from './types'; const sentryPatched = 'sentryPatched'; diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index e0de727dadc1..eb5473d02be6 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -20,8 +20,6 @@ import type { const supportedVersions = ['>=8.0.0 <11']; -const AFTER_ROUTE_SPAN_KEY = 'AFTER_ROUTE_SPAN_KEY'; - /** * Custom instrumentation for nestjs. * @@ -192,7 +190,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { return handleReturnObservable; } - console.log('start span a!'); afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); return handleReturnObservable; }); @@ -203,7 +200,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { return handleReturnObservable; } - console.log('start span a!'); afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); return handleReturnObservable; } @@ -216,7 +212,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { argsIntercept, ); - console.log(request.AFTER_ROUTE_SPAN_KEY); if (request.AFTER_ROUTE_SPAN_KEY) { return returnedObservableIntercept; } @@ -226,10 +221,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { return withActiveSpan(afterSpan ?? prevSpan, () => { const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); - - - - subscription.add(() => { afterSpan.end(); console.log('end span!'); }); + subscription.add(() => afterSpan.end()); return subscription; }); }, diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index b11327baf891..8ea1633ef8e4 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -12,7 +12,7 @@ export interface MinimalNestJsExecutionContext { path?: string; }; method?: string; - AFTER_ROUTE_SPAN_KEY?: boolean + AFTER_ROUTE_SPAN_KEY?: boolean; }; }; } From 029fec81991f8144849cc51feee6701f82df1696 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 16:16:48 +0200 Subject: [PATCH 06/17] Minor improvement --- packages/node/src/integrations/tracing/nest/helpers.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts index 1fd74591d7c2..9157421d4a19 100644 --- a/packages/node/src/integrations/tracing/nest/helpers.ts +++ b/packages/node/src/integrations/tracing/nest/helpers.ts @@ -24,14 +24,7 @@ export function isPatched(target: InjectableTarget | CatchTarget): boolean { */ // eslint-disable-next-line @typescript-eslint/explicit-function-return-type export function getMiddlewareSpanOptions(target: InjectableTarget | CatchTarget, name: string | undefined = undefined) { - let span_name; - - // default to class name if name is not provided - if (name) { - span_name = name; - } else { - span_name = target.name; - } + const span_name = name ?? target.name; // fallback to class name if no name is provided return { name: span_name, From d7bc8bbd9156c3b4772f9a7385a4ac95d3364407 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 16:20:53 +0200 Subject: [PATCH 07/17] Remove leftover type --- packages/node/src/integrations/tracing/nest/types.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 8ea1633ef8e4..a0c6070edef7 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -28,12 +28,6 @@ export interface MinimalNestJsApp { }) => void; } -export interface Observer { - next(value: T): void; - error?(err: any): void; - complete?(): void; -} - export interface Subscription { add(...args: any[]): void; } From 86b33fad44ad3bec618a83d4c0bd0cb17bca621c Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 17:23:59 +0200 Subject: [PATCH 08/17] Add tests --- .../nestjs-basic/src/app.controller.ts | 5 +- .../nestjs-basic/src/example-1.interceptor.ts | 15 ++ ...nterceptor.ts => example-2.interceptor.ts} | 4 +- .../nestjs-basic/tests/transactions.test.ts | 140 ++++++++++++++++-- .../node-nestjs-basic/src/app.controller.ts | 5 +- .../src/example-1.interceptor.ts | 15 ++ .../src/example-2.interceptor.ts | 10 ++ .../src/example.interceptor.ts | 11 -- .../tests/transactions.test.ts | 140 ++++++++++++++++-- 9 files changed, 308 insertions(+), 37 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-1.interceptor.ts rename dev-packages/e2e-tests/test-applications/nestjs-basic/src/{example.interceptor.ts => example-2.interceptor.ts} (65%) create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-1.interceptor.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-2.interceptor.ts delete mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.interceptor.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 9cda3c96f9a6..baa9be747c2b 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,11 +1,12 @@ import { Controller, Get, Param, ParseIntPipe, UseFilters, UseGuards, UseInterceptors } from '@nestjs/common'; import { flush } from '@sentry/nestjs'; import { AppService } from './app.service'; +import { ExampleInterceptor1 } from './example-1.interceptor'; +import { ExampleInterceptor2 } from './example-2.interceptor'; import { ExampleExceptionGlobalFilter } from './example-global-filter.exception'; import { ExampleExceptionLocalFilter } from './example-local-filter.exception'; import { ExampleLocalFilter } from './example-local.filter'; import { ExampleGuard } from './example.guard'; -import { ExampleInterceptor } from './example.interceptor'; @Controller() @UseFilters(ExampleLocalFilter) @@ -29,7 +30,7 @@ export class AppController { } @Get('test-interceptor-instrumentation') - @UseInterceptors(ExampleInterceptor) + @UseInterceptors(ExampleInterceptor1, ExampleInterceptor2) testInterceptorInstrumentation() { return this.appService.testSpan(); } diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-1.interceptor.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-1.interceptor.ts new file mode 100644 index 000000000000..81c9f70d30e2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-1.interceptor.ts @@ -0,0 +1,15 @@ +import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { tap } from 'rxjs'; + +@Injectable() +export class ExampleInterceptor1 implements NestInterceptor { + intercept(context: ExecutionContext, next: CallHandler) { + Sentry.startSpan({ name: 'test-interceptor-span-1' }, () => {}); + return next.handle().pipe( + tap(() => { + Sentry.startSpan({ name: 'test-interceptor-span-after-route' }, () => {}); + }), + ); + } +} diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.interceptor.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-2.interceptor.ts similarity index 65% rename from dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.interceptor.ts rename to dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-2.interceptor.ts index 75c301b4cffc..2cf9dfb9e043 100644 --- a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example.interceptor.ts +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/example-2.interceptor.ts @@ -2,9 +2,9 @@ import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nes import * as Sentry from '@sentry/nestjs'; @Injectable() -export class ExampleInterceptor implements NestInterceptor { +export class ExampleInterceptor2 implements NestInterceptor { intercept(context: ExecutionContext, next: CallHandler) { - Sentry.startSpan({ name: 'test-interceptor-span' }, () => {}); + Sentry.startSpan({ name: 'test-interceptor-span-2' }, () => {}); return next.handle().pipe(); } } 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 555b6357ade8..ea81ce71b78f 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 @@ -341,7 +341,7 @@ test('API route transaction includes nest pipe span for invalid request', async ); }); -test('API route transaction includes nest interceptor span. Spans created in and after interceptor are nested correctly', async ({ +test('API route transaction includes nest interceptor spans before route execution. Spans created in and after interceptor are nested correctly', async ({ baseURL, }) => { const pageloadTransactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { @@ -356,6 +356,7 @@ test('API route transaction includes nest interceptor span. Spans created in and const transactionEvent = await pageloadTransactionEventPromise; + // check if interceptor spans before route execution exist expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ @@ -366,7 +367,22 @@ test('API route transaction includes nest interceptor span. Spans created in and 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'ExampleInterceptor', + description: 'ExampleInterceptor1', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleInterceptor2', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -378,9 +394,13 @@ test('API route transaction includes nest interceptor span. Spans created in and }), ); - const exampleInterceptorSpan = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor'); - const exampleInterceptorSpanId = exampleInterceptorSpan?.span_id; + // get interceptor spans + const exampleInterceptor1Span = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor1'); + const exampleInterceptor1SpanId = exampleInterceptor1Span?.span_id; + const exampleInterceptor2Span = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor2'); + const exampleInterceptor2SpanId = exampleInterceptor2Span?.span_id; + // check if manually started spans exist expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ @@ -399,7 +419,18 @@ test('API route transaction includes nest interceptor span. Spans created in and span_id: expect.any(String), trace_id: expect.any(String), data: expect.any(Object), - description: 'test-interceptor-span', + description: 'test-interceptor-span-1', + 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-interceptor-span-2', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -411,12 +442,101 @@ test('API route transaction includes nest interceptor span. Spans created in and ); // verify correct span parent-child relationships - const testInterceptorSpan = transactionEvent.spans.find(span => span.description === 'test-interceptor-span'); + const testInterceptor1Span = transactionEvent.spans.find(span => span.description === 'test-interceptor-span-1'); + const testInterceptor2Span = transactionEvent.spans.find(span => span.description === 'test-interceptor-span-2'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleInterceptor1' is the parent of 'test-interceptor-span-1' + expect(testInterceptor1Span.parent_span_id).toBe(exampleInterceptor1SpanId); + + // 'ExampleInterceptor1' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptor1SpanId); + + // 'ExampleInterceptor2' is the parent of 'test-interceptor-span-2' + expect(testInterceptor2Span.parent_span_id).toBe(exampleInterceptor2SpanId); + + // 'ExampleInterceptor2' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptor2SpanId); +}); + +test('API route transaction includes exactly one nest interceptor span after route execution. Spans created in controller and in interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans after route execution exist + 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: 'Interceptor - After Route', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // check that exactly one after route span is sent + const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( + span => span.description === 'Interceptor - After Route', + ); + expect(allInterceptorSpansAfterRoute.length).toBe(1); + + // get interceptor span + const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'Interceptor - After Route', + ); + const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; + + // check if manually started span in interceptor after route exists + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-interceptor-span-after-route', + 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 testInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'test-interceptor-span-after-route', + ); const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); - // 'ExampleInterceptor' is the parent of 'test-interceptor-span' - expect(testInterceptorSpan.parent_span_id).toBe(exampleInterceptorSpanId); + // 'Interceptor - After Route' is the parent of 'test-interceptor-span-after-route' + expect(testInterceptorSpanAfterRoute.parent_span_id).toBe(exampleInterceptorSpanAfterRouteId); - // 'ExampleInterceptor' is NOT the parent of 'test-controller-span' - expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanId); + // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); }); 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 ec0a921da2c4..da93486ef2ca 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,8 +1,9 @@ import { Controller, Get, Param, ParseIntPipe, UseGuards, UseInterceptors } from '@nestjs/common'; import { flush } from '@sentry/nestjs'; import { AppService } from './app.service'; +import { ExampleInterceptor1 } from './example-1.interceptor'; +import { ExampleInterceptor2 } from './example-2.interceptor'; import { ExampleGuard } from './example.guard'; -import { ExampleInterceptor } from './example.interceptor'; @Controller() export class AppController { @@ -25,7 +26,7 @@ export class AppController { } @Get('test-interceptor-instrumentation') - @UseInterceptors(ExampleInterceptor) + @UseInterceptors(ExampleInterceptor1, ExampleInterceptor2) testInterceptorInstrumentation() { return this.appService.testSpan(); } diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-1.interceptor.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-1.interceptor.ts new file mode 100644 index 000000000000..81c9f70d30e2 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-1.interceptor.ts @@ -0,0 +1,15 @@ +import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { tap } from 'rxjs'; + +@Injectable() +export class ExampleInterceptor1 implements NestInterceptor { + intercept(context: ExecutionContext, next: CallHandler) { + Sentry.startSpan({ name: 'test-interceptor-span-1' }, () => {}); + return next.handle().pipe( + tap(() => { + Sentry.startSpan({ name: 'test-interceptor-span-after-route' }, () => {}); + }), + ); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-2.interceptor.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-2.interceptor.ts new file mode 100644 index 000000000000..2cf9dfb9e043 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example-2.interceptor.ts @@ -0,0 +1,10 @@ +import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; + +@Injectable() +export class ExampleInterceptor2 implements NestInterceptor { + intercept(context: ExecutionContext, next: CallHandler) { + Sentry.startSpan({ name: 'test-interceptor-span-2' }, () => {}); + return next.handle().pipe(); + } +} diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.interceptor.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.interceptor.ts deleted file mode 100644 index 260c1798449f..000000000000 --- a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/example.interceptor.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; -import * as Sentry from '@sentry/nestjs'; -import { Observable } from 'rxjs'; - -@Injectable() -export class ExampleInterceptor implements NestInterceptor { - intercept(context: ExecutionContext, next: CallHandler): Observable { - Sentry.startSpan({ name: 'test-interceptor-span' }, () => {}); - return next.handle().pipe(); - } -} 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 cb04bc06839e..7c8b940edacd 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 @@ -339,7 +339,7 @@ test('API route transaction includes nest pipe span for invalid request', async ); }); -test('API route transaction includes nest interceptor span. Spans created in and after interceptor are nested correctly', async ({ +test('API route transaction includes nest interceptor spans before route execution. Spans created in and after interceptor are nested correctly', async ({ baseURL, }) => { const pageloadTransactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { @@ -354,6 +354,7 @@ test('API route transaction includes nest interceptor span. Spans created in and const transactionEvent = await pageloadTransactionEventPromise; + // check if interceptor spans before route execution exist expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ @@ -364,7 +365,22 @@ test('API route transaction includes nest interceptor span. Spans created in and 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'ExampleInterceptor', + description: 'ExampleInterceptor1', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + 'sentry.op': 'middleware.nestjs', + 'sentry.origin': 'auto.middleware.nestjs', + }, + description: 'ExampleInterceptor2', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -376,9 +392,13 @@ test('API route transaction includes nest interceptor span. Spans created in and }), ); - const exampleInterceptorSpan = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor'); - const exampleInterceptorSpanId = exampleInterceptorSpan?.span_id; + // get interceptor spans + const exampleInterceptor1Span = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor1'); + const exampleInterceptor1SpanId = exampleInterceptor1Span?.span_id; + const exampleInterceptor2Span = transactionEvent.spans.find(span => span.description === 'ExampleInterceptor2'); + const exampleInterceptor2SpanId = exampleInterceptor2Span?.span_id; + // check if manually started spans exist expect(transactionEvent).toEqual( expect.objectContaining({ spans: expect.arrayContaining([ @@ -397,7 +417,18 @@ test('API route transaction includes nest interceptor span. Spans created in and span_id: expect.any(String), trace_id: expect.any(String), data: expect.any(Object), - description: 'test-interceptor-span', + description: 'test-interceptor-span-1', + 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-interceptor-span-2', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -409,12 +440,101 @@ test('API route transaction includes nest interceptor span. Spans created in and ); // verify correct span parent-child relationships - const testInterceptorSpan = transactionEvent.spans.find(span => span.description === 'test-interceptor-span'); + const testInterceptor1Span = transactionEvent.spans.find(span => span.description === 'test-interceptor-span-1'); + const testInterceptor2Span = transactionEvent.spans.find(span => span.description === 'test-interceptor-span-2'); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'ExampleInterceptor1' is the parent of 'test-interceptor-span-1' + expect(testInterceptor1Span.parent_span_id).toBe(exampleInterceptor1SpanId); + + // 'ExampleInterceptor1' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptor1SpanId); + + // 'ExampleInterceptor2' is the parent of 'test-interceptor-span-2' + expect(testInterceptor2Span.parent_span_id).toBe(exampleInterceptor2SpanId); + + // 'ExampleInterceptor2' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptor2SpanId); +}); + +test('API route transaction includes exactly one nest interceptor span after route execution. Spans created in controller and in interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans after route execution exist + 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: 'Interceptor - After Route', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // check that exactly one after route span is sent + const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( + span => span.description === 'Interceptor - After Route', + ); + expect(allInterceptorSpansAfterRoute.length).toBe(1); + + // get interceptor span + const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'Interceptor - After Route', + ); + const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; + + // check if manually started span in interceptor after route exists + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-interceptor-span-after-route', + 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 testInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'test-interceptor-span-after-route', + ); const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); - // 'ExampleInterceptor' is the parent of 'test-interceptor-span' - expect(testInterceptorSpan.parent_span_id).toBe(exampleInterceptorSpanId); + // 'Interceptor - After Route' is the parent of 'test-interceptor-span-after-route' + expect(testInterceptorSpanAfterRoute.parent_span_id).toBe(exampleInterceptorSpanAfterRouteId); - // 'ExampleInterceptor' is NOT the parent of 'test-controller-span' - expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanId); + // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); }); From 84f2db59392d7b9df506fc8cdfe40f59f3cffcfa Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 14 Aug 2024 17:52:37 +0200 Subject: [PATCH 09/17] guard subscribe --- .../nest/sentry-nest-instrumentation.ts | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index eb5473d02be6..15da1b98157c 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -216,16 +216,18 @@ export class SentryNestInstrumentation extends InstrumentationBase { return returnedObservableIntercept; } - // eslint-disable-next-line @typescript-eslint/unbound-method - returnedObservableIntercept.subscribe = new Proxy(returnedObservableIntercept.subscribe, { - apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { - return withActiveSpan(afterSpan ?? prevSpan, () => { - const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); - subscription.add(() => afterSpan.end()); - return subscription; - }); - }, - }); + if (typeof returnedObservableIntercept.subscribe === 'function') { + // eslint-disable-next-line @typescript-eslint/unbound-method + returnedObservableIntercept.subscribe = new Proxy(returnedObservableIntercept.subscribe, { + apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { + return withActiveSpan(afterSpan ?? prevSpan, () => { + const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); + subscription.add(() => afterSpan.end()); + return subscription; + }); + }, + }); + } request.AFTER_ROUTE_SPAN_KEY = true; return returnedObservableIntercept; From ffc2bf65cdea015898d9d27fb862e53c871f1c02 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Fri, 16 Aug 2024 09:31:25 +0200 Subject: [PATCH 10/17] Address pr comments --- .../tracing/nest/sentry-nest-instrumentation.ts | 17 ++++++++--------- .../node/src/integrations/tracing/nest/types.ts | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 15da1b98157c..0907b6b69f56 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -7,7 +7,7 @@ import { } from '@opentelemetry/instrumentation'; import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core'; import type { Span } from '@sentry/types'; -import { SDK_VERSION } from '@sentry/utils'; +import { addNonEnumerableProperty, SDK_VERSION } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; import type { CallHandler, @@ -186,21 +186,20 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (prevSpan) { return withActiveSpan(prevSpan, () => { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - if (request.AFTER_ROUTE_SPAN_KEY) { - return handleReturnObservable; + + if (!request._sentryInterceptorInstrumented) { + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); } - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); return handleReturnObservable; }); } else { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - if (request.AFTER_ROUTE_SPAN_KEY) { - return handleReturnObservable; + if (!request._sentryInterceptorInstrumented) { + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); } - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); return handleReturnObservable; } }, @@ -212,7 +211,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { argsIntercept, ); - if (request.AFTER_ROUTE_SPAN_KEY) { + if (request._sentryInterceptorInstrumented) { return returnedObservableIntercept; } @@ -229,7 +228,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { }); } - request.AFTER_ROUTE_SPAN_KEY = true; + addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); return returnedObservableIntercept; }); }, diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index a0c6070edef7..0069c9da3222 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -12,7 +12,7 @@ export interface MinimalNestJsExecutionContext { path?: string; }; method?: string; - AFTER_ROUTE_SPAN_KEY?: boolean; + _sentryInterceptorInstrumented?: boolean; }; }; } From e639af51c95e163b3a6e57d528153c935305fd9a Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Fri, 16 Aug 2024 09:34:35 +0200 Subject: [PATCH 11/17] Lint --- .../tracing/nest/sentry-nest-instrumentation.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index 0907b6b69f56..cc1a833909d2 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -7,7 +7,7 @@ import { } from '@opentelemetry/instrumentation'; import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core'; import type { Span } from '@sentry/types'; -import { addNonEnumerableProperty, SDK_VERSION } from '@sentry/utils'; +import { SDK_VERSION, addNonEnumerableProperty } from '@sentry/utils'; import { getMiddlewareSpanOptions, isPatched } from './helpers'; import type { CallHandler, @@ -188,7 +188,9 @@ export class SentryNestInstrumentation extends InstrumentationBase { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); if (!request._sentryInterceptorInstrumented) { - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); + afterSpan = startInactiveSpan( + getMiddlewareSpanOptions(target, 'Interceptor - After Route'), + ); } return handleReturnObservable; From 39bda546565c8c2570919a87a4a48325cc1adf8e Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 27 Aug 2024 11:23:08 +0200 Subject: [PATCH 12/17] Handle promises --- .../src/integrations/tracing/nest/helpers.ts | 23 +++++- .../nest/sentry-nest-instrumentation.ts | 73 +++++++++++-------- 2 files changed, 62 insertions(+), 34 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/helpers.ts b/packages/node/src/integrations/tracing/nest/helpers.ts index 9157421d4a19..e2d3c4c573b9 100644 --- a/packages/node/src/integrations/tracing/nest/helpers.ts +++ b/packages/node/src/integrations/tracing/nest/helpers.ts @@ -1,6 +1,7 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, withActiveSpan } from '@sentry/core'; +import type { Span } from '@sentry/types'; import { addNonEnumerableProperty } from '@sentry/utils'; -import type { CatchTarget, InjectableTarget } from './types'; +import type { CatchTarget, InjectableTarget, Observable, Subscription } from './types'; const sentryPatched = 'sentryPatched'; @@ -34,3 +35,21 @@ export function getMiddlewareSpanOptions(target: InjectableTarget | CatchTarget, }, }; } + +/** + * Adds instrumentation to a js observable and attaches the span to an active parent span. + */ +export function instrumentObservable(observable: Observable, activeSpan: Span | undefined): void { + if (activeSpan) { + // eslint-disable-next-line @typescript-eslint/unbound-method + observable.subscribe = new Proxy(observable.subscribe, { + apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { + return withActiveSpan(activeSpan, () => { + const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); + subscription.add(() => activeSpan.end()); + return subscription; + }); + }, + }); + } +} diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index cc1a833909d2..af4f93b8a460 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -7,16 +7,9 @@ import { } from '@opentelemetry/instrumentation'; import { getActiveSpan, startInactiveSpan, startSpan, startSpanManual, withActiveSpan } from '@sentry/core'; import type { Span } from '@sentry/types'; -import { SDK_VERSION, addNonEnumerableProperty } from '@sentry/utils'; -import { getMiddlewareSpanOptions, isPatched } from './helpers'; -import type { - CallHandler, - CatchTarget, - InjectableTarget, - MinimalNestJsExecutionContext, - Observable, - Subscription, -} from './types'; +import { SDK_VERSION, addNonEnumerableProperty, isThenable } from '@sentry/utils'; +import { getMiddlewareSpanOptions, instrumentObservable, isPatched } from './helpers'; +import type { CallHandler, CatchTarget, InjectableTarget, MinimalNestJsExecutionContext, Observable } from './types'; const supportedVersions = ['>=8.0.0 <11']; @@ -174,17 +167,17 @@ export class SentryNestInstrumentation extends InstrumentationBase { const next: CallHandler = argsIntercept[1]; const request = context.switchToHttp().getRequest(); - const prevSpan = getActiveSpan(); + const parentSpan = getActiveSpan(); let afterSpan: Span; - return startSpanManual(getMiddlewareSpanOptions(target), (span: Span) => { + return startSpanManual(getMiddlewareSpanOptions(target), (beforeSpan: Span) => { // eslint-disable-next-line @typescript-eslint/unbound-method next.handle = new Proxy(next.handle, { apply: (originalHandle, thisArgHandle, argsHandle) => { - span.end(); + beforeSpan.end(); - if (prevSpan) { - return withActiveSpan(prevSpan, () => { + if (parentSpan) { + return withActiveSpan(parentSpan, () => { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); if (!request._sentryInterceptorInstrumented) { @@ -207,31 +200,47 @@ export class SentryNestInstrumentation extends InstrumentationBase { }, }); - // TODO: maybe promise - const returnedObservableIntercept: Observable = originalIntercept.apply( - thisArgIntercept, - argsIntercept, - ); + let returnedObservableInterceptMaybePromise: Observable | Promise>; + + try { + returnedObservableInterceptMaybePromise = originalIntercept.apply(thisArgIntercept, argsIntercept); + } catch (e) { + if (!request._sentryInterceptorInstrumented) { + if (beforeSpan) beforeSpan.end(); + if (afterSpan) afterSpan.end(); + addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); + } + + throw e; + } if (request._sentryInterceptorInstrumented) { - return returnedObservableIntercept; + return returnedObservableInterceptMaybePromise; } - if (typeof returnedObservableIntercept.subscribe === 'function') { - // eslint-disable-next-line @typescript-eslint/unbound-method - returnedObservableIntercept.subscribe = new Proxy(returnedObservableIntercept.subscribe, { - apply: (originalSubscribe, thisArgSubscribe, argsSubscribe) => { - return withActiveSpan(afterSpan ?? prevSpan, () => { - const subscription: Subscription = originalSubscribe.apply(thisArgSubscribe, argsSubscribe); - subscription.add(() => afterSpan.end()); - return subscription; - }); + // handle async interceptor + if (isThenable(returnedObservableInterceptMaybePromise)) { + return Promise.resolve(returnedObservableInterceptMaybePromise).then( + observable => { + instrumentObservable(observable, afterSpan ?? parentSpan); + return observable; }, - }); + e => { + if (beforeSpan) beforeSpan.end(); + if (afterSpan) afterSpan.end(); + addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); + throw e; + }, + ); + } + + // handle sync interceptor + if (typeof returnedObservableInterceptMaybePromise.subscribe === 'function') { + instrumentObservable(returnedObservableInterceptMaybePromise, afterSpan ?? parentSpan); } addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); - return returnedObservableIntercept; + return returnedObservableInterceptMaybePromise; }); }, }); From cdfca8a27f6a1bb0fa41959571c4867a50975f7b Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 27 Aug 2024 12:46:34 +0200 Subject: [PATCH 13/17] TEsts --- .../nestjs-basic/src/app.controller.ts | 7 + .../src/async-example.interceptor.ts | 17 ++ .../nestjs-basic/tests/transactions.test.ts | 167 ++++++++++++++++++ .../node-nestjs-basic/src/app.controller.ts | 7 + .../src/async-example.interceptor.ts | 17 ++ .../tests/transactions.test.ts | 167 ++++++++++++++++++ 6 files changed, 382 insertions(+) create mode 100644 dev-packages/e2e-tests/test-applications/nestjs-basic/src/async-example.interceptor.ts create mode 100644 dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/async-example.interceptor.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 baa9be747c2b..1649cdb94b5f 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,6 +1,7 @@ import { Controller, Get, Param, ParseIntPipe, UseFilters, UseGuards, UseInterceptors } from '@nestjs/common'; import { flush } from '@sentry/nestjs'; import { AppService } from './app.service'; +import { AsyncInterceptor } from './async-example.interceptor'; import { ExampleInterceptor1 } from './example-1.interceptor'; import { ExampleInterceptor2 } from './example-2.interceptor'; import { ExampleExceptionGlobalFilter } from './example-global-filter.exception'; @@ -35,6 +36,12 @@ export class AppController { return this.appService.testSpan(); } + @Get('test-async-interceptor-instrumentation') + @UseInterceptors(AsyncInterceptor) + testAsyncInterceptorInstrumentation() { + return this.appService.testSpan(); + } + @Get('test-pipe-instrumentation/:id') testPipeInstrumentation(@Param('id', ParseIntPipe) id: number) { return { value: id }; diff --git a/dev-packages/e2e-tests/test-applications/nestjs-basic/src/async-example.interceptor.ts b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/async-example.interceptor.ts new file mode 100644 index 000000000000..ac0ee60acc51 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/nestjs-basic/src/async-example.interceptor.ts @@ -0,0 +1,17 @@ +import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { tap } from 'rxjs'; + +@Injectable() +export class AsyncInterceptor implements NestInterceptor { + intercept(context: ExecutionContext, next: CallHandler) { + Sentry.startSpan({ name: 'test-async-interceptor-span' }, () => {}); + return Promise.resolve( + next.handle().pipe( + tap(() => { + Sentry.startSpan({ name: 'test-async-interceptor-span-after-route' }, () => {}); + }), + ), + ); + } +} 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 ea81ce71b78f..515d9f67a974 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 @@ -540,3 +540,170 @@ test('API route transaction includes exactly one nest interceptor span after rou // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); }); + +test('API route transaction includes nest async interceptor spans before route execution. Spans created in and after async interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-async-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-async-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans before route execution exist + 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: 'AsyncInterceptor', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // get interceptor spans + const exampleAsyncInterceptor = transactionEvent.spans.find(span => span.description === 'AsyncInterceptor'); + const exampleAsyncInterceptorSpanId = exampleAsyncInterceptor?.span_id; + + // check if manually started spans exist + 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-async-interceptor-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 testAsyncInterceptorSpan = transactionEvent.spans.find( + span => span.description === 'test-async-interceptor-span', + ); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'AsyncInterceptor' is the parent of 'test-async-interceptor-span' + expect(testAsyncInterceptorSpan.parent_span_id).toBe(exampleAsyncInterceptorSpanId); + + // 'AsyncInterceptor' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleAsyncInterceptorSpanId); +}); + +test('API route transaction includes exactly one nest async interceptor span after route execution. Spans created in controller and in async interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-async-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-async-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans after route execution exist + 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: 'Interceptor - After Route', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // check that exactly one after route span is sent + const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( + span => span.description === 'Interceptor - After Route', + ); + expect(allInterceptorSpansAfterRoute.length).toBe(1); + + // get interceptor span + const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'Interceptor - After Route', + ); + const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; + + // check if manually started span in interceptor after route exists + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-async-interceptor-span-after-route', + 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 testInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'test-async-interceptor-span-after-route', + ); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'Interceptor - After Route' is the parent of 'test-interceptor-span-after-route' + expect(testInterceptorSpanAfterRoute.parent_span_id).toBe(exampleInterceptorSpanAfterRouteId); + + // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); +}); 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 da93486ef2ca..1f141dc0f966 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,6 +1,7 @@ import { Controller, Get, Param, ParseIntPipe, UseGuards, UseInterceptors } from '@nestjs/common'; import { flush } from '@sentry/nestjs'; import { AppService } from './app.service'; +import { AsyncInterceptor } from './async-example.interceptor'; import { ExampleInterceptor1 } from './example-1.interceptor'; import { ExampleInterceptor2 } from './example-2.interceptor'; import { ExampleGuard } from './example.guard'; @@ -31,6 +32,12 @@ export class AppController { return this.appService.testSpan(); } + @Get('test-async-interceptor-instrumentation') + @UseInterceptors(AsyncInterceptor) + testAsyncInterceptorInstrumentation() { + return this.appService.testSpan(); + } + @Get('test-pipe-instrumentation/:id') testPipeInstrumentation(@Param('id', ParseIntPipe) id: number) { return { value: id }; diff --git a/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/async-example.interceptor.ts b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/async-example.interceptor.ts new file mode 100644 index 000000000000..ac0ee60acc51 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/node-nestjs-basic/src/async-example.interceptor.ts @@ -0,0 +1,17 @@ +import { CallHandler, ExecutionContext, Injectable, NestInterceptor } from '@nestjs/common'; +import * as Sentry from '@sentry/nestjs'; +import { tap } from 'rxjs'; + +@Injectable() +export class AsyncInterceptor implements NestInterceptor { + intercept(context: ExecutionContext, next: CallHandler) { + Sentry.startSpan({ name: 'test-async-interceptor-span' }, () => {}); + return Promise.resolve( + next.handle().pipe( + tap(() => { + Sentry.startSpan({ name: 'test-async-interceptor-span-after-route' }, () => {}); + }), + ), + ); + } +} 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 7c8b940edacd..5501aac4990a 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 @@ -538,3 +538,170 @@ test('API route transaction includes exactly one nest interceptor span after rou // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); }); + +test('API route transaction includes nest async interceptor spans before route execution. Spans created in and after async interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-async-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-async-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans before route execution exist + 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: 'AsyncInterceptor', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // get interceptor spans + const exampleAsyncInterceptor = transactionEvent.spans.find(span => span.description === 'AsyncInterceptor'); + const exampleAsyncInterceptorSpanId = exampleAsyncInterceptor?.span_id; + + // check if manually started spans exist + 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-async-interceptor-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 testAsyncInterceptorSpan = transactionEvent.spans.find( + span => span.description === 'test-async-interceptor-span', + ); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'AsyncInterceptor' is the parent of 'test-async-interceptor-span' + expect(testAsyncInterceptorSpan.parent_span_id).toBe(exampleAsyncInterceptorSpanId); + + // 'AsyncInterceptor' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleAsyncInterceptorSpanId); +}); + +test('API route transaction includes exactly one nest async interceptor span after route execution. Spans created in controller and in async interceptor are nested correctly', async ({ + baseURL, +}) => { + const pageloadTransactionEventPromise = waitForTransaction('node-nestjs-basic', transactionEvent => { + return ( + transactionEvent?.contexts?.trace?.op === 'http.server' && + transactionEvent?.transaction === 'GET /test-async-interceptor-instrumentation' + ); + }); + + const response = await fetch(`${baseURL}/test-async-interceptor-instrumentation`); + expect(response.status).toBe(200); + + const transactionEvent = await pageloadTransactionEventPromise; + + // check if interceptor spans after route execution exist + 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: 'Interceptor - After Route', + parent_span_id: expect.any(String), + start_timestamp: expect.any(Number), + timestamp: expect.any(Number), + status: 'ok', + op: 'middleware.nestjs', + origin: 'auto.middleware.nestjs', + }, + ]), + }), + ); + + // check that exactly one after route span is sent + const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( + span => span.description === 'Interceptor - After Route', + ); + expect(allInterceptorSpansAfterRoute.length).toBe(1); + + // get interceptor span + const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'Interceptor - After Route', + ); + const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; + + // check if manually started span in interceptor after route exists + expect(transactionEvent).toEqual( + expect.objectContaining({ + spans: expect.arrayContaining([ + { + span_id: expect.any(String), + trace_id: expect.any(String), + data: expect.any(Object), + description: 'test-async-interceptor-span-after-route', + 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 testInterceptorSpanAfterRoute = transactionEvent.spans.find( + span => span.description === 'test-async-interceptor-span-after-route', + ); + const testControllerSpan = transactionEvent.spans.find(span => span.description === 'test-controller-span'); + + // 'Interceptor - After Route' is the parent of 'test-interceptor-span-after-route' + expect(testInterceptorSpanAfterRoute.parent_span_id).toBe(exampleInterceptorSpanAfterRouteId); + + // 'Interceptor - After Route' is NOT the parent of 'test-controller-span' + expect(testControllerSpan.parent_span_id).not.toBe(exampleInterceptorSpanAfterRouteId); +}); From f5e15bef5c573efad48d73294c1ad391f043801c Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 27 Aug 2024 16:46:11 +0200 Subject: [PATCH 14/17] Guard non-http context --- .../integrations/tracing/nest/sentry-nest-instrumentation.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index af4f93b8a460..c783534519aa 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -165,6 +165,11 @@ export class SentryNestInstrumentation extends InstrumentationBase { apply: (originalIntercept, thisArgIntercept, argsIntercept) => { const context: MinimalNestJsExecutionContext = argsIntercept[0]; const next: CallHandler = argsIntercept[1]; + + if (context.getType() != 'http') { + return originalIntercept.apply(thisArgIntercept, argsIntercept); + } + const request = context.switchToHttp().getRequest(); const parentSpan = getActiveSpan(); From 30d2d7bed91bd69f5d84991461f58aca3741696b Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 27 Aug 2024 17:39:51 +0200 Subject: [PATCH 15/17] Fix some styling --- .../tracing/nest/sentry-nest-instrumentation.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index c783534519aa..eb916a84838a 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -187,7 +187,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { if (!request._sentryInterceptorInstrumented) { afterSpan = startInactiveSpan( - getMiddlewareSpanOptions(target, 'Interceptor - After Route'), + getMiddlewareSpanOptions(target, 'Interceptors - After Route'), ); } @@ -197,7 +197,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); if (!request._sentryInterceptorInstrumented) { - afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptor - After Route')); + afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptors - After Route')); } return handleReturnObservable; @@ -211,8 +211,8 @@ export class SentryNestInstrumentation extends InstrumentationBase { returnedObservableInterceptMaybePromise = originalIntercept.apply(thisArgIntercept, argsIntercept); } catch (e) { if (!request._sentryInterceptorInstrumented) { - if (beforeSpan) beforeSpan.end(); - if (afterSpan) afterSpan.end(); + beforeSpan?.end(); + afterSpan?.end(); addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); } @@ -225,14 +225,14 @@ export class SentryNestInstrumentation extends InstrumentationBase { // handle async interceptor if (isThenable(returnedObservableInterceptMaybePromise)) { - return Promise.resolve(returnedObservableInterceptMaybePromise).then( + return returnedObservableInterceptMaybePromise.then( observable => { instrumentObservable(observable, afterSpan ?? parentSpan); return observable; }, e => { - if (beforeSpan) beforeSpan.end(); - if (afterSpan) afterSpan.end(); + beforeSpan?.end(); + afterSpan?.end(); addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); throw e; }, From d8c5033e471255cf2375ee1ee154806bdccec22f Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Tue, 27 Aug 2024 18:20:07 +0200 Subject: [PATCH 16/17] Fix race condition --- .../nestjs-basic/tests/transactions.test.ts | 14 ++++++++------ .../node-nestjs-basic/tests/transactions.test.ts | 12 ++++++------ .../tracing/nest/sentry-nest-instrumentation.ts | 14 +++++--------- 3 files changed, 19 insertions(+), 21 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 515d9f67a974..b499827d0de2 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 @@ -485,7 +485,7 @@ test('API route transaction includes exactly one nest interceptor span after rou 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'Interceptor - After Route', + description: 'Interceptors - After Route', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -499,13 +499,13 @@ test('API route transaction includes exactly one nest interceptor span after rou // check that exactly one after route span is sent const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); expect(allInterceptorSpansAfterRoute.length).toBe(1); // get interceptor span const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; @@ -641,6 +641,8 @@ test('API route transaction includes exactly one nest async interceptor span aft const transactionEvent = await pageloadTransactionEventPromise; + console.log(transactionEvent); + // check if interceptor spans after route execution exist expect(transactionEvent).toEqual( expect.objectContaining({ @@ -652,7 +654,7 @@ test('API route transaction includes exactly one nest async interceptor span aft 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'Interceptor - After Route', + description: 'Interceptors - After Route', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -666,13 +668,13 @@ test('API route transaction includes exactly one nest async interceptor span aft // check that exactly one after route span is sent const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); expect(allInterceptorSpansAfterRoute.length).toBe(1); // get interceptor span const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; 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 5501aac4990a..eb52dfe98585 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 @@ -483,7 +483,7 @@ test('API route transaction includes exactly one nest interceptor span after rou 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'Interceptor - After Route', + description: 'Interceptors - After Route', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -497,13 +497,13 @@ test('API route transaction includes exactly one nest interceptor span after rou // check that exactly one after route span is sent const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); expect(allInterceptorSpansAfterRoute.length).toBe(1); // get interceptor span const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; @@ -650,7 +650,7 @@ test('API route transaction includes exactly one nest async interceptor span aft 'sentry.op': 'middleware.nestjs', 'sentry.origin': 'auto.middleware.nestjs', }, - description: 'Interceptor - After Route', + description: 'Interceptors - After Route', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), timestamp: expect.any(Number), @@ -664,13 +664,13 @@ test('API route transaction includes exactly one nest async interceptor span aft // check that exactly one after route span is sent const allInterceptorSpansAfterRoute = transactionEvent.spans.filter( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); expect(allInterceptorSpansAfterRoute.length).toBe(1); // get interceptor span const exampleInterceptorSpanAfterRoute = transactionEvent.spans.find( - span => span.description === 'Interceptor - After Route', + span => span.description === 'Interceptors - After Route', ); const exampleInterceptorSpanAfterRouteId = exampleInterceptorSpanAfterRoute?.span_id; diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index eb916a84838a..b1bca8983736 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -186,6 +186,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); if (!request._sentryInterceptorInstrumented) { + addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); afterSpan = startInactiveSpan( getMiddlewareSpanOptions(target, 'Interceptors - After Route'), ); @@ -197,6 +198,7 @@ export class SentryNestInstrumentation extends InstrumentationBase { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); if (!request._sentryInterceptorInstrumented) { + addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptors - After Route')); } @@ -210,16 +212,12 @@ export class SentryNestInstrumentation extends InstrumentationBase { try { returnedObservableInterceptMaybePromise = originalIntercept.apply(thisArgIntercept, argsIntercept); } catch (e) { - if (!request._sentryInterceptorInstrumented) { - beforeSpan?.end(); - afterSpan?.end(); - addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); - } - + beforeSpan?.end(); + afterSpan?.end(); throw e; } - if (request._sentryInterceptorInstrumented) { + if (!afterSpan) { return returnedObservableInterceptMaybePromise; } @@ -233,7 +231,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { e => { beforeSpan?.end(); afterSpan?.end(); - addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); throw e; }, ); @@ -244,7 +241,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { instrumentObservable(returnedObservableInterceptMaybePromise, afterSpan ?? parentSpan); } - addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); return returnedObservableInterceptMaybePromise; }); }, From 7cb9b9a9334c462d72edf9d770daca52482f1132 Mon Sep 17 00:00:00 2001 From: nicohrubec Date: Wed, 28 Aug 2024 15:50:09 +0200 Subject: [PATCH 17/17] write property directly on context2 --- .../nestjs-basic/tests/transactions.test.ts | 2 -- .../tracing/nest/sentry-nest-instrumentation.ts | 14 ++++---------- .../node/src/integrations/tracing/nest/types.ts | 3 ++- 3 files changed, 6 insertions(+), 13 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 b499827d0de2..e33e63f319ca 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 @@ -641,8 +641,6 @@ test('API route transaction includes exactly one nest async interceptor span aft const transactionEvent = await pageloadTransactionEventPromise; - console.log(transactionEvent); - // check if interceptor spans after route execution exist expect(transactionEvent).toEqual( expect.objectContaining({ diff --git a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts index b1bca8983736..b8ea782ee66f 100644 --- a/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts +++ b/packages/node/src/integrations/tracing/nest/sentry-nest-instrumentation.ts @@ -166,12 +166,6 @@ export class SentryNestInstrumentation extends InstrumentationBase { const context: MinimalNestJsExecutionContext = argsIntercept[0]; const next: CallHandler = argsIntercept[1]; - if (context.getType() != 'http') { - return originalIntercept.apply(thisArgIntercept, argsIntercept); - } - - const request = context.switchToHttp().getRequest(); - const parentSpan = getActiveSpan(); let afterSpan: Span; @@ -185,8 +179,8 @@ export class SentryNestInstrumentation extends InstrumentationBase { return withActiveSpan(parentSpan, () => { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - if (!request._sentryInterceptorInstrumented) { - addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); + if (!context._sentryInterceptorInstrumented) { + addNonEnumerableProperty(context, '_sentryInterceptorInstrumented', true); afterSpan = startInactiveSpan( getMiddlewareSpanOptions(target, 'Interceptors - After Route'), ); @@ -197,8 +191,8 @@ export class SentryNestInstrumentation extends InstrumentationBase { } else { const handleReturnObservable = Reflect.apply(originalHandle, thisArgHandle, argsHandle); - if (!request._sentryInterceptorInstrumented) { - addNonEnumerableProperty(request, '_sentryInterceptorInstrumented', true); + if (!context._sentryInterceptorInstrumented) { + addNonEnumerableProperty(context, '_sentryInterceptorInstrumented', true); afterSpan = startInactiveSpan(getMiddlewareSpanOptions(target, 'Interceptors - After Route')); } diff --git a/packages/node/src/integrations/tracing/nest/types.ts b/packages/node/src/integrations/tracing/nest/types.ts index 0069c9da3222..18bd26b31ccb 100644 --- a/packages/node/src/integrations/tracing/nest/types.ts +++ b/packages/node/src/integrations/tracing/nest/types.ts @@ -12,9 +12,10 @@ export interface MinimalNestJsExecutionContext { path?: string; }; method?: string; - _sentryInterceptorInstrumented?: boolean; }; }; + + _sentryInterceptorInstrumented?: boolean; } export interface NestJsErrorFilter {