ref(node): Use RequestData integration in express handlers (#5990)

This switches the request, tracing, and error handlers from the Node SDK's `handlers.ts` to use the new `RequestData` integration for adding request data to events rather than the current event processor. 

Notes:

- So that this isn't a breaking change, for the moment (read: until v8) the integration will use the options passed to the request handler in place of its own options. (The request handler now stores its options in `sdkProcessingMetadata` alongside the request so that the integration will have access to them.)
- Before this change, the event processor was backwards-compatible by dint of calling `parseRequest` rather than `addRequestDataToEvent` if the request handler's options are given in the old format. Because the integration uses only `addRequestDataToEvent` under the hood, the backwards compatibility is now achieved by converting the old-style options into equivalent new-style options, using a new helper function `convertReqHandlerOptsToAddReqDataOpts`. (And yes, that function name is definitely a mouthful, but fortunately only one will has to last until v8.)
- Though in theory one should never use the error or tracing middleware without also using the request middleware, people do all sorts of weird things. All three middlewares therefore add the request to `sdkProcessingMetadata`, even though just doing so in the request handler should technically be sufficient.

Ref: #5756

Author: lobsterkatie

packages/node/src/handlers.ts

@@ -1,10 +1,11 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import { captureException, getCurrentHub, startTransaction, withScope } from '@sentry/core';
-import { Event, Span } from '@sentry/types';
+import { Span } from '@sentry/types';
 import {
   AddRequestDataToEventOptions,
   addRequestDataToTransaction,
   baggageHeaderToDynamicSamplingContext,
+  dropUndefinedKeys,
   extractPathForTransaction,
   extractTraceparentData,
   isString,
@@ -14,10 +15,9 @@ import * as domain from 'domain';
 import * as http from 'http';
 
 import { NodeClient } from './client';
-import { addRequestDataToEvent, extractRequestData } from './requestdata';
-// TODO (v8 / XXX) Remove these imports
+import { extractRequestData } from './requestdata';
+// TODO (v8 / XXX) Remove this import
 import type { ParseRequestOptions } from './requestDataDeprecated';
-import { parseRequest } from './requestDataDeprecated';
 import { flush, isAutoSessionTrackingEnabled } from './sdk';
 
 /**
@@ -66,6 +66,11 @@ export function tracingHandler(): (
         ...traceparentData,
         metadata: {
           dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
+          // The request should already have been stored in `scope.sdkProcessingMetadata` (which will become
+          // `event.sdkProcessingMetadata` the same way the metadata here will) by `sentryRequestMiddleware`, but on the
+          // off chance someone is using `sentryTracingMiddleware` without `sentryRequestMiddleware`, it doesn't hurt to
+          // be sure
+          request: req,
           source,
         },
       },
@@ -104,13 +109,41 @@ export type RequestHandlerOptions =
       flushTimeout?: number;
     };
 
+/**
+ * Backwards compatibility shim which can be removed in v8. Forces the given options to follow the
+ * `AddRequestDataToEventOptions` interface.
+ *
+ * TODO (v8): Get rid of this, and stop passing `requestDataOptionsFromExpressHandler` to `setSDKProcessingMetadata`.
+ */
+function convertReqHandlerOptsToAddReqDataOpts(
+  reqHandlerOptions: RequestHandlerOptions = {},
+): AddRequestDataToEventOptions | undefined {
+  let addRequestDataOptions: AddRequestDataToEventOptions | undefined;
+
+  if ('include' in reqHandlerOptions) {
+    addRequestDataOptions = { include: reqHandlerOptions.include };
+  } else {
+    // eslint-disable-next-line deprecation/deprecation
+    const { ip, request, transaction, user } = reqHandlerOptions as ParseRequestOptions;
+
+    if (ip || request || transaction || user) {
+      addRequestDataOptions = { include: dropUndefinedKeys({ ip, request, transaction, user }) };
+    }
+  }
+
+  return addRequestDataOptions;
+}
+
 /**
  * Express compatible request handler.
  * @see Exposed as `Handlers.requestHandler`
  */
 export function requestHandler(
   options?: RequestHandlerOptions,
 ): (req: http.IncomingMessage, res: http.ServerResponse, next: (error?: any) => void) => void {
+  // TODO (v8): Get rid of this
+  const requestDataOptions = convertReqHandlerOptsToAddReqDataOpts(options);
+
   const currentHub = getCurrentHub();
   const client = currentHub.getClient<NodeClient>();
   // Initialise an instance of SessionFlusher on the client when `autoSessionTracking` is enabled and the
@@ -130,15 +163,6 @@ export function requestHandler(
     res: http.ServerResponse,
     next: (error?: any) => void,
   ): void {
-    // TODO (v8 / XXX) Remove this shim and just use `addRequestDataToEvent`
-    let backwardsCompatibleEventProcessor: (event: Event) => Event;
-    if (options && 'include' in options) {
-      backwardsCompatibleEventProcessor = (event: Event) => addRequestDataToEvent(event, req, options);
-    } else {
-      // eslint-disable-next-line deprecation/deprecation
-      backwardsCompatibleEventProcessor = (event: Event) => parseRequest(event, req, options as ParseRequestOptions);
-    }
-
     if (options && options.flushTimeout && options.flushTimeout > 0) {
       // eslint-disable-next-line @typescript-eslint/unbound-method
       const _end = res.end;
@@ -161,7 +185,12 @@ export function requestHandler(
       const currentHub = getCurrentHub();
 
       currentHub.configureScope(scope => {
-        scope.addEventProcessor(backwardsCompatibleEventProcessor);
+        scope.setSDKProcessingMetadata({
+          request: req,
+          // TODO (v8): Stop passing this
+          requestDataOptionsFromExpressHandler: requestDataOptions,
+        });
+
         const client = currentHub.getClient<NodeClient>();
         if (isAutoSessionTrackingEnabled(client)) {
           const scope = currentHub.getScope();
@@ -240,6 +269,11 @@ export function errorHandler(options?: {
 
     if (shouldHandleError(error)) {
       withScope(_scope => {
+        // The request should already have been stored in `scope.sdkProcessingMetadata` by `sentryRequestMiddleware`,
+        // but on the off chance someone is using `sentryErrorMiddleware` without `sentryRequestMiddleware`, it doesn't
+        // hurt to be sure
+        _scope.setSDKProcessingMetadata({ request: _req });
+
         // For some reason we need to set the transaction on the scope again
         // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
         const transaction = (res as any).__sentry_transaction as Span;

packages/node/src/integrations/requestdata.ts

@@ -108,15 +108,22 @@ export class RequestData implements Integration {
     addGlobalEventProcessor(event => {
       const hub = getCurrentHub();
       const self = hub.getIntegration(RequestData);
-      const req = event.sdkProcessingMetadata && event.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;
       }
 
-      const addRequestDataOptions = 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 people to use this
+      // integration, so that all of this passing and conversion isn't necessary
+      const addRequestDataOptions =
+        sdkProcessingMetadata.requestDataOptionsFromExpressHandler ||
+        convertReqDataIntegrationOptsToAddReqDataOpts(this._options);
 
       const processedEvent = this._addRequestData(event, req, addRequestDataOptions);
 

packages/node/test/handlers.test.ts

@@ -1,5 +1,5 @@
 import * as sentryCore from '@sentry/core';
-import { Hub, Scope } from '@sentry/core';
+import { Hub, makeMain, Scope } from '@sentry/core';
 import { Transaction } from '@sentry/tracing';
 import { Event } from '@sentry/types';
 import { SentryError } from '@sentry/utils';
@@ -136,6 +136,22 @@ describe('requestHandler', () => {
       done();
     });
   });
+
+  it('stores request and request data options in `sdkProcessingMetadata`', () => {
+    const hub = new Hub(new NodeClient(getDefaultNodeClientOptions()));
+    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+
+    const requestHandlerOptions = { include: { ip: false } };
+    const sentryRequestMiddleware = requestHandler(requestHandlerOptions);
+
+    sentryRequestMiddleware(req, res, next);
+
+    const scope = sentryCore.getCurrentHub().getScope();
+    expect((scope as any)._sdkProcessingMetadata).toEqual({
+      request: req,
+      requestDataOptionsFromExpressHandler: requestHandlerOptions,
+    });
+  });
 });
 
 describe('tracingHandler', () => {
@@ -392,6 +408,19 @@ describe('tracingHandler', () => {
       done();
     });
   });
+
+  it('stores request in transaction metadata', () => {
+    const options = getDefaultNodeClientOptions({ tracesSampleRate: 1.0 });
+    const hub = new Hub(new NodeClient(options));
+
+    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
+
+    sentryTracingMiddleware(req, res, next);
+
+    const transaction = sentryCore.getCurrentHub().getScope()?.getTransaction();
+
+    expect(transaction?.metadata.request).toEqual(req);
+  });
 });
 
 describe('errorHandler()', () => {
@@ -498,4 +527,23 @@ describe('errorHandler()', () => {
     const requestSession = scope?.getRequestSession();
     expect(requestSession).toEqual(undefined);
   });
+
+  it('stores request in `sdkProcessingMetadata`', () => {
+    const options = getDefaultNodeClientOptions({});
+    client = new NodeClient(options);
+
+    const hub = new Hub(client);
+    makeMain(hub);
+
+    // `sentryErrorMiddleware` uses `withScope`, and we need access to the temporary scope it creates, so monkeypatch
+    // `captureException` in order to examine the scope as it exists inside the `withScope` callback
+    hub.captureException = function (this: Hub, _exception: any) {
+      const scope = this.getScope();
+      expect((scope as any)._sdkProcessingMetadata.request).toEqual(req);
+    } as any;
+
+    sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
+
+    expect.assertions(1);
+  });
 });

packages/node/test/integrations/requestdata.test.ts

@@ -3,6 +3,7 @@ import { Event, EventProcessor } from '@sentry/types';
 import * as http from 'http';
 
 import { NodeClient } from '../../src/client';
+import { requestHandler } from '../../src/handlers';
 import { RequestData, RequestDataIntegrationOptions } from '../../src/integrations/requestdata';
 import * as requestDataModule from '../../src/requestdata';
 import { getDefaultNodeClientOptions } from '../helper/node-client-options';
@@ -100,4 +101,24 @@ describe('`RequestData` integration', () => {
       expect(passedOptions?.include?.user).not.toEqual(expect.arrayContaining(['email']));
     });
   });
+
+  describe('usage with express request handler', () => {
+    it('uses options from request handler', async () => {
+      const sentryRequestMiddleware = requestHandler({ include: { transaction: 'methodPath' } });
+      const res = new http.ServerResponse(req);
+      const next = jest.fn();
+
+      initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' });
+
+      sentryRequestMiddleware(req, res, next);
+
+      await getCurrentHub().getScope()!.applyToEvent(event, {});
+      requestDataEventProcessor(event);
+
+      const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];
+
+      // `transaction` matches the request middleware's option, not the integration's option
+      expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'methodPath' }));
+    });
+  });
 });

packages/types/src/transaction.ts

@@ -149,6 +149,12 @@ export interface TransactionMetadata {
   /** For transactions tracing server-side request handling, the request being tracked. */
   request?: PolymorphicRequest;
 
+  /** Compatibility shim for transitioning to the `RequestData` integration. The options passed to our Express request
+   * handler controlling what request data is added to the event.
+   * TODO (v8): This should go away
+   */
+  requestDataOptionsFromExpressHandler?: { [key: string]: unknown };
+
   /** For transactions tracing server-side request handling, the path of the request being tracked. */
   /** TODO: If we rm -rf `instrumentServer`, this can go, too */
   requestPath?: string;