fix(remix): Wrap domains properly on instrumentServer (getsentry#5590)

getsentry#5570 merged, but the tests were being very flaky. I took another look and figured out that we only needed to wrap our instrumentation with a domain once. Wrapping twice (for both express and built-in) was causing problems. In addition, I moved the wrapping down to the request handling phase, which makes the behaviour more correct.
timfish · Aug 16, 2022 · c2cdf92 · c2cdf92
1 parent bdd7fde
commit c2cdf92
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 48 deletions.
diff --git 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<ReactRouterDomPkg>('react-router-dom');
   return async function (this: unknown, request: Request, loadContext?: unknown): Promise<Response> {
-    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
@@ -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
@@ -5,6 +5,7 @@ import {
   getMultipleEnvelopeRequest,
   assertSentryEvent,
 } from './utils/helpers';
+import { Event } from '@sentry/types';
 
 jest.spyOn(console, 'error').mockImplementation();
 
@@ -149,25 +150,14 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
     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',
-      },
+    envelopes.forEach(envelope => {
+      const tags = envelope[2].tags as NonNullable<Event['tags']>;
+      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);
     });
   });
 });