ref(nextjs): Use integration to add request data to transaction events

packages/nextjs/src/config/wrappers/wrapperUtils.ts

@@ -106,7 +106,11 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
 
       requestTransaction = newTransaction;
       autoEndTransactionOnResponseEnd(newTransaction, res);
+
+      // Link the transaction and the request together, so that when we would normally only have access to one, it's
+      // still possible to grab the other.
       setTransactionOnRequest(newTransaction, req);
+      newTransaction.setMetadata({ request: req });
     }
 
     const dataFetcherSpan = requestTransaction.startChild({
@@ -118,14 +122,16 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
     if (currentScope) {
       currentScope.setSpan(dataFetcherSpan);
       currentScope.addEventProcessor(event =>
-        addRequestDataToEvent(event, req, {
-          include: {
-            // When the `transaction` option is set to true, it tries to extract a transaction name from the request
-            // object. We don't want this since we already have a high-quality transaction name with a parameterized
-            // route. Setting `transaction` to `true` will clobber that transaction name.
-            transaction: false,
-          },
-        }),
+        event.type !== 'transaction'
+          ? addRequestDataToEvent(event, req, {
+              include: {
+                // When the `transaction` option is set to true, it tries to extract a transaction name from the request
+                // object. We don't want this since we already have a high-quality transaction name with a parameterized
+                // route. Setting `transaction` to `true` will clobber that transaction name.
+                transaction: false,
+              },
+            })
+          : event,
       );
     }
 

packages/nextjs/src/index.server.ts

@@ -111,6 +111,9 @@ function addServerIntegrations(options: NextjsOptions): void {
   });
   integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations);
 
+  const defaultRequestDataIntegration = new Integrations.RequestData();
+  integrations = addOrUpdateIntegration(defaultRequestDataIntegration, integrations);
+
   if (hasTracingEnabled(options)) {
     const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
     integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {

packages/nextjs/src/utils/instrumentServer.ts

@@ -244,7 +244,9 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
       const currentScope = getCurrentHub().getScope();
 
       if (currentScope) {
-        currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq));
+        currentScope.addEventProcessor(event =>
+          event.type !== 'transaction' ? addRequestDataToEvent(event, nextReq) : event,
+        );
 
         // We only want to record page and API requests
         if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) {
@@ -276,6 +278,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
                 // like `source: isDynamicRoute? 'url' : 'route'`
                 // TODO: What happens when `withSentry` is used also? Which values of `name` and `source` win?
                 source: 'url',
+                request: req,
               },
               ...traceparentData,
             },

packages/nextjs/src/utils/withSentry.ts

@@ -56,7 +56,9 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
       const currentScope = getCurrentHub().getScope();
 
       if (currentScope) {
-        currentScope.addEventProcessor(event => addRequestDataToEvent(event, req));
+        currentScope.addEventProcessor(event =>
+          event.type !== 'transaction' ? addRequestDataToEvent(event, req) : event,
+        );
 
         if (hasTracingEnabled()) {
           // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
@@ -90,6 +92,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
               metadata: {
                 dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
                 source: 'route',
+                request: req,
               },
             },
             // extra context passed to the `tracesSampler`

packages/nextjs/test/config/wrappers.test.ts

@@ -0,0 +1,70 @@
+import * as SentryCore from '@sentry/core';
+import * as SentryTracing from '@sentry/tracing';
+import { IncomingMessage, ServerResponse } from 'http';
+
+import {
+  withSentryGetServerSideProps,
+  withSentryServerSideGetInitialProps,
+  // TODO: Leaving `withSentryGetStaticProps` out for now until we figure out what to do with it
+  // withSentryGetStaticProps,
+  // TODO: Leaving these out for now until we figure out pages with no data fetchers
+  // withSentryServerSideAppGetInitialProps,
+  // withSentryServerSideDocumentGetInitialProps,
+  // withSentryServerSideErrorGetInitialProps,
+} from '../../src/config/wrappers';
+
+const startTransactionSpy = jest.spyOn(SentryCore, 'startTransaction');
+const setMetadataSpy = jest.spyOn(SentryTracing.Transaction.prototype, 'setMetadata');
+
+describe('data-fetching function wrappers', () => {
+  const route = '/tricks/[trickName]';
+  let req: IncomingMessage;
+  let res: ServerResponse;
+
+  describe('starts a transaction and puts request in metadata if tracing enabled', () => {
+    beforeEach(() => {
+      req = { headers: {}, url: 'http://dogs.are.great/tricks/kangaroo' } as IncomingMessage;
+      res = {} as ServerResponse;
+
+      jest.spyOn(SentryTracing, 'hasTracingEnabled').mockReturnValueOnce(true);
+    });
+
+    afterEach(() => {
+      jest.clearAllMocks();
+    });
+
+    test('withSentryGetServerSideProps', async () => {
+      const origFunction = jest.fn(async () => ({ props: {} }));
+
+      const wrappedOriginal = withSentryGetServerSideProps(origFunction, route);
+      await wrappedOriginal({ req, res } as any);
+
+      expect(startTransactionSpy).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: '/tricks/[trickName]',
+          op: 'nextjs.data.server',
+          metadata: expect.objectContaining({ source: 'route' }),
+        }),
+      );
+
+      expect(setMetadataSpy).toHaveBeenCalledWith({ request: req });
+    });
+
+    test('withSentryServerSideGetInitialProps', async () => {
+      const origFunction = jest.fn(async () => ({}));
+
+      const wrappedOriginal = withSentryServerSideGetInitialProps(origFunction);
+      await wrappedOriginal({ req, res, pathname: route } as any);
+
+      expect(startTransactionSpy).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: '/tricks/[trickName]',
+          op: 'nextjs.data.server',
+          metadata: expect.objectContaining({ source: 'route' }),
+        }),
+      );
+
+      expect(setMetadataSpy).toHaveBeenCalledWith({ request: req });
+    });
+  });
+});

