fix(node): Ensure isolation scope is correctly cloned for non-recordi…

…ng spans (getsentry#11503) Our Http instrumentation didn't clone the isolation scope correctly when the request span is not recording. We early returned if the span was not a server kind span. However, non-recording spans apparently have no span kind, so we wrongfully early returned in the check for non-recording spans.
cadesalaberry · Apr 19, 2024 · a5b1c21 · a5b1c21
1 parent bbf9ecb
commit a5b1c21
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 9 deletions.
diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/server.ts
@@ -0,0 +1,22 @@
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  transport: loggingTransport,
+  tracesSampleRate: 1,
+});
+
+import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
+import express from 'express';
+
+const app = express();
+
+app.get('/test/express/:id', req => {
+  throw new Error(`test_error with id ${req.params.id}`);
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);
diff --git a/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts b/dev-packages/node-integration-tests/suites/express/handle-error-tracesSampleRate-0/test.ts
@@ -0,0 +1,39 @@
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('should capture and send Express controller error with txn name if tracesSampleRate is 0', done => {
+  const runner = createRunner(__dirname, 'server.ts')
+    .ignore('session', 'sessions', 'transaction')
+    .expect({
+      event: {
+        exception: {
+          values: [
+            {
+              mechanism: {
+                type: 'middleware',
+                handled: false,
+              },
+              type: 'Error',
+              value: 'test_error with id 123',
+              stacktrace: {
+                frames: expect.arrayContaining([
+                  expect.objectContaining({
+                    function: expect.any(String),
+                    lineno: expect.any(Number),
+                    colno: expect.any(Number),
+                  }),
+                ]),
+              },
+            },
+          ],
+        },
+        transaction: 'GET /test/express/:id',
+      },
+    })
+    .start(done);
+
+  expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow();
+});
diff --git a/...sts/suites/express/handle-error/server.ts → ...le-error-tracesSampleRate-unset/server.ts b/...sts/suites/express/handle-error/server.ts → ...le-error-tracesSampleRate-unset/server.ts
@@ -12,8 +12,8 @@ import express from 'express';
 
 const app = express();
 
-app.get('/test/express', () => {
-  throw new Error('test_error');
+app.get('/test/express/:id', req => {
+  throw new Error(`test_error with id ${req.params.id}`);
 });
 
 Sentry.setupExpressErrorHandler(app);

diff --git a/...tests/suites/express/handle-error/test.ts → ...ndle-error-tracesSampleRate-unset/test.ts b/...tests/suites/express/handle-error/test.ts → ...ndle-error-tracesSampleRate-unset/test.ts
@@ -4,9 +4,9 @@ afterAll(() => {
   cleanupChildProcesses();
 });
 
-test('should capture and send Express controller error.', done => {
+test('should capture and send Express controller error if tracesSampleRate is not set.', done => {
   const runner = createRunner(__dirname, 'server.ts')
-    .ignore('session', 'sessions')
+    .ignore('session', 'sessions', 'transaction')
     .expect({
       event: {
         exception: {
@@ -17,7 +17,7 @@ test('should capture and send Express controller error.', done => {
                 handled: false,
               },
               type: 'Error',
-              value: 'test_error',
+              value: 'test_error with id 123',
               stacktrace: {
                 frames: expect.arrayContaining([
                   expect.objectContaining({
@@ -34,5 +34,5 @@ test('should capture and send Express controller error.', done => {
     })
     .start(done);
 
-  expect(() => runner.makeRequest('get', '/test/express')).rejects.toThrow();
+  expect(() => runner.makeRequest('get', '/test/express/123')).rejects.toThrow();
 });
diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts
@@ -1,4 +1,4 @@
-import type { ServerResponse } from 'http';
+import type { ClientRequest, IncomingMessage, ServerResponse } from 'http';
 import type { Span } from '@opentelemetry/api';
 import { SpanKind } from '@opentelemetry/api';
 import { registerInstrumentations } from '@opentelemetry/instrumentation';
@@ -93,7 +93,9 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
           requestHook: (span, req) => {
             addOriginToSpan(span, 'auto.http.otel.http');
 
-            if (getSpanKind(span) !== SpanKind.SERVER) {
+            // both, incoming requests and "client" requests made within the app trigger the requestHook
+            // we only want to isolate and further annotate incoming requests (IncomingMessage)
+            if (_isClientRequest(req)) {
               return;
             }
 
@@ -179,3 +181,12 @@ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMe
     },
   );
 }
+
+/**
+ * Determines if @param req is a ClientRequest, meaning the request was created within the express app
+ * and it's an outgoing request.
+ * Checking for properties instead of using `instanceOf` to avoid importing the request classes.
+ */
+function _isClientRequest(req: ClientRequest | IncomingMessage): req is ClientRequest {
+  return 'outputData' in req && 'outputSize' in req && !('client' in req) && !('statusCode' in req);
+}
diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts
@@ -1,10 +1,12 @@
 import type * as http from 'http';
 import { registerInstrumentations } from '@opentelemetry/instrumentation';
 import { ExpressInstrumentation } from '@opentelemetry/instrumentation-express';
-import { defineIntegration } from '@sentry/core';
+import { defineIntegration, getDefaultIsolationScope } from '@sentry/core';
 import { captureException, getClient, getIsolationScope } from '@sentry/core';
 import type { IntegrationFn } from '@sentry/types';
 
+import { logger } from '@sentry/utils';
+import { DEBUG_BUILD } from '../../debug-build';
 import type { NodeClient } from '../../sdk/client';
 import { addOriginToSpan } from '../../utils/addOriginToSpan';
 
@@ -19,6 +21,11 @@ const _expressIntegration = (() => {
               addOriginToSpan(span, 'auto.http.otel.express');
             },
             spanNameHook(info, defaultName) {
+              if (getIsolationScope() === getDefaultIsolationScope()) {
+                DEBUG_BUILD &&
+                  logger.warn('Isolation scope is still default isolation scope - skipping setting transactionName');
+                return defaultName;
+              }
               if (info.layerType === 'request_handler') {
                 // type cast b/c Otel unfortunately types info.request as any :(
                 const req = info.request as { method?: string };