fix(node): Ensure node-fetch does not emit spans without tracing (#13765)

mydea · web-flow · commit a9750be7919f · 2024-09-24T11:52:57.000+02:00
Found this while working on #13763. Oops, the node-otel-without-tracing E2E test was not running on CI, we forgot to add it there - and it was actually failing since we switched to the new undici instrumentation :O This PR ensures to add it to CI, and also fixes it. The main change for this is to ensure we do not emit any spans when tracing is disabled, while still ensuring that trace propagation works as expected for this case. I also pulled some general changes into this, which ensure that we patch both `http.get` and `http.request` properly.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -895,6 +895,7 @@ jobs:
             'node-express-cjs-preload',
             'node-otel-sdk-node',
             'node-otel-custom-sampler',
+            'node-otel-without-tracing',
             'ember-classic',
             'ember-embroider',
             'nextjs-app-dir',
diff --git a/.gitignore b/.gitignore
@@ -43,6 +43,7 @@ local.log
 .rpt2_cache
 
 lint-results.json
+trace.zip
 
 # legacy
 tmp.js
diff --git a/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/package.json b/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/package.json
@@ -11,10 +11,10 @@
     "test:assert": "pnpm test"
   },
   "dependencies": {
-    "@opentelemetry/sdk-trace-node": "1.25.1",
-    "@opentelemetry/exporter-trace-otlp-http": "0.52.1",
-    "@opentelemetry/instrumentation-undici": "0.4.0",
-    "@opentelemetry/instrumentation": "0.52.1",
+    "@opentelemetry/sdk-trace-node": "1.26.0",
+    "@opentelemetry/exporter-trace-otlp-http": "0.53.0",
+    "@opentelemetry/instrumentation-undici": "0.6.0",
+    "@opentelemetry/instrumentation": "0.53.0",
     "@sentry/core": "latest || *",
     "@sentry/node": "latest || *",
     "@sentry/opentelemetry": "latest || *",
diff --git a/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts b/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
@@ -5,7 +5,7 @@ const { SentrySpanProcessor, SentryPropagator } = require('@sentry/opentelemetry
 const { UndiciInstrumentation } = require('@opentelemetry/instrumentation-undici');
 const { registerInstrumentations } = require('@opentelemetry/instrumentation');
 
-const sentryClient = Sentry.init({
+Sentry.init({
   environment: 'qa', // dynamic sampling bias to keep transactions
   dsn:
     process.env.E2E_TEST_DSN ||
diff --git a/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts b/dev-packages/e2e-tests/test-applications/node-otel-without-tracing/tests/transactions.test.ts
@@ -12,7 +12,9 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
 
     const scopeSpans = json.resourceSpans?.[0]?.scopeSpans;
 
-    const httpScope = scopeSpans?.find(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http');
+    const httpScope = scopeSpans?.find(
+      scopeSpan => scopeSpan.scope.name === '@opentelemetry_sentry-patched/instrumentation-http',
+    );
 
     return (
       httpScope &&
@@ -22,7 +24,7 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
     );
   });
 
-  await fetch(`${baseURL}/test-transaction`);
+  fetch(`${baseURL}/test-transaction`);
 
   const otelData = await otelPromise;
 
@@ -38,7 +40,9 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
   // But our default node-fetch spans are not emitted
   expect(scopeSpans.length).toEqual(2);
 
-  const httpScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http');
+  const httpScopes = scopeSpans?.filter(
+    scopeSpan => scopeSpan.scope.name === '@opentelemetry_sentry-patched/instrumentation-http',
+  );
   const undiciScopes = scopeSpans?.filter(
     scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-undici',
   );
@@ -49,6 +53,38 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
   expect(undiciScopes.length).toBe(1);
   expect(undiciScopes[0].spans.length).toBe(1);
 
+  expect(undiciScopes[0].spans).toEqual([
+    {
+      traceId: expect.any(String),
+      spanId: expect.any(String),
+      name: 'GET',
+      kind: 3,
+      startTimeUnixNano: expect.any(String),
+      endTimeUnixNano: expect.any(String),
+      attributes: expect.arrayContaining([
+        { key: 'http.request.method', value: { stringValue: 'GET' } },
+        { key: 'http.request.method_original', value: { stringValue: 'GET' } },
+        { key: 'url.full', value: { stringValue: 'http://localhost:3030/test-success' } },
+        { key: 'url.path', value: { stringValue: '/test-success' } },
+        { key: 'url.query', value: { stringValue: '' } },
+        { key: 'url.scheme', value: { stringValue: 'http' } },
+        { key: 'server.address', value: { stringValue: 'localhost' } },
+        { key: 'server.port', value: { intValue: 3030 } },
+        { key: 'user_agent.original', value: { stringValue: 'node' } },
+        { key: 'network.peer.address', value: { stringValue: expect.any(String) } },
+        { key: 'network.peer.port', value: { intValue: 3030 } },
+        { key: 'http.response.status_code', value: { intValue: 200 } },
+        { key: 'http.response.header.content-length', value: { intValue: 16 } },
+      ]),
+      droppedAttributesCount: 0,
+      events: [],
+      droppedEventsCount: 0,
+      status: { code: 0 },
+      links: [],
+      droppedLinksCount: 0,
+    },
+  ]);
+
   // There may be another span from another request, we can ignore that
   const httpSpans = httpScopes[0].spans.filter(span =>
     span.attributes.some(attr => attr.key === 'http.target' && attr.value?.stringValue === '/test-transaction'),
@@ -62,104 +98,24 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
       kind: 2,
       startTimeUnixNano: expect.any(String),
       endTimeUnixNano: expect.any(String),
-      attributes: [
-        {
-          key: 'http.url',
-          value: {
-            stringValue: 'http://localhost:3030/test-transaction',
-          },
-        },
-        {
-          key: 'http.host',
-          value: {
-            stringValue: 'localhost:3030',
-          },
-        },
-        {
-          key: 'net.host.name',
-          value: {
-            stringValue: 'localhost',
-          },
-        },
-        {
-          key: 'http.method',
-          value: {
-            stringValue: 'GET',
-          },
-        },
-        {
-          key: 'http.scheme',
-          value: {
-            stringValue: 'http',
-          },
-        },
-        {
-          key: 'http.target',
-          value: {
-            stringValue: '/test-transaction',
-          },
-        },
-        {
-          key: 'http.user_agent',
-          value: {
-            stringValue: 'node',
-          },
-        },
-        {
-          key: 'http.flavor',
-          value: {
-            stringValue: '1.1',
-          },
-        },
-        {
-          key: 'net.transport',
-          value: {
-            stringValue: 'ip_tcp',
-          },
-        },
-        {
-          key: 'sentry.origin',
-          value: {
-            stringValue: 'auto.http.otel.http',
-          },
-        },
-        {
-          key: 'net.host.ip',
-          value: {
-            stringValue: expect.any(String),
-          },
-        },
-        {
-          key: 'net.host.port',
-          value: {
-            intValue: 3030,
-          },
-        },
-        {
-          key: 'net.peer.ip',
-          value: {
-            stringValue: expect.any(String),
-          },
-        },
-        {
-          key: 'net.peer.port',
-          value: {
-            intValue: expect.any(Number),
-          },
-        },
-        {
-          key: 'http.status_code',
-          value: {
-            intValue: 200,
-          },
-        },
-        {
-          key: 'http.status_text',
-          value: {
-            stringValue: 'OK',
-          },
-        },
-      ],
+      attributes: expect.arrayContaining([
+        { key: 'http.url', value: { stringValue: 'http://localhost:3030/test-transaction' } },
+        { key: 'http.host', value: { stringValue: 'localhost:3030' } },
+        { key: 'net.host.name', value: { stringValue: 'localhost' } },
+        { key: 'http.method', value: { stringValue: 'GET' } },
+        { key: 'http.scheme', value: { stringValue: 'http' } },
+        { key: 'http.target', value: { stringValue: '/test-transaction' } },
+        { key: 'http.user_agent', value: { stringValue: 'node' } },
+        { key: 'http.flavor', value: { stringValue: '1.1' } },
+        { key: 'net.transport', value: { stringValue: 'ip_tcp' } },
+        { key: 'net.host.ip', value: { stringValue: expect.any(String) } },
+        { key: 'net.host.port', value: { intValue: 3030 } },
+        { key: 'net.peer.ip', value: { stringValue: expect.any(String) } },
+        { key: 'net.peer.port', value: { intValue: expect.any(Number) } },
+        { key: 'http.status_code', value: { intValue: 200 } },
+        { key: 'http.status_text', value: { stringValue: 'OK' } },
+        { key: 'sentry.origin', value: { stringValue: 'auto.http.otel.http' } },
+      ]),
       droppedAttributesCount: 0,
       events: [],
       droppedEventsCount: 0,
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts
@@ -22,7 +22,7 @@ async function run(): Promise<void> {
   Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
 
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
-  await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
+  await makeHttpGet(`${process.env.SERVER_URL}/api/v1`);
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
 
@@ -46,3 +46,16 @@ function makeHttpRequest(url: string): Promise<void> {
       .end();
   });
 }
+
+function makeHttpGet(url: string): Promise<void> {
+  return new Promise<void>(resolve => {
+    http.get(url, httpRes => {
+      httpRes.on('data', () => {
+        // we don't care about data
+      });
+      httpRes.on('end', () => {
+        resolve();
+      });
+    });
+  });
+}
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing/scenario.ts
@@ -13,7 +13,7 @@ import * as http from 'http';
 
 async function run(): Promise<void> {
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
-  await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
+  await makeHttpGet(`${process.env.SERVER_URL}/api/v1`);
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
   await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
 
@@ -37,3 +37,16 @@ function makeHttpRequest(url: string): Promise<void> {
       .end();
   });
 }
+
+function makeHttpGet(url: string): Promise<void> {
+  return new Promise<void>(resolve => {
+    http.get(url, httpRes => {
+      httpRes.on('data', () => {
+        // we don't care about data
+      });
+      httpRes.on('end', () => {
+        resolve();
+      });
+    });
+  });
+}
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing/test.ts
@@ -38,6 +38,56 @@ test('outgoing http requests are correctly instrumented with tracing disabled',
                 },
               ],
             },
+            breadcrumbs: [
+              {
+                message: 'manual breadcrumb',
+                timestamp: expect.any(Number),
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v0`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v0',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v1`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v1',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v2`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v2',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v3`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v3',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+            ],
           },
         })
         .start(closeTestServer);
diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts
