diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 245fcee9658e..5076710970a8 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -1,12 +1,12 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ import type { Span } from '@sentry/core'; -import { getActiveTransaction, getCurrentHub, runWithAsyncContext, trace } from '@sentry/core'; +import { getActiveTransaction, getCurrentHub, runWithAsyncContext, startSpan } from '@sentry/core'; import { captureException } from '@sentry/node'; import { addExceptionMechanism, dynamicSamplingContextToSentryBaggageHeader, objectify } from '@sentry/utils'; import type { Handle, ResolveOptions } from '@sveltejs/kit'; import { isHttpError, isRedirect } from '../common/utils'; -import { getTracePropagationData } from './utils'; +import { flushIfServerless, getTracePropagationData } from './utils'; export type SentryHandleOptions = { /** @@ -118,7 +118,10 @@ export function sentryHandle(handlerOptions?: SentryHandleOptions): Handle { return sentryRequestHandler; } -function instrumentHandle({ event, resolve }: Parameters[0], options: SentryHandleOptions): ReturnType { +async function instrumentHandle( + { event, resolve }: Parameters[0], + options: SentryHandleOptions, +): Promise { if (!event.route?.id && !options.handleUnknownRoutes) { return resolve(event); } @@ -126,25 +129,32 @@ function instrumentHandle({ event, resolve }: Parameters[0], options: Se const { dynamicSamplingContext, traceparentData, propagationContext } = getTracePropagationData(event); getCurrentHub().getScope().setPropagationContext(propagationContext); - return trace( - { - op: 'http.server', - origin: 'auto.http.sveltekit', - name: `${event.request.method} ${event.route?.id || event.url.pathname}`, - status: 'ok', - ...traceparentData, - metadata: { - source: event.route?.id ? 'route' : 'url', - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + try { + const resolveResult = await startSpan( + { + op: 'http.server', + origin: 'auto.http.sveltekit', + name: `${event.request.method} ${event.route?.id || event.url.pathname}`, + status: 'ok', + ...traceparentData, + metadata: { + source: event.route?.id ? 'route' : 'url', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }, + async (span?: Span) => { + const res = await resolve(event, { transformPageChunk }); + if (span) { + span.setHttpStatus(res.status); + } + return res; }, - }, - async (span?: Span) => { - const res = await resolve(event, { transformPageChunk }); - if (span) { - span.setHttpStatus(res.status); - } - return res; - }, - sendErrorToSentry, - ); + ); + return resolveResult; + } catch (e: unknown) { + sendErrorToSentry(e); + throw e; + } finally { + await flushIfServerless(); + } } diff --git a/packages/sveltekit/src/server/handleError.ts b/packages/sveltekit/src/server/handleError.ts index 022c1c814930..938cbf612e2f 100644 --- a/packages/sveltekit/src/server/handleError.ts +++ b/packages/sveltekit/src/server/handleError.ts @@ -6,6 +6,8 @@ import { addExceptionMechanism } from '@sentry/utils'; // eslint-disable-next-line import/no-unresolved import type { HandleServerError, RequestEvent } from '@sveltejs/kit'; +import { flushIfServerless } from './utils'; + // The SvelteKit default error handler just logs the error's stack trace to the console // see: https://github.com/sveltejs/kit/blob/369e7d6851f543a40c947e033bfc4a9506fdc0a8/packages/kit/src/runtime/server/index.js#L43 function defaultErrorHandler({ error }: Parameters[0]): ReturnType { @@ -20,7 +22,7 @@ function defaultErrorHandler({ error }: Parameters[0]): Retur * @param handleError The original SvelteKit error handler. */ export function handleErrorWithSentry(handleError: HandleServerError = defaultErrorHandler): HandleServerError { - return (input: { error: unknown; event: RequestEvent }): ReturnType => { + return async (input: { error: unknown; event: RequestEvent }): Promise => { if (isNotFoundError(input)) { return handleError(input); } @@ -36,6 +38,8 @@ export function handleErrorWithSentry(handleError: HandleServerError = defaultEr return scope; }); + await flushIfServerless(); + return handleError(input); }; } diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index c286ad5e3834..e819c434e81b 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,5 +1,5 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import { getCurrentHub, trace } from '@sentry/core'; +import { getCurrentHub, startSpan } from '@sentry/core'; import { captureException } from '@sentry/node'; import type { TransactionContext } from '@sentry/types'; import { addExceptionMechanism, addNonEnumerableProperty, objectify } from '@sentry/utils'; @@ -7,7 +7,7 @@ import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; import { isHttpError, isRedirect } from '../common/utils'; -import { getTracePropagationData } from './utils'; +import { flushIfServerless, getTracePropagationData } from './utils'; type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; @@ -57,7 +57,7 @@ function sendErrorToSentry(e: unknown): unknown { // eslint-disable-next-line @typescript-eslint/no-explicit-any export function wrapLoadWithSentry any>(origLoad: T): T { return new Proxy(origLoad, { - apply: (wrappingTarget, thisArg, args: Parameters) => { + apply: async (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `Load` (see comment above function signature) // Also, this event possibly already has a sentry wrapped flag attached const event = args[0] as PatchedLoadEvent; @@ -80,7 +80,15 @@ export function wrapLoadWithSentry any>(origLoad: T) }, }; - return trace(traceLoadContext, () => wrappingTarget.apply(thisArg, args), sendErrorToSentry); + try { + // We need to await before returning, otherwise we won't catch any errors thrown by the load function + return await startSpan(traceLoadContext, () => wrappingTarget.apply(thisArg, args)); + } catch (e) { + sendErrorToSentry(e); + throw e; + } finally { + await flushIfServerless(); + } }, }); } @@ -109,7 +117,7 @@ export function wrapLoadWithSentry any>(origLoad: T) // eslint-disable-next-line @typescript-eslint/no-explicit-any export function wrapServerLoadWithSentry any>(origServerLoad: T): T { return new Proxy(origServerLoad, { - apply: (wrappingTarget, thisArg, args: Parameters) => { + apply: async (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `ServerLoad` (see comment above function signature) // Also, this event possibly already has a sentry wrapped flag attached const event = args[0] as PatchedServerLoadEvent; @@ -144,7 +152,15 @@ export function wrapServerLoadWithSentry any>(origSe ...traceparentData, }; - return trace(traceLoadContext, () => wrappingTarget.apply(thisArg, args), sendErrorToSentry); + try { + // We need to await before returning, otherwise we won't catch any errors thrown by the load function + return await startSpan(traceLoadContext, () => wrappingTarget.apply(thisArg, args)); + } catch (e: unknown) { + sendErrorToSentry(e); + throw e; + } finally { + await flushIfServerless(); + } }, }); } diff --git a/packages/sveltekit/src/server/utils.ts b/packages/sveltekit/src/server/utils.ts index 1a9e1781643c..cf591568486b 100644 --- a/packages/sveltekit/src/server/utils.ts +++ b/packages/sveltekit/src/server/utils.ts @@ -1,5 +1,6 @@ +import { flush } from '@sentry/node'; import type { StackFrame } from '@sentry/types'; -import { basename, escapeStringForRegex, GLOBAL_OBJ, join, tracingContextFromHeaders } from '@sentry/utils'; +import { basename, escapeStringForRegex, GLOBAL_OBJ, join, logger, tracingContextFromHeaders } from '@sentry/utils'; import type { RequestEvent } from '@sveltejs/kit'; import { WRAPPED_MODULE_SUFFIX } from '../vite/autoInstrument'; @@ -68,3 +69,18 @@ export function rewriteFramesIteratee(frame: StackFrame): StackFrame { return frame; } + +/** Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda ends */ +export async function flushIfServerless(): Promise { + const platformSupportsStreaming = !process.env.LAMBDA_TASK_ROOT && !process.env.VERCEL; + + if (!platformSupportsStreaming) { + try { + __DEBUG_BUILD__ && logger.log('Flushing events...'); + await flush(2000); + __DEBUG_BUILD__ && logger.log('Done flushing events'); + } catch (e) { + __DEBUG_BUILD__ && logger.log('Error while flushing events:\n', e); + } + } +} diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index c2b35bb7d2e9..e68e075c7ebd 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -21,26 +21,28 @@ vi.mock('@sentry/node', async () => { }; }); -const mockTrace = vi.fn(); +const mockStartSpan = 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); + startSpan: (...args: unknown[]) => { + mockStartSpan(...args); + return original.startSpan(...args); }, }; }); -const mockAddExceptionMechanism = vi.fn(); +const mockAddExceptionMechanism = vi.fn((_e, _m) => {}); vi.mock('@sentry/utils', async () => { const original = (await vi.importActual('@sentry/utils')) as any; return { ...original, - addExceptionMechanism: (...args: unknown[]) => mockAddExceptionMechanism(...args), + addExceptionMechanism: (...args: unknown[]) => { + return mockAddExceptionMechanism(args[0], args[1]); + }, }; }); @@ -127,10 +129,10 @@ beforeAll(() => { addTracingExtensions(); }); -beforeEach(() => { +afterEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); - mockTrace.mockClear(); + mockStartSpan.mockClear(); mockScope = new Scope(); }); @@ -203,11 +205,11 @@ describe.each([ }; } - const wrappedLoad = sentryLoadWrapperFn.call(this, load); + const wrappedLoad = sentryLoadWrapperFn(load); const res = wrappedLoad(getServerOnlyArgs()); await expect(res).rejects.toThrow(); - expect(addEventProcessorSpy).toBeCalledTimes(1); + expect(addEventProcessorSpy).toHaveBeenCalledTimes(1); expect(mockAddExceptionMechanism).toBeCalledTimes(1); expect(mockAddExceptionMechanism).toBeCalledWith( {}, @@ -226,8 +228,8 @@ describe('wrapLoadWithSentry calls trace', () => { const wrappedLoad = wrapLoadWithSentry(load); await wrappedLoad(getLoadArgs()); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.load', origin: 'auto.function.sveltekit', @@ -238,7 +240,6 @@ describe('wrapLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -246,8 +247,8 @@ describe('wrapLoadWithSentry calls trace', () => { const wrappedLoad = wrapLoadWithSentry(load); await wrappedLoad(getLoadArgsWithoutRoute()); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.load', origin: 'auto.function.sveltekit', @@ -258,7 +259,6 @@ describe('wrapLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -266,7 +266,7 @@ describe('wrapLoadWithSentry calls trace', () => { const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(wrapLoadWithSentry(load))); await wrappedLoad(getLoadArgs()); - expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledTimes(1); }); }); @@ -281,8 +281,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(getServerOnlyArgs()); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.server.load', origin: 'auto.function.sveltekit', @@ -308,7 +308,6 @@ describe('wrapServerLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -316,8 +315,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(getServerArgsWithoutTracingHeaders()); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.server.load', origin: 'auto.function.sveltekit', @@ -331,7 +330,6 @@ describe('wrapServerLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -339,8 +337,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(getServerArgsWithoutBaggageHeader()); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.server.load', origin: 'auto.function.sveltekit', @@ -358,7 +356,6 @@ describe('wrapServerLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -369,8 +366,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(event); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.server.load', origin: 'auto.function.sveltekit', @@ -396,7 +393,6 @@ describe('wrapServerLoadWithSentry calls trace', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -404,7 +400,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(wrapServerLoadWithSentry(serverLoad)); await wrappedLoad(getServerOnlyArgs()); - expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledTimes(1); }); it("doesn't invoke the proxy set on `event.route`", async () => { @@ -423,14 +419,13 @@ describe('wrapServerLoadWithSentry calls trace', () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(event); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( expect.objectContaining({ op: 'function.sveltekit.server.load', name: '/users/[id]', // <-- this shows that the route was still accessed }), expect.any(Function), - expect.any(Function), ); expect(proxyFn).not.toHaveBeenCalled();