From 8558275e89025c740d84d9d01171e169ce2f8489 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 23 Mar 2023 16:53:29 +0100 Subject: [PATCH 1/3] fix(sveltekit): Check for active domains in `sentryHandle` --- packages/sveltekit/src/server/handle.ts | 60 ++++++++++++++----------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 23319713bf87..026a315b66a6 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -64,32 +64,40 @@ export const transformPageChunk: NonNullable { +export const sentryHandle: Handle = input => { + // @ts-ignore domain.active exists if there is an active domain, TS just doesn't know + if (domain.active) { + return handleInDomain(input); + } return domain.create().bind(() => { - const sentryTraceHeader = event.request.headers.get('sentry-trace'); - const baggageHeader = event.request.headers.get('baggage'); - const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; - const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); - - return trace( - { - op: 'http.server', - name: `${event.request.method} ${event.route.id}`, - status: 'ok', - ...traceparentData, - metadata: { - source: 'route', - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, - }, - }, - async (span?: Span) => { - const res = await resolve(event, { transformPageChunk }); - if (span) { - span.setHttpStatus(res.status); - } - return res; - }, - sendErrorToSentry, - ); + return handleInDomain(input); })(); }; + +function handleInDomain({ event, resolve }: Parameters[0]): ReturnType { + const sentryTraceHeader = event.request.headers.get('sentry-trace'); + const baggageHeader = event.request.headers.get('baggage'); + const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); + + return trace( + { + op: 'http.server', + name: `${event.request.method} ${event.route.id}`, + status: 'ok', + ...traceparentData, + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }, + async (span?: Span) => { + const res = await resolve(event, { transformPageChunk }); + if (span) { + span.setHttpStatus(res.status); + } + return res; + }, + sendErrorToSentry, + ); +} From 6bc071e427fc4d8ac29f921b079729065f6e3616 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 29 Mar 2023 10:53:41 +0200 Subject: [PATCH 2/3] check for active txn instead of active domain --- packages/sveltekit/src/server/handle.ts | 14 +++--- packages/sveltekit/test/server/handle.test.ts | 46 ++++++++++++++++++- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 026a315b66a6..a5fbba3d0199 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -1,5 +1,5 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import type { Span } from '@sentry/core'; +import { getCurrentHub, Span } from '@sentry/core'; import { getActiveTransaction, trace } from '@sentry/core'; import { captureException } from '@sentry/node'; import { @@ -65,16 +65,18 @@ export const transformPageChunk: NonNullable { - // @ts-ignore domain.active exists if there is an active domain, TS just doesn't know - if (domain.active) { - return handleInDomain(input); + // if there is an active transaction, we know that this handle call is nested and hence + // we don't create a new domain for it. If we created one, nested server calls would + // create new transactions instead of adding a child span to the currently active span. + if (getCurrentHub().getScope().getSpan()) { + return instrumentHandle(input); } return domain.create().bind(() => { - return handleInDomain(input); + return instrumentHandle(input); })(); }; -function handleInDomain({ event, resolve }: Parameters[0]): ReturnType { +function instrumentHandle({ event, resolve }: Parameters[0]): ReturnType { const sentryTraceHeader = event.request.headers.get('sentry-trace'); const baggageHeader = event.request.headers.get('baggage'); const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; diff --git a/packages/sveltekit/test/server/handle.test.ts b/packages/sveltekit/test/server/handle.test.ts index ac94e72dd292..785aad1ca06e 100644 --- a/packages/sveltekit/test/server/handle.test.ts +++ b/packages/sveltekit/test/server/handle.test.ts @@ -129,7 +129,7 @@ describe('handleSentry', () => { expect(response).toEqual(mockResponse); }); - it('creates a transaction', async () => { + it("creates a transaction if there's no active span", async () => { let ref: any = undefined; client.on('finishTransaction', (transaction: Transaction) => { ref = transaction; @@ -149,6 +149,50 @@ describe('handleSentry', () => { expect(ref.metadata.source).toEqual('route'); expect(ref.endTimestamp).toBeDefined(); + expect(ref.spanRecorder.spans).toHaveLength(1); + }); + + it('creates a child span for nested server calls (i.e. if there is an active span)', async () => { + let ref: any = undefined; + let txnCount = 0; + client.on('finishTransaction', (transaction: Transaction) => { + ref = transaction; + ++txnCount; + }); + + try { + await sentryHandle({ + event: mockEvent(), + resolve: async _ => { + // simulateing a nested load call: + await sentryHandle({ + event: mockEvent({ route: { id: 'api/users/details/[id]' } }), + resolve: resolve(type, isError), + }); + return mockResponse; + }, + }); + } catch (e) { + // + } + + expect(txnCount).toEqual(1); + expect(ref).toBeDefined(); + + expect(ref.name).toEqual('GET /users/[id]'); + expect(ref.op).toEqual('http.server'); + expect(ref.status).toEqual(isError ? 'internal_error' : 'ok'); + expect(ref.metadata.source).toEqual('route'); + + expect(ref.endTimestamp).toBeDefined(); + + expect(ref.spanRecorder.spans).toHaveLength(2); + expect(ref.spanRecorder.spans).toEqual( + expect.arrayContaining([ + expect.objectContaining({ op: 'http.server', description: 'GET /users/[id]' }), + expect.objectContaining({ op: 'http.server', description: 'GET api/users/details/[id]' }), + ]), + ); }); it('creates a transaction from sentry-trace header', async () => { From e424a44d8e439f8744cdd4dd11b592c7508e3bec Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 29 Mar 2023 11:28:30 +0200 Subject: [PATCH 3/3] fix imports --- packages/sveltekit/src/server/handle.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index a5fbba3d0199..fa6c10648741 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -1,6 +1,6 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import { getCurrentHub, Span } from '@sentry/core'; -import { getActiveTransaction, trace } from '@sentry/core'; +import type { Span } from '@sentry/core'; +import { getActiveTransaction, getCurrentHub, trace } from '@sentry/core'; import { captureException } from '@sentry/node'; import { addExceptionMechanism,