From e95527e516ab939bd9b823bf20339178a31054dd Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 20:29:20 -0700 Subject: [PATCH 1/5] add request to transaction/scope metadata in express request/tracing/error handlers --- packages/node/src/handlers.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 68155cb9559a..18ac5adade8e 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -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, }, }, @@ -162,6 +167,8 @@ export function requestHandler( currentHub.configureScope(scope => { scope.addEventProcessor(backwardsCompatibleEventProcessor); + scope.setSDKProcessingMetadata({ request: req }); + const client = currentHub.getClient(); if (isAutoSessionTrackingEnabled(client)) { const scope = currentHub.getScope(); @@ -240,6 +247,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; From bc1004e2e2c969cce77cfd409e80b9b8353ef96e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 20:40:02 -0700 Subject: [PATCH 2/5] store express request handler options in scope metadata --- packages/node/src/handlers.ts | 35 ++++++++++++++++++++++++++++++- packages/types/src/transaction.ts | 6 ++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 18ac5adade8e..46c532508362 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -5,6 +5,7 @@ import { AddRequestDataToEventOptions, addRequestDataToTransaction, baggageHeaderToDynamicSamplingContext, + dropUndefinedKeys, extractPathForTransaction, extractTraceparentData, isString, @@ -109,6 +110,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` @@ -116,6 +142,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 @@ -167,7 +196,11 @@ export function requestHandler( currentHub.configureScope(scope => { scope.addEventProcessor(backwardsCompatibleEventProcessor); - scope.setSDKProcessingMetadata({ request: req }); + scope.setSDKProcessingMetadata({ + request: req, + // TODO (v8): Stop passing this + requestDataOptionsFromExpressHandler: requestDataOptions, + }); const client = currentHub.getClient(); if (isAutoSessionTrackingEnabled(client)) { 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; From 9d14753041423dd2460c7bde592c72a4f5ecdba4 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 8 Sep 2022 20:42:57 -0700 Subject: [PATCH 3/5] stop adding request data event processor in express request handler --- packages/node/src/handlers.ts | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 46c532508362..0f7465a91be4 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -1,6 +1,6 @@ /* 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, @@ -15,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'; /** @@ -164,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; @@ -195,7 +185,6 @@ export function requestHandler( const currentHub = getCurrentHub(); currentHub.configureScope(scope => { - scope.addEventProcessor(backwardsCompatibleEventProcessor); scope.setSDKProcessingMetadata({ request: req, // TODO (v8): Stop passing this From f6b93a793cbd6d4b37466bcfc4105ac8e347807b Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 17 Oct 2022 14:23:56 -0700 Subject: [PATCH 4/5] use express handler request data options from `sdkProcessingMetadata` if available --- packages/node/src/integrations/requestdata.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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); From def4408f808bd3055922da762e7c2a0ad6a5a274 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 18 Oct 2022 20:39:12 -0700 Subject: [PATCH 5/5] add tests --- packages/node/test/handlers.test.ts | 50 ++++++++++++++++++- .../test/integrations/requestdata.test.ts | 21 ++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) 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' })); + }); + }); });