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.

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

feat(node): Replace usages of domain run and bind with runWithHub #7745

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 3 additions & 3 deletions packages/nextjs/src/server/utils/wrapperUtils.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { captureException, getActiveTransaction, getCurrentHub, startTransaction } from '@sentry/core';
import { runWithHub } from '@sentry/node';
import type { Transaction } from '@sentry/types';
import { baggageHeaderToDynamicSamplingContext, extractTraceparentData } from '@sentry/utils';
import * as domain from 'domain';
import type { IncomingMessage, ServerResponse } from 'http';

import { platformSupportsStreaming } from './platformSupportsStreaming';
Expand Down Expand Up @@ -75,7 +75,7 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
},
): (...params: Parameters<F>) => Promise<ReturnType<F>> {
return async function (this: unknown, ...args: Parameters<F>): Promise<ReturnType<F>> {
return domain.create().bind(async () => {
return runWithHub(async () => {
let requestTransaction: Transaction | undefined = getTransactionFromRequest(req);
let dataFetcherSpan;

Expand Down Expand Up @@ -154,7 +154,7 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
await flushQueue();
}
}
})();
});
};
}

Expand Down
12 changes: 3 additions & 9 deletions packages/nextjs/src/server/wrapApiHandlerWithSentry.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { hasTracingEnabled } from '@sentry/core';
import { captureException, getCurrentHub, startTransaction } from '@sentry/node';
import { captureException, getCurrentHub, runWithHub, startTransaction } from '@sentry/node';
import type { Transaction } from '@sentry/types';
import {
addExceptionMechanism,
Expand All @@ -10,7 +10,6 @@ import {
objectify,
stripUrlQueryAndFragment,
} from '@sentry/utils';
import * as domain from 'domain';

import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from './types';
import { platformSupportsStreaming } from './utils/platformSupportsStreaming';
Expand Down Expand Up @@ -61,16 +60,11 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri
}
req.__withSentry_applied__ = true;

// use a domain in order to prevent scope bleed between requests
const local = domain.create();
local.add(req);
local.add(res);

// `local.bind` causes everything to run inside a domain, just like `local.run` does, but it also lets the callback
// return a value. In our case, all any of the codepaths return is a promise of `void`, but nextjs still counts on
// getting that before it will finish the response.
// eslint-disable-next-line complexity
const boundHandler = local.bind(async () => {
const boundHandler = runWithHub(async () => {
let transaction: Transaction | undefined;
const hub = getCurrentHub();
const currentScope = hub.getScope();
Expand Down Expand Up @@ -213,7 +207,7 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri

// Since API route handlers are all async, nextjs always awaits the return value (meaning it's fine for us to return
// a promise here rather than a real result, and it saves us the overhead of an `await` call.)
return boundHandler();
return boundHandler;
},
});
}
6 changes: 3 additions & 3 deletions packages/nextjs/src/server/wrapServerComponentWithSentry.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addTracingExtensions, captureException, getCurrentHub, startTransaction } from '@sentry/core';
import { runWithHub } from '@sentry/node';
import { baggageHeaderToDynamicSamplingContext, extractTraceparentData } from '@sentry/utils';
import * as domain from 'domain';

import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils';
import type { ServerComponentContext } from '../common/types';
Expand All @@ -21,7 +21,7 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any>
// hook. 🤯
return new Proxy(appDirComponent, {
apply: (originalFunction, thisArg, args) => {
return domain.create().bind(() => {
return runWithHub(() => {
let maybePromiseResult;

const traceparentData = context.sentryTraceHeader
Expand Down Expand Up @@ -85,7 +85,7 @@ export function wrapServerComponentWithSentry<F extends (...args: any[]) => any>
transaction.finish();
return maybePromiseResult;
}
})();
});
},
});
}
70 changes: 34 additions & 36 deletions packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ import {
logger,
normalize,
} from '@sentry/utils';
import * as domain from 'domain';
import type * as http from 'http';

import type { NodeClient } from './client';
import { extractRequestData } from './requestdata';
// TODO (v8 / XXX) Remove this import
import type { ParseRequestOptions } from './requestDataDeprecated';
import { flush, isAutoSessionTrackingEnabled } from './sdk';
import { runWithHub } from './utils';

