ref(node): Refactor node integrations to use processEvent

mydea · mydea · commit f91a17ec5c40 · 2023-09-21T09:26:45.000+02:00
diff --git a/packages/node/src/integrations/context.ts b/packages/node/src/integrations/context.ts
@@ -6,7 +6,6 @@ import type {
   CultureContext,
   DeviceContext,
   Event,
-  EventProcessor,
   Integration,
   OsContext,
 } from '@sentry/types';
@@ -63,17 +62,26 @@ export class Context implements Integration {
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
-    addGlobalEventProcessor(event => this.addContext(event));
+  public setupOnce(_addGlobaleventProcessor: unknown, _getCurrentHub: unknown): void {
+    // noop
+  }
+
+  /** @inheritDoc */
+  public processEvent(event: Event): Promise<Event> {
+    return this.addContext(event);
   }
 
-  /** Processes an event and adds context */
+  /**
+   * Processes an event and adds context.
+   *
+   * TODO (v8): Make this private/internal.
+   */
   public async addContext(event: Event): Promise<Event> {
     if (this._cachedContext === undefined) {
       this._cachedContext = this._getContexts();
     }
 
-    const updatedContext = this._updateContext(await this._cachedContext);
+    const updatedContext = _updateContext(await this._cachedContext);
 
     event.contexts = {
       ...event.contexts,
@@ -87,22 +95,6 @@ export class Context implements Integration {
     return event;
   }
 
-  /**
-   * Updates the context with dynamic values that can change
-   */
-  private _updateContext(contexts: Contexts): Contexts {
-    // Only update properties if they exist
-    if (contexts?.app?.app_memory) {
-      contexts.app.app_memory = process.memoryUsage().rss;
-    }
-
-    if (contexts?.device?.free_memory) {
-      contexts.device.free_memory = os.freemem();
-    }
-
-    return contexts;
-  }
-
   /**
    * Gets the contexts for the current environment
    */
@@ -137,6 +129,22 @@ export class Context implements Integration {
   }
 }
 
+/**
+ * Updates the context with dynamic values that can change
+ */
+function _updateContext(contexts: Contexts): Contexts {
+  // Only update properties if they exist
+  if (contexts?.app?.app_memory) {
+    contexts.app.app_memory = process.memoryUsage().rss;
+  }
+
+  if (contexts?.device?.free_memory) {
+    contexts.device.free_memory = os.freemem();
+  }
+
+  return contexts;
+}
+
 /**
  * Returns the operating system context.
  *
diff --git a/packages/node/src/integrations/contextlines.ts b/packages/node/src/integrations/contextlines.ts
@@ -1,4 +1,4 @@
-import type { Event, EventProcessor, Hub, Integration, StackFrame } from '@sentry/types';
+import type { Event, Integration, StackFrame } from '@sentry/types';
 import { addContextToFrame } from '@sentry/utils';
 import { readFile } from 'fs';
 import { LRUMap } from 'lru_map';
@@ -56,17 +56,20 @@ export class ContextLines implements Integration {
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
-    addGlobalEventProcessor(event => {
-      const self = getCurrentHub().getIntegration(ContextLines);
-      if (!self) {
-        return event;
-      }
-      return this.addSourceContext(event);
-    });
+  public setupOnce(_addGlobaleventProcessor: unknown, _getCurrentHub: unknown): void {
+    // noop
   }
 
-  /** Processes an event and adds context lines */
+  /** @inheritDoc */
+  public processEvent(event: Event): Promise<Event> {
+    return this.addSourceContext(event);
+  }
+
+  /**
+   * Processes an event and adds context lines.
+   *
+   * TODO (v8): Make this internal/private
+   *  */
   public async addSourceContext(event: Event): Promise<Event> {
     // keep a lookup map of which files we've already enqueued to read,
     // so we don't enqueue the same file multiple times which would cause multiple i/o reads
@@ -117,7 +120,11 @@ export class ContextLines implements Integration {
     return event;
   }
 
-  /** Adds context lines to frames */
+  /**
+   * Adds context lines to frames.
+   *
+   * TODO (v8): Make this internal/private
+   */
   public addSourceContextToFrames(frames: StackFrame[]): void {
     for (const frame of frames) {
       // Only add context if we have a filename and it hasn't already been added
diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts
@@ -1,4 +1,14 @@
-import type { Event, EventProcessor, Exception, Hub, Integration, StackFrame, StackParser } from '@sentry/types';
+import type {
+  Client,
+  Event,
+  EventHint,
+  EventProcessor,
+  Exception,
+  Hub,
+  Integration,
+  StackFrame,
+  StackParser,
+} from '@sentry/types';
 import { logger } from '@sentry/utils';
 import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector';
 import { LRUMap } from 'lru_map';
@@ -278,29 +288,39 @@ export class LocalVariables implements Integration {
     this._setup(addGlobalEventProcessor, getCurrentHub().getClient()?.getOptions());
   }
 
+  /** @inheritDoc */
+  public processEvent(event: Event, hint: EventHint | undefined, client: Client): Event {
+    const clientOptions = client.getOptions() as NodeClientOptions;
+    if (!this._session || !clientOptions.includeLocalVariables) {
+      return event;
+    }
+
+    return this._addLocalVariables(event);
+  }
+
   /** Setup in a way that's easier to call from tests */
   private _setup(
     addGlobalEventProcessor: (callback: EventProcessor) => void,
     clientOptions: NodeClientOptions | undefined,
   ): void {
-    if (this._session && clientOptions?.includeLocalVariables) {
-      // Only setup this integration if the Node version is >= v18
-      // https://github.com/getsentry/sentry-javascript/issues/7697
-      const unsupportedNodeVersion = (NODE_VERSION.major || 0) < 18;
-
-      if (unsupportedNodeVersion) {
-        logger.log('The `LocalVariables` integration is only supported on Node >= v18.');
-        return;
-      }
+    if (!this._session || !clientOptions?.includeLocalVariables) {
+      return;
+    }
 
-      this._session.configureAndConnect(
-        (ev, complete) =>
-          this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
-        !!this._options.captureAllExceptions,
-      );
+    // Only setup this integration if the Node version is >= v18
+    // https://github.com/getsentry/sentry-javascript/issues/7697
+    const unsupportedNodeVersion = (NODE_VERSION.major || 0) < 18;
 
-      addGlobalEventProcessor(async event => this._addLocalVariables(event));
+    if (unsupportedNodeVersion) {
+      logger.log('The `LocalVariables` integration is only supported on Node >= v18.');
+      return;
     }
+
+    this._session.configureAndConnect(
+      (ev, complete) =>
+        this._handlePaused(clientOptions.stackParser, ev as InspectorNotification<PausedExceptionEvent>, complete),
+      !!this._options.captureAllExceptions,
+    );
   }
 
   /**
diff --git a/packages/node/src/integrations/modules.ts b/packages/node/src/integrations/modules.ts
@@ -1,4 +1,4 @@
-import type { EventProcessor, Hub, Integration } from '@sentry/types';
+import type { Event, Integration } from '@sentry/types';
 import { existsSync, readFileSync } from 'fs';
 import { dirname, join } from 'path';
 
@@ -80,26 +80,26 @@ export class Modules implements Integration {
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
-    addGlobalEventProcessor(event => {
-      if (!getCurrentHub().getIntegration(Modules)) {
-        return event;
-      }
-      return {
-        ...event,
-        modules: {
-          ...event.modules,
-          ...this._getModules(),
-        },
-      };
-    });
+  public setupOnce(_addGlobaleventProcessor: unknown, _getCurrentHub: unknown): void {
+    // noop
   }
 
-  /** Fetches the list of modules and the versions loaded by the entry file for your node.js app. */
-  private _getModules(): { [key: string]: string } {
-    if (!moduleCache) {
-      moduleCache = collectModules();
-    }
-    return moduleCache;
+  /** @inheritDoc */
+  public processEvent(event: Event): Event {
+    return {
+      ...event,
+      modules: {
+        ...event.modules,
+        ..._getModules(),
+      },
+    };
+  }
+}
+
+/** Fetches the list of modules and the versions loaded by the entry file for your node.js app. */
+function _getModules(): { [key: string]: string } {
+  if (!moduleCache) {
+    moduleCache = collectModules();
   }
+  return moduleCache;
 }
diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts
@@ -1,7 +1,7 @@
 // TODO (v8 or v9): Whenever this becomes a default integration for `@sentry/browser`, move this to `@sentry/core`. For
 // now, we leave it in `@sentry/integrations` so that it doesn't contribute bytes to our CDN bundles.
 
-import type { Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types';
+import type { Client, Event, EventHint, Integration, PolymorphicRequest, Transaction } from '@sentry/types';
 import { extractPathForTransaction } from '@sentry/utils';
 
 import type { AddRequestDataToEventOptions, TransactionNamingScheme } from '../requestdata';
@@ -99,65 +99,66 @@ export class RequestData implements Integration {
   /**
    * @inheritDoc
    */
-  public setupOnce(addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, getCurrentHub: () => Hub): void {
+  public setupOnce(_addGlobaleventProcessor: unknown, _getCurrentHub: unknown): void {
+    // noop
+  }
+
+  /** @inheritDoc */
+  public processEvent(event: Event, hint: EventHint | undefined, client: Client): Event {
     // Note: In the long run, most of the logic here should probably move into the request data utility functions. For
     // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed.
     // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once
     // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.)
-    const { transactionNamingScheme } = this._options;
 
-    addGlobalEventProcessor(event => {
-      const hub = getCurrentHub();
-      const self = hub.getIntegration(RequestData);
+    const { transactionNamingScheme } = this._options;
 
-      const { sdkProcessingMetadata = {} } = event;
-      const req = sdkProcessingMetadata.request;
+    const { sdkProcessingMetadata = {} } = event;
+    const req = sdkProcessingMetadata.request;
 
-      // If the globally installed instance of this integration isn't associated with the current hub, `self` will be
-      // undefined
-      if (!self || !req) {
-        return event;
-      }
+    // If the globally installed instance of this integration isn't associated with the current hub, `self` will be
+    // undefined
+    if (!req) {
+      return event;
+    }
 
-      // The Express request handler takes a similar `include` option to that which can be passed to this integration.
-      // If passed there, we store it in `sdkProcessingMetadata`. TODO(v8): Force express and GCP people to use this
-      // integration, so that all of this passing and conversion isn't necessary
-      const addRequestDataOptions =
-        sdkProcessingMetadata.requestDataOptionsFromExpressHandler ||
-        sdkProcessingMetadata.requestDataOptionsFromGCPWrapper ||
-        convertReqDataIntegrationOptsToAddReqDataOpts(this._options);
+    // The Express request handler takes a similar `include` option to that which can be passed to this integration.
+    // If passed there, we store it in `sdkProcessingMetadata`. TODO(v8): Force express and GCP people to use this
+    // integration, so that all of this passing and conversion isn't necessary
+    const addRequestDataOptions =
+      sdkProcessingMetadata.requestDataOptionsFromExpressHandler ||
+      sdkProcessingMetadata.requestDataOptionsFromGCPWrapper ||
+      convertReqDataIntegrationOptsToAddReqDataOpts(this._options);
 
-      const processedEvent = this._addRequestData(event, req, addRequestDataOptions);
+    const processedEvent = this._addRequestData(event, req, addRequestDataOptions);
 
-      // Transaction events already have the right `transaction` value
-      if (event.type === 'transaction' || transactionNamingScheme === 'handler') {
-        return processedEvent;
-      }
+    // Transaction events already have the right `transaction` value
+    if (event.type === 'transaction' || transactionNamingScheme === 'handler') {
+      return processedEvent;
+    }
 
-      // In all other cases, use the request's associated transaction (if any) to overwrite the event's `transaction`
-      // value with a high-quality one
-      const reqWithTransaction = req as { _sentryTransaction?: Transaction };
-      const transaction = reqWithTransaction._sentryTransaction;
-      if (transaction) {
-        // TODO (v8): Remove the nextjs check and just base it on `transactionNamingScheme` for all SDKs. (We have to
-        // keep it the way it is for the moment, because changing the names of transactions in Sentry has the potential
-        // to break things like alert rules.)
-        const shouldIncludeMethodInTransactionName =
-          getSDKName(hub) === 'sentry.javascript.nextjs'
-            ? transaction.name.startsWith('/api')
-            : transactionNamingScheme !== 'path';
-
-        const [transactionValue] = extractPathForTransaction(req, {
-          path: true,
-          method: shouldIncludeMethodInTransactionName,
-          customRoute: transaction.name,
-        });
-
-        processedEvent.transaction = transactionValue;
-      }
+    // In all other cases, use the request's associated transaction (if any) to overwrite the event's `transaction`
+    // value with a high-quality one
+    const reqWithTransaction = req as { _sentryTransaction?: Transaction };
+    const transaction = reqWithTransaction._sentryTransaction;
+    if (transaction) {
+      // TODO (v8): Remove the nextjs check and just base it on `transactionNamingScheme` for all SDKs. (We have to
+      // keep it the way it is for the moment, because changing the names of transactions in Sentry has the potential
+      // to break things like alert rules.)
+      const shouldIncludeMethodInTransactionName =
+        getSDKName(client) === 'sentry.javascript.nextjs'
+          ? transaction.name.startsWith('/api')
+          : transactionNamingScheme !== 'path';
+
+      const [transactionValue] = extractPathForTransaction(req, {
+        path: true,
+        method: shouldIncludeMethodInTransactionName,
+        customRoute: transaction.name,
+      });
+
+      processedEvent.transaction = transactionValue;
+    }
 
-      return processedEvent;
-    });
+    return processedEvent;
   }
 }
 
@@ -203,12 +204,12 @@ function convertReqDataIntegrationOptsToAddReqDataOpts(
   };
 }
 
-function getSDKName(hub: Hub): string | undefined {
+function getSDKName(client: Client): string | undefined {
   try {
     // For a long chain like this, it's fewer bytes to combine a try-catch with assuming everything is there than to
     // write out a long chain of `a && a.b && a.b.c && ...`
     // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-    return hub.getClient()!.getOptions()!._metadata!.sdk!.name;
+    return client.getOptions()!._metadata!.sdk!.name;
   } catch (err) {
     // In theory we should never get here
     return undefined;
diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts
