From 1bd55158cc27449a46b9b8030b72e0736c78180f Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 14 Sep 2022 22:12:01 -0700 Subject: [PATCH 1/7] add `request` to `TransactionMetadata` type --- packages/types/src/transaction.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 21508bef4c44..be8e0fef4ea4 100644 --- a/packages/types/src/transaction.ts +++ b/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; + /** 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. */ From 26263565096bec5679160f386e05555bee08a70c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 5 Sep 2022 23:49:58 -0700 Subject: [PATCH 2/7] add request to transaction metadata --- packages/nextjs/src/config/wrappers/wrapperUtils.ts | 4 ++++ packages/nextjs/src/utils/instrumentServer.ts | 1 + packages/nextjs/src/utils/withSentry.ts | 1 + 3 files changed, 6 insertions(+) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index 0b64d17acf9d..1aa15aa8d738 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -106,7 +106,11 @@ export function callTracedServerSideDataFetcher 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({ diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 5fe439024ae8..0e618e157bb4 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/packages/nextjs/src/utils/instrumentServer.ts @@ -276,6 +276,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, }, diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index bd6ca7c715ab..923929137f92 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -90,6 +90,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler = metadata: { dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, source: 'route', + request: req, }, }, // extra context passed to the `tracesSampler` From be7471f52018cf936f375b5ff6eea9060636c724 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Mon, 5 Sep 2022 23:48:53 -0700 Subject: [PATCH 3/7] add `RequestData` integration --- packages/node/src/index.ts | 2 +- packages/node/src/integrations/index.ts | 1 + packages/node/src/integrations/requestdata.ts | 116 ++++++++++++++++++ packages/node/src/requestdata.ts | 2 +- 4 files changed, 119 insertions(+), 2 deletions(-) create mode 100644 packages/node/src/integrations/requestdata.ts diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index a637e6c44865..278561ce4c68 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -47,7 +47,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'; diff --git a/packages/node/src/integrations/index.ts b/packages/node/src/integrations/index.ts index 05a25d7cbad3..cdb5145b0ea1 100644 --- a/packages/node/src/integrations/index.ts +++ b/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'; diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts new file mode 100644 index 000000000000..ee82532119b1 --- /dev/null +++ b/packages/node/src/integrations/requestdata.ts @@ -0,0 +1,116 @@ +// 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 } 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; + }; + + /** + * 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, + }, +}; + +/** 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 = {}) { + 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, + }; +} diff --git a/packages/node/src/requestdata.ts b/packages/node/src/requestdata.ts index 058f2ea75c68..14a05cd8aa7c 100644 --- a/packages/node/src/requestdata.ts +++ b/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 From e44ed8ffc01e33b30ae9699b17018a732ce3f131 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 6 Sep 2022 20:58:54 -0700 Subject: [PATCH 4/7] add `RequestData` to default server integrations --- packages/nextjs/src/index.server.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts index 2dfb9730b19e..cc1c314a5eda 100644 --- a/packages/nextjs/src/index.server.ts +++ b/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, { From 57f2f38708385520ba8fb66699ab9f1cc1f1ee3a Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Sun, 11 Sep 2022 19:25:52 -0700 Subject: [PATCH 5/7] stop using existing request data event processors on transaction events --- .../nextjs/src/config/wrappers/wrapperUtils.ts | 18 ++++++++++-------- packages/nextjs/src/utils/instrumentServer.ts | 4 +++- packages/nextjs/src/utils/withSentry.ts | 4 +++- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts index 1aa15aa8d738..df546a018377 100644 --- a/packages/nextjs/src/config/wrappers/wrapperUtils.ts +++ b/packages/nextjs/src/config/wrappers/wrapperUtils.ts @@ -122,14 +122,16 @@ export function callTracedServerSideDataFetcher 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, ); } diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts index 0e618e157bb4..80c24b008388 100644 --- a/packages/nextjs/src/utils/instrumentServer.ts +++ b/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)) { diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index 923929137f92..6158008f6921 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/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) From fed0ff3c39c6ef87779e068d2194121efd323fad Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Wed, 7 Sep 2022 21:45:41 -0700 Subject: [PATCH 6/7] add and fix tests --- packages/nextjs/test/config/wrappers.test.ts | 70 +++++++++++++++++++ packages/nextjs/test/index.server.test.ts | 2 + packages/nextjs/test/utils/withSentry.test.ts | 3 +- 3 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 packages/nextjs/test/config/wrappers.test.ts diff --git a/packages/nextjs/test/config/wrappers.test.ts b/packages/nextjs/test/config/wrappers.test.ts new file mode 100644 index 000000000000..90af29200c00 --- /dev/null +++ b/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 }); + }); + }); +}); diff --git a/packages/nextjs/test/index.server.test.ts b/packages/nextjs/test/index.server.test.ts index 8c876c30cd8d..3f62158392db 100644 --- a/packages/nextjs/test/index.server.test.ts +++ b/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', () => { diff --git a/packages/nextjs/test/utils/withSentry.test.ts b/packages/nextjs/test/utils/withSentry.test.ts index 105d23735e09..ee68e17f9499 100644 --- a/packages/nextjs/test/utils/withSentry.test.ts +++ b/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' }) }, From 124fd6d35905f0711c329ab192c528cac6d10eb4 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 16 Sep 2022 18:46:01 -0700 Subject: [PATCH 7/7] add `transactionNamingScheme` option to `RequestData` integration --- packages/node/src/index.ts | 1 + packages/node/src/integrations/requestdata.ts | 11 ++++++++++- packages/node/src/requestdata.ts | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 278561ce4c68..566340c6b7c6 100644 --- a/packages/node/src/index.ts +++ b/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 { diff --git a/packages/node/src/integrations/requestdata.ts b/packages/node/src/integrations/requestdata.ts index ee82532119b1..e5ef0b6ff989 100644 --- a/packages/node/src/integrations/requestdata.ts +++ b/packages/node/src/integrations/requestdata.ts @@ -3,7 +3,12 @@ import { EventProcessor, Hub, Integration } from '@sentry/types'; -import { addRequestDataToEvent, AddRequestDataToEventOptions, DEFAULT_USER_INCLUDES } from '../requestdata'; +import { + addRequestDataToEvent, + AddRequestDataToEventOptions, + DEFAULT_USER_INCLUDES, + TransactionNamingScheme, +} from '../requestdata'; type RequestDataOptions = { /** @@ -19,6 +24,9 @@ type RequestDataOptions = { user?: boolean | Array; }; + /** 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. @@ -39,6 +47,7 @@ const DEFAULT_OPTIONS = { 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` diff --git a/packages/node/src/requestdata.ts b/packages/node/src/requestdata.ts index 14a05cd8aa7c..616164c2c67d 100644 --- a/packages/node/src/requestdata.ts +++ b/packages/node/src/requestdata.ts @@ -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`