From a67a69ecad5f2f6925d25cc4e44137c86db255ae Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 8 Aug 2024 18:37:53 +0200 Subject: [PATCH] feat(sveltekit): Add `wrapServerRouteWithSentry` wrapper (#13247) Add a wrapper for SvelteKit server routes. The reason is that some errors (e.g. sveltekit `error()` calls) are not caught within server (API) routes, as reported in #13224 because in contrast to `load` function we don't directly try/catch the function invokation. For now, users will have to add this wrapper manually. At a later time we can think about auto instrumentation, similarly to `load` functions but for now this will remain manual. --- .../sveltekit-2/src/routes/+page.svelte | 3 + .../src/routes/wrap-server-route/+page.svelte | 7 + .../src/routes/wrap-server-route/+page.ts | 7 + .../routes/wrap-server-route/api/+server.ts | 6 + .../sveltekit-2/tests/errors.server.test.ts | 33 +++++ packages/sveltekit/src/server/handle.ts | 40 +----- packages/sveltekit/src/server/index.ts | 1 + packages/sveltekit/src/server/load.ts | 46 +----- packages/sveltekit/src/server/serverRoute.ts | 67 +++++++++ packages/sveltekit/src/server/utils.ts | 43 +++++- .../sveltekit/test/server/serverRoute.test.ts | 133 ++++++++++++++++++ 11 files changed, 307 insertions(+), 79 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts create mode 100644 packages/sveltekit/src/server/serverRoute.ts create mode 100644 packages/sveltekit/test/server/serverRoute.test.ts diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte index e7788b6433cd..0cfae1c54741 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte @@ -38,4 +38,7 @@
  • Component Tracking
    • +
  • + server routes +
    • diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte new file mode 100644 index 000000000000..adc04d52c0ea --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.svelte @@ -0,0 +1,7 @@ + + +

    + Message from API: {data.myMessage} +

    diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts new file mode 100644 index 000000000000..3f3f5942366e --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/+page.ts @@ -0,0 +1,7 @@ +export const load = async ({ fetch }) => { + const res = await fetch('/wrap-server-route/api'); + const myMessage = await res.json(); + return { + myMessage: myMessage.myMessage, + }; +}; diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts new file mode 100644 index 000000000000..6ba210690ad5 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/wrap-server-route/api/+server.ts @@ -0,0 +1,6 @@ +import { wrapServerRouteWithSentry } from '@sentry/sveltekit'; +import { error } from '@sveltejs/kit'; + +export const GET = wrapServerRouteWithSentry(async () => { + error(500, 'error() error'); +}); diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts index c9dc56b9c96b..fd2e58e9c2a3 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/tests/errors.server.test.ts @@ -58,4 +58,37 @@ test.describe('server-side errors', () => { expect(errorEvent.transaction).toEqual('GET /server-route-error'); }); + + test('captures error() thrown in server route with `wrapServerRouteWithSentry`', async ({ page }) => { + const errorEventPromise = waitForError('sveltekit-2', errorEvent => { + return errorEvent?.exception?.values?.[0]?.value === "'HttpError' captured as exception with keys: body, status"; + }); + + await page.goto('/wrap-server-route'); + + expect(await errorEventPromise).toMatchObject({ + exception: { + values: [ + { + value: "'HttpError' captured as exception with keys: body, status", + mechanism: { + handled: false, + data: { + function: 'serverRoute', + }, + }, + stacktrace: { frames: expect.any(Array) }, + }, + ], + }, + extra: { + __serialized__: { + body: { + message: 'error() error', + }, + status: 500, + }, + }, + }); + }); }); diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 56ddc23e1885..7f4b8d980ad8 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -11,21 +11,15 @@ import { withIsolationScope, } from '@sentry/core'; import { startSpan } from '@sentry/core'; -import { captureException, continueTrace } from '@sentry/node'; +import { continueTrace } from '@sentry/node'; import type { Span } from '@sentry/types'; -import { - dynamicSamplingContextToSentryBaggageHeader, - logger, - objectify, - winterCGRequestToRequestData, -} from '@sentry/utils'; +import { dynamicSamplingContextToSentryBaggageHeader, logger, winterCGRequestToRequestData } from '@sentry/utils'; import type { Handle, ResolveOptions } from '@sveltejs/kit'; import { getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry'; import { DEBUG_BUILD } from '../common/debug-build'; -import { isHttpError, isRedirect } from '../common/utils'; -import { flushIfServerless, getTracePropagationData } from './utils'; +import { flushIfServerless, getTracePropagationData, sendErrorToSentry } from './utils'; export type SentryHandleOptions = { /** @@ -62,32 +56,6 @@ export type SentryHandleOptions = { fetchProxyScriptNonce?: string; }; -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 - // store a seen flag on it. - const objectifiedErr = objectify(e); - - // similarly to the `load` function, we don't want to capture 4xx errors or redirects - if ( - isRedirect(objectifiedErr) || - (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) - ) { - return objectifiedErr; - } - - captureException(objectifiedErr, { - mechanism: { - type: 'sveltekit', - handled: false, - data: { - function: 'handle', - }, - }, - }); - - return objectifiedErr; -} - /** * Exported only for testing */ @@ -225,7 +193,7 @@ async function instrumentHandle( ); return resolveResult; } catch (e: unknown) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'handle'); throw e; } finally { await flushIfServerless(); diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index 99813d01ceba..32dd6627d7a6 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -129,6 +129,7 @@ export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; export { wrapLoadWithSentry, wrapServerLoadWithSentry } from './load'; export { sentryHandle } from './handle'; +export { wrapServerRouteWithSentry } from './serverRoute'; /** * Tracks the Svelte component's initialization and mounting operation as well as diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index fe61ed0913bd..82a8c548c6ec 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,49 +1,13 @@ -import { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - captureException, - startSpan, -} from '@sentry/node'; -import { addNonEnumerableProperty, objectify } from '@sentry/utils'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, startSpan } from '@sentry/node'; +import { addNonEnumerableProperty } from '@sentry/utils'; import type { LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; -import { isHttpError, isRedirect } from '../common/utils'; -import { flushIfServerless } from './utils'; +import { flushIfServerless, sendErrorToSentry } from './utils'; type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; -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 - // store a seen flag on it. - const objectifiedErr = objectify(e); - - // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error - // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they - // could be noisy. - // Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown - // `Redirect`s as they're not errors but expected behaviour - if ( - isRedirect(objectifiedErr) || - (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) - ) { - return objectifiedErr; - } - - captureException(objectifiedErr, { - mechanism: { - type: 'sveltekit', - handled: false, - data: { - function: 'load', - }, - }, - }); - - return objectifiedErr; -} - /** * @inheritdoc */ @@ -81,7 +45,7 @@ export function wrapLoadWithSentry any>(origLoad: T) () => wrappingTarget.apply(thisArg, args), ); } catch (e) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'load'); throw e; } finally { await flushIfServerless(); @@ -146,7 +110,7 @@ export function wrapServerLoadWithSentry any>(origSe () => wrappingTarget.apply(thisArg, args), ); } catch (e: unknown) { - sendErrorToSentry(e); + sendErrorToSentry(e, 'load'); throw e; } finally { await flushIfServerless(); diff --git a/packages/sveltekit/src/server/serverRoute.ts b/packages/sveltekit/src/server/serverRoute.ts new file mode 100644 index 000000000000..a5f13f9a73ca --- /dev/null +++ b/packages/sveltekit/src/server/serverRoute.ts @@ -0,0 +1,67 @@ +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, startSpan } from '@sentry/node'; +import { addNonEnumerableProperty } from '@sentry/utils'; +import type { RequestEvent } from '@sveltejs/kit'; +import { flushIfServerless, sendErrorToSentry } from './utils'; + +type PatchedServerRouteEvent = RequestEvent & { __sentry_wrapped__?: boolean }; + +/** + * Wraps a server route handler for API or server routes registered in `+server.(js|js)` files. + * + * This function will automatically capture any errors that occur during the execution of the route handler + * and it will start a span for the duration of your route handler. + * + * @example + * ```js + * import { wrapServerRouteWithSentry } from '@sentry/sveltekit'; + * + * const get = async event => { + * return new Response(JSON.stringify({ message: 'hello world' })); + * } + * + * export const GET = wrapServerRouteWithSentry(get); + * ``` + * + * @param originalRouteHandler your server route handler + * @param httpMethod the HTTP method of your route handler + * + * @returns a wrapped version of your server route handler + */ +export function wrapServerRouteWithSentry( + originalRouteHandler: (request: RequestEvent) => Promise, +): (requestEvent: RequestEvent) => Promise { + return new Proxy(originalRouteHandler, { + apply: async (wrappingTarget, thisArg, args) => { + const event = args[0] as PatchedServerRouteEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + const routeId = event.route && event.route.id; + const httpMethod = event.request.method; + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + + try { + return await startSpan( + { + name: `${httpMethod} ${routeId || 'Server Route'}`, + op: `function.sveltekit.server.${httpMethod.toLowerCase()}`, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + onlyIfParent: true, + }, + () => wrappingTarget.apply(thisArg, args), + ); + } catch (e) { + sendErrorToSentry(e, 'serverRoute'); + throw e; + } finally { + await flushIfServerless(); + } + }, + }); +} diff --git a/packages/sveltekit/src/server/utils.ts b/packages/sveltekit/src/server/utils.ts index 0db8ee893783..8d7f2c649331 100644 --- a/packages/sveltekit/src/server/utils.ts +++ b/packages/sveltekit/src/server/utils.ts @@ -1,8 +1,9 @@ -import { flush } from '@sentry/node'; -import { logger } from '@sentry/utils'; +import { captureException, flush } from '@sentry/node'; +import { logger, objectify } from '@sentry/utils'; import type { RequestEvent } from '@sveltejs/kit'; import { DEBUG_BUILD } from '../common/debug-build'; +import { isHttpError, isRedirect } from '../common/utils'; /** * Takes a request event and extracts traceparent and DSC data @@ -31,3 +32,41 @@ export async function flushIfServerless(): Promise { } } } + +/** + * Extracts a server-side sveltekit error, filters a couple of known errors we don't want to capture + * and captures the error via `captureException`. + * + * @param e error + * + * @returns an objectified version of @param e + */ +export function sendErrorToSentry(e: unknown, handlerFn: 'handle' | 'load' | 'serverRoute'): object { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error + // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they + // could be noisy. + // Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown + // `Redirect`s as they're not errors but expected behaviour + if ( + isRedirect(objectifiedErr) || + (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) + ) { + return objectifiedErr; + } + + captureException(objectifiedErr, { + mechanism: { + type: 'sveltekit', + handled: false, + data: { + function: handlerFn, + }, + }, + }); + + return objectifiedErr; +} diff --git a/packages/sveltekit/test/server/serverRoute.test.ts b/packages/sveltekit/test/server/serverRoute.test.ts new file mode 100644 index 000000000000..de99db5a548e --- /dev/null +++ b/packages/sveltekit/test/server/serverRoute.test.ts @@ -0,0 +1,133 @@ +import * as SentryNode from '@sentry/node'; +import type { NumericRange } from '@sveltejs/kit'; +import { type RequestEvent, error, redirect } from '@sveltejs/kit'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + wrapServerRouteWithSentry, +} from '../../src/server'; + +describe('wrapServerRouteWithSentry', () => { + const originalRouteHandler = vi.fn(); + + const getRequestEventMock = () => + ({ + request: { + method: 'GET', + }, + route: { + id: '/api/users/:id', + }, + }) as RequestEvent; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('wraps a server route span around the original server route handler', () => { + const startSpanSpy = vi.spyOn(SentryNode, 'startSpan'); + + it('assigns the route id as name if available', () => { + const wrappedRouteHandler = wrapServerRouteWithSentry(originalRouteHandler); + + wrappedRouteHandler(getRequestEventMock() as RequestEvent); + + expect(startSpanSpy).toHaveBeenCalledWith( + { + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + name: 'GET /api/users/:id', + onlyIfParent: true, + op: 'function.sveltekit.server.get', + }, + expect.any(Function), + ); + + expect(originalRouteHandler).toHaveBeenCalledTimes(1); + }); + + it('falls back to a generic name if route id is not available', () => { + const wrappedRouteHandler = wrapServerRouteWithSentry(originalRouteHandler); + + wrappedRouteHandler({ ...getRequestEventMock(), route: undefined } as unknown as RequestEvent); + + expect(startSpanSpy).toHaveBeenCalledWith( + { + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + name: 'GET Server Route', + onlyIfParent: true, + op: 'function.sveltekit.server.get', + }, + expect.any(Function), + ); + + expect(originalRouteHandler).toHaveBeenCalledTimes(1); + }); + }); + + const captureExceptionSpy = vi.spyOn(SentryNode, 'captureException'); + describe('captures server route errors', () => { + it('captures and rethrows normal server route error', async () => { + const error = new Error('Server Route Error'); + + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + throw error; + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrowError('Server Route Error'); + + expect(captureExceptionSpy).toHaveBeenCalledWith(error, { + mechanism: { type: 'sveltekit', handled: false, data: { function: 'serverRoute' } }, + }); + }); + + it.each([500, 501, 599])('captures and rethrows %s error() calls', async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + error(status as NumericRange<400, 599>, `error(${status}) error`); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).toHaveBeenCalledWith( + { body: { message: `error(${status}) error` }, status }, + { + mechanism: { type: 'sveltekit', handled: false, data: { function: 'serverRoute' } }, + }, + ); + }); + + it.each([400, 401, 499])("doesn't capture but rethrows %s error() calls", async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + error(status as NumericRange<400, 599>, `error(${status}) error`); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + + it.each([300, 301, 308])("doesn't capture redirect(%s) calls", async status => { + const wrappedRouteHandler = wrapServerRouteWithSentry(() => { + redirect(status as NumericRange<300, 308>, '/redirect'); + }); + + await expect(async () => { + await wrappedRouteHandler(getRequestEventMock() as RequestEvent); + }).rejects.toThrow(); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + }); + }); +});