From 3af0c9411dbca60f2859bd78090ff24b10c871b0 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 14:41:31 -0400 Subject: [PATCH 1/2] chore(remix): Comment out flaky test --- .../integration/test/server/loader.test.ts | 69 ++++++++++--------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 39ffead32272..ee6500822e32 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -136,38 +136,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', - }, - }); - }); + // TODO: This test flakes. Let's fix it. + // 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', + // }, + // }); + // }); }); From ef6089ed6c5fdc81c4cfbb84e1dde7d10a18c82f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 16 Aug 2022 15:01:25 -0400 Subject: [PATCH 2/2] fix(remix): Wrap domains properly on instrumentServer --- packages/remix/src/utils/instrumentServer.ts | 28 +++++---- .../remix/src/utils/serverAdapters/express.ts | 25 +++----- .../integration/test/server/loader.test.ts | 59 ++++++++----------- 3 files changed, 48 insertions(+), 64 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index e4896b163f46..04107fe07939 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -320,22 +320,25 @@ 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 options = hub.getClient()?.getOptions(); + 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); - } + 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; + })(); }; } @@ -421,8 +424,7 @@ function makeWrappedCreateRequestHandler( const newBuild = instrumentBuild(build); const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); - const local = domain.create(); - return local.bind(() => wrapRequestHandler(requestHandler, newBuild))(); + return wrapRequestHandler(requestHandler, newBuild); }; } diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index a0111b392529..04344e99990c 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -3,7 +3,6 @@ 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, @@ -43,23 +42,17 @@ function wrapExpressRequestHandler( // eslint-disable-next-line @typescript-eslint/unbound-method res.end = wrapEndMethod(res.end); - const local = domain.create(); - local.add(req); - local.add(res); + const request = extractRequestData(req); + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); - local.run(async () => { - const request = extractRequestData(req); - const hub = getCurrentHub(); - const options = hub.getClient()?.getOptions(); + if (!options || !hasTracingEnabled(options) || !request.url || !request.method) { + return origRequestHandler.call(this, req, res, next); + } - if (!options || !hasTracingEnabled(options) || !request.url || !request.method) { - return origRequestHandler.call(this, req, res, next); - } - - const url = new URL(request.url); - startRequestHandlerTransaction(url, request.method, routes, hub, pkg); - await origRequestHandler.call(this, req, res, next); - }); + const url = new URL(request.url); + startRequestHandlerTransaction(url, request.method, routes, hub, pkg); + return origRequestHandler.call(this, req, res, next); }; } diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index ee6500822e32..8120bfd257e6 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -5,6 +5,7 @@ import { getMultipleEnvelopeRequest, assertSentryEvent, } from './utils/helpers'; +import { Event } from '@sentry/types'; jest.spyOn(console, 'error').mockImplementation(); @@ -136,39 +137,27 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada }); }); - // TODO: This test flakes. Let's fix it. - // 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', - // }, - // }); - // }); + 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)); + + envelopes.forEach(envelope => { + const tags = envelope[2].tags as NonNullable; + const customTagArr = Object.keys(tags).filter(t => t.startsWith('tag')); + expect(customTagArr).toHaveLength(1); + + const key = customTagArr[0]; + const val = key[key.length - 1]; + expect(tags[key]).toEqual(val); + }); + }); });