feat(utils): Refactor addInstrumentationHandler to dedicated methods (#9542)

This is mostly an internal utility, but changes a bit. The previous
implementation for adding instrumentation had a few issues:

* It was impossible to tree shake. So even if some instrumentation is
never used, the whole code for it is always included. Splitting this up
opens up future improvements there as well.
* It was not type safe, and actually there _were_ some subtle issues in
the code because of this - things could fail in unexpected ways, as we
have been doing a lot of type casting from/to any etc.


This PR splits up `addInstrumentationHandler` into e.g.
`addFetchInstrumentationHandler` etc. methods, which improves the actual
type safety a lot, because currently we've been duplicating the types
everywhere, sometimes in slightly differing ways, leading to potential
issues. **We had a bunch of issues with xhr & fetch data in Replay
recently, so I figured it would finally be a good time to clean this up
so we can ensure that the data we get is consistent & reliable.**
Hopefully this change will prevent issues like we have/had in replay
from popping up in the future.

Some additional notes:
* For the dom instrumentation, I picked the name
`addClickKeypressInstrumentationHandler()`. I'm open for other naming
ideas, but I found `addDomInstrumentation` a bit too broad, as this
could mean a lot of things - and we are strictly only instrumenting
clicks and keypresses here. Another name (if we want to be a bit
broader) could be e.g. `addDomEventInstrumentation` or something like
this?
* I updated the type for the XHR data handler to make the method & url
required. This was previously optional (but not in some of the code,
where we expected this to always be), plus we also did not handle the
case that `url` can be a `URL` instead of a string, which we now
convert.
* On the XHR data, we have one field `args` that 1. is not really used
and 2. was actually "buggy", as we always returned the args, but for
`open` and `send` the args are very different things (but we pretended
it's always the "open" args). Now, this is actually always the method &
url, and I also deprecated this for v8.
* On the XHR data, we also had an unused/unset `data` field, which I
removed (this was not set anywhere)
* All old code should still work normally, but I deprecated the generic
`addInstrumentationHandler()` method, for removal in v8.
* I also took the opportunity and split the instrumentations into
dedicated files, previously this was one mega-file which made it harder
to reason about things.

Author: mydea

packages/browser/src/integrations/breadcrumbs.ts

@@ -1,15 +1,26 @@
-/* eslint-disable @typescript-eslint/no-unsafe-member-access */
 /* eslint-disable max-lines */
 import { getCurrentHub } from '@sentry/core';
-import type { Event as SentryEvent, HandlerDataFetch, HandlerDataXhr, Integration } from '@sentry/types';
+import type {
+  Event as SentryEvent,
+  HandlerDataConsole,
+  HandlerDataDom,
+  HandlerDataFetch,
+  HandlerDataHistory,
+  HandlerDataXhr,
+  Integration,
+} from '@sentry/types';
 import type {
   FetchBreadcrumbData,
   FetchBreadcrumbHint,
   XhrBreadcrumbData,
   XhrBreadcrumbHint,
 } from '@sentry/types/build/types/breadcrumb';
 import {
-  addInstrumentationHandler,
+  addClickKeypressInstrumentationHandler,
+  addConsoleInstrumentationHandler,
+  addFetchInstrumentationHandler,
+  addHistoryInstrumentationHandler,
+  addXhrInstrumentationHandler,
   getEventDescription,
   htmlTreeAsString,
   logger,
@@ -22,8 +33,6 @@ import {
 import { getClient } from '../exports';
 import { WINDOW } from '../helpers';
 
-type HandlerData = Record<string, unknown>;
-
 /** JSDoc */
 interface BreadcrumbsOptions {
   console: boolean;
@@ -89,19 +98,19 @@ export class Breadcrumbs implements Integration {
    */
   public setupOnce(): void {
     if (this.options.console) {
-      addInstrumentationHandler('console', _consoleBreadcrumb);
+      addConsoleInstrumentationHandler(_consoleBreadcrumb);
     }
     if (this.options.dom) {
-      addInstrumentationHandler('dom', _domBreadcrumb(this.options.dom));
+      addClickKeypressInstrumentationHandler(_domBreadcrumb(this.options.dom));
     }
     if (this.options.xhr) {
-      addInstrumentationHandler('xhr', _xhrBreadcrumb);
+      addXhrInstrumentationHandler(_xhrBreadcrumb);
     }
     if (this.options.fetch) {
-      addInstrumentationHandler('fetch', _fetchBreadcrumb);
+      addFetchInstrumentationHandler(_fetchBreadcrumb);
     }
     if (this.options.history) {
-      addInstrumentationHandler('history', _historyBreadcrumb);
+      addHistoryInstrumentationHandler(_historyBreadcrumb);
     }
     if (this.options.sentry) {
       const client = getClient();
@@ -131,8 +140,8 @@ function addSentryBreadcrumb(event: SentryEvent): void {
  * A HOC that creaes a function that creates breadcrumbs from DOM API calls.
  * This is a HOC so that we get access to dom options in the closure.
  */
-function _domBreadcrumb(dom: BreadcrumbsOptions['dom']): (handlerData: HandlerData) => void {
-  function _innerDomBreadcrumb(handlerData: HandlerData): void {
+function _domBreadcrumb(dom: BreadcrumbsOptions['dom']): (handlerData: HandlerDataDom) => void {
+  function _innerDomBreadcrumb(handlerData: HandlerDataDom): void {
     let target;
     let keyAttrs = typeof dom === 'object' ? dom.serializeAttribute : undefined;
 
@@ -183,7 +192,7 @@ function _domBreadcrumb(dom: BreadcrumbsOptions['dom']): (handlerData: HandlerDa
 /**
  * Creates breadcrumbs from console API calls
  */
-function _consoleBreadcrumb(handlerData: HandlerData & { args: unknown[]; level: string }): void {
+function _consoleBreadcrumb(handlerData: HandlerDataConsole): void {
   const breadcrumb = {
     category: 'console',
     data: {
@@ -213,7 +222,7 @@ function _consoleBreadcrumb(handlerData: HandlerData & { args: unknown[]; level:
 /**
  * Creates breadcrumbs from XHR API calls
  */
-function _xhrBreadcrumb(handlerData: HandlerData & HandlerDataXhr): void {
+function _xhrBreadcrumb(handlerData: HandlerDataXhr): void {
   const { startTimestamp, endTimestamp } = handlerData;
 
   const sentryXhrData = handlerData.xhr[SENTRY_XHR_DATA_KEY];
@@ -251,7 +260,7 @@ function _xhrBreadcrumb(handlerData: HandlerData & HandlerDataXhr): void {
 /**
  * Creates breadcrumbs from fetch API calls
  */
-function _fetchBreadcrumb(handlerData: HandlerData & HandlerDataFetch & { response?: Response }): void {
+function _fetchBreadcrumb(handlerData: HandlerDataFetch): void {
   const { startTimestamp, endTimestamp } = handlerData;
 
   // We only capture complete fetch requests
@@ -283,13 +292,14 @@ function _fetchBreadcrumb(handlerData: HandlerData & HandlerDataFetch & { respon
       hint,
     );
   } else {
+    const response = handlerData.response as Response | undefined;
     const data: FetchBreadcrumbData = {
       ...handlerData.fetchData,
-      status_code: handlerData.response && handlerData.response.status,
+      status_code: response && response.status,
     };
     const hint: FetchBreadcrumbHint = {
       input: handlerData.args,
-      response: handlerData.response,
+      response,
       startTimestamp,
       endTimestamp,
     };
@@ -307,15 +317,15 @@ function _fetchBreadcrumb(handlerData: HandlerData & HandlerDataFetch & { respon
 /**
  * Creates breadcrumbs from history API calls
  */
-function _historyBreadcrumb(handlerData: HandlerData & { from: string; to: string }): void {
+function _historyBreadcrumb(handlerData: HandlerDataHistory): void {
   let from: string | undefined = handlerData.from;
   let to: string | undefined = handlerData.to;
   const parsedLoc = parseUrl(WINDOW.location.href);
-  let parsedFrom = parseUrl(from);
+  let parsedFrom = from ? parseUrl(from) : undefined;
   const parsedTo = parseUrl(to);
 
   // Initial pushState doesn't provide `from` information
-  if (!parsedFrom.path) {
+  if (!parsedFrom || !parsedFrom.path) {
     parsedFrom = parsedLoc;
   }
 

packages/browser/src/integrations/globalhandlers.ts

@@ -1,7 +1,15 @@
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 import { getCurrentHub } from '@sentry/core';
 import type { Event, Hub, Integration, Primitive, StackParser } from '@sentry/types';
-import { addInstrumentationHandler, getLocationHref, isErrorEvent, isPrimitive, isString, logger } from '@sentry/utils';
+import {
+  addGlobalErrorInstrumentationHandler,
+  addGlobalUnhandledRejectionInstrumentationHandler,
+  getLocationHref,
+  isErrorEvent,
+  isPrimitive,
+  isString,
+  logger,
+} from '@sentry/utils';
 
 import type { BrowserClient } from '../client';
 import { eventFromUnknownInput } from '../eventbuilder';
@@ -68,96 +76,97 @@ export class GlobalHandlers implements Integration {
   }
 }
 
-/** JSDoc */
 function _installGlobalOnErrorHandler(): void {
-  addInstrumentationHandler(
-    'error',
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (data: { msg: any; url: any; line: any; column: any; error: any }) => {
-      const [hub, stackParser, attachStacktrace] = getHubAndOptions();
-      if (!hub.getIntegration(GlobalHandlers)) {
-        return;
-      }
-      const { msg, url, line, column, error } = data;
-      if (shouldIgnoreOnError() || (error && error.__sentry_own_request__)) {
-        return;
-      }
+  addGlobalErrorInstrumentationHandler(data => {
+    const [hub, stackParser, attachStacktrace] = getHubAndOptions();
+    if (!hub.getIntegration(GlobalHandlers)) {
+      return;
+    }
+    const { msg, url, line, column, error } = data;
+    if (shouldIgnoreOnError()) {
+      return;
+    }
 
-      const event =
-        error === undefined && isString(msg)
-          ? _eventFromIncompleteOnError(msg, url, line, column)
-          : _enhanceEventWithInitialFrame(
-              eventFromUnknownInput(stackParser, error || msg, undefined, attachStacktrace, false),
-              url,
-              line,
-              column,
-            );
-
-      event.level = 'error';
-
-      hub.captureEvent(event, {
-        originalException: error,
-        mechanism: {
-          handled: false,
-          type: 'onerror',
-        },
-      });
-    },
-  );
+    const event =
+      error === undefined && isString(msg)
+        ? _eventFromIncompleteOnError(msg, url, line, column)
+        : _enhanceEventWithInitialFrame(
+            eventFromUnknownInput(stackParser, error || msg, undefined, attachStacktrace, false),
+            url,
+            line,
+            column,
+          );
+
+    event.level = 'error';
+
+    hub.captureEvent(event, {
+      originalException: error,
+      mechanism: {
+        handled: false,
+        type: 'onerror',
+      },
+    });
+  });
 }
 
-/** JSDoc */
 function _installGlobalOnUnhandledRejectionHandler(): void {
-  addInstrumentationHandler(
-    'unhandledrejection',
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    (e: any) => {
-      const [hub, stackParser, attachStacktrace] = getHubAndOptions();
-      if (!hub.getIntegration(GlobalHandlers)) {
-        return;
-      }
-      let error = e;
-
-      // dig the object of the rejection out of known event types
-      try {
-        // PromiseRejectionEvents store the object of the rejection under 'reason'
-        // see https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent
-        if ('reason' in e) {
-          error = e.reason;
-        }
-        // something, somewhere, (likely a browser extension) effectively casts PromiseRejectionEvents
-        // to CustomEvents, moving the `promise` and `reason` attributes of the PRE into
-        // the CustomEvent's `detail` attribute, since they're not part of CustomEvent's spec
-        // see https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent and
-        // https://github.com/getsentry/sentry-javascript/issues/2380
-        else if ('detail' in e && 'reason' in e.detail) {
-          error = e.detail.reason;
-        }
-      } catch (_oO) {
-        // no-empty
-      }
+  addGlobalUnhandledRejectionInstrumentationHandler(e => {
+    const [hub, stackParser, attachStacktrace] = getHubAndOptions();
+    if (!hub.getIntegration(GlobalHandlers)) {
+      return;
+    }
 
-      if (shouldIgnoreOnError() || (error && error.__sentry_own_request__)) {
-        return true;
-      }
+    if (shouldIgnoreOnError()) {
+      return true;
+    }
 
-      const event = isPrimitive(error)
-        ? _eventFromRejectionWithPrimitive(error)
-        : eventFromUnknownInput(stackParser, error, undefined, attachStacktrace, true);
+    const error = _getUnhandledRejectionError(e as unknown);
 
-      event.level = 'error';
+    const event = isPrimitive(error)
+      ? _eventFromRejectionWithPrimitive(error)
+      : eventFromUnknownInput(stackParser, error, undefined, attachStacktrace, true);
 
-      hub.captureEvent(event, {
-        originalException: error,
-        mechanism: {
-          handled: false,
-          type: 'onunhandledrejection',
-        },
-      });
+    event.level = 'error';
 
-      return;
-    },
-  );
+    hub.captureEvent(event, {
+      originalException: error,
+      mechanism: {
+        handled: false,
+        type: 'onunhandledrejection',
+      },
+    });
+
+    return;
+  });
+}
+
+function _getUnhandledRejectionError(error: unknown): unknown {
+  if (isPrimitive(error)) {
+    return error;
+  }
+
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const e = error as any;
+
+  // dig the object of the rejection out of known event types
+  try {
+    // PromiseRejectionEvents store the object of the rejection under 'reason'
+    // see https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent
+    if ('reason' in e) {
+      return e.reason;
+    }
+
+    // something, somewhere, (likely a browser extension) effectively casts PromiseRejectionEvents
+    // to CustomEvents, moving the `promise` and `reason` attributes of the PRE into
+    // the CustomEvent's `detail` attribute, since they're not part of CustomEvent's spec
+    // see https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent and
+    // https://github.com/getsentry/sentry-javascript/issues/2380
+    else if ('detail' in e && 'reason' in e.detail) {
+      return e.detail.reason;
+    }
+  } catch {} // eslint-disable-line no-empty
+
+  return error;
 }
 
 /**

packages/browser/src/sdk.ts

@@ -8,7 +8,12 @@ import {
   Integrations as CoreIntegrations,
 } from '@sentry/core';
 import type { UserFeedback } from '@sentry/types';
-import { addInstrumentationHandler, logger, stackParserFromStackParserOptions, supportsFetch } from '@sentry/utils';
+import {
+  addHistoryInstrumentationHandler,
+  logger,
+  stackParserFromStackParserOptions,
+  supportsFetch,
+} from '@sentry/utils';
 
 import type { BrowserClientOptions, BrowserOptions } from './client';
 import { BrowserClient } from './client';
@@ -255,9 +260,9 @@ function startSessionTracking(): void {
   startSessionOnHub(hub);
 
   // We want to create a session for every navigation as well
-  addInstrumentationHandler('history', ({ from, to }) => {
+  addHistoryInstrumentationHandler(({ from, to }) => {
     // Don't create an additional session for the initial route or if the location did not change
-    if (!(from === undefined || from === to)) {
+    if (from !== undefined && from !== to) {
       startSessionOnHub(getCurrentHub());
     }
   });

packages/browser/test/integration/suites/onunhandledrejection.js

@@ -243,24 +243,4 @@ describe('window.onunhandledrejection', function () {
       }
     });
   });
-
-  it('should skip our own failed requests that somehow bubbled-up to unhandledrejection handler', function () {
-    return runInSandbox(sandbox, function () {
-      if (supportsOnunhandledRejection()) {
-        Promise.reject({
-          __sentry_own_request__: true,
-        });
-        Promise.reject({
-          __sentry_own_request__: false,
-        });
-        Promise.reject({});
-      } else {
-        window.resolveTest({ window: window });
-      }
-    }).then(function (summary) {
-      if (summary.window.supportsOnunhandledRejection()) {
-        assert.equal(summary.events.length, 2);
-      }
-    });
-  });
 });