packages/nextjs/test/index.server.test.ts

@@ -150,8 +150,10 @@ describe('Server init()', () => {
 
       const nodeInitOptions = nodeInit.mock.calls[0][0] as ModifiedInitOptions;
       const rewriteFramesIntegration = findIntegrationByName(nodeInitOptions.integrations, 'RewriteFrames');
+      const requestDataIntegration = findIntegrationByName(nodeInitOptions.integrations, 'RequestData');
 
       expect(rewriteFramesIntegration).toBeDefined();
+      expect(requestDataIntegration).toBeDefined();
     });
 
     it('supports passing unrelated integrations through options', () => {

packages/nextjs/test/utils/withSentry.test.ts

@@ -104,7 +104,7 @@ describe('withSentry', () => {
   });
 
   describe('tracing', () => {
-    it('starts a transaction when tracing is enabled', async () => {
+    it('starts a transaction and sets metadata when tracing is enabled', async () => {
       jest
         .spyOn(hub.Hub.prototype, 'getClient')
         .mockReturnValueOnce({ getOptions: () => ({ tracesSampleRate: 1 } as ClientOptions) } as Client);
@@ -118,6 +118,7 @@ describe('withSentry', () => {
 
           metadata: {
             source: 'route',
+            request: expect.objectContaining({ url: 'http://dogs.are.great' }),
           },
         },
         { request: expect.objectContaining({ url: 'http://dogs.are.great' }) },

packages/node/src/index.ts

@@ -18,6 +18,7 @@ export type {
 } from '@sentry/types';
 export type { AddRequestDataToEventOptions } from '@sentry/utils';
 
+export type { TransactionNamingScheme } from './requestdata';
 export type { NodeOptions } from './types';
 
 export {
@@ -47,7 +48,7 @@ export {
 export { NodeClient } from './client';
 export { makeNodeTransport } from './transports';
 export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk';
-export { addRequestDataToEvent, extractRequestData } from './requestdata';
+export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from './requestdata';
 export { deepReadDirSync } from './utils';
 
 import { Integrations as CoreIntegrations } from '@sentry/core';

packages/node/src/integrations/index.ts

@@ -6,3 +6,4 @@ export { LinkedErrors } from './linkederrors';
 export { Modules } from './modules';
 export { ContextLines } from './contextlines';
 export { Context } from './context';
+export { RequestData } from './requestdata';

packages/node/src/integrations/requestdata.ts

@@ -0,0 +1,125 @@
+// 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 { EventProcessor, Hub, Integration } from '@sentry/types';
+
+import {
+  addRequestDataToEvent,
+  AddRequestDataToEventOptions,
+  DEFAULT_USER_INCLUDES,
+  TransactionNamingScheme,
+} from '../requestdata';
+
+type RequestDataOptions = {
+  /**
+   * Controls what data is pulled from the request and added to the event
+   */
+  include: {
+    cookies?: boolean;
+    data?: boolean;
+    headers?: boolean;
+    ip?: boolean;
+    query_string?: boolean;
+    url?: boolean;
+    user?: boolean | Array<typeof DEFAULT_USER_INCLUDES[number]>;
+  };
+
+  /** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
+  transactionNamingScheme: TransactionNamingScheme;
+
+  /**
+   * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but
+   * left injectable so this integration can be moved to `@sentry/core` and used in browser-based SDKs in the future.
+   *
+   * @hidden
+   */
+  addRequestData: typeof addRequestDataToEvent;
+};
+
+const DEFAULT_OPTIONS = {
+  addRequestData: addRequestDataToEvent,
+  include: {
+    cookies: true,
+    data: true,
+    headers: true,
+    ip: false,
+    query_string: true,
+    url: true,
+    user: DEFAULT_USER_INCLUDES,
+  },
+  transactionNamingScheme: 'methodpath',
+};
+
+/** Add data about a request to an event. Primarily for use in Node-based SDKs, but included in `@sentry/integrations`
+ * so it can be used in cross-platform SDKs like `@sentry/nextjs`. */
+export class RequestData implements Integration {
+  /**
+   * @inheritDoc
+   */
+  public static id: string = 'RequestData';
+
+  /**
+   * @inheritDoc
+   */
+  public name: string = RequestData.id;
+
+  private _options: RequestDataOptions;
+
+  /**
+   * @inheritDoc
+   */
+  public constructor(options: Partial<RequestDataOptions> = {}) {
+    this._options = {
+      ...DEFAULT_OPTIONS,
+      ...options,
+      include: {
+        // @ts-ignore It's mad because `method` isn't a known `include` key. (It's only here and not set by default in
+        // `addRequestDataToEvent` for legacy reasons. TODO (v8): Change that.)
+        method: true,
+        ...DEFAULT_OPTIONS.include,
+        ...options.include,
+      },
+    };
+  }
+
+  /**
+   * @inheritDoc
+   */
+  public setupOnce(addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, getCurrentHub: () => Hub): void {
+    const { include, addRequestData } = this._options;
+
+    addGlobalEventProcessor(event => {
+      const self = getCurrentHub().getIntegration(RequestData);
+      const req = event.sdkProcessingMetadata && event.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;
+      }
+
+      return addRequestData(event, req, { include: formatIncludeOption(include) });
+    });
+  }
+}
+
+/** Convert `include` option to match what `addRequestDataToEvent` expects */
+/** TODO: Can possibly be deleted once https://github.com/getsentry/sentry-javascript/issues/5718 is fixed */
+function formatIncludeOption(
+  integrationInclude: RequestDataOptions['include'] = {},
+): AddRequestDataToEventOptions['include'] {
+  const { ip, user, ...requestOptions } = integrationInclude;
+
+  const requestIncludeKeys: string[] = [];
+  for (const [key, value] of Object.entries(requestOptions)) {
+    if (value) {
+      requestIncludeKeys.push(key);
+    }
+  }
+
+  return {
+    ip,
+    user,
+    request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined,
+  };
+}

packages/node/src/requestdata.ts

@@ -10,7 +10,7 @@ const DEFAULT_INCLUDES = {
   user: true,
 };
 const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
-const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];
+export const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];
 
 /**
  * Options deciding what parts of the request to use when enhancing an event
@@ -25,7 +25,7 @@ export interface AddRequestDataToEventOptions {
   };
 }
 
-type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
+export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
 
 /**
  * Sets parameterized route as transaction name e.g.: `GET /users/:id`

packages/types/src/transaction.ts

@@ -1,7 +1,9 @@
 import { DynamicSamplingContext } from './envelope';
 import { MeasurementUnit } from './measurement';
 import { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc';
+import { PolymorphicRequest } from './polymorphics';
 import { Span, SpanContext } from './span';
+
 /**
  * Interface holding Transaction-specific properties
  */
@@ -145,7 +147,11 @@ export interface TransactionMetadata {
    */
   dynamicSamplingContext?: Partial<DynamicSamplingContext>;
 
+  /** For transactions tracing server-side request handling, the request being tracked. */
+  request?: PolymorphicRequest;
+
   /** 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;
 
   /** Information on how a transaction name was generated. */