From 411c60755840a2fd3361d6df1d05a166c66e8f3b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 11 Aug 2022 14:41:08 -0400 Subject: [PATCH 01/20] add route id to remix spans --- packages/remix/src/utils/instrumentServer.ts | 26 +++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index fec537d65a56..cc1aa23303ed 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -203,7 +203,7 @@ function makeWrappedDocumentRequestFunction( }; } -function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader'): DataFunction { +function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action' | 'loader'): DataFunction { return async function (this: unknown, args: DataFunctionArgs): Promise { let res: Response | AppData; const activeTransaction = getActiveTransaction(); @@ -216,8 +216,7 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' try { const span = activeTransaction.startChild({ op: `remix.server.${name}`, - // TODO: Consider using the `id` of the route this function belongs to - description: activeTransaction.name, + description: id, tags: { name, }, @@ -241,13 +240,17 @@ function makeWrappedDataFunction(origFn: DataFunction, name: 'action' | 'loader' }; } -function makeWrappedAction(origAction: DataFunction): DataFunction { - return makeWrappedDataFunction(origAction, 'action'); -} +const makeWrappedAction = + (id: string) => + (origAction: DataFunction): DataFunction => { + return makeWrappedDataFunction(origAction, id, 'action'); + }; -function makeWrappedLoader(origLoader: DataFunction): DataFunction { - return makeWrappedDataFunction(origLoader, 'loader'); -} +const makeWrappedLoader = + (id: string) => + (origLoader: DataFunction): DataFunction => { + return makeWrappedDataFunction(origLoader, id, 'loader'); + }; function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } { const transaction = getActiveTransaction(); @@ -424,14 +427,15 @@ function makeWrappedCreateRequestHandler( fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction); for (const [id, route] of Object.entries(build.routes)) { + console.log(id, route); const wrappedRoute = { ...route, module: { ...route.module } }; if (wrappedRoute.module.action) { - fill(wrappedRoute.module, 'action', makeWrappedAction); + fill(wrappedRoute.module, 'action', makeWrappedAction(id)); } if (wrappedRoute.module.loader) { - fill(wrappedRoute.module, 'loader', makeWrappedLoader); + fill(wrappedRoute.module, 'loader', makeWrappedLoader(id)); } // Entry module should have a loader function to provide `sentry-trace` and `baggage` From 9b2e0aa0bffc9a34b2b11e0dcedb1f1e9bedce23 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 11 Aug 2022 15:04:19 -0400 Subject: [PATCH 02/20] fix tests --- packages/remix/src/utils/instrumentServer.ts | 1 - packages/remix/test/integration/test/server/action.test.ts | 5 +---- packages/remix/test/integration/test/server/loader.test.ts | 5 +---- 3 files changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index cc1aa23303ed..9ff16e2f716e 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -427,7 +427,6 @@ function makeWrappedCreateRequestHandler( fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction); for (const [id, route] of Object.entries(build.routes)) { - console.log(id, route); const wrappedRoute = { ...route, module: { ...route.module } }; if (wrappedRoute.module.action) { diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index 3a27251e2bdc..af350e8cb7ad 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -22,11 +22,8 @@ describe('Remix API Actions', () => { description: 'routes/action-json-response/$id', op: 'remix.server.action', }, - // TODO: These two spans look exactly the same, but they are not. - // One is from the parent route, and the other is from the route we are reaching. - // We need to pass the names of the routes as their descriptions while wrapping loaders and actions. { - description: 'routes/action-json-response/$id', + description: 'root', op: 'remix.server.loader', }, { diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 2d79a0601333..267658debbfb 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -58,11 +58,8 @@ describe('Remix API Loaders', () => { source: 'route', }, spans: [ - // TODO: These two spans look exactly the same, but they are not. - // One is from the parent route, and the other is from the route we are reaching. - // We need to pass the names of the routes as their descriptions while wrapping loaders and actions. { - description: 'routes/loader-json-response/$id', + description: 'root', op: 'remix.server.loader', }, { From 77b682cb96659b270371765802185cdbbbed033b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 11 Aug 2022 16:04:07 -0400 Subject: [PATCH 03/20] fix(remix): Use domains to prevent scope bleed Create a domain and manually push and pop the domain stack to isolate requests. This helps prevent scope bleed issues between transactions. --- packages/remix/src/utils/instrumentServer.ts | 59 +++++++++++--------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 9ff16e2f716e..e5415a7c6e44 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -2,6 +2,7 @@ import { captureException, getCurrentHub } from '@sentry/node'; import { getActiveTransaction, hasTracingEnabled } from '@sentry/tracing'; import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils'; +import * as domain from 'domain'; // Types vendored from @remix-run/server-runtime@1.6.0: // https://github.com/remix-run/remix/blob/f3691d51027b93caa3fd2cdfe146d7b62a6eb8f2/packages/remix-server-runtime/server.ts @@ -356,36 +357,42 @@ 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 hub = getCurrentHub(); - const currentScope = hub.getScope(); - - const url = new URL(request.url); - const matches = matchServerRoutes(routes, url.pathname, pkg); - - const match = matches && getRequestMatch(url, matches); - const name = match === null ? url.pathname : match.route.id; - const source = match === null ? 'url' : 'route'; - const transaction = hub.startTransaction({ - name, - op: 'http.server', - tags: { - method: request.method, - }, - metadata: { - source, - }, - }); + const local = domain.create(); + local.enter(); + try { + const hub = getCurrentHub(); + const currentScope = hub.getScope(); + + const url = new URL(request.url); + const matches = matchServerRoutes(routes, url.pathname, pkg); + + const match = matches && getRequestMatch(url, matches); + const name = match === null ? url.pathname : match.route.id; + const source = match === null ? 'url' : 'route'; + const transaction = hub.startTransaction({ + name, + op: 'http.server', + tags: { + method: request.method, + }, + metadata: { + source, + }, + }); - if (transaction) { - currentScope?.setSpan(transaction); - } + if (transaction) { + currentScope?.setSpan(transaction); + } - const res = (await origRequestHandler.call(this, request, loadContext)) as Response; + const res = (await origRequestHandler.call(this, request, loadContext)) as Response; - transaction?.setHttpStatus(res.status); - transaction?.finish(); + transaction?.setHttpStatus(res.status); + transaction?.finish(); - return res; + return res; + } finally { + local.exit(); + } }; } From 760b833cd07075b9c69569366f53555b753efdfb Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 11 Aug 2022 16:38:06 -0400 Subject: [PATCH 04/20] add scope bleed test --- .../integration/app/routes/scope-bleed/$id.tsx | 18 ++++++++++++++++++ .../integration/test/server/loader.test.ts | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 packages/remix/test/integration/app/routes/scope-bleed/$id.tsx 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..7ce284fe86da --- /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 { useActionData } from '@remix-run/react'; + +import * as Sentry from '@sentry/remix'; + +export const loader: LoaderFunction = async ({ params: { id } }) => { + Sentry.setTag(`tag${id}`, id); + + return json({ test: 'test' }); +}; + +export default function ActionJSONResponse() { + 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 267658debbfb..7f5810156e9d 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -131,4 +131,22 @@ describe('Remix API Loaders', () => { }, }); }); + + it('makes sure scope does not bleed between requests', async () => { + const baseURL = await runServer(); + + await Promise.all( + Array.from(Array(10).keys()).map(async (id: number) => { + const url = `${baseURL}/scope-bleed/${id}`; + const envelope = await getEnvelopeRequest(url); + const transaction = envelope[2]; + + assertSentryTransaction(transaction, { + tags: { + [`tag${id}`]: String(id), + }, + }); + }), + ); + }); }); From 0b4eef67122418a5602005ccd605c6a8df9435ee Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 11 Aug 2022 17:18:09 -0400 Subject: [PATCH 05/20] switch to use domain.bind --- packages/remix/src/utils/instrumentServer.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index e5415a7c6e44..786cef15f5b2 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -356,10 +356,10 @@ function matchServerRoutes( function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler { const routes = createRoutes(build.routes); const pkg = loadModule('react-router-dom'); + return async function (this: unknown, request: Request, loadContext?: unknown): Promise { const local = domain.create(); - local.enter(); - try { + return local.bind(async () => { const hub = getCurrentHub(); const currentScope = hub.getScope(); @@ -390,9 +390,7 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui transaction?.finish(); return res; - } finally { - local.exit(); - } + })(); }; } From d4b8f64e55257a3d2c7e26acdbd2c1251a276918 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 12 Aug 2022 11:10:50 -0400 Subject: [PATCH 06/20] Update packages/remix/test/integration/app/routes/scope-bleed/$id.tsx Co-authored-by: Onur Temizkan --- packages/remix/test/integration/app/routes/scope-bleed/$id.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 7ce284fe86da..d43ea7655f44 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -1,5 +1,4 @@ import { json, LoaderFunction } from '@remix-run/node'; -import { useActionData } from '@remix-run/react'; import * as Sentry from '@sentry/remix'; From db365af88f30e7674ce0ca993cc1d25bc00d53dc Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 12 Aug 2022 11:10:54 -0400 Subject: [PATCH 07/20] Update packages/remix/test/integration/app/routes/scope-bleed/$id.tsx Co-authored-by: Onur Temizkan --- packages/remix/test/integration/app/routes/scope-bleed/$id.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index d43ea7655f44..0c53c37e415d 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -8,7 +8,7 @@ export const loader: LoaderFunction = async ({ params: { id } }) => { return json({ test: 'test' }); }; -export default function ActionJSONResponse() { +export default function ScopeBleed() { return (

Hello

From f7e4a894fb5e4359460dbb3bb19282e598629785 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 16:29:08 -0400 Subject: [PATCH 08/20] domain on express as well --- packages/remix/src/utils/instrumentServer.ts | 23 +----------------- .../remix/src/utils/serverAdapters/express.ts | 24 ++++++++++++------- 2 files changed, 16 insertions(+), 31 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 643af16498e9..49f05c3fca0f 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -321,33 +321,12 @@ export function startRequestHandlerTransaction( function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBuild): RequestHandler { const routes = createRoutes(build.routes); const pkg = loadModule('react-router-dom'); - return async function (this: unknown, request: Request, loadContext?: unknown): Promise { const local = domain.create(); return local.bind(async () => { - const hub = getCurrentHub(); - const currentScope = hub.getScope(); - const url = new URL(request.url); - const matches = matchServerRoutes(routes, url.pathname, pkg); - - const match = matches && getRequestMatch(url, matches); - const name = match === null ? url.pathname : match.route.id; - const source = match === null ? 'url' : 'route'; - const transaction = hub.startTransaction({ - name, - op: 'http.server', - tags: { - method: request.method, - }, - metadata: { - source, - }, - }); + const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); - if (transaction) { - currentScope?.setSpan(transaction); - } const res = (await origRequestHandler.call(this, request, loadContext)) as Response; transaction?.setHttpStatus(res.status); diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index b67ec5187705..f7b9410904e6 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,4 +1,5 @@ import { extractRequestData, loadModule } from '@sentry/utils'; +import * as domain from 'domain'; import { createRoutes, @@ -35,20 +36,25 @@ function wrapExpressRequestHandler( res: ExpressResponse, next: ExpressNextFunction, ): Promise { - const request = extractRequestData(req); + const local = domain.create(); + local.add(req); + local.add(res); - if (!request.url || !request.method) { - return origRequestHandler.call(this, req, res, next); - } + local.run(async () => { + const request = extractRequestData(req); - const url = new URL(request.url); + if (!request.url || !request.method) { + return origRequestHandler.call(this, req, res, next); + } - const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + const url = new URL(request.url); + const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); - await origRequestHandler.call(this, req, res, next); + await origRequestHandler.call(this, req, res, next); - transaction?.setHttpStatus(res.statusCode); - transaction?.finish(); + transaction?.setHttpStatus(res.statusCode); + transaction?.finish(); + }); }; } From 9945932ae7fc4a3609600f5d6de79871e73df433 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 16:42:36 -0400 Subject: [PATCH 09/20] introduce timing --- packages/remix/test/integration/test/server/loader.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index ca0d16fbe112..ff5df0f9fd6d 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -141,8 +141,9 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const baseURL = await runServer(); await Promise.all( - Array.from(Array(10).keys()).map(async (id: number) => { + Array.from(Array(5).keys()).map(async (id: number) => { const url = `${baseURL}/scope-bleed/${id}`; + await new Promise(resolve => setTimeout(resolve, 5000 - id * 1000)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From f92c512a63805039ece027c9ad54fd2684662a2b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 16:55:57 -0400 Subject: [PATCH 10/20] clean up timing algo --- packages/remix/test/integration/app/routes/scope-bleed/$id.tsx | 2 ++ packages/remix/test/integration/test/server/loader.test.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 0c53c37e415d..1b82feafc115 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -3,7 +3,9 @@ import { json, LoaderFunction } from '@remix-run/node'; import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { + await new Promise(resolve => setTimeout(resolve, parseInt(id || '', 10) * 1000 - 1000)); Sentry.setTag(`tag${id}`, id); + await new Promise(resolve => setTimeout(resolve, 1000)); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index ff5df0f9fd6d..5ffc8e710c8d 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -143,7 +143,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada await Promise.all( Array.from(Array(5).keys()).map(async (id: number) => { const url = `${baseURL}/scope-bleed/${id}`; - await new Promise(resolve => setTimeout(resolve, 5000 - id * 1000)); + await new Promise(resolve => setTimeout(resolve, 5000 - id * 1000 - 1000)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From cde925bd04c8fa9226577959047c64e9cee5062f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 17:10:59 -0400 Subject: [PATCH 11/20] clean up timing --- packages/remix/test/integration/app/routes/scope-bleed/$id.tsx | 3 +-- packages/remix/test/integration/test/server/loader.test.ts | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 1b82feafc115..422ef8af709a 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -3,10 +3,9 @@ import { json, LoaderFunction } from '@remix-run/node'; import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { + // Set delay to simulate requests at the same time await new Promise(resolve => setTimeout(resolve, parseInt(id || '', 10) * 1000 - 1000)); Sentry.setTag(`tag${id}`, id); - await new Promise(resolve => setTimeout(resolve, 1000)); - return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 5ffc8e710c8d..fcd96c133eed 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -138,12 +138,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('makes sure scope does not bleed between requests', async () => { - const baseURL = await runServer(); + const baseURL = await runServer(adapter); await Promise.all( Array.from(Array(5).keys()).map(async (id: number) => { const url = `${baseURL}/scope-bleed/${id}`; - await new Promise(resolve => setTimeout(resolve, 5000 - id * 1000 - 1000)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From 51391c4fa5644adaddce57993dfd75ba8e61b61b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 17:19:27 -0400 Subject: [PATCH 12/20] set timeout in test body --- packages/remix/test/integration/test/server/loader.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index fcd96c133eed..ab964b469b20 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -143,6 +143,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada await Promise.all( Array.from(Array(5).keys()).map(async (id: number) => { const url = `${baseURL}/scope-bleed/${id}`; + await new Promise(resolve => setTimeout(resolve, 1000)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From d2d989c48f1ea86f2dc8781a09134484ad8133e8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 15 Aug 2022 17:25:11 -0400 Subject: [PATCH 13/20] plz dont be flaky --- .../remix/test/integration/app/routes/scope-bleed/$id.tsx | 2 +- packages/remix/test/integration/test/server/loader.test.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 422ef8af709a..db0c8b40bed5 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -4,7 +4,7 @@ import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { // Set delay to simulate requests at the same time - await new Promise(resolve => setTimeout(resolve, parseInt(id || '', 10) * 1000 - 1000)); + await new Promise(resolve => setTimeout(resolve, 5000 - (parseInt(id || '', 10) * 1000 - 1000 - 14))); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index ab964b469b20..8dafabf41a74 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -141,9 +141,12 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const baseURL = await runServer(adapter); await Promise.all( - Array.from(Array(5).keys()).map(async (id: number) => { + Array.from(Array(3).keys()).map(async (id: number) => { const url = `${baseURL}/scope-bleed/${id}`; - await new Promise(resolve => setTimeout(resolve, 1000)); + // Send requests with 1 sec delays, but they should all resolve at the same time + // See packages/remix/test/integration/app/routes/scope-bleed/$id.tsx + // for server-side set up. Add 13 for a bit of randomness. + await new Promise(resolve => setTimeout(resolve, id * 1000 - 1000 + 13)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From 58315e4065e3426923e2f695c476189b9802dc08 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 09:28:39 -0400 Subject: [PATCH 14/20] transaction not undefined --- packages/remix/src/utils/instrumentServer.ts | 27 +++++++++++-------- .../remix/src/utils/serverAdapters/express.ts | 8 ++++-- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 49f05c3fca0f..04107fe07939 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,7 +1,7 @@ /* 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'; @@ -292,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); @@ -312,9 +312,7 @@ export function startRequestHandlerTransaction( }, }); - if (transaction) { - currentScope?.setSpan(transaction); - } + currentScope?.setSpan(transaction); return transaction; } @@ -324,13 +322,20 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui return async function (this: unknown, request: Request, loadContext?: unknown): Promise { const local = domain.create(); return local.bind(async () => { + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); + + if (!options || !hasTracingEnabled(options)) { + return origRequestHandler.call(this, request, loadContext); + } + const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + 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; })(); diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index f7b9410904e6..4ecf8c166089 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -17,6 +17,8 @@ import { ReactRouterDomPkg, ServerBuild, } from '../types'; +import { getCurrentHub } from '@sentry/hub'; +import { hasTracingEnabled } from '@sentry/tracing'; function wrapExpressRequestHandler( origRequestHandler: ExpressRequestHandler, @@ -42,13 +44,15 @@ function wrapExpressRequestHandler( local.run(async () => { const request = extractRequestData(req); + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); - if (!request.url || !request.method) { + if (!options || !hasTracingEnabled(options) || !request.url || !request.method) { return origRequestHandler.call(this, req, res, next); } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(url, request.method, routes, pkg); + const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); await origRequestHandler.call(this, req, res, next); From 4b6f920f786c075d1850d44190bbc8eed6fcc19d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 09:32:54 -0400 Subject: [PATCH 15/20] actually introduce random numbers --- packages/remix/src/utils/serverAdapters/express.ts | 6 ++++-- .../remix/test/integration/app/routes/scope-bleed/$id.tsx | 3 ++- packages/remix/test/integration/test/server/loader.test.ts | 5 +++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index 4ecf8c166089..35a79db8af19 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,3 +1,5 @@ +import { getCurrentHub } from '@sentry/hub'; +import { hasTracingEnabled } from '@sentry/tracing'; import { extractRequestData, loadModule } from '@sentry/utils'; import * as domain from 'domain'; @@ -17,8 +19,6 @@ import { ReactRouterDomPkg, ServerBuild, } from '../types'; -import { getCurrentHub } from '@sentry/hub'; -import { hasTracingEnabled } from '@sentry/tracing'; function wrapExpressRequestHandler( origRequestHandler: ExpressRequestHandler, @@ -67,7 +67,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 }); diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index db0c8b40bed5..2443018dd198 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -4,7 +4,8 @@ import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { // Set delay to simulate requests at the same time - await new Promise(resolve => setTimeout(resolve, 5000 - (parseInt(id || '', 10) * 1000 - 1000 - 14))); + const randomNum = Math.floor(Math.random() * 15) + 1; + await new Promise(resolve => setTimeout(resolve, 4000 - (parseInt(id || '', 10) * 1000 - randomNum))); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 8dafabf41a74..72f41303949a 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -145,8 +145,9 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada const url = `${baseURL}/scope-bleed/${id}`; // Send requests with 1 sec delays, but they should all resolve at the same time // See packages/remix/test/integration/app/routes/scope-bleed/$id.tsx - // for server-side set up. Add 13 for a bit of randomness. - await new Promise(resolve => setTimeout(resolve, id * 1000 - 1000 + 13)); + // for server-side set up. + const randomNum = Math.floor(Math.random() * 15) + 1; + await new Promise(resolve => setTimeout(resolve, id * 1000 - 1000 + randomNum)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From 34f9c6d176e33a5434770aded4a138777af3c137 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 10:27:01 -0400 Subject: [PATCH 16/20] shorten timestamps --- packages/remix/test/integration/app/routes/scope-bleed/$id.tsx | 2 +- packages/remix/test/integration/test/server/loader.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 2443018dd198..5ffd2f6b7ad5 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -5,7 +5,7 @@ import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { // Set delay to simulate requests at the same time const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, 4000 - (parseInt(id || '', 10) * 1000 - randomNum))); + await new Promise(resolve => setTimeout(resolve, 400 - (parseInt(id || '', 10) * 100 - randomNum))); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 72f41303949a..6f081d03b8c4 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -147,7 +147,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada // See packages/remix/test/integration/app/routes/scope-bleed/$id.tsx // for server-side set up. const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, id * 1000 - 1000 + randomNum)); + await new Promise(resolve => setTimeout(resolve, id * 100 - 100 + randomNum)); const envelope = await getEnvelopeRequest(url); const transaction = envelope[2]; From 8d707e0a5d146228097d2b0f4052402cb5402581 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 11:31:02 -0400 Subject: [PATCH 17/20] fix test - but not the fix --- .../remix/test/integration/app/routes/scope-bleed/$id.tsx | 2 +- .../remix/test/integration/test/server/loader.test.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index 5ffd2f6b7ad5..d91f3b3d1385 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -5,7 +5,7 @@ import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { // Set delay to simulate requests at the same time const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, 400 - (parseInt(id || '', 10) * 100 - randomNum))); + await new Promise(resolve => setTimeout(resolve, 300 - (parseInt(id || '', 10) * 100 - randomNum))); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 2e64301c18b9..0d6cac38f0b2 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -138,17 +138,17 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('makes sure scope does not bleed between requests', async () => { - const baseURL = await runServer(adapter); + const config = await runServer(adapter); await Promise.all( Array.from(Array(3).keys()).map(async (id: number) => { - const url = `${baseURL}/scope-bleed/${id}`; + const url = `${config.url}/scope-bleed/${id}`; // Send requests with 1 sec delays, but they should all resolve at the same time // See packages/remix/test/integration/app/routes/scope-bleed/$id.tsx // for server-side set up. const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, id * 100 - 100 + randomNum)); - const envelope = await getEnvelopeRequest(url); + await new Promise(resolve => setTimeout(resolve, id * 100 + randomNum)); + const envelope = await getEnvelopeRequest({ ...config, url }); const transaction = envelope[2]; assertSentryTransaction(transaction, { From 51e5c0f75fa0f29c30707fc4c14a037c4231fd21 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 12:04:25 -0400 Subject: [PATCH 18/20] changes to make this work --- packages/remix/src/utils/instrumentServer.ts | 28 ++++----- .../remix/src/utils/serverAdapters/express.ts | 62 +++++++++++++++++-- .../app/routes/scope-bleed/$id.tsx | 3 - .../integration/test/server/loader.test.ts | 44 +++++++------ 4 files changed, 94 insertions(+), 43 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 04107fe07939..e4896b163f46 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -320,25 +320,22 @@ 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 local = domain.create(); - return local.bind(async () => { - const hub = getCurrentHub(); - const options = hub.getClient()?.getOptions(); + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); - if (!options || !hasTracingEnabled(options)) { - return origRequestHandler.call(this, request, loadContext); - } + 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 url = new URL(request.url); + const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); - const res = (await origRequestHandler.call(this, request, loadContext)) as Response; + const res = (await origRequestHandler.call(this, request, loadContext)) as Response; - transaction.setHttpStatus(res.status); - transaction.finish(); + transaction.setHttpStatus(res.status); + transaction.finish(); - return res; - })(); + return res; }; } @@ -424,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 35a79db8af19..ee24acb495f8 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,6 +1,7 @@ import { getCurrentHub } from '@sentry/hub'; import { hasTracingEnabled } from '@sentry/tracing'; -import { extractRequestData, loadModule } from '@sentry/utils'; +import { Transaction } from '@sentry/types'; +import { extractRequestData, loadModule, logger } from '@sentry/utils'; import * as domain from 'domain'; import { @@ -38,6 +39,9 @@ function wrapExpressRequestHandler( res: ExpressResponse, next: ExpressNextFunction, ): Promise { + // eslint-disable-next-line @typescript-eslint/unbound-method + res.end = wrapEndMethod(res.end); + const local = domain.create(); local.add(req); local.add(res); @@ -52,12 +56,8 @@ function wrapExpressRequestHandler( } const url = new URL(request.url); - const transaction = startRequestHandlerTransaction(url, request.method, routes, hub, pkg); - + startRequestHandlerTransaction(url, request.method, routes, hub, pkg); await origRequestHandler.call(this, req, res, next); - - transaction?.setHttpStatus(res.statusCode); - transaction?.finish(); }); }; } @@ -77,3 +77,53 @@ 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(); + }); + }); + } +} diff --git a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx index d91f3b3d1385..bcef8c591dff 100644 --- a/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx +++ b/packages/remix/test/integration/app/routes/scope-bleed/$id.tsx @@ -3,9 +3,6 @@ import { json, LoaderFunction } from '@remix-run/node'; import * as Sentry from '@sentry/remix'; export const loader: LoaderFunction = async ({ params: { id } }) => { - // Set delay to simulate requests at the same time - const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, 300 - (parseInt(id || '', 10) * 100 - randomNum))); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 0d6cac38f0b2..7264906b4893 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -138,25 +138,31 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); it('makes sure scope does not bleed between requests', async () => { - const config = await runServer(adapter); + const { url, server, scope } = await runServer(adapter); - await Promise.all( - Array.from(Array(3).keys()).map(async (id: number) => { - const url = `${config.url}/scope-bleed/${id}`; - // Send requests with 1 sec delays, but they should all resolve at the same time - // See packages/remix/test/integration/app/routes/scope-bleed/$id.tsx - // for server-side set up. - const randomNum = Math.floor(Math.random() * 15) + 1; - await new Promise(resolve => setTimeout(resolve, id * 100 + randomNum)); - const envelope = await getEnvelopeRequest({ ...config, url }); - const transaction = envelope[2]; - - assertSentryTransaction(transaction, { - tags: { - [`tag${id}`]: String(id), - }, - }); - }), - ); + 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 }), + ]); + + scope.persist(false); + await new Promise(resolve => server.close(resolve)); + + assertSentryTransaction(envelopes[0][2], { + tags: { + tag1: '1', + }, + }); + assertSentryTransaction(envelopes[1][2], { + tags: { + tag2: '2', + }, + }); + assertSentryTransaction(envelopes[2][2], { + tags: { + tag3: '3', + }, + }); }); }); From af261819fc39d7b1b98f01c7a2aa0cc65a25b982 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 12:10:12 -0400 Subject: [PATCH 19/20] remove logger --- packages/remix/src/utils/serverAdapters/express.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index ee24acb495f8..708cdbf971fc 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,7 +1,7 @@ import { getCurrentHub } from '@sentry/hub'; import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { extractRequestData, loadModule, logger } from '@sentry/utils'; +import { extractRequestData, loadModule } from '@sentry/utils'; import * as domain from 'domain'; import { From 464904fd44c5edbe6c302809a0aa071b0d1f5509 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 13:47:42 -0400 Subject: [PATCH 20/20] flush events in res.end --- .../remix/src/utils/serverAdapters/express.ts | 13 ++++++++++- .../app/routes/scope-bleed/$id.tsx | 2 ++ .../integration/test/server/loader.test.ts | 23 +++++++++++-------- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index 708cdbf971fc..a0111b392529 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -1,7 +1,8 @@ import { getCurrentHub } from '@sentry/hub'; +import { flush } from '@sentry/node'; import { hasTracingEnabled } from '@sentry/tracing'; import { Transaction } from '@sentry/types'; -import { extractRequestData, loadModule } from '@sentry/utils'; +import { extractRequestData, loadModule, logger } from '@sentry/utils'; import * as domain from 'domain'; import { @@ -126,4 +127,14 @@ async function finishSentryProcessing(res: AugmentedExpressResponse): Promise { + const timeTil = parseInt(id || '', 10) * 1000; + await new Promise(resolve => setTimeout(resolve, 3000 - timeTil)); Sentry.setTag(`tag${id}`, id); return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 7264906b4893..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: [ { @@ -144,6 +143,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada 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); @@ -151,17 +151,22 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada assertSentryTransaction(envelopes[0][2], { tags: { - tag1: '1', + tag4: '4', }, }); assertSentryTransaction(envelopes[1][2], { tags: { - tag2: '2', + tag3: '3', }, }); assertSentryTransaction(envelopes[2][2], { tags: { - tag3: '3', + tag2: '2', + }, + }); + assertSentryTransaction(envelopes[3][2], { + tags: { + tag1: '1', }, }); });