fix(node): Fix baggage propagation (#11363)

The node integration tests (correctly!) showed that we were not
propagating baggage 100% correct anymore. Turns out there were a few
issues with what we did before:

## Do not freeze DSC onto span `traceState` when not continuing incoming
trace

The main issue was, that we did not actually set the `transaction`,
`sampled` and `sample_rate` baggage entries. This was the case because
we only picked these from the trace state, but we only set this fully in
our own `startSpan()` APIs, not when auto instrumentation created spans.
So I changed this to _not_ set the trace state in `startSpan()` APIs,
but instead leave it empty _unless_ we are actually continuing an
incoming trace. Whenever we are _not_ continuing an incoming trace,
we'll now generate the DSC at injection-time based of the actual span
data.

So to summarize, we now set the `dsc` trace state in two places:

1. In the propagator, when we `extract` an incoming trace.
2. In our trace functions, when we use `forceTransaction` which we treat
as continuing an incoming trace.

## No merging of existing baggage headers

Another issue that came up is that we did not properly handled other
`baggage` entires being set. This is because of how OTEL does injection,
where it passes the `carrier` as object which we treated as `unknown`
and basically ignored. This meant that if a user did e.g. `fetch({
headers: { baggage: 'my=value' }})`, we would overwrite the baggage and
not keep the exiting value(s). This PR fixes this as well by ensuring we
keep this around.

## Future improvements

After talking with @lforst, we may be able to improve/streamline this
further by getting rid of the DSC trace state.
The logic could be:

1. If there is a DSC on the scope's propagation context, use that.
2. Else, use the root span

We may update to this logic internally in a follow up step!

Author: mydea

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

@@ -86,9 +86,7 @@ test('Should not propagate baggage and ignore original 3rd party baggage entries
   });
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
+test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
@@ -105,9 +103,7 @@ test.skip('Should populate and propagate sentry baggage if sentry-trace header d
   });
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
+test('Should populate Sentry and ignore 3rd party content if sentry-trace header does not exist', async () => {
   const runner = createRunner(__dirname, '..', 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts

@@ -26,9 +26,8 @@ Sentry.setUser({ id: 'user123' });
 app.use(cors());
 
 app.get('/test/express', (_req, res) => {
-  // eslint-disable-next-line deprecation/deprecation
-  const transaction = Sentry.getCurrentScope().getTransaction();
-  const traceId = transaction?.spanContext().traceId;
+  const span = Sentry.getActiveSpan();
+  const traceId = span?.spanContext().traceId;
   const headers = http.get('http://somewhere.not.sentry/').getHeaders();
   if (traceId) {
     headers['baggage'] = (headers['baggage'] as string).replace(traceId, '__SENTRY_TRACE_ID__');

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts

@@ -5,21 +5,28 @@ afterAll(() => {
   cleanupChildProcesses();
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('should attach a baggage header to an outgoing request.', async () => {
+test('should attach a baggage header to an outgoing request.', async () => {
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
 
   expect(response).toBeDefined();
+
+  const baggage = response?.test_data.baggage?.split(',').sort();
+
+  expect(baggage).toEqual([
+    'sentry-environment=prod',
+    'sentry-public_key=public',
+    'sentry-release=1.0',
+    'sentry-sample_rate=1',
+    'sentry-sampled=true',
+    'sentry-trace_id=__SENTRY_TRACE_ID__',
+    'sentry-transaction=GET%20%2Ftest%2Fexpress',
+  ]);
+
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage:
-        'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public' +
-        ',sentry-trace_id=__SENTRY_TRACE_ID__,sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress' +
-        ',sentry-sampled=true',
     },
   });
 });

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

@@ -5,9 +5,7 @@ afterAll(() => {
   cleanupChildProcesses();
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
+test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with incoming DSC', async () => {
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
@@ -16,34 +14,52 @@ test.skip('should ignore sentry-values in `baggage` header of a third party vend
   });
 
   expect(response).toBeDefined();
+
+  const baggage = response?.test_data.baggage?.split(',').sort();
+
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: [
-        'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item',
-        'sentry-release=2.1.0,sentry-environment=myEnv',
-      ],
     },
   });
+
+  expect(baggage).toEqual([
+    'foo=bar',
+    'last=item',
+    'other=vendor',
+    'sentry-environment=myEnv',
+    'sentry-release=2.1.0',
+    'sentry-sample_rate=0.54',
+    'third=party',
+  ]);
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
+test('should ignore sentry-values in `baggage` header of a third party vendor and overwrite them with new DSC', async () => {
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');
 
   expect(response).toBeDefined();
+
+  const baggage = response?.test_data.baggage?.split(',').sort();
+
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: [
-        'other=vendor,foo=bar,third=party,sentry-release=9.9.9,sentry-environment=staging,sentry-sample_rate=0.54,last=item',
-        expect.stringMatching(
-          /sentry-environment=prod,sentry-release=1\.0,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32},sentry-sample_rate=1,sentry-transaction=GET%20%2Ftest%2Fexpress/,
-        ),
-      ],
     },
   });
+
+  expect(baggage).toEqual([
+    'foo=bar',
+    'last=item',
+    'other=vendor',
+    'sentry-environment=prod',
+    'sentry-public_key=public',
+    'sentry-release=1.0',
+    'sentry-sample_rate=1',
+    'sentry-sampled=true',
+    expect.stringMatching(/sentry-trace_id=[0-9a-f]{32}/),
+    'sentry-transaction=GET%20%2Ftest%2Fexpress',
+    'third=party',
+  ]);
 });

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts

@@ -5,9 +5,7 @@ afterAll(() => {
   cleanupChildProcesses();
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
+test('should merge `baggage` header of a third party vendor with the Sentry DSC baggage items', async () => {
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express', {
@@ -19,7 +17,7 @@ test.skip('should merge `baggage` header of a third party vendor with the Sentry
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: ['other=vendor,foo=bar,third=party', 'sentry-release=2.0.0,sentry-environment=myEnv'],
+      baggage: 'other=vendor,foo=bar,third=party,sentry-release=2.0.0,sentry-environment=myEnv',
     },
   });
 });

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts

@@ -1,5 +1,4 @@
 import { loggingTransport } from '@sentry-internal/node-integration-tests';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core';
 import * as Sentry from '@sentry/node';
 
 export type TestAPIResponse = { test_data: { host: string; 'sentry-trace': string; baggage: string } };
@@ -31,10 +30,6 @@ Sentry.setUser({ id: 'user123' });
 app.use(cors());
 
 app.get('/test/express', (_req, res) => {
-  // eslint-disable-next-line deprecation/deprecation
-  const transaction = Sentry.getCurrentScope().getTransaction();
-  transaction?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route');
-
   const headers = http.get('http://somewhere.not.sentry/').getHeaders();
   // Responding with the headers outgoing request headers back to the assertions.
   res.send({ test_data: headers });

dev-packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts

@@ -5,9 +5,7 @@ afterAll(() => {
   cleanupChildProcesses();
 });
 
-// TODO(v8): Fix this test
-// eslint-disable-next-line jest/no-disabled-tests
-test.skip('Includes transaction in baggage if the transaction name is parameterized', async () => {
+test('Includes transaction in baggage if the transaction name is parameterized', async () => {
   const runner = createRunner(__dirname, 'server.ts').start();
 
   const response = await runner.makeRequest<TestAPIResponse>('get', '/test/express');

dev-packages/node-integration-tests/suites/tracing/spans/scenario.ts

@@ -12,9 +12,21 @@ import * as http from 'http';
 
 // eslint-disable-next-line @typescript-eslint/no-floating-promises
 Sentry.startSpan({ name: 'test_transaction' }, async () => {
-  http.get(`${process.env.SERVER_URL}/api/v0`);
-  http.get(`${process.env.SERVER_URL}/api/v1`);
-
-  // Give it a tick to resolve...
-  await new Promise(resolve => setTimeout(resolve, 100));
+  await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
+  await makeHttpRequest(`${process.env.SERVER_URL}/api/v1`);
 });
+
+function makeHttpRequest(url: string): Promise<void> {
+  return new Promise<void>(resolve => {
+    http
+      .request(url, httpRes => {
+        httpRes.on('data', () => {
+          // we don't care about data
+        });
+        httpRes.on('end', () => {
+          resolve();
+        });
+      })
+      .end();
+  });
+}

packages/core/test/lib/utils/spanUtils.test.ts

@@ -234,7 +234,7 @@ describe('getRootSpan', () => {
     expect(getRootSpan(root)).toBe(root);
   });
 
-  it('returns the root span of a child span xxx', () => {
+  it('returns the root span of a child span', () => {
     startSpan({ name: 'outer' }, root => {
       startSpan({ name: 'inner' }, inner => {
         expect(getRootSpan(inner)).toBe(root);

packages/opentelemetry/src/propagator.ts

@@ -1,9 +1,10 @@
-import type { Baggage, Context, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
+import type { Baggage, Context, Span, SpanContext, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
 import { context } from '@opentelemetry/api';
 import { TraceFlags, propagation, trace } from '@opentelemetry/api';
 import { TraceState, W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import type { continueTrace } from '@sentry/core';
+import { getRootSpan } from '@sentry/core';
 import { spanToJSON } from '@sentry/core';
 import { getClient, getCurrentScope, getDynamicSamplingContextFromClient, getIsolationScope } from '@sentry/core';
 import type { DynamicSamplingContext, Options, PropagationContext } from '@sentry/types';
@@ -14,6 +15,7 @@ import {
   dynamicSamplingContextToSentryBaggageHeader,
   generateSentryTraceHeader,
   logger,
+  parseBaggageHeader,
   propagationContextFromHeaders,
   stringMatchesSomePattern,
 } from '@sentry/utils';
@@ -27,18 +29,27 @@ import {
 } from './constants';
 import { DEBUG_BUILD } from './debug-build';
 import { getScopesFromContext, setScopesOnContext } from './utils/contextData';
+import { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';
+import { getSamplingDecision } from './utils/getSamplingDecision';
 import { setIsSetup } from './utils/setupCheck';
 
 /** Get the Sentry propagation context from a span context. */
-export function getPropagationContextFromSpanContext(spanContext: SpanContext): PropagationContext {
+export function getPropagationContextFromSpan(span: Span): PropagationContext {
+  const spanContext = span.spanContext();
   const { traceId, spanId, traceState } = spanContext;
 
+  // When we have a dsc trace state, it means this came from the incoming trace
+  // Then this takes presedence over the root span
   const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined;
-  const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
+  const traceStateDsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
+
   const parentSpanId = traceState ? traceState.get(SENTRY_TRACE_STATE_PARENT_SPAN_ID) : undefined;
 
   const sampled = getSamplingDecision(spanContext);
 
+  // No trace state? --> Take DSC from root span
+  const dsc = traceStateDsc || getDynamicSamplingContextFromSpan(getRootSpan(span));
+
   return {
     traceId,
     spanId,
@@ -85,10 +96,21 @@ export class SentryPropagator extends W3CBaggagePropagator {
       return;
     }
 
+    const existingBaggageHeader = getExistingBaggage(carrier);
     let baggage = propagation.getBaggage(context) || propagation.createBaggage({});
 
     const { dynamicSamplingContext, traceId, spanId, sampled } = getInjectionData(context);
 
+    if (existingBaggageHeader) {
+      const baggageEntries = parseBaggageHeader(existingBaggageHeader);
+
+      if (baggageEntries) {
+        Object.entries(baggageEntries).forEach(([key, value]) => {
+          baggage = baggage.setEntry(key, { value });
+        });
+      }
+    }
+
     if (dynamicSamplingContext) {
       baggage = Object.entries(dynamicSamplingContext).reduce<Baggage>((b, [dscKey, dscValue]) => {
         if (dscValue) {
@@ -196,7 +218,8 @@ function getInjectionData(context: Context): {
   // If we have a local span, we can just pick everything from it
   if (span && !spanIsRemote) {
     const spanContext = span.spanContext();
-    const propagationContext = getPropagationContextFromSpanContext(spanContext);
+
+    const propagationContext = getPropagationContextFromSpan(span);
     const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId);
     return {
       dynamicSamplingContext,
@@ -220,9 +243,9 @@ function getInjectionData(context: Context): {
   }
 
   // Else, we look at the remote span context
-  const spanContext = trace.getSpanContext(context);
-  if (spanContext) {
-    const propagationContext = getPropagationContextFromSpanContext(spanContext);
+  if (span) {
+    const spanContext = span.spanContext();
+    const propagationContext = getPropagationContextFromSpan(span);
     const dynamicSamplingContext = getDynamicSamplingContext(propagationContext, spanContext.traceId);
 
     return {
@@ -301,40 +324,12 @@ export function continueTraceAsRemoteSpan<T>(
   return context.with(ctxWithSpanContext, callback);
 }
 
-/**
- * OpenTelemetry only knows about SAMPLED or NONE decision,
- * but for us it is important to differentiate between unset and unsampled.
- *
- * Both of these are identified as `traceFlags === TracegFlags.NONE`,
- * but we additionally look at a special trace state to differentiate between them.
- */
-export function getSamplingDecision(spanContext: SpanContext): boolean | undefined {
-  const { traceFlags, traceState } = spanContext;
-
-  const sampledNotRecording = traceState ? traceState.get(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING) === '1' : false;
-
-  // If trace flag is `SAMPLED`, we interpret this as sampled
-  // If it is `NONE`, it could mean either it was sampled to be not recorder, or that it was not sampled at all
-  // For us this is an important difference, sow e look at the SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING
-  // to identify which it is
-  if (traceFlags === TraceFlags.SAMPLED) {
-    return true;
-  }
-
-  if (sampledNotRecording) {
-    return false;
-  }
-
-  // Fall back to DSC as a last resort, that may also contain `sampled`...
-  const dscString = traceState ? traceState.get(SENTRY_TRACE_STATE_DSC) : undefined;
-  const dsc = dscString ? baggageHeaderToDynamicSamplingContext(dscString) : undefined;
-
-  if (dsc?.sampled === 'true') {
-    return true;
-  }
-  if (dsc?.sampled === 'false') {
-    return false;
+/** Try to get the existing baggage header so we can merge this in. */
+function getExistingBaggage(carrier: unknown): string | undefined {
+  try {
+    const baggage = (carrier as Record<string, string | string[]>)[SENTRY_BAGGAGE_HEADER];
+    return Array.isArray(baggage) ? baggage.join(',') : baggage;
+  } catch {
+    return undefined;
   }
-
-  return undefined;
 }