Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(node): Move non-handler code out of handlers module #5190

Merged
merged 4 commits into from
Jun 4, 2022
Merged

ref(node): Move non-handler code out of handlers module #5190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
663d2cd
break up handlers module and fix imports
lobsterkatie Jun 1, 2022
3dbd1c0
s/parseRequest/addRequestDataToEvent
lobsterkatie Jun 1, 2022
405b0c0
move node-version- and server-name-adding code to client
lobsterkatie Jun 1, 2022
31c6675
fix tests
lobsterkatie Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions packages/nextjs/src/utils/instrumentServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
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();
Copy link
Member

@AbhiPrasad AbhiPrasad Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about putting this event mutation into a default integration instead of in the prepareEvent step here?

This is the pattern most of the other SDKs follow for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. I looked but didn't actually find any examples where we're using an integration to just set a few pieces of metadata. Which ones were you thinking of?

TBH, my gut feeling on it is that it feels kinda like overkill - it's a lot of machinery to wrap two lines of code, no? (Also, I know bundle size isn't as much of a thing on the node side of things, but given serverless functions it's still something to be at least vaguely aware of, so I'd only want to do this if it seemed worth the extra code.)

Let's wait and see if anyone complains, and then if we need to pull it into an integration so that people can shut it off, we can do that.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that sounds fine to me!

return super._prepareEvent(event, hint, scope);
}

Expand Down
Loading