diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 68155cb9559a..0f7465a91be4 100644 --- a/packages/node/src/handlers.ts +++ b/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,6 +109,31 @@ 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` @@ -111,6 +141,9 @@ export type RequestHandlerOptions = 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(); // 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(); 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; diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts index 0d9d7d92662e..9f00d1bb4fe4 100644 --- a/packages/node/src/integrations/requestdata.ts +++ b/packages/node/src/integrations/requestdata.ts @@ -108,7 +108,9 @@ 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 @@ -116,7 +118,12 @@ export class RequestData implements Integration { 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); diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index ed35c01ef25d..1bf422f59621 100644 --- a/packages/node/test/handlers.test.ts +++ b/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); + }); }); diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts index e1fbaa88b556..8ec956beac8b 100644 --- a/packages/node/test/integrations/requestdata.test.ts +++ b/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' })); + }); + }); }); diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 8ee09ef26bd3..3859438866d0 100644 --- a/packages/types/src/transaction.ts +++ b/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;