feat(integration): Ensure LinkedErrors integration runs before all event processors (#8956)

While looking through our existing integrations, I noticed that the
`LinkedErrors` integration in node had some weird/custom code to
manually run the context lines integration after it processed, as we
cannot guarantee any order etc.

I figured it would be much cleaner to solve this with a proper hook (I
went with `preprocessEvent`), as it actually makes sense for this to
generally run before all other event processors run, IMHO, and we can
decouple these integrations from each other.

Author: mydea

packages/browser/src/integrations/linkederrors.ts

@@ -1,4 +1,4 @@
-import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types';
+import type { Client, Event, EventHint, Integration } from '@sentry/types';
 import { applyAggregateErrorsToEvent } from '@sentry/utils';
 
 import { exceptionFromError } from '../eventbuilder';
@@ -42,31 +42,25 @@ export class LinkedErrors implements Integration {
     this._limit = options.limit || DEFAULT_LIMIT;
   }
 
+  /** @inheritdoc */
+  public setupOnce(): void {
+    // noop
+  }
+
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
-    addGlobalEventProcessor((event: Event, hint?: EventHint) => {
-      const hub = getCurrentHub();
-      const client = hub.getClient();
-      const self = hub.getIntegration(LinkedErrors);
-
-      if (!client || !self) {
-        return event;
-      }
-
-      const options = client.getOptions();
-      applyAggregateErrorsToEvent(
-        exceptionFromError,
-        options.stackParser,
-        options.maxValueLength,
-        self._key,
-        self._limit,
-        event,
-        hint,
-      );
+  public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void {
+    const options = client.getOptions();
 
-      return event;
-    });
+    applyAggregateErrorsToEvent(
+      exceptionFromError,
+      options.stackParser,
+      options.maxValueLength,
+      this._key,
+      this._limit,
+      event,
+      hint,
+    );
   }
 }

packages/core/src/baseclient.ts

@@ -285,7 +285,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    */
   public setupIntegrations(): void {
     if (this._isEnabled() && !this._integrationsInitialized) {
-      this._integrations = setupIntegrations(this._options.integrations);
+      this._integrations = setupIntegrations(this, this._options.integrations);
       this._integrationsInitialized = true;
     }
   }
@@ -315,7 +315,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    * @inheritDoc
    */
   public addIntegration(integration: Integration): void {
-    setupIntegration(integration, this._integrations);
+    setupIntegration(this, integration, this._integrations);
   }
 
   /**
@@ -376,16 +376,23 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   }
 
   // Keep on() & emit() signatures in sync with types' client.ts interface
+  /* eslint-disable @typescript-eslint/unified-signatures */
 
   /** @inheritdoc */
-  public on(hook: 'startTransaction' | 'finishTransaction', callback: (transaction: Transaction) => void): void;
+  public on(hook: 'startTransaction', callback: (transaction: Transaction) => void): void;
+
+  /** @inheritdoc */
+  public on(hook: 'finishTransaction', callback: (transaction: Transaction) => void): void;
 
   /** @inheritdoc */
   public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;
 
   /** @inheritdoc */
   public on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint) => void): void;
 
+  /** @inheritdoc */
+  public on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint) => void): void;
+
   /** @inheritdoc */
   public on(
     hook: 'afterSendEvent',
@@ -412,14 +419,20 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   }
 
   /** @inheritdoc */
-  public emit(hook: 'startTransaction' | 'finishTransaction', transaction: Transaction): void;
+  public emit(hook: 'startTransaction', transaction: Transaction): void;
+
+  /** @inheritdoc */
+  public emit(hook: 'finishTransaction', transaction: Transaction): void;
 
   /** @inheritdoc */
   public emit(hook: 'beforeEnvelope', envelope: Envelope): void;
 
   /** @inheritdoc */
   public emit(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;
 
+  /** @inheritdoc */
+  public emit(hook: 'preprocessEvent', event: Event, hint?: EventHint): void;
+
   /** @inheritdoc */
   public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;
 
@@ -439,6 +452,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
   }
 
