Skip to content

Commit d69cafc

Browse files
authored
ref(node): Move request-data-extraction functions to@sentry/utils (#5257)
(See #5190 for a detailed description of the motivation for and main substance of this change. TL;DR, isomorphic frameworks like Nextjs need this code to live in a neutral location (rather than in the node package) so it can be used in a browser setting as well.) This is a second take on #5190, which had to be reverted because it relied on a peer dependency (`cookie`) which browser-only apps wouldn't have installed. Even if those code didn't _use_ `cookie`, they still failed to compile because without `cookie` installed, `@sentry/utils` didn't typecheck correctly. This fixes that problem by using `cookie` (and `url`, for node 8 compatibility) only as injected dependencies, not as direct ones. It also creates node-specific versions of the relevant functions (`parseRequest`, now renamed `addRequestDataToEvent`, and `extractRequestData`) which do the injection automatically. If the dependencies aren't passed (as would be the case in a browser setting, or when using the functions directly from `@sentry/utils`), the code about cookies no-ops, and the code about URLs uses `URL`, which should be defined in all modern browsers and Node 10 and above. Other changes from the original PR: - Now only the backwards-compatibility-preserving wrappers of the legacy code live in `handlers.ts`, while the legacy code itself lives in its own file. This both makes it easier to delete in the future and ensures that treeshaking algorithms which only work file-by-file (rather than function-by-function) are able to exclude that code. (Though it's being kept until v8 because it's part of the node package's public API, it's no longer used anywhere in the SDK.) - Using DI changed the options interface for both of the functions in question, which in turn required a bit more gymnastics in terms of preserving and test backwards compatibility, specifically in the serverless package and the backwards-compatibility tests. This will make it a little harder to delete the old stuff when it comes time, but there are TODOs which hopefully will make it clear enough what needs to happen. - There's a new `CrossPlatformRequest` type for use in place of `ExpressRequest`. A few helper functions have also been renamed to make them less Express-centric.
1 parent 7bd7170 commit d69cafc

20 files changed

+1202
-745
lines changed

packages/nextjs/src/utils/instrumentServer.ts

+2-4
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
/* eslint-disable max-lines */
22
import {
3+
addRequestDataToEvent,
34
captureException,
45
configureScope,
56
deepReadDirSync,
67
getCurrentHub,
7-
Handlers,
88
startTransaction,
99
} from '@sentry/node';
1010
import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
@@ -22,8 +22,6 @@ import { default as createNextServer } from 'next';
2222
import * as querystring from 'querystring';
2323
import * as url from 'url';
2424

25-
const { parseRequest } = Handlers;
26-
2725
// eslint-disable-next-line @typescript-eslint/no-explicit-any
2826
type PlainObject<T = any> = { [key: string]: T };
2927

@@ -246,7 +244,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
246244
const currentScope = getCurrentHub().getScope();
247245

248246
if (currentScope) {
249-
currentScope.addEventProcessor(event => parseRequest(event, nextReq));
247+
currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq));
250248

251249
// We only want to record page and API requests
252250
if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) {

packages/nextjs/src/utils/withSentry.ts

+2-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node';
1+
import { addRequestDataToEvent, captureException, flush, getCurrentHub, startTransaction } from '@sentry/node';
22
import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing';
33
import { Transaction } from '@sentry/types';
44
import {
@@ -12,8 +12,6 @@ import {
1212
import * as domain from 'domain';
1313
import { NextApiHandler, NextApiRequest, NextApiResponse } from 'next';
1414

15-
const { parseRequest } = Handlers;
16-
1715
// This is the same as the `NextApiHandler` type, except instead of having a return type of `void | Promise<void>`, it's
1816
// only `Promise<void>`, because wrapped handlers are always async
1917
export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise<void>;
@@ -43,7 +41,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
4341
const currentScope = getCurrentHub().getScope();
4442

4543
if (currentScope) {
46-
currentScope.addEventProcessor(event => parseRequest(event, req));
44+
currentScope.addEventProcessor(event => addRequestDataToEvent(event, req));
4745

4846
if (hasTracingEnabled()) {
4947
// If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)

packages/node/src/client.ts

+10-3
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core';
22
import { SessionFlusher } from '@sentry/hub';
33
import { Event, EventHint, Severity, SeverityLevel } from '@sentry/types';
44
import { logger, resolvedSyncPromise } from '@sentry/utils';
5+
import * as os from 'os';
56
import { TextEncoder } from 'util';
67

78
import { eventFromMessage, eventFromUnknownInput } from './eventbuilder';
@@ -139,9 +140,15 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
139140
*/
140141
protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike<Event | null> {
141142
event.platform = event.platform || 'node';
142-
if (this.getOptions().serverName) {
143-
event.server_name = this.getOptions().serverName;
144-
}
143+
event.contexts = {
144+
...event.contexts,
145+
runtime: event.contexts?.runtime || {
146+
name: 'node',
147+
version: global.process.version,
148+
},
149+
};
150+
event.server_name =
151+
event.server_name || this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname();
145152
return super._prepareEvent(event, hint, scope);
146153
}
147154

0 commit comments

Comments
 (0)