Skip to content

Commit

Permalink
ref(node): Move non-handler code out of handlers module (#5190)
Browse files Browse the repository at this point in the history
Currently, the `handlers` module includes not just our middleware handlers but also a bunch of code for extracting context data and applying it to events. That latter code is indeed used in the handlers, but it's also used in a number of other places in the repo. This pulls that code into its own module in `@sentry/utils`, to make it possible to use in a platform-agnostic context.

Key changes:

-  A new `requestData` module in `@sentry/utils` to hold the data extraction code (and a new test file for its tests).
- Wrapper functions in the `handlers` module for backwards compatibility. (IOW, calling `Handlers.someFunc()` still works, but it's now just a passthrough to `someFunc` from the new module.)
- Deprecation notices in the docstrings of the wrappers, and TODOs so we remember to make the switch in v8.
- Wrapper `describe`s in the tests, to run the tests against both the real functions and their wrappers, to make sure the wrappers work identically to the real functions. (This means that for the moment, the test file needs to live in `@sentry/node`, since it depends on the wrappers as well as the real functions. Once the wrappers are removed, the test file can be moved to `@sentry/utils`.)
- `parseRequest` has been renamed `addRequestDataToEvent` for greater accuracy and clarity for the reader. (The corresponding options type has also been renamed.)
- Two pieces of context data which have until now been set by `parseRequest`, but which have nothing to do with the request and are Node-only (namely `runtime` and `server_name`) are now set by the Node client. As a result, they no longer need to be set separately in the serverless package. (The exception is `server_name` in the AWS integration, because it pulls from a non-standard location.)

Note: The original impetus for this change is the need to call `parseRequest`/`addRequestDataToEvent` in the nextjs SDK, in code which can run either in Node or the browser, which means it can't be exported from `@sentry/node`.
  • Loading branch information
lobsterkatie committed Jun 4, 2022
1 parent d48f6fd commit 7c521e3
Show file tree
Hide file tree
Showing 17 changed files with 843 additions and 718 deletions.
6 changes: 2 additions & 4 deletions packages/nextjs/src/utils/instrumentServer.ts
@@ -1,10 +1,10 @@
/* eslint-disable max-lines */
import {
addRequestDataToEvent,
captureException,
configureScope,
deepReadDirSync,
getCurrentHub,
Handlers,
startTransaction,
} from '@sentry/node';
import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
Expand All @@ -22,8 +22,6 @@ import { default as createNextServer } from 'next';
import * as querystring from 'querystring';
import * as url from 'url';

const { parseRequest } = Handlers;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type PlainObject<T = any> = { [key: string]: T };

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

if (currentScope) {
currentScope.addEventProcessor(event => parseRequest(event, nextReq));
currentScope.addEventProcessor(event => addRequestDataToEvent(event, nextReq));

// We only want to record page and API requests
if (hasTracingEnabled() && shouldTraceRequest(nextReq.url, publicDirFiles)) {
Expand Down
6 changes: 2 additions & 4 deletions packages/nextjs/src/utils/withSentry.ts
@@ -1,4 +1,4 @@
import { captureException, flush, getCurrentHub, Handlers, startTransaction } from '@sentry/node';
import { addRequestDataToEvent, captureException, flush, getCurrentHub, startTransaction } from '@sentry/node';
import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing';
import { Transaction } from '@sentry/types';
import {
Expand All @@ -12,8 +12,6 @@ import {
import * as domain from 'domain';
import { NextApiHandler, NextApiRequest, NextApiResponse } from 'next';

const { parseRequest } = Handlers;

// This is the same as the `NextApiHandler` type, except instead of having a return type of `void | Promise<void>`, it's
// only `Promise<void>`, because wrapped handlers are always async
export type WrappedNextApiHandler = (req: NextApiRequest, res: NextApiResponse) => Promise<void>;
Expand Down Expand Up @@ -43,7 +41,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
const currentScope = getCurrentHub().getScope();

if (currentScope) {
currentScope.addEventProcessor(event => parseRequest(event, req));
currentScope.addEventProcessor(event => addRequestDataToEvent(event, req));

if (hasTracingEnabled()) {
// If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
Expand Down
12 changes: 9 additions & 3 deletions packages/node/src/client.ts
Expand Up @@ -2,6 +2,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core';
import { SessionFlusher } from '@sentry/hub';
import { Event, EventHint, Severity, SeverityLevel } from '@sentry/types';
import { logger, resolvedSyncPromise } from '@sentry/utils';
import * as os from 'os';
import { TextEncoder } from 'util';

import { eventFromMessage, eventFromUnknownInput } from './eventbuilder';
Expand Down Expand Up @@ -139,9 +140,14 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
*/
protected _prepareEvent(event: Event, hint: EventHint, scope?: Scope): PromiseLike<Event | null> {
event.platform = event.platform || 'node';
if (this.getOptions().serverName) {
event.server_name = this.getOptions().serverName;
}
event.contexts = {
...event.contexts,
runtime: {
name: 'node',
version: global.process.version,
},
};
event.server_name = this.getOptions().serverName || global.process.env.SENTRY_NAME || os.hostname();
return super._prepareEvent(event, hint, scope);
}

Expand Down

0 comments on commit 7c521e3

Please sign in to comment.