From 8c7a152604a47e3d15678f8896d5bd42adbc355b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 20 Mar 2023 19:29:10 +0100 Subject: [PATCH 1/5] feat(sveltekit): Add performance monitoring for client load --- packages/sveltekit/src/client/load.ts | 56 +++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index fbaa5f98799f..d2aee8485d66 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,6 +1,7 @@ -import { captureException } from '@sentry/svelte'; +import type { Span } from '@sentry/core'; +import { captureException, getCurrentHub } from '@sentry/svelte'; import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; -import type { ServerLoad } from '@sveltejs/kit'; +import type { Load } from '@sveltejs/kit'; function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can @@ -30,21 +31,60 @@ function sendErrorToSentry(e: unknown): unknown { * * @param origLoad SvelteKit user defined load function */ -export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { +export function wrapLoadWithSentry(origLoad: Load): Load { return new Proxy(origLoad, { - apply: (wrappingTarget, thisArg, args: Parameters) => { + apply: (wrappingTarget, thisArg, args: Parameters) => { let maybePromiseResult; + const [event] = args; + + const scope = getCurrentHub().getScope(); + + let activeSpan: Span | undefined = undefined; + let parentSpan: Span | undefined = undefined; + + if (scope) { + parentSpan = scope.getSpan(); + if (parentSpan) { + activeSpan = parentSpan.startChild({ + op: 'function.sveltekit.load', + description: event.route.id || '/', + }); + + scope.setSpan(activeSpan); + } + } + try { maybePromiseResult = wrappingTarget.apply(thisArg, args); } catch (e) { - throw sendErrorToSentry(e); + if (activeSpan) { + activeSpan.setStatus('internal_error'); + activeSpan.finish(); + } + const sentryError = sendErrorToSentry(e); + throw sentryError; } if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then(null, e => { - sendErrorToSentry(e); - }); + Promise.resolve(maybePromiseResult).then( + () => { + if (activeSpan) { + activeSpan.finish(); + } + }, + e => { + if (activeSpan) { + activeSpan.setStatus('internal_error'); + activeSpan.finish(); + } + sendErrorToSentry(e); + }, + ); + } else { + if (activeSpan) { + activeSpan.finish(); + } } return maybePromiseResult; From 41c68b91c381c3176b821ced609a00ebee235095 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:06:03 +0100 Subject: [PATCH 2/5] rewrite using trace --- packages/sveltekit/src/client/load.ts | 65 ++++++--------------------- 1 file changed, 13 insertions(+), 52 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index d2aee8485d66..c5df8e369222 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,4 +1,4 @@ -import type { Span } from '@sentry/core'; +import { trace } from '@sentry/core'; import { captureException, getCurrentHub } from '@sentry/svelte'; import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; import type { Load } from '@sveltejs/kit'; @@ -34,60 +34,21 @@ function sendErrorToSentry(e: unknown): unknown { export function wrapLoadWithSentry(origLoad: Load): Load { return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { - let maybePromiseResult; - const [event] = args; - const scope = getCurrentHub().getScope(); - - let activeSpan: Span | undefined = undefined; - let parentSpan: Span | undefined = undefined; - - if (scope) { - parentSpan = scope.getSpan(); - if (parentSpan) { - activeSpan = parentSpan.startChild({ - op: 'function.sveltekit.load', - description: event.route.id || '/', - }); - - scope.setSpan(activeSpan); - } - } - - try { - maybePromiseResult = wrappingTarget.apply(thisArg, args); - } catch (e) { - if (activeSpan) { - activeSpan.setStatus('internal_error'); - activeSpan.finish(); - } - const sentryError = sendErrorToSentry(e); - throw sentryError; - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - () => { - if (activeSpan) { - activeSpan.finish(); - } - }, - e => { - if (activeSpan) { - activeSpan.setStatus('internal_error'); - activeSpan.finish(); - } - sendErrorToSentry(e); + const routeId = event.route.id; + return trace( + { + op: 'function.sveltekit.load', + name: routeId ? routeId : event.url.pathname, + status: 'ok', + metadata: { + source: routeId ? 'route' : 'url', }, - ); - } else { - if (activeSpan) { - activeSpan.finish(); - } - } - - return maybePromiseResult; + }, + () => wrappingTarget.apply(thisArg, args), + sendErrorToSentry, + ); }, }); } From 6e30ae34764f599d3070f4a2480347cd784f9fe6 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:09:35 +0100 Subject: [PATCH 3/5] rewrite using trace --- packages/sveltekit/src/client/load.ts | 4 +- packages/sveltekit/test/client/load.test.ts | 57 ++++++++++++++++++--- 2 files changed, 53 insertions(+), 8 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index c5df8e369222..32f310e3bd2d 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,6 +1,6 @@ import { trace } from '@sentry/core'; -import { captureException, getCurrentHub } from '@sentry/svelte'; -import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; +import { captureException } from '@sentry/svelte'; +import { addExceptionMechanism, objectify } from '@sentry/utils'; import type { Load } from '@sveltejs/kit'; function sendErrorToSentry(e: unknown): unknown { diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 7cbfd3593c03..7bb08f177124 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,5 +1,5 @@ -import { Scope } from '@sentry/svelte'; -import type { ServerLoad } from '@sveltejs/kit'; +import { addTracingExtensions, Scope } from '@sentry/svelte'; +import type { Load } from '@sveltejs/kit'; import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/client/load'; @@ -19,6 +19,19 @@ vi.mock('@sentry/svelte', async () => { }; }); +const mockTrace = vi.fn(); + +vi.mock('@sentry/core', async () => { + const original = (await vi.importActual('@sentry/core')) as any; + return { + ...original, + trace: (...args: unknown[]) => { + mockTrace(...args); + return original.trace(...args); + }, + }; +}); + const mockAddExceptionMechanism = vi.fn(); vi.mock('@sentry/utils', async () => { @@ -33,22 +46,54 @@ function getById(_id?: string) { throw new Error('error'); } +const MOCK_LOAD_ARGS: any = { + params: { id: '123' }, + route: { + id: '/users/[id]', + }, + url: new URL('http://localhost:3000/users/123'), + 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; + }, + }, + }, +}; + +beforeAll(() => { + addTracingExtensions(); +}); + describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + mockTrace.mockClear(); mockScope = new Scope(); }); it('calls captureException', async () => { - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), }; } 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(1); @@ -60,14 +105,14 @@ describe('wrapLoadWithSentry', () => { return mockScope; }); - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), }; } 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 95679d72f786a674d9613e801d97c782f8e5721b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:09:52 +0100 Subject: [PATCH 4/5] test --- packages/sveltekit/test/client/load.test.ts | 37 +++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 7bb08f177124..d8d4857852ae 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -99,6 +99,43 @@ describe('wrapLoadWithSentry', () => { 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(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + 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), + ); + }); + it('adds an exception mechanism', async () => { const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { void callback({}, { event_id: 'fake-event-id' }); From e04378ae3ff944b065b6f6a31cb6b5f1f2f7add5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 16:01:35 +0100 Subject: [PATCH 5/5] fix this --- packages/sveltekit/test/client/load.test.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index d8d4857852ae..f4d18c9f9909 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -114,20 +114,8 @@ describe('wrapLoadWithSentry', () => { { 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', }, },