ref(node): Use RequestData integration in express handlers

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;