+  /* eslint-enable @typescript-eslint/unified-signatures */
+
   /** Updates existing session based on the provided event */
   protected _updateSessionFromEvent(session: Session, event: Event): void {
     let crashed = false;
@@ -527,6 +542,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     if (!hint.integrations && integrations.length > 0) {
       hint.integrations = integrations;
     }
+
+    this.emit('preprocessEvent', event, hint);
+
     return prepareEvent(options, event, hint, scope).then(evt => {
       if (evt === null) {
         return evt;

packages/core/src/integration.ts

@@ -1,4 +1,4 @@
-import type { Integration, Options } from '@sentry/types';
+import type { Client, Integration, Options } from '@sentry/types';
 import { arrayify, logger } from '@sentry/utils';
 
 import { getCurrentHub } from './hub';
@@ -84,28 +84,34 @@ export function getIntegrationsToSetup(options: Options): Integration[] {
  * @param integrations array of integration instances
  * @param withDefault should enable default integrations
  */
-export function setupIntegrations(integrations: Integration[]): IntegrationIndex {
+export function setupIntegrations(client: Client, integrations: Integration[]): IntegrationIndex {
   const integrationIndex: IntegrationIndex = {};
 
   integrations.forEach(integration => {
     // guard against empty provided integrations
     if (integration) {
-      setupIntegration(integration, integrationIndex);
+      setupIntegration(client, integration, integrationIndex);
     }
   });
 
   return integrationIndex;
 }
 
 /** Setup a single integration.  */
-export function setupIntegration(integration: Integration, integrationIndex: IntegrationIndex): void {
+export function setupIntegration(client: Client, integration: Integration, integrationIndex: IntegrationIndex): void {
   integrationIndex[integration.name] = integration;
 
   if (installedIntegrations.indexOf(integration.name) === -1) {
     integration.setupOnce(addGlobalEventProcessor, getCurrentHub);
     installedIntegrations.push(integration.name);
-    __DEBUG_BUILD__ && logger.log(`Integration installed: ${integration.name}`);
   }
+
+  if (client.on && typeof integration.preprocessEvent === 'function') {
+    const callback = integration.preprocessEvent.bind(integration);
+    client.on('preprocessEvent', (event, hint) => callback(event, hint, client));
+  }
+
+  __DEBUG_BUILD__ && logger.log(`Integration installed: ${integration.name}`);
 }
 
 // Polyfill for Array.findIndex(), which is not supported in ES5

packages/node/src/integrations/linkederrors.ts

@@ -1,8 +1,7 @@
-import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types';
+import type { Client, Event, EventHint, Integration } from '@sentry/types';
 import { applyAggregateErrorsToEvent } from '@sentry/utils';
 
 import { exceptionFromError } from '../eventbuilder';
-import { ContextLines } from './contextlines';
 
 const DEFAULT_KEY = 'cause';
 const DEFAULT_LIMIT = 5;
@@ -37,39 +36,25 @@ export class LinkedErrors implements Integration {
     this._limit = options.limit || DEFAULT_LIMIT;
   }
 
+  /** @inheritdoc */
+  public setupOnce(): void {
+    // noop
+  }
+
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
-    addGlobalEventProcessor(async (event: Event, hint?: EventHint) => {
-      const hub = getCurrentHub();
-      const client = hub.getClient();
-      const self = hub.getIntegration(LinkedErrors);
-
-      if (!client || !self) {
-        return event;
-      }
-
-      const options = client.getOptions();
-
-      applyAggregateErrorsToEvent(
-        exceptionFromError,
-        options.stackParser,
-        options.maxValueLength,
-        self._key,
-        self._limit,
-        event,
-        hint,
-      );
-
-      // If the ContextLines integration is enabled, we add source code context to linked errors
-      // because we can't guarantee the order that integrations are run.
-      const contextLines = getCurrentHub().getIntegration(ContextLines);
-      if (contextLines) {
-        await contextLines.addSourceContext(event);
-      }
-
-      return event;
-    });
+  public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void {
+    const options = client.getOptions();
+
+    applyAggregateErrorsToEvent(
+      exceptionFromError,
+      options.stackParser,
+      options.maxValueLength,
+      this._key,
+      this._limit,
+      event,
+      hint,
+    );
   }
 }

packages/types/src/client.ts

@@ -165,21 +165,38 @@ export interface Client<O extends ClientOptions = ClientOptions> {
 
   // HOOKS
   // TODO(v8): Make the hooks non-optional.
+  /* eslint-disable @typescript-eslint/unified-signatures */
 
   /**
-   * Register a callback for transaction start and finish.
+   * Register a callback for transaction start.
+   * Receives the transaction as argument.
    */
-  on?(hook: 'startTransaction' | 'finishTransaction', callback: (transaction: Transaction) => void): void;
+  on?(hook: 'startTransaction', callback: (transaction: Transaction) => void): void;
+
+  /**
+   * Register a callback for transaction finish.
+   * Receives the transaction as argument.
+   */
+  on?(hook: 'finishTransaction', callback: (transaction: Transaction) => void): void;
 
   /**
    * Register a callback for transaction start and finish.
    */
   on?(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;
 
   /**
-   * Register a callback for before an event is sent.
+   * Register a callback for before sending an event.
+   * This is called right before an event is sent and should not be used to mutate the event.
+   * Receives an Event & EventHint as arguments.
    */
-  on?(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | void) => void): void;
+  on?(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;
+
+  /**
+   * Register a callback for preprocessing an event,
+   * before it is passed to (global) event processors.
+   * Receives an Event & EventHint as arguments.
+   */
+  on?(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;
 
   /**
    * Register a callback for when an event has been sent.
@@ -206,23 +223,36 @@ export interface Client<O extends ClientOptions = ClientOptions> {
   on?(hook: 'otelSpanEnd', callback: (otelSpan: unknown, mutableOptions: { drop: boolean }) => void): void;
 
   /**
-   * Fire a hook event for transaction start and finish. Expects to be given a transaction as the
-   * second argument.
+   * Fire a hook event for transaction start.
+   * Expects to be given a transaction as the second argument.
    */
-  emit?(hook: 'startTransaction' | 'finishTransaction', transaction: Transaction): void;
+  emit?(hook: 'startTransaction', transaction: Transaction): void;
+
+  /**
+   * Fire a hook event for transaction finish.
+   * Expects to be given a transaction as the second argument.
+   */
+  emit?(hook: 'finishTransaction', transaction: Transaction): void;
 
   /*
    * Fire a hook event for envelope creation and sending. Expects to be given an envelope as the
    * second argument.
    */
   emit?(hook: 'beforeEnvelope', envelope: Envelope): void;
 
-  /*
-   * Fire a hook event before sending an event. Expects to be given an Event & EventHint as the
-   * second/third argument.
+  /**
+   * Fire a hook event before sending an event.
+   * This is called right before an event is sent and should not be used to mutate the event.
+   * Expects to be given an Event & EventHint as the second/third argument.
    */
   emit?(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;
 
+  /**
+   * Fire a hook event to process events before they are passed to (global) event processors.
+   * Expects to be given an Event & EventHint as the second/third argument.
+   */
+  emit?(hook: 'preprocessEvent', event: Event, hint?: EventHint): void;
+
   /*
    * Fire a hook event after sending an event. Expects to be given an Event as the
    * second argument.
@@ -245,4 +275,6 @@ export interface Client<O extends ClientOptions = ClientOptions> {
    * The option argument may be mutated to drop the span.
    */
   emit?(hook: 'otelSpanEnd', otelSpan: unknown, mutableOptions: { drop: boolean }): void;
+
+  /* eslint-enable @typescript-eslint/unified-signatures */
 }

packages/types/src/integration.ts

@@ -1,3 +1,5 @@
+import type { Client } from './client';
+import type { Event, EventHint } from './event';
 import type { EventProcessor } from './eventprocessor';
 import type { Hub } from './hub';
 
@@ -23,4 +25,9 @@ export interface Integration {
    * This takes no options on purpose, options should be passed in the constructor
    */
   setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void;
+
+  /**
+   * An optional hook that allows to preprocess an event _before_ it is passed to all other event processors.
+   */
+  preprocessEvent?(event: Event, hint: EventHint | undefined, client: Client): void;
 }