Set global event processor and pass scope data for transactions in serverless (#2975)

marshall-lee · web-flow · commit 2ed3b464ead5 · 2020-10-20T12:21:49.000+02:00
diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts
@@ -4,14 +4,12 @@ import {
   flush,
   getCurrentHub,
   Scope,
-  SDK_VERSION,
   Severity,
   startTransaction,
   withScope,
 } from '@sentry/node';
 import * as Sentry from '@sentry/node';
 import { Integration } from '@sentry/types';
-import { addExceptionMechanism } from '@sentry/utils';
 // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
 // eslint-disable-next-line import/no-unresolved
 import { Context, Handler } from 'aws-lambda';
@@ -20,6 +18,7 @@ import { performance } from 'perf_hooks';
 import { types } from 'util';
 
 import { AWSServices } from './awsservices';
+import { serverlessEventProcessor } from './utils';
 
 export * from '@sentry/node';
 
@@ -54,37 +53,8 @@ export function init(options: Sentry.NodeOptions = {}): void {
   if (options.defaultIntegrations === undefined) {
     options.defaultIntegrations = defaultIntegrations;
   }
-  return Sentry.init(options);
-}
-
-/**
- * Add event processor that will override SDK details to point to the serverless SDK instead of Node,
- * as well as set correct mechanism type, which should be set to `handled: false`.
- * We do it like this, so that we don't introduce any side-effects in this module, which makes it tree-shakeable.
- * @param scope Scope that processor should be added to
- */
-function addServerlessEventProcessor(scope: Scope): void {
-  scope.addEventProcessor(event => {
-    event.sdk = {
-      ...event.sdk,
-      name: 'sentry.javascript.serverless',
-      integrations: [...((event.sdk && event.sdk.integrations) || []), 'AWSLambda'],
-      packages: [
-        ...((event.sdk && event.sdk.packages) || []),
-        {
-          name: 'npm:@sentry/serverless',
-          version: SDK_VERSION,
-        },
-      ],
-      version: SDK_VERSION,
-    };
-
-    addExceptionMechanism(event, {
-      handled: false,
-    });
-
-    return event;
-  });
+  Sentry.init(options);
+  Sentry.addGlobalEventProcessor(serverlessEventProcessor('AWSLambda'));
 }
 
 /**
@@ -125,20 +95,6 @@ function enhanceScopeWithEnvironmentData(scope: Scope, context: Context): void {
   });
 }
 
-/**
- * Capture exception with a a context.
- *
- * @param e exception to be captured
- * @param context Context
- */
-function captureExceptionWithContext(e: unknown, context: Context): void {
-  withScope(scope => {
-    addServerlessEventProcessor(scope);
-    enhanceScopeWithEnvironmentData(scope, context);
-    captureException(e);
-  });
-}
-
 /**
  * Wraps a lambda handler adding it error capture and tracing capabilities.
  *
@@ -205,8 +161,6 @@ export function wrapHandler<TEvent, TResult>(
 
       timeoutWarningTimer = setTimeout(() => {
         withScope(scope => {
-          addServerlessEventProcessor(scope);
-          enhanceScopeWithEnvironmentData(scope, context);
           scope.setTag('timeout', humanReadableTimeout);
           captureMessage(`Possible function timeout: ${context.functionName}`, Severity.Warning);
         });
@@ -217,22 +171,24 @@ export function wrapHandler<TEvent, TResult>(
       name: context.functionName,
       op: 'awslambda.handler',
     });
-    // We put the transaction on the scope so users can attach children to it
-    getCurrentHub().configureScope(scope => {
-      scope.setSpan(transaction);
-    });
 
+    const hub = getCurrentHub();
+    const scope = hub.pushScope();
     let rv: TResult | undefined;
     try {
+      enhanceScopeWithEnvironmentData(scope, context);
+      // We put the transaction on the scope so users can attach children to it
+      scope.setSpan(transaction);
       rv = await asyncHandler(event, context);
     } catch (e) {
-      captureExceptionWithContext(e, context);
+      captureException(e);
       if (options.rethrowAfterCapture) {
         throw e;
       }
     } finally {
       clearTimeout(timeoutWarningTimer);
       transaction.finish();
+      hub.popScope();
       await flush(options.flushTimeout);
     }
     return rv;
diff --git a/packages/serverless/src/gcpfunction/cloud_events.ts b/packages/serverless/src/gcpfunction/cloud_events.ts
@@ -4,10 +4,10 @@ import {
   CloudEventFunction,
   CloudEventFunctionWithCallback,
 } from '@google-cloud/functions-framework/build/src/functions';
-import { flush, getCurrentHub, startTransaction } from '@sentry/node';
+import { captureException, flush, getCurrentHub, startTransaction } from '@sentry/node';
 import { logger } from '@sentry/utils';
 
-import { captureEventError, getActiveDomain, WrapperOptions } from './general';
+import { configureScopeWithContext, getActiveDomain, WrapperOptions } from './general';
 
 export type CloudEventFunctionWrapperOptions = WrapperOptions;
 
@@ -32,20 +32,22 @@ export function wrapCloudEventFunction(
       op: 'gcp.function.cloud_event',
     });
 
-    // We put the transaction on the scope so users can attach children to it
+    // getCurrentHub() is expected to use current active domain as a carrier
+    // since functions-framework creates a domain for each incoming request.
+    // So adding of event processors every time should not lead to memory bloat.
     getCurrentHub().configureScope(scope => {
+      configureScopeWithContext(scope, context);
+      // We put the transaction on the scope so users can attach children to it
       scope.setSpan(transaction);
     });
 
     const activeDomain = getActiveDomain();
 
-    activeDomain.on('error', err => {
-      captureEventError(err, context);
-    });
+    activeDomain.on('error', captureException);
 
     const newCallback = activeDomain.bind((...args: unknown[]) => {
       if (args[0] !== null && args[0] !== undefined) {
-        captureEventError(args[0], context);
+        captureException(args[0]);
       }
       transaction.finish();
 
diff --git a/packages/serverless/src/gcpfunction/events.ts b/packages/serverless/src/gcpfunction/events.ts
@@ -1,10 +1,10 @@
 // '@google-cloud/functions-framework/build/src/functions' import is expected to be type-only so it's erased in the final .js file.
 // When TypeScript compiler is upgraded, use `import type` syntax to explicitly assert that we don't want to load a module here.
 import { EventFunction, EventFunctionWithCallback } from '@google-cloud/functions-framework/build/src/functions';
-import { flush, getCurrentHub, startTransaction } from '@sentry/node';
+import { captureException, flush, getCurrentHub, startTransaction } from '@sentry/node';
 import { logger } from '@sentry/utils';
 
-import { captureEventError, getActiveDomain, WrapperOptions } from './general';
+import { configureScopeWithContext, getActiveDomain, WrapperOptions } from './general';
 
 export type EventFunctionWrapperOptions = WrapperOptions;
 
@@ -29,20 +29,22 @@ export function wrapEventFunction(
       op: 'gcp.function.event',
     });
 
-    // We put the transaction on the scope so users can attach children to it
+    // getCurrentHub() is expected to use current active domain as a carrier
+    // since functions-framework creates a domain for each incoming request.
+    // So adding of event processors every time should not lead to memory bloat.
     getCurrentHub().configureScope(scope => {
+      configureScopeWithContext(scope, context);
+      // We put the transaction on the scope so users can attach children to it
       scope.setSpan(transaction);
     });
 
     const activeDomain = getActiveDomain();
 
-    activeDomain.on('error', err => {
-      captureEventError(err, context);
-    });
+    activeDomain.on('error', captureException);
 
     const newCallback = activeDomain.bind((...args: unknown[]) => {
       if (args[0] !== null && args[0] !== undefined) {
-        captureEventError(args[0], context);
+        captureException(args[0]);
       }
       transaction.finish();
 
diff --git a/packages/serverless/src/gcpfunction/general.ts b/packages/serverless/src/gcpfunction/general.ts
@@ -1,9 +1,8 @@
 // '@google-cloud/functions-framework/build/src/functions' import is expected to be type-only so it's erased in the final .js file.
 // When TypeScript compiler is upgraded, use `import type` syntax to explicitly assert that we don't want to load a module here.
 import { Context } from '@google-cloud/functions-framework/build/src/functions';
-import { captureException, Scope, SDK_VERSION, withScope } from '@sentry/node';
+import { Scope } from '@sentry/node';
 import { Context as SentryContext } from '@sentry/types';
-import { addExceptionMechanism } from '@sentry/utils';
 import * as domain from 'domain';
 import { hostname } from 'os';
 
@@ -12,52 +11,18 @@ export interface WrapperOptions {
 }
 
 /**
- * Capture exception with additional event information.
+ * Enhances the scope with additional event information.
  *
- * @param e exception to be captured
+ * @param scope scope
  * @param context event context
  */
-export function captureEventError(e: unknown, context: Context): void {
-  withScope(scope => {
-    addServerlessEventProcessor(scope);
-    scope.setContext('runtime', {
-      name: 'node',
-      version: global.process.version,
-    });
-    scope.setTag('server_name', process.env.SENTRY_NAME || hostname());
-    scope.setContext('gcp.function.context', { ...context } as SentryContext);
-    captureException(e);
-  });
-}
-
-/**
- * Add event processor that will override SDK details to point to the serverless SDK instead of Node,
- * as well as set correct mechanism type, which should be set to `handled: false`.
- * We do it like this, so that we don't introduce any side-effects in this module, which makes it tree-shakeable.
- * @param scope Scope that processor should be added to
- */
-export function addServerlessEventProcessor(scope: Scope): void {
-  scope.addEventProcessor(event => {
-    event.sdk = {
-      ...event.sdk,
-      name: 'sentry.javascript.serverless',
-      integrations: [...((event.sdk && event.sdk.integrations) || []), 'GCPFunction'],
-      packages: [
-        ...((event.sdk && event.sdk.packages) || []),
-        {
-          name: 'npm:@sentry/serverless',
-          version: SDK_VERSION,
-        },
-      ],
-      version: SDK_VERSION,
-    };
-
-    addExceptionMechanism(event, {
-      handled: false,
-    });
-
-    return event;
+export function configureScopeWithContext(scope: Scope, context: Context): void {
+  scope.setContext('runtime', {
+    name: 'node',
+    version: global.process.version,
   });
+  scope.setTag('server_name', process.env.SENTRY_NAME || hostname());
+  scope.setContext('gcp.function.context', { ...context } as SentryContext);
 }
 
 /**
diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts
@@ -1,10 +1,10 @@
 // '@google-cloud/functions-framework/build/src/functions' import is expected to be type-only so it's erased in the final .js file.
 // When TypeScript compiler is upgraded, use `import type` syntax to explicitly assert that we don't want to load a module here.
 import { HttpFunction } from '@google-cloud/functions-framework/build/src/functions';
-import { captureException, flush, getCurrentHub, Handlers, startTransaction, withScope } from '@sentry/node';
+import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node';
 import { logger, stripUrlQueryAndFragment } from '@sentry/utils';
 
-import { addServerlessEventProcessor, getActiveDomain, WrapperOptions } from './general';
+import { getActiveDomain, WrapperOptions } from './general';
 
 type Request = Parameters<HttpFunction>[0];
 type Response = Parameters<HttpFunction>[1];
@@ -18,21 +18,6 @@ export { Request, Response };
 
 const { parseRequest } = Handlers;
 
-/**
- * Capture exception with additional request information.
- *
- * @param e exception to be captured
- * @param req incoming request
- * @param options request capture options
- */
-function captureRequestError(e: unknown, req: Request, options: ParseRequestOptions): void {
-  withScope(scope => {
-    addServerlessEventProcessor(scope);
-    scope.addEventProcessor(event => parseRequest(event, req, options));
-    captureException(e);
-  });
-}
-
 /**
  * Wraps an HTTP function handler adding it error capture and tracing capabilities.
  *
@@ -58,8 +43,12 @@ export function wrapHttpFunction(
       op: 'gcp.function.http',
     });
 
-    // We put the transaction on the scope so users can attach children to it
+    // getCurrentHub() is expected to use current active domain as a carrier
+    // since functions-framework creates a domain for each incoming request.
+    // So adding of event processors every time should not lead to memory bloat.
     getCurrentHub().configureScope(scope => {
+      scope.addEventProcessor(event => parseRequest(event, req, options.parseRequestOptions));
+      // We put the transaction on the scope so users can attach children to it
       scope.setSpan(transaction);
     });
 
@@ -71,7 +60,7 @@ export function wrapHttpFunction(
     // functions-framework creates a domain for each incoming request so we take advantage of this fact and add an error handler.
     // BTW this is the only way to catch any exception occured during request lifecycle.
     getActiveDomain().on('error', err => {
-      captureRequestError(err, req, options.parseRequestOptions);
+      captureException(err);
     });
 
     // eslint-disable-next-line @typescript-eslint/unbound-method
diff --git a/packages/serverless/src/gcpfunction/index.ts b/packages/serverless/src/gcpfunction/index.ts
@@ -1,4 +1,15 @@
+import * as Sentry from '@sentry/node';
+
+import { serverlessEventProcessor } from '../utils';
+
 export * from './http';
 export * from './events';
 export * from './cloud_events';
-export { init } from '@sentry/node';
+
+/**
+ * @see {@link Sentry.init}
+ */
+export function init(options: Sentry.NodeOptions = {}): void {
+  Sentry.init(options);
+  Sentry.addGlobalEventProcessor(serverlessEventProcessor('GCPFunction'));
+}
diff --git a/packages/serverless/src/utils.ts b/packages/serverless/src/utils.ts
@@ -0,0 +1,33 @@
+import { Event, SDK_VERSION } from '@sentry/node';
+import { addExceptionMechanism } from '@sentry/utils';
+
+/**
+ * Event processor that will override SDK details to point to the serverless SDK instead of Node,
+ * as well as set correct mechanism type, which should be set to `handled: false`.
+ * We do it like this, so that we don't introduce any side-effects in this module, which makes it tree-shakeable.
+ * @param event Event
+ * @param integration Name of the serverless integration ('AWSLambda', 'GCPFunction', etc)
+ */
+export function serverlessEventProcessor(integration: string): (event: Event) => Event {
+  return event => {
+    event.sdk = {
+      ...event.sdk,
+      name: 'sentry.javascript.serverless',
+      integrations: [...((event.sdk && event.sdk.integrations) || []), integration],
+      packages: [
+        ...((event.sdk && event.sdk.packages) || []),
+        {
+          name: 'npm:@sentry/serverless',
+          version: SDK_VERSION,
+        },
+      ],
+      version: SDK_VERSION,
+    };
+
+    addExceptionMechanism(event, {
+      handled: false,
+    });
+
+    return event;
+  };
+}
diff --git a/packages/serverless/test/__mocks__/@sentry/node.ts b/packages/serverless/test/__mocks__/@sentry/node.ts
diff --git a/packages/serverless/test/awslambda.test.ts b/packages/serverless/test/awslambda.test.ts
diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts
