Skip to content

Commit

Permalink
fix(sveltekit): Only instrument SvelteKit fetch if the SDK client i…
Browse files Browse the repository at this point in the history
…s valid (#8381)

In the client-side SvelteKit fetch instrumentation, our previous type
cast when retrieving the SDK client was wrong, causing us to not guard
the fetch instrumentation correctly if the client was undefined. This fix
adds an undefined check.

fixes #8290
  • Loading branch information
Lms24 authored Jun 22, 2023
1 parent da2487e commit ee4e37e
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 11 deletions.
25 changes: 19 additions & 6 deletions packages/sveltekit/src/client/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { BaseClient } from '@sentry/core';
import { getCurrentHub, trace } from '@sentry/core';
import type { Breadcrumbs, BrowserTracing } from '@sentry/svelte';
import { captureException } from '@sentry/svelte';
import type { ClientOptions, SanitizedRequestData } from '@sentry/types';
import type { Client, ClientOptions, SanitizedRequestData } from '@sentry/types';
import {
addExceptionMechanism,
addNonEnumerableProperty,
Expand Down Expand Up @@ -122,12 +122,14 @@ type SvelteKitFetch = LoadEvent['fetch'];
* @returns a proxy of SvelteKit's fetch implementation
*/
function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch {
const client = getCurrentHub().getClient() as BaseClient<ClientOptions>;
const client = getCurrentHub().getClient();

const browserTracingIntegration =
client.getIntegrationById && (client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined);
const breadcrumbsIntegration =
client.getIntegrationById && (client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined);
if (!isValidClient(client)) {
return originalFetch;
}

const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined;
const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined;

const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options;

Expand Down Expand Up @@ -270,3 +272,14 @@ function addFetchBreadcrumb(
},
);
}

type MaybeClientWithGetIntegrationsById =
| (Client & { getIntegrationById?: BaseClient<ClientOptions>['getIntegrationById'] })
| undefined;

type ClientWithGetIntegrationById = Required<MaybeClientWithGetIntegrationsById> &
Exclude<MaybeClientWithGetIntegrationsById, undefined>;

function isValidClient(client: MaybeClientWithGetIntegrationsById): client is ClientWithGetIntegrationById {
return !!client && typeof client.getIntegrationById === 'function';
}
34 changes: 29 additions & 5 deletions packages/sveltekit/test/client/load.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ const mockedGetIntegrationById = vi.fn(id => {
return undefined;
});

const mockedGetClient = vi.fn(() => {
return {
getIntegrationById: mockedGetIntegrationById,
};
});

vi.mock('@sentry/core', async () => {
const original = (await vi.importActual('@sentry/core')) as any;
return {
Expand All @@ -62,11 +68,7 @@ vi.mock('@sentry/core', async () => {
},
getCurrentHub: () => {
return {
getClient: () => {
return {
getIntegrationById: mockedGetIntegrationById,
};
},
getClient: mockedGetClient,
getScope: () => {
return {
getSpan: () => {
Expand Down Expand Up @@ -427,6 +429,28 @@ describe('wrapLoadWithSentry', () => {
});
});

it.each([
['is undefined', undefined],
["doesn't have a `getClientById` method", {}],
])("doesn't instrument fetch if the client %s", async (_, client) => {
// @ts-expect-error: we're mocking the client
mockedGetClient.mockImplementationOnce(() => client);

async function load(_event: Parameters<Load>[0]): Promise<ReturnType<Load>> {
return {
msg: 'hi',
};
}
const wrappedLoad = wrapLoadWithSentry(load);

const originalFetch = MOCK_LOAD_ARGS.fetch;
await wrappedLoad(MOCK_LOAD_ARGS);

expect(MOCK_LOAD_ARGS.fetch).toStrictEqual(originalFetch);

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

it('adds an exception mechanism', async () => {
const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => {
void callback({}, { event_id: 'fake-event-id' });
Expand Down

0 comments on commit ee4e37e

Please sign in to comment.