ref(sveltekit): Remove custom client fetch instrumentation and use default instrumentation (#8802)

Remove our custom SvelteKit client fetch instrumentation which
we created when we initially worked on the SDK. Back then I didn't think
that it's possible to use our default fetch instrumentation from
`BrowserTracing`, due to timing issues where Kit would store away
`window.fetch` (and use the stored version in `load` functions) before
our SDK was initialized.

After receiving some
[hints](sveltejs/kit#9530 (comment))
how it might be possible, we now have a way to instrument `fetch`
everywhere on the client (including the one in `load`) functions.

This works in two parts:

1. During the initial page load request, our server-side `handle`
wrapper injects a script into the returned HTML that wraps
`window.fetch` and adds a proxy handler (`window._sentryFetchProxy`)
that at this time just forwards the fetch call to the original fetch.

After this script is evaluated by the browser, at some point, SvelteKit
loads its initial client-side bundle that stores away `window.fetch`.
Kit also patches `window.fetch` itself at this time.

Sometime later, the code from the `hooks.client.js` file is evaluated in
the browser, including our `Sentry.init` call:

2. During `Sentry.init` we now swap `window.fetch` with
`window._sentryFetchProxy` which will make our `BrowserTracing`
integration patch our proxy with our default fetch instrumentation.
After the init, we swap the two fetches back and we're done.

Author: Lms24

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",

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<SentryWrappedFlag>;
 
@@ -80,7 +67,6 @@ export function wrapLoadWithSentry<T extends (...args: any) => any>(origLoad: T)
 
       const patchedEvent: PatchedLoadEvent = {
         ...event,
-        fetch: instrumentSvelteKitFetch(event.fetch),
       };
 
       addNonEnumerableProperty(patchedEvent as unknown as Record<string, unknown>, '__sentry_wrapped__', true);
@@ -101,182 +87,3 @@ export function wrapLoadWithSentry<T extends (...args: any) => 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<LoadEvent['fetch']>) => {
-      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<Response>;
-
-      function callFetchTarget(span?: Span): Promise<Response> {
-        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<Response>,
-  requestData: SanitizedRequestData,
-  args: Parameters<SvelteKitFetch>,
-): 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<ClientOptions>['getIntegrationById'] })
-  | undefined;
-
-type ClientWithGetIntegrationById = Required<MaybeClientWithGetIntegrationsById> &
-  Exclude<MaybeClientWithGetIntegrationsById, undefined>;
-
-function isValidClient(client: MaybeClientWithGetIntegrationsById): client is ClientWithGetIntegrationById {
-  return !!client && typeof client.getIntegrationById === 'function';
-}

packages/sveltekit/src/client/sdk.ts

@@ -1,11 +1,15 @@
 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 { 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 +23,17 @@ export function init(options: BrowserOptions): void {
 
   addClientIntegrations(options);
 
+  // 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);
 
+  // 3. Restore the original fetch now that our proxy is instrumented
+  if (actualFetch) {
+    restoreFetch(actualFetch);
+  }
+
   configureScope(scope => {
     scope.setTag('runtime', 'browser');
   });
@@ -45,3 +58,43 @@ function addClientIntegrations(options: BrowserOptions): void {
 
   options.integrations = integrations;
 }
+
+/**
+ * During server-side page load, we injected a <script> that wraps `window.fetch` so that
+ * before a `fetch` call is forwarded to the original `window.fetch`, a function we control
+ * is also invoked. This function is put onto the global object (`window._sentryFetchProxy`)
+ * so that we can access it here.
+ *
+ * In this function we briefly set our fetch proxy as `window.fetch` so that the SDK can
+ * instrument it.
+ *
+ * After initializing the SDK, `restoreFetch` must be called to put back whatever was on `window.fetch` before.
+ *
+ * @see ../server/handle.ts (https://github.com/getsentry/sentry-javascript/blob/8d92180c900c2ac98fd127d53703a415c1f191dd/packages/sveltekit/src/server/handle.ts#L60-L81)
+ *
+ * @returns the function that was previously on `window.fetch`.
+ */
+function switchToFetchProxy(): typeof fetch | undefined {
+  const globalWithSentryFetchProxy: WindowWithSentryFetchProxy = WINDOW as WindowWithSentryFetchProxy;
+
+  // eslint-disable-next-line @typescript-eslint/unbound-method
+  const actualFetch = globalWithSentryFetchProxy.fetch;
+
+  if (globalWithSentryFetchProxy._sentryFetchProxy && actualFetch) {
+    globalWithSentryFetchProxy.fetch = globalWithSentryFetchProxy._sentryFetchProxy;
+    return actualFetch;
+  }
+  return undefined;
+}
+
+/**
+ * Restores the function @param actualFetch to `window.fetch`
+ * and puts our fetch proxy back onto `window._sentryFetchProxy`.
+ */
+function restoreFetch(actualFetch: typeof fetch): void {
+  const globalWithSentryFetchProxy: WindowWithSentryFetchProxy = WINDOW as WindowWithSentryFetchProxy;
+
+  // eslint-disable-next-line @typescript-eslint/unbound-method
+  globalWithSentryFetchProxy._sentryFetchProxy = globalWithSentryFetchProxy.fetch;
+  globalWithSentryFetchProxy.fetch = actualFetch;
+}