diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index b8fce1738171..e4896b163f46 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,8 +1,9 @@ /* eslint-disable max-lines */ -import { captureException, getCurrentHub } from '@sentry/node'; -import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing'; -import { WrappedFunction } from '@sentry/types'; +import { captureException, getCurrentHub, Hub } from '@sentry/node'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; +import { Transaction, WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils'; +import * as domain from 'domain'; import { AppData, @@ -291,9 +292,9 @@ export function startRequestHandlerTransaction( url: URL, method: string, routes: ServerRoute[], + hub: Hub, pkg?: ReactRouterDomPkg, -): Span | undefined { - const hub = getCurrentHub(); +): Transaction { const currentScope = hub.getScope(); const matches = matchServerRoutes(routes, url.pathname, pkg); @@ -311,9 +312,7 @@ export function startRequestHandlerTransaction( }, }); - if (transaction) { - currentScope?.setSpan(transaction); - } + currentScope?.setSpan(transaction); return transaction; } @@ -321,14 +320,20 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui const routes = createRoutes(build.routes); const pkg = loadModule('react-router-dom'); return async function (this: unknown, request: Request, loadContext?: unknown): Promise { - const url = new URL(request.url); + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); - const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + if (!options || !hasTracingEnabled(options)) { + return origRequestHandler.call(this, request, loadContext); + } + + const url = new URL(request.url); + const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); const res = (await origRequestHandler.call(this, request, loadContext)) as Response; - transaction?.setHttpStatus(res.status); - transaction?.finish(); + transaction.setHttpStatus(res.status); + transaction.finish(); return res; }; @@ -416,7 +421,8 @@ function makeWrappedCreateRequestHandler( const newBuild = instrumentBuild(build); const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); - return wrapRequestHandler(requestHandler, newBuild); + const local = domain.create(); + return local.bind(() => wrapRequestHandler(requestHandler, newBuild))(); }; } diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index b67ec5187705..a0111b392529 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,4 +1,9 @@ -import { extractRequestData, loadModule } from '@sentry/utils'; +import { getCurrentHub } from '@sentry/hub'; +import { flush } from '@sentry/node'; +import { hasTracingEnabled } from '@sentry/tracing'; +import { Transaction } from '@sentry/types'; +import { extractRequestData, loadModule, logger } from '@sentry/utils'; +import * as domain from 'domain'; import { createRoutes, @@ -35,20 +40,26 @@ function wrapExpressRequestHandler( res: ExpressResponse, next: ExpressNextFunction, ): Promise { - const request = extractRequestData(req); + // eslint-disable-next-line @typescript-eslint/unbound-method + res.end = wrapEndMethod(res.end); - if (!request.url || !request.method) { - return origRequestHandler.call(this, req, res, next); - } + const local = domain.create(); + local.add(req); + local.add(res); - const url = new URL(request.url); + local.run(async () => { + const request = extractRequestData(req); + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); - const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + if (!options || !hasTracingEnabled(options) || !request.url || !request.method) { + return origRequestHandler.call(this, req, res, next); + } - await origRequestHandler.call(this, req, res, next); - - transaction?.setHttpStatus(res.statusCode); - transaction?.finish(); + const url = new URL(request.url); + startRequestHandlerTransaction(url, request.method, routes, hub, pkg); + await origRequestHandler.call(this, req, res, next); + }); }; } @@ -57,7 +68,9 @@ function wrapExpressRequestHandler( */ export function wrapExpressCreateRequestHandler( origCreateRequestHandler: ExpressCreateRequestHandler, + // eslint-disable-next-line @typescript-eslint/no-explicit-any ): (options: any) => ExpressRequestHandler { + // eslint-disable-next-line @typescript-eslint/no-explicit-any return function (this: unknown, options: any): ExpressRequestHandler { const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build); const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild }); @@ -65,3 +78,63 @@ export function wrapExpressCreateRequestHandler( return wrapExpressRequestHandler(requestHandler, newBuild); }; } + +export type AugmentedExpressResponse = ExpressResponse & { + __sentryTransaction?: Transaction; +}; + +type ResponseEndMethod = AugmentedExpressResponse['end']; +type WrappedResponseEndMethod = AugmentedExpressResponse['end']; + +/** + * Wrap `res.end()` so that it closes the transaction and flushes events before letting the request finish. + * + * Note: This wraps a sync method with an async method. While in general that's not a great idea in terms of keeping + * things in the right order, in this case it's safe, because the native `.end()` actually *is* async, and its run + * actually *is* awaited, just manually so (which reflects the fact that the core of the request/response code in Node + * by far predates the introduction of `async`/`await`). When `.end()` is done, it emits the `prefinish` event, and + * only once that fires does request processing continue. See + * https://github.com/nodejs/node/commit/7c9b607048f13741173d397795bac37707405ba7. + * + * @param origEnd The original `res.end()` method + * @returns The wrapped version + */ +function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { + return async function newEnd(this: AugmentedExpressResponse, ...args: unknown[]) { + await finishSentryProcessing(this); + + return origEnd.call(this, ...args); + }; +} + +/** + * Close the open transaction (if any) and flush events to Sentry. + * + * @param res The outgoing response for this request, on which the transaction is stored + */ +async function finishSentryProcessing(res: AugmentedExpressResponse): Promise { + const { __sentryTransaction: transaction } = res; + + if (transaction) { + transaction.setHttpStatus(res.statusCode); + + // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the + // transaction closes, and make sure to wait until that's done before flushing events + await new Promise(resolve => { + setImmediate(() => { + transaction.finish(); + resolve(); + }); + }); + } + + // Flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda + // ends. If there was an error, rethrow it so that the normal exception-handling mechanisms can apply. + try { + __DEBUG_BUILD__ && logger.log('Flushing events...'); + await flush(2000); + __DEBUG_BUILD__ && logger.log('Done flushing events'); + } catch (e) { + __DEBUG_BUILD__ && logger.log('Error while flushing events:\n', e); + } +} diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx new file mode 100644 index 000000000000..5b6aba13b0c9 --- /dev/null +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -0,0 +1,18 @@ +import { json, LoaderFunction } from '@remix-run/node'; + +import * as Sentry from '@sentry/remix'; + +export const loader: LoaderFunction = async ({ params: { id } }) => { + const timeTil = parseInt(id || '', 10) * 1000; + await new Promise(resolve => setTimeout(resolve, 3000 - timeTil)); + Sentry.setTag(`tag${id}`, id); + return json({ test: 'test' }); +}; + +export default function ScopeBleed() { + return ( +
+

Hello

+
+ ); +} diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index f5e3adb58c26..39ffead32272 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -14,13 +14,12 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const config = await runServer(adapter); const url = `${config.url}/loader-json-response/-2`; - let [transaction, event] = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 }); + const envelopes = await getMultipleEnvelopeRequest({ ...config, url }, { count: 2 }); - // The event envelope is returned before the transaction envelope when using express adapter. - // We can update this when we merge the envelope filtering utility. - adapter === 'express' && ([event, transaction] = [transaction, event]); + const event = envelopes[0][2].type === 'transaction' ? envelopes[1][2] : envelopes[0][2]; + const transaction = envelopes[0][2].type === 'transaction' ? envelopes[0][2] : envelopes[1][2]; - assertSentryTransaction(transaction[2], { + assertSentryTransaction(transaction, { contexts: { trace: { status: 'internal_error', @@ -31,7 +30,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }); - assertSentryEvent(event[2], { + assertSentryEvent(event, { exception: { values: [ { @@ -136,4 +135,39 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }, }); }); + + it('makes sure scope does not bleed between requests', async () => { + const { url, server, scope } = await runServer(adapter); + + const envelopes = await Promise.all([ + getEnvelopeRequest({ url: `${url}/scope-bleed/1`, server, scope }, { endServer: false }), + getEnvelopeRequest({ url: `${url}/scope-bleed/2`, server, scope }, { endServer: false }), + getEnvelopeRequest({ url: `${url}/scope-bleed/3`, server, scope }, { endServer: false }), + getEnvelopeRequest({ url: `${url}/scope-bleed/4`, server, scope }, { endServer: false }), + ]); + + scope.persist(false); + await new Promise(resolve => server.close(resolve)); + + assertSentryTransaction(envelopes[0][2], { + tags: { + tag4: '4', + }, + }); + assertSentryTransaction(envelopes[1][2], { + tags: { + tag3: '3', + }, + }); + assertSentryTransaction(envelopes[2][2], { + tags: { + tag2: '2', + }, + }); + assertSentryTransaction(envelopes[3][2], { + tags: { + tag1: '1', + }, + }); + }); });