/**
* Express-compatible tracing handler.
Expand Down Expand Up @@ -181,46 +181,44 @@ export function requestHandler(
});
};
}
const local = domain.create();
local.add(req);
local.add(res);

local.run(() => {
const currentHub = getCurrentHub();

currentHub.configureScope(scope => {
scope.setSDKProcessingMetadata({
request: req,
// TODO (v8): Stop passing this
requestDataOptionsFromExpressHandler: requestDataOptions,
});
runWithHub(
currentHub => {
currentHub.configureScope(scope => {
scope.setSDKProcessingMetadata({
request: req,
// TODO (v8): Stop passing this
requestDataOptionsFromExpressHandler: requestDataOptions,
});

const client = currentHub.getClient<NodeClient>();
if (isAutoSessionTrackingEnabled(client)) {
const scope = currentHub.getScope();
if (scope) {
// Set `status` of `RequestSession` to Ok, at the beginning of the request
scope.setRequestSession({ status: 'ok' });
const client = currentHub.getClient<NodeClient>();
if (isAutoSessionTrackingEnabled(client)) {
const scope = currentHub.getScope();
if (scope) {
// Set `status` of `RequestSession` to Ok, at the beginning of the request
scope.setRequestSession({ status: 'ok' });
}
}
}
});
});

res.once('finish', () => {
const client = currentHub.getClient<NodeClient>();
if (isAutoSessionTrackingEnabled(client)) {
setImmediate(() => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
if (client && (client as any)._captureRequestSession) {
// Calling _captureRequestSession to capture request session at the end of the request by incrementing
// the correct SessionAggregates bucket i.e. crashed, errored or exited
res.once('finish', () => {
const client = currentHub.getClient<NodeClient>();
if (isAutoSessionTrackingEnabled(client)) {
setImmediate(() => {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(client as any)._captureRequestSession();
}
});
}
});
next();
});
if (client && (client as any)._captureRequestSession) {
// Calling _captureRequestSession to capture request session at the end of the request by incrementing
// the correct SessionAggregates bucket i.e. crashed, errored or exited
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
(client as any)._captureRequestSession();
}
});
}
});
next();
},
[req, res],
);
};
}

Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export { NodeClient } from './client';
export { makeNodeTransport } from './transports';
export { defaultIntegrations, init, defaultStackParser, lastEventId, flush, close, getSentryRelease } from './sdk';
export { addRequestDataToEvent, DEFAULT_USER_INCLUDES, extractRequestData } from './requestdata';
export { deepReadDirSync } from './utils';
export { deepReadDirSync, runWithHub } from './utils';

import { getMainCarrier, Integrations as CoreIntegrations } from '@sentry/core';
import * as domain from 'domain';
Expand Down
20 changes: 20 additions & 0 deletions packages/node/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
import type { Hub } from '@sentry/core';
import { getCurrentHub } from '@sentry/core';
import * as domain from 'domain';
import type { EventEmitter } from 'events';
import * as fs from 'fs';
import * as path from 'path';

Expand Down Expand Up @@ -36,3 +40,19 @@ export function deepReadDirSync(targetDir: string): string[] {

return deepReadCurrentDir(targetDirAbsPath).map(absPath => path.relative(targetDirAbsPath, absPath));
}

/**
* Runs a callback in it's own domain and passes it the hub.
*/
export function runWithHub<T>(callback: (hub: Hub) => T, emitters: EventEmitter[] = []): T {
const local = domain.create();

for (const emitter of emitters) {
local.add(emitter);
}

return local.bind(() => {
const hub = getCurrentHub();
return callback(hub);
})();
}
9 changes: 3 additions & 6 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* eslint-disable max-lines */
import { getActiveTransaction, hasTracingEnabled } from '@sentry/core';
import type { Hub } from '@sentry/node';
import { captureException, getCurrentHub } from '@sentry/node';
import { captureException, getCurrentHub, runWithHub } from '@sentry/node';
import type { Transaction, TransactionSource, WrappedFunction } from '@sentry/types';
import {
addExceptionMechanism,
Expand All @@ -13,7 +13,6 @@ import {
loadModule,
logger,
} from '@sentry/utils';
import * as domain from 'domain';

import type {
AppData,
Expand Down Expand Up @@ -314,13 +313,11 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');

return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise<Response> {
const local = domain.create();

// Waiting for the next tick to make sure that the domain is active
// https://github.com/nodejs/node/issues/40999#issuecomment-1002719169
await new Promise(resolve => setImmediate(resolve));

return local.bind(async () => {
return runWithHub(async () => {
const hub = getCurrentHub();
const options = hub.getClient()?.getOptions();
const scope = hub.getScope();
Expand Down Expand Up @@ -365,7 +362,7 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
transaction.finish();

return res;
})();
});
};
}

Expand Down
7 changes: 3 additions & 4 deletions packages/sveltekit/src/server/handle.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */
import type { Span } from '@sentry/core';
import { getActiveTransaction, getCurrentHub, trace } from '@sentry/core';
import { captureException } from '@sentry/node';
import { captureException, runWithHub } from '@sentry/node';
import { addExceptionMechanism, dynamicSamplingContextToSentryBaggageHeader, objectify } from '@sentry/utils';
import type { Handle, ResolveOptions } from '@sveltejs/kit';
import * as domain from 'domain';

import { getTracePropagationData } from './utils';

Expand Down Expand Up @@ -67,9 +66,9 @@ export const sentryHandle: Handle = input => {
if (getCurrentHub().getScope().getSpan()) {
return instrumentHandle(input);
}
return domain.create().bind(() => {
return runWithHub(() => {
return instrumentHandle(input);
})();
});
};

function instrumentHandle({ event, resolve }: Parameters<Handle>[0]): ReturnType<Handle> {
Expand Down