fix(node-core): Recycle propagationContext for each request (#19835)

This PR fixes a bug in our node-core `httpServerIntegration`
(user-facing it's `httpIntegration`), which caused traceIds (or rather
our propagationContext) to stay the same across requests. This would
surface in SDK setups where tracing is not explicitly enabled (e.g.
missing `tracesSampleRate`), causing caught errors across request to be
associated with the same trace.

This PR now recycles the propagationContext on the current as well as
isolation scope to ensure traces are isolated on a request level. Added
node(-core) integration tests to demonstrate that traceIds are now
scoped to requests, when tracing is enabled or disabled. Prior to this
PR, the test for tracing being disabled failed.

Note: This should only have an effect on SDKs configured for tracing
without spans (i.e. (and confusingly) no `tracesSampleRate` set), as for
tracing with spans, we take the trace data from the active span
directly. I added a test demonstrating this, just to be sure.

closes #19815
ref #17101

---------

Co-authored-by: Charly Gomez <charly.gomez1310@gmail.com>

Author: Lms24

dev-packages/node-core-integration-tests/suites/tracing/traceid-recycling-with-spans/server.js

@@ -0,0 +1,23 @@
+const { loggingTransport } = require('@sentry-internal/node-core-integration-tests');
+const Sentry = require('@sentry/node-core');
+const { setupOtel } = require('../../../utils/setupOtel.js');
+
+const client = Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  transport: loggingTransport,
+  tracesSampleRate: 1.0,
+});
+
+setupOtel(client);
+
+const express = require('express');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-core-integration-tests');
+
+const app = express();
+
+app.get('/test', (_req, res) => {
+  Sentry.captureException(new Error('test error'));
+  res.json({ success: true });
+});
+
+startExpressServerAndSendPortToRunner(app);

dev-packages/node-core-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts

@@ -0,0 +1,39 @@
+import { afterAll, expect, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('errors from in different requests each get a unique traceId when tracing is enabled', async () => {
+  const eventTraceIds: string[] = [];
+
+  const runner = createRunner(__dirname, 'server.js')
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .start();
+
+  await runner.makeRequest('get', '/test');
+  await runner.makeRequest('get', '/test');
+  await runner.makeRequest('get', '/test');
+
+  await runner.completed();
+
+  expect(new Set(eventTraceIds).size).toBe(3);
+  for (const traceId of eventTraceIds) {
+    expect(traceId).toMatch(/^[a-f\d]{32}$/);
+  }
+});

dev-packages/node-core-integration-tests/suites/tracing/traceid-recycling/server.js

@@ -0,0 +1,23 @@
+const { loggingTransport } = require('@sentry-internal/node-core-integration-tests');
+const Sentry = require('@sentry/node-core');
+const { setupOtel } = require('../../../utils/setupOtel.js');
+
+const client = Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  transport: loggingTransport,
+});
+
+setupOtel(client);
+
+const express = require('express');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-core-integration-tests');
+
+const app = express();
+
+app.get('/test', (_req, res) => {
+  Sentry.captureException(new Error('test error'));
+  const traceId = Sentry.getCurrentScope().getPropagationContext().traceId;
+  res.json({ traceId });
+});
+
+startExpressServerAndSendPortToRunner(app);

dev-packages/node-core-integration-tests/suites/tracing/traceid-recycling/test.ts

@@ -0,0 +1,43 @@
+import { afterAll, expect, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('each request gets a unique traceId when tracing is disabled', async () => {
+  const eventTraceIds: string[] = [];
+
+  const runner = createRunner(__dirname, 'server.js')
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .start();
+
+  const propagationContextTraceIds = [
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+  ];
+
+  await runner.completed();
+
+  expect(new Set(propagationContextTraceIds).size).toBe(3);
+  for (const traceId of propagationContextTraceIds) {
+    expect(traceId).toMatch(/^[a-f\d]{32}$/);
+  }
+
+  expect(eventTraceIds).toEqual(propagationContextTraceIds);
+});

dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/server.ts

@@ -0,0 +1,23 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  transport: loggingTransport,
+  tracesSampleRate: 1.0,
+});
+
+import express from 'express';
+
+const app = express();
+
+app.get('/test', async (_req, res) => {
+  Sentry.captureException(new Error('test error'));
+  // calling Sentry.flush() here to ensure that the order in which we send transaction and errors
+  // is guaranteed to be 1. error, 2. transaction (repeated 3x in test)
+  await Sentry.flush();
+  res.json({ success: true });
+});
+
+startExpressServerAndSendPortToRunner(app);

