Skip to content

Commit 5660d9f

Browse files
authored
ref(nextjs): Use integration to add request data to transaction events (#5703)
In most of our Node-based SDKs, we use domains to prevent scope bleed between requests, running the entire wrapped event-handling process inside a single domain. This works because in those cases, there is only one request-handling entry point for us to wrap, so we can stick the entire thing in a single `domain.run()` call and (more or less) rely on the fact that we've got a single, consistent, unique-to-that-request `Scope` object that we're dealing with. We've followed this pattern in the nextjs SDK as well, both in `instrumentServer` and `withSentry`. The new auto-wrapping of data-fetching functions presents a challenge, though, because a single request may involve a number of our wrapped functions running at different points in the request handling process, with no shared (little S) scope between them. While we still use domains when wrapping the data fetchers, a single request may pass through multiple domains during its lifecycle, preventing us from using any given domain as a universal `Scope`-carrier for a single `Scope` instance. One place where this becomes is problem is our use of `addRequestDataToEvent`, which up until now we've just been calling inside a single-request event processor added to the current `Scope`. In this system, each event processor holds a reference to its particular `req` object. In the case of the data-fetchers, however, the `Scope` instance to which we might add such an event processor isn't the same as the one which will be active when the event is being processed, so the current way doesn't work. But given that the only way in which our current, single-request-specific event processors differ is by the request to which they hold a reference, they can be replaced by a single, and universal, event processor, as long as we can access `req` a different way besides keeping it in a closure as we do now. This PR does just that (that is, switches to using a single event processor) for transaction events. First, a reference to `req` is stored in the transaction's metadata (which is then available to event processors as `sdkProcessingMetadata`). Then a new default integration, `RequestData`, pulls `req` out of the metadata and uses it to add a `request` property to the event. Notes: - The options API for the new integration is inspired by, but different from, the options API for our Express request handler. (When we work on cleaning up the request data utility functions as part of fixing #5718, we might consider bringing those options in line with these.) The primary differences are: - The options have been (almost entirely) flattened. (It never made sense that inside of options for request data, you had a `request` key holding a subset of the options.) Now everything is at the same level and only takes a boolean. The one exception is `user`, which can still take a boolean or a list of attributes. - In the handler options, `transaction` can either be a boolean or a `TransactionNamingScheme`. In the integration, it can no longer be a boolean - events are going to have transactions, one way or another, so we shouldn't let people set it to `false`. Further, since it's now not about whether `transaction` is or isn't included, it's been moved out of `include` into its own `transactionNamingScheme` option. (Note that the new option is not yet used, but will be in future PRs.) - `method` has also been moved out of include, and has been hardcoded to `true`, since it's an integral part of naming the request. We currently include it in the transaction name, regardless of the setting, so again here, letting people set it to `false` makes no sense. - Though `req` has been added to transaction metadata everywhere we start transactions, the existing domain/`Scope`-based event processors haven't yet been removed, because this new method only works for transactions, not errors. (Solving that will be the subject of a future PR.) The existing processors have been modified to not apply to transaction events, however. - Though we may at some point use the `RequestData` integration added here in browser-based SDKs as well, both in this PR and in near-term future PRs it will only be used in a Node context. It's therefore been added to `@sentry/node`, to prevent a repeat of the dependency injection mess [we just undid](#5759). - No integration tests were added specifically for this change, because the existing integration tests already test that transaction events include a `request` property, so as long as they continue to pass, we know that using the integration instead of an event processor is working. Ref: #5505
1 parent c072b89 commit 5660d9f

File tree

12 files changed

+235
-14
lines changed

12 files changed

+235
-14
lines changed

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

+14-8
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,11 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
106106

107107
requestTransaction = newTransaction;
108108
autoEndTransactionOnResponseEnd(newTransaction, res);
109+
110+
// Link the transaction and the request together, so that when we would normally only have access to one, it's
111+
// still possible to grab the other.
109112
setTransactionOnRequest(newTransaction, req);
113+
newTransaction.setMetadata({ request: req });
110114
}
111115

112116
const dataFetcherSpan = requestTransaction.startChild({
@@ -118,14 +122,16 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
118122
if (currentScope) {
119123
currentScope.setSpan(dataFetcherSpan);
120124
currentScope.addEventProcessor(event =>
121-
addRequestDataToEvent(event, req, {
122-
include: {
123-
// When the `transaction` option is set to true, it tries to extract a transaction name from the request
124-
// object. We don't want this since we already have a high-quality transaction name with a parameterized
125-
// route. Setting `transaction` to `true` will clobber that transaction name.
126-
transaction: false,
127-
},
128-
}),
125+
event.type !== 'transaction'
126+
? addRequestDataToEvent(event, req, {
127+
include: {
128+
// When the `transaction` option is set to true, it tries to extract a transaction name from the request
129+
// object. We don't want this since we already have a high-quality transaction name with a parameterized
130+
// route. Setting `transaction` to `true` will clobber that transaction name.
131+
transaction: false,
132+
},
133+
})
134+
: event,
129135
);
130136
}
131137

packages/nextjs/src/index.server.ts

+3
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,9 @@ function addServerIntegrations(options: NextjsOptions): void {
111111
});
112112
integrations = addOrUpdateIntegration(defaultRewriteFramesIntegration, integrations);
113113

114+
const defaultRequestDataIntegration = new Integrations.RequestData();
115+
integrations = addOrUpdateIntegration(defaultRequestDataIntegration, integrations);
116+
114117
if (hasTracingEnabled(options)) {
115118
const defaultHttpTracingIntegration = new Integrations.Http({ tracing: true });
116119
integrations = addOrUpdateIntegration(defaultHttpTracingIntegration, integrations, {

packages/nextjs/src/utils/instrumentServer.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,9 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
244244
const currentScope = getCurrentHub().getScope();
245245

246246
if (currentScope) {
247-
currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq));
247+
currentScope.addEventProcessor(event =>
248+
event.type !== 'transaction' ? addRequestDataToEvent(event, nextReq) : event,
249+
);
248250

249251
// We only want to record page and API requests
250252
if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) {
@@ -276,6 +278,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
276278
// like `source: isDynamicRoute? 'url' : 'route'`
277279
// TODO: What happens when `withSentry` is used also? Which values of `name` and `source` win?
278280
source: 'url',
281+
request: req,
279282
},
280283
...traceparentData,
281284
},

packages/nextjs/src/utils/withSentry.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,9 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
5656
const currentScope = getCurrentHub().getScope();
5757

5858
if (currentScope) {
59-
currentScope.addEventProcessor(event => addRequestDataToEvent(event, req));
59+
currentScope.addEventProcessor(event =>
60+
event.type !== 'transaction' ? addRequestDataToEvent(event, req) : event,
61+
);
6062

6163
if (hasTracingEnabled()) {
6264
// 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 =
9092
metadata: {
9193
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
9294
source: 'route',
95+
request: req,
9396
},
9497
},
9598
// extra context passed to the `tracesSampler`
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
import * as SentryCore from '@sentry/core';
2+
import * as SentryTracing from '@sentry/tracing';
3+
import { IncomingMessage, ServerResponse } from 'http';
4+
5+
import {
6+
withSentryGetServerSideProps,
7+
withSentryServerSideGetInitialProps,
8+
// TODO: Leaving `withSentryGetStaticProps` out for now until we figure out what to do with it
9+
// withSentryGetStaticProps,
10+
// TODO: Leaving these out for now until we figure out pages with no data fetchers
11+
// withSentryServerSideAppGetInitialProps,
12+
// withSentryServerSideDocumentGetInitialProps,
13+
// withSentryServerSideErrorGetInitialProps,
14+
} from '../../src/config/wrappers';
15+
16+
const startTransactionSpy = jest.spyOn(SentryCore, 'startTransaction');
17+
const setMetadataSpy = jest.spyOn(SentryTracing.Transaction.prototype, 'setMetadata');
18+
19+
describe('data-fetching function wrappers', () => {
20+
const route = '/tricks/[trickName]';
21+
let req: IncomingMessage;
22+
let res: ServerResponse;
23+
24+
describe('starts a transaction and puts request in metadata if tracing enabled', () => {
25+
beforeEach(() => {
26+
req = { headers: {}, url: 'http://dogs.are.great/tricks/kangaroo' } as IncomingMessage;
27+
res = {} as ServerResponse;
28+
29+
jest.spyOn(SentryTracing, 'hasTracingEnabled').mockReturnValueOnce(true);
30+
});
31+
32+
afterEach(() => {
33+
jest.clearAllMocks();
34+
});
35+
36+
test('withSentryGetServerSideProps', async () => {
37+
const origFunction = jest.fn(async () => ({ props: {} }));
38+
39+
const wrappedOriginal = withSentryGetServerSideProps(origFunction, route);
40+
await wrappedOriginal({ req, res } as any);
41+
42+
expect(startTransactionSpy).toHaveBeenCalledWith(
43+
expect.objectContaining({
44+
name: '/tricks/[trickName]',
45+
op: 'nextjs.data.server',
46+
metadata: expect.objectContaining({ source: 'route' }),
47+
}),
48+
);
49+
50+
expect(setMetadataSpy).toHaveBeenCalledWith({ request: req });
51+
});
52+
53+
test('withSentryServerSideGetInitialProps', async () => {
54+
const origFunction = jest.fn(async () => ({}));
55+
56+
const wrappedOriginal = withSentryServerSideGetInitialProps(origFunction);
57+
await wrappedOriginal({ req, res, pathname: route } as any);
58+
59+
expect(startTransactionSpy).toHaveBeenCalledWith(
60+
expect.objectContaining({
61+
name: '/tricks/[trickName]',
62+
op: 'nextjs.data.server',
63+
metadata: expect.objectContaining({ source: 'route' }),
64+
}),
65+
);
66+
67+
expect(setMetadataSpy).toHaveBeenCalledWith({ request: req });
68+
});
69+
});
70+
});

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

+2
Original file line numberDiff line numberDiff line change
@@ -150,8 +150,10 @@ describe('Server init()', () => {
150150

151151
const nodeInitOptions = nodeInit.mock.calls[0][0] as ModifiedInitOptions;
152152
const rewriteFramesIntegration = findIntegrationByName(nodeInitOptions.integrations, 'RewriteFrames');
153+
const requestDataIntegration = findIntegrationByName(nodeInitOptions.integrations, 'RequestData');
153154

154155
expect(rewriteFramesIntegration).toBeDefined();
156+
expect(requestDataIntegration).toBeDefined();
155157
});
156158

157159
it('supports passing unrelated integrations through options', () => {

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ describe('withSentry', () => {
104104
});
105105

106106
describe('tracing', () => {
107-
it('starts a transaction when tracing is enabled', async () => {
107+
it('starts a transaction and sets metadata when tracing is enabled', async () => {
108108
jest
109109
.spyOn(hub.Hub.prototype, 'getClient')
110110
.mockReturnValueOnce({ getOptions: () => ({ tracesSampleRate: 1 } as ClientOptions) } as Client);
@@ -118,6 +118,7 @@ describe('withSentry', () => {
118118

119119
metadata: {
120120
source: 'route',
121+
request: expect.objectContaining({ url: 'http://dogs.are.great' }),
121122
},
122123
},
123124
{ request: expect.objectContaining({ url: 'http://dogs.are.great' }) },

packages/node/src/index.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export type {
1818
} from '@sentry/types';
1919
export type { AddRequestDataToEventOptions } from '@sentry/utils';
2020

21+
export type { TransactionNamingScheme } from './requestdata';
2122
export type { NodeOptions } from './types';
2223

2324
export {
@@ -47,7 +48,7 @@ export {
4748
export { NodeClient } from './client';
4849
export { makeNodeTransport } from './transports';
4950
export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk';
50-
export { addRequestDataToEvent, extractRequestData } from './requestdata';
51+
export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from './requestdata';
5152
export { deepReadDirSync } from './utils';
5253

5354
import { Integrations as CoreIntegrations } from '@sentry/core';

packages/node/src/integrations/index.ts

+1
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@ export { LinkedErrors } from './linkederrors';
66
export { Modules } from './modules';
77
export { ContextLines } from './contextlines';
88
export { Context } from './context';
9+
export { RequestData } from './requestdata';
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
// TODO (v8 or v9): Whenever this becomes a default integration for `@sentry/browser`, move this to `@sentry/core`. For
2+
// now, we leave it in `@sentry/integrations` so that it doesn't contribute bytes to our CDN bundles.
3+
4+
import { EventProcessor, Hub, Integration } from '@sentry/types';
5+
6+
import {
7+
addRequestDataToEvent,
8+
AddRequestDataToEventOptions,
9+
DEFAULT_USER_INCLUDES,
10+
TransactionNamingScheme,
11+
} from '../requestdata';
12+
13+
type RequestDataOptions = {
14+
/**
15+
* Controls what data is pulled from the request and added to the event
16+
*/
17+
include: {
18+
cookies?: boolean;
19+
data?: boolean;
20+
headers?: boolean;
21+
ip?: boolean;
22+
query_string?: boolean;
23+
url?: boolean;
24+
user?: boolean | Array<typeof DEFAULT_USER_INCLUDES[number]>;
25+
};
26+
27+
/** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
28+
transactionNamingScheme: TransactionNamingScheme;
29+
30+
/**
31+
* Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but
32+
* left injectable so this integration can be moved to `@sentry/core` and used in browser-based SDKs in the future.
33+
*
34+
* @hidden
35+
*/
36+
addRequestData: typeof addRequestDataToEvent;
37+
};
38+
39+
const DEFAULT_OPTIONS = {
40+
addRequestData: addRequestDataToEvent,
41+
include: {
42+
cookies: true,
43+
data: true,
44+
headers: true,
45+
ip: false,
46+
query_string: true,
47+
url: true,
48+
user: DEFAULT_USER_INCLUDES,
49+
},
50+
transactionNamingScheme: 'methodpath',
51+
};
52+
53+
/** Add data about a request to an event. Primarily for use in Node-based SDKs, but included in `@sentry/integrations`
54+
* so it can be used in cross-platform SDKs like `@sentry/nextjs`. */
55+
export class RequestData implements Integration {
56+
/**
57+
* @inheritDoc
58+
*/
59+
public static id: string = 'RequestData';
60+
61+
/**
62+
* @inheritDoc
63+
*/
64+
public name: string = RequestData.id;
65+
66+
private _options: RequestDataOptions;
67+
68+
/**
69+
* @inheritDoc
70+
*/
71+
public constructor(options: Partial<RequestDataOptions> = {}) {
72+
this._options = {
73+
...DEFAULT_OPTIONS,
74+
...options,
75+
include: {
76+
// @ts-ignore It's mad because `method` isn't a known `include` key. (It's only here and not set by default in
77+
// `addRequestDataToEvent` for legacy reasons. TODO (v8): Change that.)
78+
method: true,
79+
...DEFAULT_OPTIONS.include,
80+
...options.include,
81+
},
82+
};
83+
}
84+
85+
/**
86+
* @inheritDoc
87+
*/
88+
public setupOnce(addGlobalEventProcessor: (eventProcessor: EventProcessor) => void, getCurrentHub: () => Hub): void {
89+
const { include, addRequestData } = this._options;
90+
91+
addGlobalEventProcessor(event => {
92+
const self = getCurrentHub().getIntegration(RequestData);
93+
const req = event.sdkProcessingMetadata && event.sdkProcessingMetadata.request;
94+
95+
// If the globally installed instance of this integration isn't associated with the current hub, `self` will be
96+
// undefined
97+
if (!self || !req) {
98+
return event;
99+
}
100+
101+
return addRequestData(event, req, { include: formatIncludeOption(include) });
102+
});
103+
}
104+
}
105+
106+
/** Convert `include` option to match what `addRequestDataToEvent` expects */
107+
/** TODO: Can possibly be deleted once https://github.com/getsentry/sentry-javascript/issues/5718 is fixed */
108+
function formatIncludeOption(
109+
integrationInclude: RequestDataOptions['include'] = {},
110+
): AddRequestDataToEventOptions['include'] {
111+
const { ip, user, ...requestOptions } = integrationInclude;
112+
113+
const requestIncludeKeys: string[] = [];
114+
for (const [key, value] of Object.entries(requestOptions)) {
115+
if (value) {
116+
requestIncludeKeys.push(key);
117+
}
118+
}
119+
120+
return {
121+
ip,
122+
user,
123+
request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined,
124+
};
125+
}

packages/node/src/requestdata.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ const DEFAULT_INCLUDES = {
1010
user: true,
1111
};
1212
const DEFAULT_REQUEST_INCLUDES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
13-
const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];
13+
export const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];
1414

1515
/**
1616
* Options deciding what parts of the request to use when enhancing an event
@@ -25,7 +25,7 @@ export interface AddRequestDataToEventOptions {
2525
};
2626
}
2727

28-
type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
28+
export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
2929

3030
/**
3131
* Sets parameterized route as transaction name e.g.: `GET /users/:id`

packages/types/src/transaction.ts

+6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import { DynamicSamplingContext } from './envelope';
22
import { MeasurementUnit } from './measurement';
33
import { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc';
4+
import { PolymorphicRequest } from './polymorphics';
45
import { Span, SpanContext } from './span';
6+
57
/**
68
* Interface holding Transaction-specific properties
79
*/
@@ -145,7 +147,11 @@ export interface TransactionMetadata {
145147
*/
146148
dynamicSamplingContext?: Partial<DynamicSamplingContext>;
147149

150+
/** For transactions tracing server-side request handling, the request being tracked. */
151+
request?: PolymorphicRequest;
152+
148153
/** For transactions tracing server-side request handling, the path of the request being tracked. */
154+
/** TODO: If we rm -rf `instrumentServer`, this can go, too */
149155
requestPath?: string;
150156

151157
/** Information on how a transaction name was generated. */

0 commit comments

Comments
 (0)