From d21c1fceced485eaa9926223d4d6f7e5c5668062 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 20 Mar 2023 19:20:25 +0100 Subject: [PATCH 1/6] feat(sveltekit): Add performance monitoring for server load --- packages/sveltekit/src/server/load.ts | 94 +++++++++++++++++++++++---- 1 file changed, 80 insertions(+), 14 deletions(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index ef0433091a9e..8117dfadf723 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,6 +1,15 @@ -import { captureException } from '@sentry/node'; -import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; +/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ +import type { Span } from '@sentry/core'; +import { captureException, getCurrentHub } from '@sentry/node'; +import { + addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, + extractTraceparentData, + isThenable, + objectify, +} from '@sentry/utils'; import type { HttpError, ServerLoad } from '@sveltejs/kit'; +import * as domain from 'domain'; function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; @@ -36,6 +45,10 @@ function sendErrorToSentry(e: unknown): unknown { return objectifiedErr; } +function setSpan(span: Span | undefined): void { + getCurrentHub().getScope()?.setSpan(span); +} + /** * Wrap load function with Sentry * @@ -44,21 +57,74 @@ function sendErrorToSentry(e: unknown): unknown { export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { - let maybePromiseResult; + return domain.create().bind(() => { + let maybePromiseResult; + + const [event] = args; + const hub = getCurrentHub(); + const scope = hub.getScope(); + + const parentSpan = scope?.getSpan(); + + let activeSpan: Span | undefined = undefined; + + function finishActiveSpan(): void { + activeSpan?.finish(); + setSpan(parentSpan); + } + + if (parentSpan) { + activeSpan = parentSpan.startChild({ + op: 'function.sveltekit.load', + description: event.route.id || 'load', + status: 'ok', + }); + } else { + 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); + + activeSpan = hub.startTransaction({ + op: 'function.sveltekit.load', + name: event.route.id || 'load', + status: 'ok', + ...traceparentData, + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }); + } + + setSpan(activeSpan); - try { - maybePromiseResult = wrappingTarget.apply(thisArg, args); - } catch (e) { - throw sendErrorToSentry(e); - } + try { + maybePromiseResult = wrappingTarget.apply(thisArg, args); + } catch (e) { + activeSpan?.setStatus('internal_error'); + const sentryError = sendErrorToSentry(e); + finishActiveSpan(); + throw sentryError; + } - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then(null, e => { - sendErrorToSentry(e); - }); - } + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then( + () => { + finishActiveSpan(); + }, + e => { + activeSpan?.setStatus('internal_error'); + sendErrorToSentry(e); + finishActiveSpan(); + }, + ); + } else { + finishActiveSpan(); + } - return maybePromiseResult; + return maybePromiseResult; + })(); }, }); } From d23b2e77444d0f147bd9d3fbb1142fddc6bfe205 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 20:23:39 +0100 Subject: [PATCH 2/6] use trace function --- packages/sveltekit/src/server/load.ts | 74 +++++---------------------- 1 file changed, 12 insertions(+), 62 deletions(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index 8117dfadf723..6df9c8e866d6 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,11 +1,10 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import type { Span } from '@sentry/core'; -import { captureException, getCurrentHub } from '@sentry/node'; +import { trace } from '@sentry/core'; +import { captureException } from '@sentry/node'; import { addExceptionMechanism, baggageHeaderToDynamicSamplingContext, extractTraceparentData, - isThenable, objectify, } from '@sentry/utils'; import type { HttpError, ServerLoad } from '@sveltejs/kit'; @@ -45,10 +44,6 @@ function sendErrorToSentry(e: unknown): unknown { return objectifiedErr; } -function setSpan(span: Span | undefined): void { - getCurrentHub().getScope()?.setSpan(span); -} - /** * Wrap load function with Sentry * @@ -58,34 +53,15 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { return domain.create().bind(() => { - let maybePromiseResult; - const [event] = args; - const hub = getCurrentHub(); - const scope = hub.getScope(); - - const parentSpan = scope?.getSpan(); - let activeSpan: Span | undefined = undefined; - - function finishActiveSpan(): void { - activeSpan?.finish(); - setSpan(parentSpan); - } - - if (parentSpan) { - activeSpan = parentSpan.startChild({ - op: 'function.sveltekit.load', - description: event.route.id || 'load', - status: 'ok', - }); - } else { - 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); + 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); - activeSpan = hub.startTransaction({ + return trace( + { op: 'function.sveltekit.load', name: event.route.id || 'load', status: 'ok', @@ -94,36 +70,10 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { source: 'route', dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, - }); - } - - setSpan(activeSpan); - - try { - maybePromiseResult = wrappingTarget.apply(thisArg, args); - } catch (e) { - activeSpan?.setStatus('internal_error'); - const sentryError = sendErrorToSentry(e); - finishActiveSpan(); - throw sentryError; - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - () => { - finishActiveSpan(); - }, - e => { - activeSpan?.setStatus('internal_error'); - sendErrorToSentry(e); - finishActiveSpan(); - }, - ); - } else { - finishActiveSpan(); - } - - return maybePromiseResult; + }, + () => wrappingTarget.apply(thisArg, args), + sendErrorToSentry, + ); })(); }, }); From b81386aa807d4370d71de994aad6bf0f6f427ef9 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 13:43:29 +0100 Subject: [PATCH 3/6] making progress --- packages/sveltekit/test/server/load.test.ts | 69 ++++++++++++++++++++- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index ec2503b945c4..3ada90a46771 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -20,6 +20,19 @@ vi.mock('@sentry/node', async () => { }; }); +const mockTrace = vi.fn(); + +vi.mock('@sentry/core', async () => { + const original = (await vi.importActual('@sentry/core')) as any; + return { + ...original, + trace: (...args) => { + mockTrace(...args); + return original.trace(...args); + }, + }; +}); + const mockAddExceptionMechanism = vi.fn(); vi.mock('@sentry/utils', async () => { @@ -34,14 +47,41 @@ function getById(_id?: string) { throw new Error('error'); } +const MOCK_LOAD_ARGS: any = { + params: { id: '1' }, + route: { + id: '/users/[id]', + }, + request: { + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, + }, + }, +}; + describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + mockTrace.mockClear(); mockScope = new Scope(); }); - it('calls captureException', async () => { + it.only('calls captureException', async () => { async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), @@ -49,12 +89,37 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); + // create promise that waits for timeout + await new Promise(resolve => setTimeout(resolve, 1000, 'timeout')); + expect(mockCaptureException).toHaveBeenCalledTimes(1); }); + it('calls trace function', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad({ + params: { id: '1' }, + route: { + id: '', + }, + headers: { 'sentry-trace': '1234567890abcdef1234567890abcdef-1234567890abcdef-1' }, + } as any); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith({ + op: 'function.sveltekit.load', + }); + }); + describe('with error() helper', () => { it.each([ // [statusCode, timesCalled] From 75ffe17583d0991144f976608ff6de9f6161af81 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 14:54:54 +0100 Subject: [PATCH 4/6] add tests --- packages/node/src/integrations/undici.ts | 18 ++++++++ packages/sveltekit/src/server/load.ts | 5 +- packages/sveltekit/test/server/load.test.ts | 51 +++++++++++++++------ 3 files changed, 57 insertions(+), 17 deletions(-) create mode 100644 packages/node/src/integrations/undici.ts diff --git a/packages/node/src/integrations/undici.ts b/packages/node/src/integrations/undici.ts new file mode 100644 index 000000000000..954dac30e6d6 --- /dev/null +++ b/packages/node/src/integrations/undici.ts @@ -0,0 +1,18 @@ +import type { Integration, EventProcessor, Hub } from '@sentry/types'; + +export class Undici implements Integration { + /** + * @inheritDoc + */ + public static id: string = 'Undici'; + + /** + * @inheritDoc + */ + public name: string = Undici.id; + + /** + * @inheritDoc + */ + public setupOnce(_addGlobalEventProcessor: (callback: EventProcessor) => void, _getCurrentHub: () => Hub): void {} +} diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index 6df9c8e866d6..5e773365e4a4 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -60,14 +60,15 @@ export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); + const routeId = event.route.id; return trace( { op: 'function.sveltekit.load', - name: event.route.id || 'load', + name: routeId ? routeId : event.url.pathname, status: 'ok', ...traceparentData, metadata: { - source: 'route', + source: routeId ? 'route' : 'url', dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, }, diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 3ada90a46771..4d72cbd41b2b 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -1,3 +1,4 @@ +import { addTracingExtensions } from '@sentry/core'; import { Scope } from '@sentry/node'; import type { ServerLoad } from '@sveltejs/kit'; import { error } from '@sveltejs/kit'; @@ -26,7 +27,7 @@ vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { ...original, - trace: (...args) => { + trace: (...args: unknown[]) => { mockTrace(...args); return original.trace(...args); }, @@ -48,10 +49,11 @@ function getById(_id?: string) { } const MOCK_LOAD_ARGS: any = { - params: { id: '1' }, + params: { id: '123' }, route: { id: '/users/[id]', }, + url: new URL('http://localhost:3000/users/123'), request: { headers: { get: (key: string) => { @@ -73,6 +75,10 @@ const MOCK_LOAD_ARGS: any = { }, }; +beforeAll(() => { + addTracingExtensions(); +}); + describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); @@ -81,7 +87,7 @@ describe('wrapLoadWithSentry', () => { mockScope = new Scope(); }); - it.only('calls captureException', async () => { + it('calls captureException', async () => { async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), @@ -106,18 +112,33 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad({ - params: { id: '1' }, - route: { - id: '', - }, - headers: { 'sentry-trace': '1234567890abcdef1234567890abcdef-1234567890abcdef-1' }, - } as any); + await wrappedLoad(MOCK_LOAD_ARGS); expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith({ - op: 'function.sveltekit.load', - }); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + parentSampled: true, + parentSpanId: '1234567890abcdef', + status: 'ok', + traceId: '1234567890abcdef1234567890abcdef', + metadata: { + dynamicSamplingContext: { + environment: 'production', + public_key: 'dogsarebadatkeepingsecrets', + release: '1.0.0', + sample_rate: '1', + trace_id: '1234567890abcdef1234567890abcdef', + transaction: 'dogpark', + user_segment: 'segmentA', + }, + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); }); describe('with error() helper', () => { @@ -140,7 +161,7 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(times); @@ -160,7 +181,7 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(addEventProcessorSpy).toBeCalledTimes(1); From b30e5dc1cc4e2864ad5952b7cb50f2888a77ba2d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:01:09 +0100 Subject: [PATCH 5/6] delete --- packages/node/src/integrations/undici.ts | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 packages/node/src/integrations/undici.ts diff --git a/packages/node/src/integrations/undici.ts b/packages/node/src/integrations/undici.ts deleted file mode 100644 index 954dac30e6d6..000000000000 --- a/packages/node/src/integrations/undici.ts +++ /dev/null @@ -1,18 +0,0 @@ -import type { Integration, EventProcessor, Hub } from '@sentry/types'; - -export class Undici implements Integration { - /** - * @inheritDoc - */ - public static id: string = 'Undici'; - - /** - * @inheritDoc - */ - public name: string = Undici.id; - - /** - * @inheritDoc - */ - public setupOnce(_addGlobalEventProcessor: (callback: EventProcessor) => void, _getCurrentHub: () => Hub): void {} -} From 02fa299810f593b4e79da1dc2915440be6ca3466 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:10:12 +0100 Subject: [PATCH 6/6] remove promise.resolve --- packages/sveltekit/test/server/load.test.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 4d72cbd41b2b..81b067689093 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -98,9 +98,6 @@ describe('wrapLoadWithSentry', () => { const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); - // create promise that waits for timeout - await new Promise(resolve => setTimeout(resolve, 1000, 'timeout')); - expect(mockCaptureException).toHaveBeenCalledTimes(1); });