ref: Make dynamic sampling context mutable (#5710)

This PR makes Dynamic Sampling Context in Head-SDKs mutable. List of changes: - Remove our `Baggage` abstraction (keeping track of foreign baggage and mutability is not necessary) - Replace `Baggage` abstraction with functions that convert `baggage` headers into DSC and vice-versa - Replace `getBaggage()` method on the `Transaction` class with `getDynamicSamplingContext()` which doesn't freeze DSC upon retrieval - Refactor the adding of Sentry `baggage` items so that we're (almost) always adding an additional independent `baggage` header, in addition to any that were already there. This was done to reduce code complexity and bundle size. An exception here is in the browser where there is a case where I don't know if adding an additional header might break things. - Explicitly handle the case of multiple incoming baggage headers. (if there are multiple items that are named the same, last one always wins) - Freeze DSC in the transaction creation if DSC was provided via `transactionContext.metadata.dynamicSamplingContext` - used in non-head-SDKs - Fix `extractTraceparentData` so that it returns `undefined` (ie. "invalid") when passed an empty string - also added a test for that.
getsentry · Sep 15, 2022 · 347fbe4 · 347fbe4
1 parent e26e43a
commit 347fbe4
Show file tree

Hide file tree

Showing 37 changed files with 502 additions and 855 deletions.
diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts
@@ -1,7 +1,5 @@
 import {
-  Baggage,
   DsnComponents,
-  DynamicSamplingContext,
   Event,
   EventEnvelope,
   EventEnvelopeHeaders,
@@ -13,7 +11,7 @@ import {
   SessionEnvelope,
   SessionItem,
 } from '@sentry/types';
-import { createEnvelope, dropUndefinedKeys, dsnToString, getSentryBaggageItems } from '@sentry/utils';
+import { createEnvelope, dropUndefinedKeys, dsnToString } from '@sentry/utils';
 
 /** Extract sdk info from from the API metadata */
 function getSdkMetadataForEnvelopeHeader(metadata?: SdkMetadata): SdkInfo | undefined {
@@ -101,8 +99,7 @@ function createEventEnvelopeHeaders(
   tunnel: string | undefined,
   dsn: DsnComponents,
 ): EventEnvelopeHeaders {
-  const baggage: Baggage | undefined = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
-  const dynamicSamplingContext = baggage && getSentryBaggageItems(baggage);
+  const dynamicSamplingContext = event.sdkProcessingMetadata && event.sdkProcessingMetadata.dynamicSamplingContext;
 
   return {
     event_id: event.event_id as string,
@@ -111,7 +108,7 @@ function createEventEnvelopeHeaders(
     ...(!!tunnel && { dsn: dsnToString(dsn) }),
     ...(event.type === 'transaction' &&
       dynamicSamplingContext && {
-        trace: dropUndefinedKeys({ ...dynamicSamplingContext }) as DynamicSamplingContext,
+        trace: dropUndefinedKeys({ ...dynamicSamplingContext }),
       }),
   };
 }
diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts
@@ -20,7 +20,7 @@ describe('createEventEnvelope', () => {
         {
           type: 'transaction',
           sdkProcessingMetadata: {
-            baggage: [{ trace_id: '1234', public_key: 'pubKey123' }, '', false],
+            dynamicSamplingContext: { trace_id: '1234', public_key: 'pubKey123' },
           },
         },
         { trace_id: '1234', public_key: 'pubKey123' },
@@ -30,7 +30,12 @@ describe('createEventEnvelope', () => {
         {
           type: 'transaction',
           sdkProcessingMetadata: {
-            baggage: [{ environment: 'prod', release: '1.0.0', public_key: 'pubKey123', trace_id: '1234' }, '', false],
+            dynamicSamplingContext: {
+              environment: 'prod',
+              release: '1.0.0',
+              public_key: 'pubKey123',
+              trace_id: '1234',
+            },
           },
         },
         { release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' },
@@ -40,19 +45,15 @@ describe('createEventEnvelope', () => {
         {
           type: 'transaction',
           sdkProcessingMetadata: {
-            baggage: [
-              {
-                environment: 'prod',
-                release: '1.0.0',
-                transaction: 'TX',
-                user_segment: 'segmentA',
-                sample_rate: '0.95',
-                public_key: 'pubKey123',
-                trace_id: '1234',
-              },
-              '',
-              false,
-            ],
+            dynamicSamplingContext: {
+              environment: 'prod',
+              release: '1.0.0',
+              transaction: 'TX',
+              user_segment: 'segmentA',
+              sample_rate: '0.95',
+              public_key: 'pubKey123',
+              trace_id: '1234',
+            },
           },
         },
         {

diff --git a/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts b/packages/nextjs/src/config/wrappers/withSentryGetServerSideProps.ts
@@ -1,5 +1,5 @@
 import { hasTracingEnabled } from '@sentry/tracing';
-import { serializeBaggage } from '@sentry/utils';
+import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils';
 import { GetServerSideProps } from 'next';
 
 import { isBuild } from '../../utils/isBuild';
@@ -45,7 +45,9 @@ export function withSentryGetServerSideProps(
         const requestTransaction = getTransactionFromRequest(req);
         if (requestTransaction) {
           serverSideProps.props._sentryTraceData = requestTransaction.toTraceparent();
-          serverSideProps.props._sentryBaggage = serializeBaggage(requestTransaction.getBaggage());
+
+          const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext();
+          serverSideProps.props._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
         }
       }
 

diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideAppGetInitialProps.ts
@@ -1,5 +1,5 @@
 import { hasTracingEnabled } from '@sentry/tracing';
-import { serializeBaggage } from '@sentry/utils';
+import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils';
 import App from 'next/app';
 
 import { isBuild } from '../../utils/isBuild';
@@ -51,7 +51,10 @@ export function withSentryServerSideAppGetInitialProps(origAppGetInitialProps: A
       const requestTransaction = getTransactionFromRequest(req!);
       if (requestTransaction) {
         appGetInitialProps.pageProps._sentryTraceData = requestTransaction.toTraceparent();
-        appGetInitialProps.pageProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage());
+
+        const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext();
+        appGetInitialProps.pageProps._sentryBaggage =
+          dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
       }
 
       return appGetInitialProps;

diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideErrorGetInitialProps.ts
@@ -1,5 +1,5 @@
 import { hasTracingEnabled } from '@sentry/tracing';
-import { serializeBaggage } from '@sentry/utils';
+import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils';
 import { NextPageContext } from 'next';
 import { ErrorProps } from 'next/error';
 
@@ -52,7 +52,9 @@ export function withSentryServerSideErrorGetInitialProps(
       const requestTransaction = getTransactionFromRequest(req!);
       if (requestTransaction) {
         errorGetInitialProps._sentryTraceData = requestTransaction.toTraceparent();
-        errorGetInitialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage());
+
+        const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext();
+        errorGetInitialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
       }
 
       return errorGetInitialProps;

diff --git a/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts b/packages/nextjs/src/config/wrappers/withSentryServerSideGetInitialProps.ts
@@ -1,5 +1,5 @@
 import { hasTracingEnabled } from '@sentry/tracing';
-import { serializeBaggage } from '@sentry/utils';
+import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils';
 import { NextPage } from 'next';
 
 import { isBuild } from '../../utils/isBuild';
@@ -42,7 +42,9 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit
       const requestTransaction = getTransactionFromRequest(req!);
       if (requestTransaction) {
         initialProps._sentryTraceData = requestTransaction.toTraceparent();
-        initialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage());
+
+        const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext();
+        initialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
       }
 
       return initialProps;

diff --git a/packages/nextjs/src/config/wrappers/wrapperUtils.ts b/packages/nextjs/src/config/wrappers/wrapperUtils.ts
@@ -2,7 +2,7 @@ import { captureException, getCurrentHub, startTransaction } from '@sentry/core'
 import { addRequestDataToEvent } from '@sentry/node';
 import { getActiveTransaction } from '@sentry/tracing';
 import { Transaction } from '@sentry/types';
-import { extractTraceparentData, fill, isString, parseBaggageSetMutability } from '@sentry/utils';
+import { baggageHeaderToDynamicSamplingContext, extractTraceparentData, fill } from '@sentry/utils';
 import * as domain from 'domain';
 import { IncomingMessage, ServerResponse } from 'http';
 
@@ -88,19 +88,19 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
 
     if (requestTransaction === undefined) {
       const sentryTraceHeader = req.headers['sentry-trace'];
-      const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage;
+      const rawBaggageString = req.headers && req.headers.baggage;
       const traceparentData =
         typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined;
 
-      const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
+      const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString);
 
       const newTransaction = startTransaction({
         op: 'nextjs.data.server',
         name: options.requestedRouteName,
         ...traceparentData,
         metadata: {
           source: 'route',
-          baggage,
+          dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
         },
       });
 

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
@@ -1,10 +1,10 @@
 import { getCurrentHub } from '@sentry/hub';
 import { Primitive, TraceparentData, Transaction, TransactionContext, TransactionSource } from '@sentry/types';
 import {
+  baggageHeaderToDynamicSamplingContext,
   extractTraceparentData,
   getGlobalObject,
   logger,
-  parseBaggageHeader,
   stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import type { NEXT_DATA as NextData } from 'next/dist/next-server/lib/utils';
@@ -128,6 +128,7 @@ export function nextRouterInstrumentation(
 
   if (startTransactionOnPageLoad) {
     const source = route ? 'route' : 'url';
+    const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggage);
 
     activeTransaction = startTransactionCb({
       name: prevLocationName,
@@ -137,7 +138,7 @@ export function nextRouterInstrumentation(
       ...traceParentData,
       metadata: {
         source,
-        ...(baggage && { baggage: parseBaggageHeader(baggage) }),
+        dynamicSamplingContext: traceParentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
       },
     });
   }

diff --git a/packages/nextjs/src/utils/instrumentServer.ts b/packages/nextjs/src/utils/instrumentServer.ts
@@ -10,10 +10,10 @@ import {
 import { extractTraceparentData, getActiveTransaction, hasTracingEnabled } from '@sentry/tracing';
 import {
   addExceptionMechanism,
+  baggageHeaderToDynamicSamplingContext,
   fill,
   isString,
   logger,
-  parseBaggageSetMutability,
   stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import * as domain from 'domain';
@@ -255,8 +255,8 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
             __DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
           }
 
-          const rawBaggageString = nextReq.headers && isString(nextReq.headers.baggage) && nextReq.headers.baggage;
-          const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
+          const baggageHeader = nextReq.headers && nextReq.headers.baggage;
+          const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader);
 
           // pull off query string, if any
           const reqPath = stripUrlQueryAndFragment(nextReq.url);
@@ -270,7 +270,7 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
               name: `${namePrefix}${reqPath}`,
               op: 'http.server',
               metadata: {
-                baggage,
+                dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
                 requestPath: reqPath,
                 // TODO: Investigate if there's a way to tell if this is a dynamic route, so that we can make this more
                 // like `source: isDynamicRoute? 'url' : 'route'`

diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts
@@ -3,10 +3,10 @@ import { extractTraceparentData, hasTracingEnabled } from '@sentry/tracing';
 import { Transaction } from '@sentry/types';
 import {
   addExceptionMechanism,
+  baggageHeaderToDynamicSamplingContext,
   isString,
   logger,
   objectify,
-  parseBaggageSetMutability,
   stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import * as domain from 'domain';
@@ -66,8 +66,8 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
             __DEBUG_BUILD__ && logger.log(`[Tracing] Continuing trace ${traceparentData?.traceId}.`);
           }
 
-          const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage;
-          const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData);
+          const baggageHeader = req.headers && req.headers.baggage;
+          const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader);
 
           const url = `${req.url}`;
           // pull off query string, if any
@@ -87,7 +87,10 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
               name: `${reqMethod}${reqPath}`,
               op: 'http.server',
               ...traceparentData,
-              metadata: { baggage, source: 'route' },
+              metadata: {
+                dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
+                source: 'route',
+              },
             },
             // extra context passed to the `tracesSampler`
             { request: req },

diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts
@@ -142,7 +142,7 @@ describe('nextRouterInstrumentation', () => {
           },
           metadata: {
             source: 'route',
-            baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true],
+            dynamicSamplingContext: { environment: 'myEnv', release: '2.1.0' },
           },
           traceId: 'c82b8554881b4d28ad977de04a4fb40a',
           parentSpanId: 'a755953cd3394d5f',
@@ -168,7 +168,7 @@ describe('nextRouterInstrumentation', () => {
           },
           metadata: {
             source: 'route',
-            baggage: [{ environment: 'myEnv', release: '2.1.0' }, '', true],
+            dynamicSamplingContext: { environment: 'myEnv', release: '2.1.0' },
           },
           traceId: 'c82b8554881b4d28ad977de04a4fb40a',
           parentSpanId: 'a755953cd3394d5f',

diff --git a/packages/nextjs/test/utils/withSentry.test.ts b/packages/nextjs/test/utils/withSentry.test.ts
@@ -117,7 +117,6 @@ describe('withSentry', () => {
           op: 'http.server',
 
           metadata: {
-            baggage: expect.any(Array),
             source: 'route',
           },
         },

diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts
@@ -35,7 +35,7 @@ test('Should propagate sentry trace baggage data from an incoming to an outgoing
   });
 });
 
-test('Should propagate empty baggage if sentry-trace header is present in incoming request but no baggage header', async () => {
+test('Should not propagate baggage if sentry-trace header is present in incoming request but no baggage header', async () => {
   const env = await TestEnv.init(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await env.getAPIResponse(`${env.url}/express`, {
@@ -46,12 +46,11 @@ test('Should propagate empty baggage if sentry-trace header is present in incomi
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: '',
     },
   });
 });
 
-test('Should propagate empty sentry and ignore original 3rd party baggage entries if sentry-trace header is present', async () => {
+test('Should not propagate baggage and ignore original 3rd party baggage entries if sentry-trace header is present', async () => {
   const env = await TestEnv.init(__dirname, `${path.resolve(__dirname, '..')}/server.ts`);
 
   const response = (await env.getAPIResponse(`${env.url}/express`, {
@@ -63,7 +62,6 @@ test('Should propagate empty sentry and ignore original 3rd party baggage entrie
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: '',
     },
   });
 });

diff --git a/...ation-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/...ation-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts
@@ -15,7 +15,10 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: 'other=vendor,foo=bar,third=party,last=item,sentry-release=2.1.0,sentry-environment=myEnv',
+      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',
+      ],
     },
   });
 });
@@ -29,9 +32,12 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: expect.stringContaining(
-        'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
-      ),
+      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-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public,sentry-trace_id=[0-9a-f]{32},sentry-sample_rate=1/,
+        ),
+      ],
     },
   });
 });
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/test.ts
@@ -15,7 +15,7 @@ test('should merge `baggage` header of a third party vendor with the Sentry DSC
   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'],
     },
   });
 });