feat(utils): Refactor addInstrumentationHandler to dedicated methods

This is mostly an internal utility, but changes a bit. This 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.

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,
@@ -21,8 +32,6 @@ import {
 
 import { WINDOW } from '../helpers';
 
-type HandlerData = Record<string, unknown>;
-
 /** JSDoc */
 interface BreadcrumbsOptions {
   console: boolean;
@@ -88,19 +97,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 = getCurrentHub().getClient();
@@ -130,8 +139,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;
 
@@ -182,7 +191,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: {
@@ -212,7 +221,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];
@@ -250,7 +259,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
@@ -282,13 +291,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,
     };
@@ -306,15 +316,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,9 +1,18 @@
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 import { getCurrentHub } from '@sentry/core';
-import type { Event, EventHint, Hub, Integration, Primitive, StackParser } from '@sentry/types';
+import type {
+  Event,
+  EventHint,
+  HandlerDataUnhandledRejection,
+  Hub,
+  Integration,
+  Primitive,
+  StackParser,
+} from '@sentry/types';
 import {
   addExceptionMechanism,
-  addInstrumentationHandler,
+  addGlobalErrorInstrumentationHandler,
+  addGlobalUnhandledRejectionInstrumentationHandler,
   getLocationHref,
   isErrorEvent,
   isPrimitive,
@@ -78,81 +87,73 @@ 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,
-            );
+    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';
+    event.level = 'error';
 
-      addMechanismAndCapture(hub, error, event, 'onerror');
-    },
-  );
+    addMechanismAndCapture(hub, error, event, '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;
+  addGlobalUnhandledRejectionInstrumentationHandler(e => {
+    const [hub, stackParser, attachStacktrace] = getHubAndOptions();
+    if (!hub.getIntegration(GlobalHandlers)) {
+      return;
+    }
+    let error: Error | HandlerDataUnhandledRejection = 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 && e.reason) {
+        error = e.reason;
       }
-      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
+      // 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 && e.detail && 'reason' in e.detail && e.detail.reason) {
+        error = e.detail.reason;
       }
+    } catch (_oO) {
+      // no-empty
+    }
 
-      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 event = isPrimitive(error)
+      ? _eventFromRejectionWithPrimitive(error)
+      : eventFromUnknownInput(stackParser, error, undefined, attachStacktrace, true);
 
-      event.level = 'error';
+    event.level = 'error';
 
-      addMechanismAndCapture(hub, error, event, 'onunhandledrejection');
-      return;
-    },
-  );
+    addMechanismAndCapture(hub, error, event, 'onunhandledrejection');
+    return;
+  });
 }
 
 /**

packages/browser/src/sdk.ts

@@ -7,7 +7,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';
@@ -240,9 +245,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/core/src/tracing/errors.ts

@@ -1,4 +1,8 @@
-import { addInstrumentationHandler, logger } from '@sentry/utils';
+import {
+  addGlobalErrorInstrumentationHandler,
+  addGlobalUnhandledRejectionInstrumentationHandler,
+  logger,
+} from '@sentry/utils';
 
 import type { SpanStatusType } from './span';
 import { getActiveTransaction } from './utils';
@@ -14,8 +18,8 @@ export function registerErrorInstrumentation(): void {
   }
 
   errorsInstrumented = true;
-  addInstrumentationHandler('error', errorCallback);
-  addInstrumentationHandler('unhandledrejection', errorCallback);
+  addGlobalErrorInstrumentationHandler(errorCallback);
+  addGlobalUnhandledRejectionInstrumentationHandler(errorCallback);
 }
 
 /**