fix(node): Ensure DSC is correctly set in envelope headers (#11628)

mydea · web-flow · commit 379a9e51d729 · 2024-04-16T16:34:47.000+02:00
This was not correctly set before when we had an active span - the `error-active-span` & `error-active-span-unsampled` scenarios failed before and had no `trace` set on the envelope headers. The fix was to actually fix our `setupEventContextTrace` utility for OTEL, where we now just set the dsc on the event sdkProcessingMetadata, which is picked up when event processing. I also moved this to `preprocessEvent` to ensure this runs before other stuff. This should fix https://github.com/getsentry/sentry-javascript-examples/pull/7/files#r1561220216 which @s1gr1d found in her example app. 🚀
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/scenario.ts
@@ -0,0 +1,15 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 0,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'test span' }, () => {
+  Sentry.captureException(new Error('foo'));
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span-unsampled/test.ts
@@ -0,0 +1,18 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for error event during active unsampled span is correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions', 'transaction')
+    .expectHeader({
+      event: {
+        trace: {
+          trace_id: expect.any(String),
+          public_key: 'public',
+          environment: 'production',
+          release: '1.0',
+          sampled: 'false',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/scenario.ts
@@ -0,0 +1,15 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 1,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'test span' }, () => {
+  Sentry.captureException(new Error('foo'));
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error-active-span/test.ts
@@ -0,0 +1,20 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for error event during active span is correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions', 'transaction')
+    .expectHeader({
+      event: {
+        trace: {
+          trace_id: expect.any(String),
+          public_key: 'public',
+          environment: 'production',
+          release: '1.0',
+          sample_rate: '1',
+          sampled: 'true',
+          transaction: 'test span',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/scenario.ts
@@ -0,0 +1,13 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 0,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.captureException(new Error('foo'));
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/error/test.ts
@@ -0,0 +1,17 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for error events is correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions')
+    .expectHeader({
+      event: {
+        trace: {
+          trace_id: expect.any(String),
+          environment: 'production',
+          public_key: 'public',
+          release: '1.0',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/scenario.ts
@@ -0,0 +1,26 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 1,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.startSpan(
+  {
+    name: 'GET /route',
+    attributes: {
+      'http.method': 'GET',
+      'http.route': '/route',
+      [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
+      [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
+    },
+  },
+  () => {
+    // noop
+  },
+);
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-route/test.ts
@@ -0,0 +1,20 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for transaction event of route correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions')
+    .expectHeader({
+      transaction: {
+        trace: {
+          trace_id: expect.any(String),
+          public_key: 'public',
+          transaction: 'GET /route',
+          environment: 'production',
+          release: '1.0',
+          sample_rate: '1',
+          sampled: 'true',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/scenario.ts
@@ -0,0 +1,26 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 1,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.startSpan(
+  {
+    name: 'GET /route/1',
+    attributes: {
+      'http.method': 'GET',
+      'http.route': '/route',
+      [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
+      [Sentry.SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+    },
+  },
+  () => {
+    // noop
+  },
+);
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction-url/test.ts
@@ -0,0 +1,19 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for transaction event with source=url correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions')
+    .expectHeader({
+      transaction: {
+        trace: {
+          trace_id: expect.any(String),
+          public_key: 'public',
+          environment: 'production',
+          release: '1.0',
+          sample_rate: '1',
+          sampled: 'true',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/scenario.ts
@@ -0,0 +1,15 @@
+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',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  tracesSampleRate: 1,
+  integrations: [],
+  transport: loggingTransport,
+});
+
+Sentry.startSpan({ name: 'test span' }, () => {
+  // noop
+});
diff --git a/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts b/dev-packages/node-integration-tests/suites/tracing/envelope-header/transaction/test.ts
@@ -0,0 +1,20 @@
+import { createRunner } from '../../../../utils/runner';
+
+test('envelope header for transaction event is correct', done => {
+  createRunner(__dirname, 'scenario.ts')
+    .ignore('session', 'sessions')
+    .expectHeader({
+      transaction: {
+        trace: {
+          trace_id: expect.any(String),
+          public_key: 'public',
+          environment: 'production',
+          release: '1.0',
+          sample_rate: '1',
+          sampled: 'true',
+          transaction: 'test span',
+        },
+      },
+    })
+    .start(done);
+});
diff --git a/dev-packages/node-integration-tests/utils/runner.ts b/dev-packages/node-integration-tests/utils/runner.ts
@@ -1,6 +1,15 @@
+/* eslint-disable max-lines */
 import { spawn, spawnSync } from 'child_process';
 import { join } from 'path';
-import type { Envelope, EnvelopeItemType, Event, SerializedSession, SessionAggregates } from '@sentry/types';
+import { SDK_VERSION } from '@sentry/node';
+import type {
+  Envelope,
+  EnvelopeItemType,
+  Event,
+  EventEnvelope,
+  SerializedSession,
+  SessionAggregates,
+} from '@sentry/types';
 import axios from 'axios';
 import { createBasicSentryServer } from './server';
 
@@ -29,6 +38,18 @@ export function assertSentryTransaction(actual: Event, expected: Partial<Event>)
   });
 }
 
+export function assertEnvelopeHeader(actual: Envelope[0], expected: Partial<Envelope[0]>): void {
+  expect(actual).toEqual({
+    event_id: expect.any(String),
+    sent_at: expect.any(String),
+    sdk: {
+      name: 'sentry.javascript.node',
+      version: SDK_VERSION,
+    },
+    ...expected,
+  });
+}
+
 const CLEANUP_STEPS = new Set<VoidFunction>();
 
 export function cleanupChildProcesses(): void {
@@ -118,12 +139,19 @@ type Expected =
       sessions: Partial<SessionAggregates> | ((event: SessionAggregates) => void);
     };
 
+type ExpectedEnvelopeHeader =
+  | { event: Partial<EventEnvelope[0]> }
+  | { transaction: Partial<Envelope[0]> }
+  | { session: Partial<Envelope[0]> }
+  | { sessions: Partial<Envelope[0]> };
+
 /** Creates a test runner */
 // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
 export function createRunner(...paths: string[]) {
   const testPath = join(...paths);
 
   const expectedEnvelopes: Expected[] = [];
+  let expectedEnvelopeHeaders: ExpectedEnvelopeHeader[] | undefined = undefined;
   const flags: string[] = [];
   const ignored: EnvelopeItemType[] = [];
   let withEnv: Record<string, string> = {};
@@ -141,6 +169,14 @@ export function createRunner(...paths: string[]) {
       expectedEnvelopes.push(expected);
       return this;
     },
+    expectHeader: function (expected: ExpectedEnvelopeHeader) {
+      if (!expectedEnvelopeHeaders) {
+        expectedEnvelopeHeaders = [];
+      }
+
+      expectedEnvelopeHeaders.push(expected);
+      return this;
+    },
     expectError: function () {
       expectError = true;
       return this;
@@ -170,7 +206,7 @@ export function createRunner(...paths: string[]) {
       return this;
     },
     start: function (done?: (e?: unknown) => void) {
-      const expectedEnvelopeCount = expectedEnvelopes.length;
+      const expectedEnvelopeCount = Math.max(expectedEnvelopes.length, (expectedEnvelopeHeaders || []).length);
 
       let envelopeCount = 0;
       let scenarioServerPort: number | undefined;
@@ -198,6 +234,25 @@ export function createRunner(...paths: string[]) {
             continue;
           }
 
+          if (expectedEnvelopeHeaders) {
+            const header = envelope[0];
+            const expected = expectedEnvelopeHeaders.shift()?.[envelopeItemType as keyof ExpectedEnvelopeHeader];
+
+            try {
+              if (!expected) {
+                throw new Error(`No more expected envelope items but we received ${JSON.stringify(header)}`);
+              }
+
+              assertEnvelopeHeader(header, expected);
+
+              expectCallbackCalled();
+            } catch (e) {
+              complete(e as Error);
+            }
+
+            return;
+          }
+
           const expected = expectedEnvelopes.shift();
 
           // Catch any error or failed assertions and pass them to done to end the test quickly
diff --git a/packages/opentelemetry/src/setupEventContextTrace.ts b/packages/opentelemetry/src/setupEventContextTrace.ts
@@ -1,18 +1,19 @@
 import { getRootSpan } from '@sentry/core';
 import type { Client } from '@sentry/types';
 import { dropUndefinedKeys } from '@sentry/utils';
+import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
 
 import { getActiveSpan } from './utils/getActiveSpan';
 import { spanHasName, spanHasParentId } from './utils/spanTypes';
 
 /** Ensure the `trace` context is set on all events. */
 export function setupEventContextTrace(client: Client): void {
-  client.addEventProcessor(event => {
+  client.on('preprocessEvent', event => {
     const span = getActiveSpan();
     // For transaction events, this is handled separately
     // Because the active span may not be the span that is actually the transaction event
     if (!span || event.type === 'transaction') {
-      return event;
+      return;
     }
 
     const spanContext = span.spanContext();
@@ -27,12 +28,15 @@ export function setupEventContextTrace(client: Client): void {
       ...event.contexts,
     };
 
+    event.sdkProcessingMetadata = {
+      dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
+      ...event.sdkProcessingMetadata,
+    };
+
     const rootSpan = getRootSpan(span);
     const transactionName = spanHasName(rootSpan) ? rootSpan.name : undefined;
     if (transactionName && !event.transaction) {
       event.transaction = transactionName;
     }
-
-    return event;
   });
 }
