Skip to content

fix(sveltekit): Avoid capturing Http 4xx errors on the client #10571

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 9, 2024
Merged

fix(sveltekit): Avoid capturing Http 4xx errors on the client #10571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b979a58
fix(sveltekit): Avoid capturing Http 400 errors on the client
Lms24 Feb 8, 2024
caf5da1
cleanup
Lms24 Feb 8, 2024
6e0c249
Update packages/sveltekit/src/client/load.ts
Lms24 Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/sveltekit/src/client/load.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { addNonEnumerableProperty, objectify } from '@sentry/utils';
import type { LoadEvent } from '@sveltejs/kit';

import type { SentryWrappedFlag } from '../common/utils';
import { isRedirect } from '../common/utils';
import { isHttpError, isRedirect } from '../common/utils';

type PatchedLoadEvent = LoadEvent & Partial<SentryWrappedFlag>;

Expand All @@ -14,7 +14,11 @@ function sendErrorToSentry(e: unknown): unknown {
const objectifiedErr = objectify(e);

// We don't want to capture thrown `Redirect`s as these are not errors but expected behaviour
if (isRedirect(objectifiedErr)) {
// Neither 4xx errors, given that they are not valuable.
if (
isRedirect(objectifiedErr) ||
(isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400)
Copy link
Member

Choose a reason for hiding this comment

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

so, the issue linked complains about 403 errors being captured, it talks about ignoring all 4xx errors, but we only ignore 400 - is that correct/on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's on purpose in the sense that we have the same logic on the server side and generally, afaik, we never want to capture 4xx errors by default.

Copy link
Member

Choose a reason for hiding this comment

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

ahh wait, I got confused by the comment which says neither 400 errors and thought this was filtering only 400 itself, not 403. all good then!

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh sorry, that was my mistake. 4xx is definitely the better description

) {
return objectifiedErr;
}

Expand Down
24 changes: 24 additions & 0 deletions packages/sveltekit/test/client/load.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,30 @@ describe('wrapLoadWithSentry', () => {
expect(mockCaptureException).not.toHaveBeenCalled();
});

it.each([400, 404, 499])("doesn't call captureException for thrown `HttpError`s with status %s", async status => {
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
throw { status, body: 'error' };
}

const wrappedLoad = wrapLoadWithSentry(load);
const res = wrappedLoad(MOCK_LOAD_ARGS);
await expect(res).rejects.toThrow();

expect(mockCaptureException).not.toHaveBeenCalled();
});

it.each([500, 501, 599])('calls captureException for thrown `HttpError`s with status %s', async status => {
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
throw { status, body: 'error' };
}

const wrappedLoad = wrapLoadWithSentry(load);
const res = wrappedLoad(MOCK_LOAD_ARGS);
await expect(res).rejects.toThrow();

expect(mockCaptureException).toHaveBeenCalledTimes(1);
});

describe('calls trace function', async () => {
it('creates a load span', async () => {
async function load({ params }: Parameters<Load>[0]): Promise<ReturnType<Load>> {
Expand Down