From c996143b5a8187ce7f3a99272c0f9f2bc16dcd39 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 11 Aug 2023 13:54:32 +0200 Subject: [PATCH 1/4] remove sveltekit-specific client fetch instrumentation --- packages/sveltekit/package.json | 2 +- packages/sveltekit/src/client/load.ts | 197 +----------------- packages/sveltekit/src/client/sdk.ts | 20 +- .../src/client/vendor/buildSelector.ts | 57 ----- packages/sveltekit/src/client/vendor/hash.ts | 51 ----- .../src/client/vendor/lookUpCache.ts | 79 ------- packages/sveltekit/src/server/handle.ts | 13 +- .../test/client/vendor/lookUpCache.test.ts | 45 ---- 8 files changed, 33 insertions(+), 431 deletions(-) delete mode 100644 packages/sveltekit/src/client/vendor/buildSelector.ts delete mode 100644 packages/sveltekit/src/client/vendor/hash.ts delete mode 100644 packages/sveltekit/src/client/vendor/lookUpCache.ts delete mode 100644 packages/sveltekit/test/client/vendor/lookUpCache.test.ts diff --git a/packages/sveltekit/package.json b/packages/sveltekit/package.json index ebcfb5a01158..b69c69f00386 100644 --- a/packages/sveltekit/package.json +++ b/packages/sveltekit/package.json @@ -44,7 +44,7 @@ "build:types": "tsc -p tsconfig.types.json", "build:watch": "run-p build:transpile:watch build:types:watch", "build:dev:watch": "yarn build:watch", - "build:transpile:watch": "rollup -c rollup.npm.config.js --watch", + "build:transpile:watch": "rollup -c rollup.npm.config.js --bundleConfigAsCjs --watch", "build:types:watch": "tsc -p tsconfig.types.json --watch", "build:tarball": "ts-node ../../scripts/prepack.ts && npm pack ./build", "circularDepCheck": "madge --circular src/index.client.ts && madge --circular src/index.server.ts && madge --circular src/index.types.ts", diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index e12ed19a6cae..44430a3b9b1c 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,23 +1,10 @@ -import { addTracingHeadersToFetchRequest } from '@sentry-internal/tracing'; -import type { BaseClient } from '@sentry/core'; -import { getCurrentHub, trace } from '@sentry/core'; -import type { Breadcrumbs, BrowserTracing } from '@sentry/svelte'; +import { trace } from '@sentry/core'; import { captureException } from '@sentry/svelte'; -import type { Client, ClientOptions, SanitizedRequestData, Span } from '@sentry/types'; -import { - addExceptionMechanism, - addNonEnumerableProperty, - getSanitizedUrlString, - objectify, - parseFetchArgs, - parseUrl, - stringMatchesSomePattern, -} from '@sentry/utils'; +import { addExceptionMechanism, addNonEnumerableProperty, objectify } from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; -import { isRequestCached } from './vendor/lookUpCache'; type PatchedLoadEvent = LoadEvent & Partial; @@ -80,7 +67,6 @@ export function wrapLoadWithSentry any>(origLoad: T) const patchedEvent: PatchedLoadEvent = { ...event, - fetch: instrumentSvelteKitFetch(event.fetch), }; addNonEnumerableProperty(patchedEvent as unknown as Record, '__sentry_wrapped__', true); @@ -101,182 +87,3 @@ export function wrapLoadWithSentry any>(origLoad: T) }, }); } - -type SvelteKitFetch = LoadEvent['fetch']; - -/** - * Instruments SvelteKit's client `fetch` implementation which is passed to the client-side universal `load` functions. - * - * We need to instrument this in addition to the native fetch we instrument in BrowserTracing because SvelteKit - * stores the native fetch implementation before our SDK is initialized. - * - * see: https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/client/fetcher.js - * - * This instrumentation takes the fetch-related options from `BrowserTracing` to determine if we should - * instrument fetch for perfomance monitoring, create a span for or attach our tracing headers to the given request. - * - * To dertermine if breadcrumbs should be recorded, this instrumentation relies on the availability of and the options - * set in the `BreadCrumbs` integration. - * - * @param originalFetch SvelteKit's original fetch implemenetation - * - * @returns a proxy of SvelteKit's fetch implementation - */ -function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch { - const client = getCurrentHub().getClient(); - - if (!isValidClient(client)) { - return originalFetch; - } - - const options = client.getOptions(); - - const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined; - const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined; - - const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options; - - const shouldTraceFetch = browserTracingOptions && browserTracingOptions.traceFetch; - const shouldAddFetchBreadcrumb = breadcrumbsIntegration && breadcrumbsIntegration.options.fetch; - - /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ - const shouldCreateSpan = - browserTracingOptions && typeof browserTracingOptions.shouldCreateSpanForRequest === 'function' - ? browserTracingOptions.shouldCreateSpanForRequest - : (_: string) => shouldTraceFetch; - - /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ - const shouldAttachHeaders: (url: string) => boolean = url => { - return ( - !!shouldTraceFetch && - stringMatchesSomePattern( - url, - options.tracePropagationTargets || browserTracingOptions.tracePropagationTargets || ['localhost', /^\//], - ) - ); - }; - - return new Proxy(originalFetch, { - apply: (wrappingTarget, thisArg, args: Parameters) => { - const [input, init] = args; - - if (isRequestCached(input, init)) { - return wrappingTarget.apply(thisArg, args); - } - - const { url: rawUrl, method } = parseFetchArgs(args); - - // TODO: extract this to a util function (and use it in breadcrumbs integration as well) - if (rawUrl.match(/sentry_key/)) { - // We don't create spans or breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests) - return wrappingTarget.apply(thisArg, args); - } - - const urlObject = parseUrl(rawUrl); - - const requestData: SanitizedRequestData = { - url: getSanitizedUrlString(urlObject), - 'http.method': method, - ...(urlObject.search && { 'http.query': urlObject.search.substring(1) }), - ...(urlObject.hash && { 'http.hash': urlObject.hash.substring(1) }), - }; - - const patchedInit: RequestInit = { ...init }; - const hub = getCurrentHub(); - const scope = hub.getScope(); - const client = hub.getClient(); - - let fetchPromise: Promise; - - function callFetchTarget(span?: Span): Promise { - if (client && shouldAttachHeaders(rawUrl)) { - const headers = addTracingHeadersToFetchRequest(input as string | Request, client, scope, patchedInit, span); - patchedInit.headers = headers as HeadersInit; - } - const patchedFetchArgs = [input, patchedInit]; - return wrappingTarget.apply(thisArg, patchedFetchArgs); - } - - if (shouldCreateSpan(rawUrl)) { - fetchPromise = trace( - { - name: `${method} ${requestData.url}`, // this will become the description of the span - op: 'http.client', - data: requestData, - }, - span => { - const promise = callFetchTarget(span); - if (span) { - promise.then(res => span.setHttpStatus(res.status)).catch(_ => span.setStatus('internal_error')); - } - return promise; - }, - ); - } else { - fetchPromise = callFetchTarget(); - } - - if (shouldAddFetchBreadcrumb) { - addFetchBreadcrumb(fetchPromise, requestData, args); - } - - return fetchPromise; - }, - }); -} - -/* Adds a breadcrumb for the given fetch result */ -function addFetchBreadcrumb( - fetchResult: Promise, - requestData: SanitizedRequestData, - args: Parameters, -): void { - const breadcrumbStartTimestamp = Date.now(); - fetchResult.then( - response => { - getCurrentHub().addBreadcrumb( - { - type: 'http', - category: 'fetch', - data: { - ...requestData, - status_code: response.status, - }, - }, - { - input: args, - response, - startTimestamp: breadcrumbStartTimestamp, - endTimestamp: Date.now(), - }, - ); - }, - error => { - getCurrentHub().addBreadcrumb( - { - type: 'http', - category: 'fetch', - level: 'error', - data: requestData, - }, - { - input: args, - data: error, - startTimestamp: breadcrumbStartTimestamp, - endTimestamp: Date.now(), - }, - ); - }, - ); -} - -type MaybeClientWithGetIntegrationsById = - | (Client & { getIntegrationById?: BaseClient['getIntegrationById'] }) - | undefined; - -type ClientWithGetIntegrationById = Required & - Exclude; - -function isValidClient(client: MaybeClientWithGetIntegrationsById): client is ClientWithGetIntegrationById { - return !!client && typeof client.getIntegrationById === 'function'; -} diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 9bf1d2cb140b..84990f52b530 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -1,11 +1,16 @@ import { hasTracingEnabled } from '@sentry/core'; import type { BrowserOptions } from '@sentry/svelte'; -import { BrowserTracing, configureScope, init as initSvelteSdk } from '@sentry/svelte'; +import { BrowserTracing, configureScope, init as initSvelteSdk, WINDOW } from '@sentry/svelte'; +import type { InternalGlobal } from '@sentry/utils'; import { addOrUpdateIntegration } from '@sentry/utils'; import { applySdkMetadata } from '../common/metadata'; import { svelteKitRoutingInstrumentation } from './router'; +type WindowWithSentryFetchProxy = typeof WINDOW & { + _sentryFetchProxy?: typeof fetch; +}; + // Treeshakable guard to remove all code related to tracing declare const __SENTRY_TRACING__: boolean; @@ -19,8 +24,21 @@ export function init(options: BrowserOptions): void { addClientIntegrations(options); + const globalWithSentryFetchProxy: WindowWithSentryFetchProxy = WINDOW as WindowWithSentryFetchProxy; + + // eslint-disable-next-line @typescript-eslint/unbound-method + const currentFetch = WINDOW.fetch; + // @ts-expect-error TS thinks window.fetch is always defined but let's just make sure it really is + if (globalWithSentryFetchProxy._sentryFetchProxy && currentFetch) { + globalWithSentryFetchProxy.fetch = globalWithSentryFetchProxy._sentryFetchProxy; + } + initSvelteSdk(options); + // eslint-disable-next-line @typescript-eslint/unbound-method + globalWithSentryFetchProxy._sentryFetchProxy = globalWithSentryFetchProxy.fetch; + globalWithSentryFetchProxy.fetch = currentFetch; + configureScope(scope => { scope.setTag('runtime', 'browser'); }); diff --git a/packages/sveltekit/src/client/vendor/buildSelector.ts b/packages/sveltekit/src/client/vendor/buildSelector.ts deleted file mode 100644 index 9ff0ddebe7c7..000000000000 --- a/packages/sveltekit/src/client/vendor/buildSelector.ts +++ /dev/null @@ -1,57 +0,0 @@ -/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ - -// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js -// with types only changes. - -// The MIT License (MIT) - -// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) - -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files(the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: - -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. - -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -import { hash } from './hash'; - -/** - * Build the cache key for a given request - * @param {URL | RequestInfo} resource - * @param {RequestInit} [opts] - */ -export function build_selector(resource: URL | RequestInfo, opts: RequestInit | undefined): string { - const url = JSON.stringify(resource instanceof Request ? resource.url : resource); - - let selector = `script[data-sveltekit-fetched][data-url=${url}]`; - - if (opts?.headers || opts?.body) { - /** @type {import('types').StrictBody[]} */ - const values = []; - - if (opts.headers) { - // @ts-ignore - TS complains but this is a 1:1 copy of the original code and apparently it works - values.push([...new Headers(opts.headers)].join(',')); - } - - if (opts.body && (typeof opts.body === 'string' || ArrayBuffer.isView(opts.body))) { - values.push(opts.body); - } - - selector += `[data-hash="${hash(...values)}"]`; - } - - return selector; -} diff --git a/packages/sveltekit/src/client/vendor/hash.ts b/packages/sveltekit/src/client/vendor/hash.ts deleted file mode 100644 index 1723dac703a6..000000000000 --- a/packages/sveltekit/src/client/vendor/hash.ts +++ /dev/null @@ -1,51 +0,0 @@ -/* eslint-disable no-bitwise */ - -// Vendored from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/hash.js -// with types only changes. - -// The MIT License (MIT) - -// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) - -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files(the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: - -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. - -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -import type { StrictBody } from '@sveltejs/kit/types/internal'; - -/** - * Hash using djb2 - * @param {import('types').StrictBody[]} values - */ -export function hash(...values: StrictBody[]): string { - let hash = 5381; - - for (const value of values) { - if (typeof value === 'string') { - let i = value.length; - while (i) hash = (hash * 33) ^ value.charCodeAt(--i); - } else if (ArrayBuffer.isView(value)) { - const buffer = new Uint8Array(value.buffer, value.byteOffset, value.byteLength); - let i = buffer.length; - while (i) hash = (hash * 33) ^ buffer[--i]; - } else { - throw new TypeError('value must be a string or TypedArray'); - } - } - - return (hash >>> 0).toString(36); -} diff --git a/packages/sveltekit/src/client/vendor/lookUpCache.ts b/packages/sveltekit/src/client/vendor/lookUpCache.ts deleted file mode 100644 index afcaf676b40d..000000000000 --- a/packages/sveltekit/src/client/vendor/lookUpCache.ts +++ /dev/null @@ -1,79 +0,0 @@ -/* eslint-disable no-bitwise */ - -// Parts of this code are taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js -// Attribution given directly in the function code below - -// The MIT License (MIT) - -// Copyright (c) 2020 [these people](https://github.com/sveltejs/kit/graphs/contributors) - -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files(the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: - -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. - -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -import { WINDOW } from '@sentry/svelte'; -import { getDomElement } from '@sentry/utils'; - -import { build_selector } from './buildSelector'; - -/** - * Checks if a request is cached by looking for a script tag with the same selector as the constructed selector of the request. - * - * This function is a combination of the cache lookups in sveltekit's internal client-side fetch functions - * - initial_fetch (used during hydration) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L76 - * - subsequent_fetch (used afterwards) https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L98 - * - * Parts of this function's logic is taken from SvelteKit source code. - * These lines are annotated with attribution in comments above them. - * - * @param input first fetch param - * @param init second fetch param - * @returns true if a cache hit was encountered, false otherwise - */ -export function isRequestCached(input: URL | RequestInfo, init: RequestInit | undefined): boolean { - // build_selector call copied from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L77 - const selector = build_selector(input, init); - - const script = getDomElement(selector); - - if (!script) { - return false; - } - - // If the script has a data-ttl attribute, we check if we're still in the TTL window: - try { - // ttl retrieval taken from https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L83-L84 - const ttl = Number(script.getAttribute('data-ttl')) * 1000; - - if (isNaN(ttl)) { - return false; - } - - if (ttl) { - // cache hit determination taken from: https://github.com/sveltejs/kit/blob/1c1ddd5e34fce28e6f89299d6c59162bed087589/packages/kit/src/runtime/client/fetcher.js#L105-L106 - return ( - WINDOW.performance.now() < ttl && - ['default', 'force-cache', 'only-if-cached', undefined].includes(init && init.cache) - ); - } - } catch { - return false; - } - - // Otherwise, we check if the script has a content and return true in that case - return !!script.textContent; -} diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 5c90f42f4c9e..36f814abfe86 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -57,14 +57,23 @@ function sendErrorToSentry(e: unknown): unknown { return objectifiedErr; } +const FETCH_PROXY_SCRIPT = ` + const f = window.fetch; + window._sentryFetchProxy = function(...a){return f(...a)} + window.fetch = function(...a){return window._sentryFetchProxy(...a)} +`; + export const transformPageChunk: NonNullable = ({ html }) => { const transaction = getActiveTransaction(); if (transaction) { const traceparentData = transaction.toTraceparent(); const dynamicSamplingContext = dynamicSamplingContextToSentryBaggageHeader(transaction.getDynamicSamplingContext()); const content = ` - - `; + + + + `; return html.replace('', content); } diff --git a/packages/sveltekit/test/client/vendor/lookUpCache.test.ts b/packages/sveltekit/test/client/vendor/lookUpCache.test.ts deleted file mode 100644 index 29b13494be12..000000000000 --- a/packages/sveltekit/test/client/vendor/lookUpCache.test.ts +++ /dev/null @@ -1,45 +0,0 @@ -import { JSDOM } from 'jsdom'; -import { vi } from 'vitest'; - -import { isRequestCached } from '../../../src/client/vendor/lookUpCache'; - -globalThis.document = new JSDOM().window.document; - -vi.useFakeTimers().setSystemTime(new Date('2023-06-22')); -vi.spyOn(performance, 'now').mockReturnValue(1000); - -describe('isRequestCached', () => { - it('should return true if a script tag with the same selector as the constructed request selector is found', () => { - globalThis.document.body.innerHTML = - ''; - - expect(isRequestCached('/api/todos/1', undefined)).toBe(true); - }); - - it('should return false if a script with the same selector as the constructed request selector is not found', () => { - globalThis.document.body.innerHTML = ''; - - expect(isRequestCached('/api/todos/1', undefined)).toBe(false); - }); - - it('should return true if a script with the same selector as the constructed request selector is found and its TTL is valid', () => { - globalThis.document.body.innerHTML = - ''; - - expect(isRequestCached('/api/todos/1', undefined)).toBe(true); - }); - - it('should return false if a script with the same selector as the constructed request selector is found and its TTL is expired', () => { - globalThis.document.body.innerHTML = - ''; - - expect(isRequestCached('/api/todos/1', undefined)).toBe(false); - }); - - it("should return false if the TTL is set but can't be parsed as a number", () => { - globalThis.document.body.innerHTML = - ''; - - expect(isRequestCached('/api/todos/1', undefined)).toBe(false); - }); -}); From 0d62030ad69c848a526e266e5003357c23209173 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 11 Aug 2023 16:26:23 +0200 Subject: [PATCH 2/4] remove now obsolete tests --- packages/sveltekit/test/client/load.test.ts | 348 ++------------------ 1 file changed, 19 insertions(+), 329 deletions(-) diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 6373ea1ff571..01c2377ddbf2 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,17 +1,10 @@ import { addTracingExtensions, Scope } from '@sentry/svelte'; -import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; import type { Load } from '@sveltejs/kit'; import { redirect } from '@sveltejs/kit'; import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/client/load'; -const SENTRY_TRACE_HEADER = '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; -const BAGGAGE_HEADER = - 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1'; - const mockCaptureException = vi.fn(); let mockScope = new Scope(); @@ -27,44 +20,8 @@ vi.mock('@sentry/svelte', async () => { }; }); -vi.mock('../../src/client/vendor/lookUpCache', () => { - return { - isRequestCached: () => false, - }; -}); - const mockTrace = vi.fn(); -const mockedBrowserTracing = { - options: { - tracePropagationTargets: ['example.com', /^\\/], - traceFetch: true, - shouldCreateSpanForRequest: undefined as undefined | (() => boolean), - }, -}; - -const mockedBreadcrumbs = { - options: { - fetch: true, - }, -}; - -const mockedGetIntegrationById = vi.fn(id => { - if (id === 'BrowserTracing') { - return mockedBrowserTracing; - } else if (id === 'Breadcrumbs') { - return mockedBreadcrumbs; - } - return undefined; -}); - -const mockedGetClient = vi.fn(() => { - return { - getIntegrationById: mockedGetIntegrationById, - getOptions: () => ({}), - }; -}); - vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { @@ -73,38 +30,10 @@ vi.mock('@sentry/core', async () => { mockTrace(...args); return original.trace(...args); }, - getCurrentHub: () => { - return { - getClient: mockedGetClient, - getScope: () => { - return { - getPropagationContext: () => ({ - traceId: '1234567890abcdef1234567890abcdef', - spanId: '1234567890abcdef', - sampled: false, - }), - getSpan: () => { - return { - transaction: { - getDynamicSamplingContext: () => { - return baggageHeaderToDynamicSamplingContext(BAGGAGE_HEADER); - }, - }, - toTraceparent: () => { - return SENTRY_TRACE_HEADER; - }, - }; - }, - }; - }, - addBreadcrumb: mockedAddBreadcrumb, - }; - }, }; }); const mockAddExceptionMechanism = vi.fn(); -const mockedAddBreadcrumb = vi.fn(); vi.mock('@sentry/utils', async () => { const original = (await vi.importActual('@sentry/utils')) as any; @@ -118,30 +47,12 @@ function getById(_id?: string) { throw new Error('error'); } -const mockedSveltekitFetch = vi.fn().mockReturnValue(Promise.resolve({ status: 200 })); - const MOCK_LOAD_ARGS: any = { params: { id: '123' }, route: { id: '/users/[id]', }, url: new URL('http://localhost:3000/users/123'), - request: { - headers: { - get: (key: string) => { - if (key === 'sentry-trace') { - return SENTRY_TRACE_HEADER; - } - - if (key === 'baggage') { - return BAGGAGE_HEADER; - } - - return null; - }, - }, - }, - fetch: mockedSveltekitFetch, }; beforeAll(() => { @@ -153,9 +64,6 @@ describe('wrapLoadWithSentry', () => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); mockTrace.mockClear(); - mockedGetIntegrationById.mockClear(); - mockedSveltekitFetch.mockClear(); - mockedAddBreadcrumb.mockClear(); mockScope = new Scope(); }); @@ -211,253 +119,35 @@ describe('wrapLoadWithSentry', () => { ); }); - describe.each([ - [ - 'fetch call with fragment and params', - ['example.com/api/users/?id=123#testfragment'], - { - op: 'http.client', - name: 'GET example.com/api/users/', - data: { - 'http.method': 'GET', - url: 'example.com/api/users/', - 'http.hash': 'testfragment', - 'http.query': 'id=123', - }, - }, - ], - [ - 'fetch call with options object', - ['example.com/api/users/?id=123#testfragment', { method: 'POST' }], - { - op: 'http.client', - name: 'POST example.com/api/users/', - data: { - 'http.method': 'POST', - url: 'example.com/api/users/', - 'http.hash': 'testfragment', - 'http.query': 'id=123', - }, - }, - ], - [ - 'fetch call with custom headers in options ', - ['example.com/api/users/?id=123#testfragment', { method: 'POST', headers: { 'x-my-header': 'some value' } }], - { - op: 'http.client', - name: 'POST example.com/api/users/', - data: { - 'http.method': 'POST', - url: 'example.com/api/users/', - 'http.hash': 'testfragment', - 'http.query': 'id=123', - }, - }, - ], - [ - 'fetch call with a Request object ', - [{ url: '/api/users?id=123', headers: { 'x-my-header': 'value' } } as unknown as Request], - { - op: 'http.client', - name: 'GET /api/users', - data: { - 'http.method': 'GET', - url: '/api/users', - 'http.query': 'id=123', - }, - }, - ], - ])('instruments fetch (%s)', (_, originalFetchArgs, spanCtx) => { - beforeEach(() => { - mockedBrowserTracing.options = { - tracePropagationTargets: ['example.com', /^\//], - traceFetch: true, - shouldCreateSpanForRequest: undefined, - }; - }); - - const load = async ({ params, fetch }) => { - await fetch(...originalFetchArgs); + it("falls back to the raw URL if `even.route.id` isn't available", async () => { + async function load({ params }: Parameters[0]): Promise> { return { post: params.id, }; - }; - - it('creates a fetch span and attaches tracing headers by default when event.fetch was called', async () => { - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(2); - expect(mockTrace).toHaveBeenNthCalledWith( - 1, - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', - }, - }, - expect.any(Function), - expect.any(Function), - ); - expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); - - const hasSecondArg = originalFetchArgs.length > 1; - const expectedFetchArgs = [ - originalFetchArgs[0], - { - ...(hasSecondArg && (originalFetchArgs[1] as RequestInit)), - headers: { - // @ts-ignore that's fine - ...(hasSecondArg && (originalFetchArgs[1].headers as RequestInit['headers'])), - baggage: expect.any(String), - 'sentry-trace': expect.any(String), - }, - }, - ]; - - expect(mockedSveltekitFetch).toHaveBeenCalledWith(...expectedFetchArgs); - }); - - it("only creates a span but doesn't propagate headers if traceProgagationTargets don't match", async () => { - const previousPropagationTargets = mockedBrowserTracing.options.tracePropagationTargets; - mockedBrowserTracing.options.tracePropagationTargets = []; - - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(2); - expect(mockTrace).toHaveBeenNthCalledWith( - 1, - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', - }, - }, - expect.any(Function), - expect.any(Function), - ); - expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); - - expect(mockedSveltekitFetch).toHaveBeenCalledWith( - ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], - ); - - mockedBrowserTracing.options.tracePropagationTargets = previousPropagationTargets; - }); - - it("doesn't create a span nor propagate headers, if `Browsertracing.options.traceFetch` is false", async () => { - mockedBrowserTracing.options.traceFetch = false; - - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', - }, - }, - expect.any(Function), - expect.any(Function), - ); - - expect(mockedSveltekitFetch).toHaveBeenCalledWith( - ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], - ); - - mockedBrowserTracing.options.traceFetch = true; - }); - - it("doesn't create a span if `shouldCreateSpanForRequest` returns false", async () => { - mockedBrowserTracing.options.shouldCreateSpanForRequest = () => false; - - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', - }, - }, - expect.any(Function), - expect.any(Function), - ); + } + const wrappedLoad = wrapLoadWithSentry(load); - mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true; - }); + const event = { ...MOCK_LOAD_ARGS }; + delete event.route.id; - it('adds a breadcrumb for the fetch call', async () => { - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); + await wrappedLoad(MOCK_LOAD_ARGS); - expect(mockedAddBreadcrumb).toHaveBeenCalledWith( - { - category: 'fetch', - data: { - ...spanCtx.data, - status_code: 200, - }, - type: 'http', - }, - { - endTimestamp: expect.any(Number), - input: [...originalFetchArgs], - response: { - status: 200, - }, - startTimestamp: expect.any(Number), + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/123', + status: 'ok', + metadata: { + source: 'url', }, - ); - }); - - it("doesn't add a breadcrumb if fetch breadcrumbs are deactivated in the integration", async () => { - mockedBreadcrumbs.options.fetch = false; - - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockedAddBreadcrumb).not.toHaveBeenCalled(); - - mockedBreadcrumbs.options.fetch = true; - }); + }, + expect.any(Function), + expect.any(Function), + ); }); }); - it.each([ - ['is undefined', undefined], - ["doesn't have a `getClientById` method", {}], - ])("doesn't instrument fetch if the client %s", async (_, client) => { - mockedGetClient.mockImplementationOnce(() => client); - - async function load(_event: Parameters[0]): Promise> { - 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' }); From 8d92180c900c2ac98fd127d53703a415c1f191dd Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 11 Aug 2023 17:41:56 +0200 Subject: [PATCH 3/4] cleanup --- packages/sveltekit/src/client/sdk.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 84990f52b530..7775282720f4 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -27,7 +27,8 @@ export function init(options: BrowserOptions): void { const globalWithSentryFetchProxy: WindowWithSentryFetchProxy = WINDOW as WindowWithSentryFetchProxy; // eslint-disable-next-line @typescript-eslint/unbound-method - const currentFetch = WINDOW.fetch; + const currentFetch = globalWithSentryFetchProxy.fetch; + // @ts-expect-error TS thinks window.fetch is always defined but let's just make sure it really is if (globalWithSentryFetchProxy._sentryFetchProxy && currentFetch) { globalWithSentryFetchProxy.fetch = globalWithSentryFetchProxy._sentryFetchProxy; From 9ea49665efe05017dc2477c0fd8b62edf0fec9ea Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 16 Aug 2023 18:39:06 +0200 Subject: [PATCH 4/4] add tests && cleanup --- packages/sveltekit/src/client/sdk.ts | 60 +++++++++++++++----- packages/sveltekit/src/server/handle.ts | 1 + packages/sveltekit/test/client/fetch.test.ts | 55 ++++++++++++++++++ packages/sveltekit/test/vitest.setup.ts | 4 +- 4 files changed, 106 insertions(+), 14 deletions(-) create mode 100644 packages/sveltekit/test/client/fetch.test.ts diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 7775282720f4..c399a4a2ad02 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -1,7 +1,6 @@ import { hasTracingEnabled } from '@sentry/core'; import type { BrowserOptions } from '@sentry/svelte'; import { BrowserTracing, configureScope, init as initSvelteSdk, WINDOW } from '@sentry/svelte'; -import type { InternalGlobal } from '@sentry/utils'; import { addOrUpdateIntegration } from '@sentry/utils'; import { applySdkMetadata } from '../common/metadata'; @@ -24,21 +23,16 @@ export function init(options: BrowserOptions): void { addClientIntegrations(options); - const globalWithSentryFetchProxy: WindowWithSentryFetchProxy = WINDOW as WindowWithSentryFetchProxy; - - // eslint-disable-next-line @typescript-eslint/unbound-method - const currentFetch = globalWithSentryFetchProxy.fetch; - - // @ts-expect-error TS thinks window.fetch is always defined but let's just make sure it really is - if (globalWithSentryFetchProxy._sentryFetchProxy && currentFetch) { - globalWithSentryFetchProxy.fetch = globalWithSentryFetchProxy._sentryFetchProxy; - } + // 1. Switch window.fetch to our fetch proxy we injected earlier + const actualFetch = switchToFetchProxy(); + // 2. Initialize the SDK which will instrument our proxy initSvelteSdk(options); - // eslint-disable-next-line @typescript-eslint/unbound-method - globalWithSentryFetchProxy._sentryFetchProxy = globalWithSentryFetchProxy.fetch; - globalWithSentryFetchProxy.fetch = currentFetch; + // 3. Restore the original fetch now that our proxy is instrumented + if (actualFetch) { + restoreFetch(actualFetch); + } configureScope(scope => { scope.setTag('runtime', 'browser'); @@ -64,3 +58,43 @@ function addClientIntegrations(options: BrowserOptions): void { options.integrations = integrations; } + +/** + * During server-side page load, we injected a