dev-packages/node-integration-tests/suites/tracing/traceid-recycling-with-spans/test.ts

@@ -0,0 +1,57 @@
+import { afterAll, expect, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('errors and transactions get a unique traceId per request, when tracing is enabled', async () => {
+  const eventTraceIds: string[] = [];
+  const transactionTraceIds: string[] = [];
+
+  const runner = createRunner(__dirname, 'server.ts')
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      transaction: transaction => {
+        transactionTraceIds.push(transaction.spans?.[0]?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      transaction: transaction => {
+        transactionTraceIds.push(transaction.spans?.[0]?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      transaction: transaction => {
+        transactionTraceIds.push(transaction.spans?.[0]?.trace_id || '');
+      },
+    })
+    .start();
+
+  await runner.makeRequest('get', '/test');
+  await runner.makeRequest('get', '/test');
+  await runner.makeRequest('get', '/test');
+
+  await runner.completed();
+
+  expect(new Set(transactionTraceIds).size).toBe(3);
+  for (const traceId of transactionTraceIds) {
+    expect(traceId).toMatch(/^[a-f\d]{32}$/);
+  }
+
+  expect(eventTraceIds).toEqual(transactionTraceIds);
+});

dev-packages/node-integration-tests/suites/tracing/traceid-recycling/server.ts

@@ -0,0 +1,20 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport, startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  transport: loggingTransport,
+});
+
+import express from 'express';
+
+const app = express();
+
+app.get('/test', (_req, res) => {
+  Sentry.captureException(new Error('test error'));
+  const traceId = Sentry.getCurrentScope().getPropagationContext().traceId;
+  res.json({ traceId });
+});
+
+startExpressServerAndSendPortToRunner(app);

dev-packages/node-integration-tests/suites/tracing/traceid-recycling/test.ts

@@ -0,0 +1,43 @@
+import { afterAll, expect, test } from 'vitest';
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+afterAll(() => {
+  cleanupChildProcesses();
+});
+
+test('each request gets a unique traceId when tracing is disabled', async () => {
+  const eventTraceIds: string[] = [];
+
+  const runner = createRunner(__dirname, 'server.ts')
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .expect({
+      event: event => {
+        eventTraceIds.push(event.contexts?.trace?.trace_id || '');
+      },
+    })
+    .start();
+
+  const propagationContextTraceIds = [
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+    ((await runner.makeRequest('get', '/test')) as { traceId: string }).traceId,
+  ];
+
+  await runner.completed();
+
+  expect(new Set(propagationContextTraceIds).size).toBe(3);
+  for (const traceId of propagationContextTraceIds) {
+    expect(traceId).toMatch(/^[a-f\d]{32}$/);
+  }
+
+  expect(eventTraceIds).toEqual(propagationContextTraceIds);
+});

packages/node-core/src/integrations/http/httpServerIntegration.ts

@@ -6,9 +6,11 @@ import type { Socket } from 'node:net';
 import { context, createContextKey, propagation } from '@opentelemetry/api';
 import type { AggregationCounts, Client, Integration, IntegrationFn, Scope } from '@sentry/core';
 import {
+  _INTERNAL_safeMathRandom,
   addNonEnumerableProperty,
   debug,
   generateSpanId,
+  generateTraceId,
   getClient,
   getCurrentScope,
   getIsolationScope,
@@ -218,10 +220,19 @@ function instrumentServer(
       }
 
       return withIsolationScope(isolationScope, () => {
-        // Set a new propagationSpanId for this request
-        // We rely on the fact that `withIsolationScope()` will implicitly also fork the current scope
-        // This way we can save an "unnecessary" `withScope()` invocation
-        getCurrentScope().getPropagationContext().propagationSpanId = generateSpanId();
+        const newPropagationContext = {
+          traceId: generateTraceId(),
+          sampleRand: _INTERNAL_safeMathRandom(),
+          propagationSpanId: generateSpanId(),
+        };
+        // - Set a fresh propagation context so each request gets a unique traceId.
+        //   When there are incoming trace headers, propagation.extract() below sets a remote
+        //   span on the OTel context which takes precedence in getTraceContextForScope().
+        // - We can write directly to the current scope here because it is forked implicitly via
+        //   `context.with` in `withIsolationScope` (See `SentryContextManager`).
+        // - explicitly making a deep copy to avoid mutation of original PC on the other scope
+        getCurrentScope().setPropagationContext({ ...newPropagationContext });
+        isolationScope.setPropagationContext({ ...newPropagationContext });
 
         const ctx = propagation
           .extract(context.active(), normalizedRequest.headers)