Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sveltekit): Add option to control handling of unknown server routes #8201

Merged
merged 2 commits into from May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 41 additions & 14 deletions packages/sveltekit/src/server/handle.ts
Expand Up @@ -7,6 +7,24 @@ import type { Handle, ResolveOptions } from '@sveltejs/kit';

import { getTracePropagationData } from './utils';

export type SentryHandleOptions = {
/**
* Controls whether the SDK should capture errors and traces in requests that don't belong to a
* route defined in your SvelteKit application.
*
* By default, this option is set to `false` to reduce noise (e.g. bots sending random requests to your server).
*
* Set this option to `true` if you want to monitor requests events without a route. This might be useful in certain
* scenarios, for instance if you registered other handlers that handle these requests.
* If you set this option, you might want adjust the the transaction name in the `beforeSendTransaction`
* callback of your server-side `Sentry.init` options. You can also use `beforeSendTransaction` to filter out
* transactions that you still don't want to be sent to Sentry.
Comment on lines +19 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to determine a name for these transactions automatically? Maybe there's something on the event to serve as a sensible parameterized default? Didn't check but still wanted to ask ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I'm aware of. I don't think there's anything better that's on all these requests we can fall back to than the URL pathname.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description with a better example for future reference.

*
* @default false
*/
handleUnknownRoutes?: boolean;
};

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.
Expand Down Expand Up @@ -59,33 +77,42 @@ export const transformPageChunk: NonNullable<ResolveOptions['transformPageChunk'
* // export const handle = sequence(sentryHandle(), yourCustomHandler);
* ```
*/
export function sentryHandle(): Handle {
export function sentryHandle(handlerOptions?: SentryHandleOptions): Handle {
const options = {
handleUnknownRoutes: false,
...handlerOptions,
};

const sentryRequestHandler: Handle = 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, options);
}
return runWithAsyncContext(() => {
return instrumentHandle(input, options);
});
};

return sentryRequestHandler;
}

const sentryRequestHandler: Handle = 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);
function instrumentHandle({ event, resolve }: Parameters<Handle>[0], options: SentryHandleOptions): ReturnType<Handle> {
if (!event.route?.id && !options.handleUnknownRoutes) {
return resolve(event);
}
return runWithAsyncContext(() => {
return instrumentHandle(input);
});
};

function instrumentHandle({ event, resolve }: Parameters<Handle>[0]): ReturnType<Handle> {
const { traceparentData, dynamicSamplingContext } = getTracePropagationData(event);

return trace(
{
op: 'http.server',
name: `${event.request.method} ${event.route.id}`,
name: `${event.request.method} ${event.route?.id || event.url.pathname}`,
status: 'ok',
...traceparentData,
metadata: {
source: 'route',
source: event.route?.id ? 'route' : 'url',
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
},
},
Expand Down
33 changes: 33 additions & 0 deletions packages/sveltekit/test/server/handle.test.ts
Expand Up @@ -305,6 +305,39 @@ describe('handleSentry', () => {
expect(mockResolve).toHaveBeenCalledTimes(1);
expect(mockResolve).toHaveBeenCalledWith(event, { transformPageChunk: expect.any(Function) });
});

it("doesn't create a transaction if there's no route", async () => {
let ref: any = undefined;
client.on('finishTransaction', (transaction: Transaction) => {
ref = transaction;
});

try {
await sentryHandle()({ event: mockEvent({ route: undefined }), resolve: resolve(type, isError) });
} catch {
//
}

expect(ref).toBeUndefined();
});

it("Creates a transaction if there's no route but `handleUnknownRequests` is true", async () => {
let ref: any = undefined;
client.on('finishTransaction', (transaction: Transaction) => {
ref = transaction;
});

try {
await sentryHandle({ handleUnknownRoutes: true })({
event: mockEvent({ route: undefined }),
resolve: resolve(type, isError),
});
} catch {
//
}

expect(ref).toBeDefined();
});
});
});

Expand Down