ref(tracing): Unify DSC key names in envelope and baggage headers (#5302)

Lms24 · web-flow · commit b9f2122150e0 · 2022-06-24T11:05:00.000+02:00
Unify the key names in the envelope `trace` header and  the `baggage` Http header. As a result, we get rid of the mapping that was necessary before, thereby loosing a little JS bundle weight ;) 
Furthermore, the PR renames the `EventTraceContext` interface and aligns the `Baggage` type so that we can ensure that both DSC transport mechanisms are based on the same keys.
diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts
@@ -1,10 +1,11 @@
 import {
+  Baggage,
   DsnComponents,
+  DynamicSamplingContext,
   Event,
   EventEnvelope,
   EventEnvelopeHeaders,
   EventItem,
-  EventTraceContext,
   SdkInfo,
   SdkMetadata,
   Session,
@@ -100,31 +101,17 @@ function createEventEnvelopeHeaders(
   tunnel: string | undefined,
   dsn: DsnComponents,
 ): EventEnvelopeHeaders {
-  const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
-  const { environment, release, transaction, userid, usersegment, samplerate, publickey, traceid } =
-    (baggage && getSentryBaggageItems(baggage)) || {};
+  const baggage: Baggage | undefined = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
+  const dynamicSamplingContext = baggage && getSentryBaggageItems(baggage);
 
   return {
     event_id: event.event_id as string,
     sent_at: new Date().toISOString(),
     ...(sdkInfo && { sdk: sdkInfo }),
     ...(!!tunnel && { dsn: dsnToString(dsn) }),
     ...(event.type === 'transaction' &&
-      baggage && {
-        trace: dropUndefinedKeys({
-          trace_id: traceid,
-          public_key: publickey,
-          sample_rate: samplerate,
-          environment,
-          release,
-          transaction,
-          ...((userid || usersegment) && {
-            user: {
-              id: userid,
-              segment: usersegment,
-            },
-          }),
-        }) as EventTraceContext,
+      dynamicSamplingContext && {
+        trace: dropUndefinedKeys({ ...dynamicSamplingContext }) as DynamicSamplingContext,
       }),
   };
 }
diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts
@@ -1,4 +1,4 @@
-import { DsnComponents, Event, EventTraceContext } from '@sentry/types';
+import { DsnComponents, DynamicSamplingContext, Event } from '@sentry/types';
 
 import { createEventEnvelope } from '../../src/envelope';
 
@@ -14,13 +14,13 @@ describe('createEventEnvelope', () => {
       expect(envelopeHeaders.trace).toBeUndefined();
     });
 
-    const testTable: Array<[string, Event, EventTraceContext]> = [
+    const testTable: Array<[string, Event, DynamicSamplingContext]> = [
       [
         'adds minimal baggage items',
         {
           type: 'transaction',
           sdkProcessingMetadata: {
-            baggage: [{ traceid: '1234', publickey: 'pubKey123' }, '', false],
+            baggage: [{ trace_id: '1234', public_key: 'pubKey123' }, '', false],
           },
         },
         { trace_id: '1234', public_key: 'pubKey123' },
@@ -30,7 +30,7 @@ describe('createEventEnvelope', () => {
         {
           type: 'transaction',
           sdkProcessingMetadata: {
-            baggage: [{ environment: 'prod', release: '1.0.0', publickey: 'pubKey123', traceid: '1234' }, '', false],
+            baggage: [{ environment: 'prod', release: '1.0.0', public_key: 'pubKey123', trace_id: '1234' }, '', false],
           },
         },
         { release: '1.0.0', environment: 'prod', trace_id: '1234', public_key: 'pubKey123' },
@@ -44,26 +44,27 @@ describe('createEventEnvelope', () => {
               {
                 environment: 'prod',
                 release: '1.0.0',
-                userid: 'bob',
-                usersegment: 'segmentA',
                 transaction: 'TX',
-                samplerate: '0.95',
-                publickey: 'pubKey123',
-                traceid: '1234',
+                user_id: 'bob',
+                user_segment: 'segmentA',
+                sample_rate: '0.95',
+                public_key: 'pubKey123',
+                trace_id: '1234',
               },
               '',
               false,
             ],
           },
         },
         {
-          release: '1.0.0',
           environment: 'prod',
-          user: { id: 'bob', segment: 'segmentA' },
+          release: '1.0.0',
           transaction: 'TX',
-          trace_id: '1234',
-          public_key: 'pubKey123',
+          user_id: 'bob',
+          user_segment: 'segmentA',
           sample_rate: '0.95',
+          public_key: 'pubKey123',
+          trace_id: '1234',
         },
       ],
     ];
diff --git a/packages/integration-tests/suites/tracing/browsertracing/meta/template.html b/packages/integration-tests/suites/tracing/browsertracing/meta/template.html
@@ -4,7 +4,7 @@
     <meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-1" />
     <meta
       name="baggage"
-      content="sentry-release=2.1.12,sentry-publickey=public,sentry-traceid=123,sentry-samplerate=0.3232"
+      content="sentry-release=2.1.12,sentry-public_key=public,sentry-trace_id=123,sentry-sample_rate=0.3232"
     />
   </head>
 </html>
diff --git a/packages/integration-tests/suites/tracing/envelope-header/test.ts b/packages/integration-tests/suites/tracing/envelope-header/test.ts
@@ -5,7 +5,7 @@ import { sentryTest } from '../../../utils/fixtures';
 import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
 
 sentryTest(
-  'should send dynamic sampling context data in transaction envelope header',
+  'should send dynamic sampling context data in trace envelope header of a transaction envelope',
   async ({ getLocalTestPath, page }) => {
     const url = await getLocalTestPath({ testDir: __dirname });
 
@@ -15,10 +15,8 @@ sentryTest(
     expect(envHeader.trace).toEqual({
       environment: 'production',
       transaction: expect.stringContaining('index.html'),
-      user: {
-        id: 'user123',
-        segment: 'segmentB',
-      },
+      user_id: 'user123',
+      user_segment: 'segmentB',
       sample_rate: '1',
       trace_id: expect.any(String),
       public_key: 'public',
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
@@ -78,8 +78,8 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
     test_data: {
       host: 'somewhere.not.sentry',
       baggage: expect.stringContaining(
-        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-samplerate=1,' +
-          'sentry-publickey=public,sentry-traceid=',
+        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
+          'sentry-public_key=public,sentry-trace_id=',
       ),
     },
   });
@@ -99,7 +99,7 @@ test('Should populate Sentry and propagate 3rd party content if sentry-trace hea
       // TraceId changes, hence we only expect that the string contains the traceid key
       baggage: expect.stringContaining(
         'foo=bar,bar=baz,sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
-          'sentry-samplerate=1,sentry-publickey=public,sentry-traceid=',
+          'sentry-public_key=public,sentry-trace_id=',
       ),
     },
   });
diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts
@@ -108,10 +108,11 @@ describe('tracing', () => {
     const baggageHeader = request.getHeader('baggage') as string;
 
     expect(baggageHeader).toBeDefined();
+    expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-userid=uid123,' +
-        'sentry-usersegment=segmentA,sentry-samplerate=1,sentry-publickey=dogsarebadatkeepingsecrets,' +
-        'sentry-traceid=12312012123120121231201212312012',
+      'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_id=uid123,' +
+        'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
+        'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
     );
   });
 
@@ -124,10 +125,11 @@ describe('tracing', () => {
     const baggageHeader = request.getHeader('baggage') as string;
 
     expect(baggageHeader).toBeDefined();
+    expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
       'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
-        'sentry-userid=uid123,sentry-usersegment=segmentA,sentry-samplerate=1,' +
-        'sentry-publickey=dogsarebadatkeepingsecrets,sentry-traceid=12312012123120121231201212312012',
+        'sentry-user_id=uid123,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
+        'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
     );
   });
 
diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts
@@ -1,6 +1,7 @@
 import { getCurrentHub, Hub } from '@sentry/hub';
 import {
   Baggage,
+  BaggageObj,
   Event,
   Measurements,
   Transaction as TransactionInterface,
@@ -10,11 +11,11 @@ import {
 import {
   createBaggage,
   dropUndefinedKeys,
+  getSentryBaggageItems,
+  getThirdPartyBaggage,
   isBaggageMutable,
   isSentryBaggageEmpty,
   logger,
-  setBaggageImmutable,
-  setBaggageValue,
 } from '@sentry/utils';
 
 import { Span as SpanClass, SpanRecorder } from './span';
@@ -228,40 +229,34 @@ export class Transaction extends SpanClass implements TransactionInterface {
     const hub: Hub = this._hub || getCurrentHub();
     const client = hub && hub.getClient();
 
-    const { environment, release } = (client && client.getOptions()) || {};
-    const { publicKey } = (client && client.getDsn()) || {};
-
-    const sampleRate = this.metadata && this.metadata.transactionSampling && this.metadata.transactionSampling.rate;
-    const traceId = this.traceId;
-    const transactionName = this.name;
-
-    let userId, userSegment;
-    hub.configureScope(scope => {
-      const { id, segment } = scope.getUser() || {};
-      userId = id;
-      userSegment = segment;
-    });
-
-    environment && setBaggageValue(baggage, 'environment', environment);
-    release && setBaggageValue(baggage, 'release', release);
-    transactionName && setBaggageValue(baggage, 'transaction', transactionName);
-    userId && setBaggageValue(baggage, 'userid', userId);
-    userSegment && setBaggageValue(baggage, 'usersegment', userSegment);
-    sampleRate &&
-      setBaggageValue(
-        baggage,
-        'samplerate',
-        // This will make sure that expnent notation (e.g. 1.45e-14) is converted to simple decimal representation
-        // Another edge case would be something like Number.NEGATIVE_INFINITY in which case we could still
-        // add something like .replace(/-?∞/, '0'). For the sake of saving bytes, I'll not add this until
-        // it becomes a problem
-        sampleRate.toLocaleString('fullwide', { useGrouping: false, maximumFractionDigits: 16 }),
-      );
-    publicKey && setBaggageValue(baggage, 'publickey', publicKey);
-    traceId && setBaggageValue(baggage, 'traceid', traceId);
-
-    setBaggageImmutable(baggage);
-
-    return baggage;
+    if (!client) return baggage;
+
+    const { environment, release } = client.getOptions() || {};
+    const { publicKey: public_key } = client.getDsn() || {};
+
+    const rate = this.metadata && this.metadata.transactionSampling && this.metadata.transactionSampling.rate;
+    const sample_rate =
+      rate !== undefined
+        ? rate.toLocaleString('fullwide', { useGrouping: false, maximumFractionDigits: 16 })
+        : undefined;
+
+    const scope = hub.getScope();
+    const { id: user_id, segment: user_segment } = (scope && scope.getUser()) || {};
+
+    return createBaggage(
+      dropUndefinedKeys({
+        environment,
+        release,
+        transaction: this.name,
+        user_id,
+        user_segment,
+        public_key,
+        trace_id: this.traceId,
+        sample_rate,
+        ...getSentryBaggageItems(baggage), // keep user-added values
+      } as BaggageObj),
+      getThirdPartyBaggage(baggage), // TODO: remove once we ignore 3rd party baggage
+      false, // set baggage immutable
+    );
   }
 }
diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts
@@ -507,9 +507,9 @@ describe('BrowserTracing', () => {
         expect(baggage[0]).toEqual({
           release: '1.0.0',
           environment: 'production',
-          publickey: 'pubKey',
-          traceid: expect.any(String),
           transaction: 'blank',
+          public_key: 'pubKey',
+          trace_id: expect.not.stringMatching('12312012123120121231201212312012'),
         });
         expect(baggage[1]).toBeDefined();
         expect(baggage[1]).toEqual('');
diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts
@@ -444,8 +444,8 @@ describe('Span', () => {
         release: '1.0.1',
         environment: 'production',
         transaction: 'tx',
-        samplerate: '0.56',
-        traceid: expect.any(String),
+        sample_rate: '0.56',
+        trace_id: expect.any(String),
       });
       expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
     });
@@ -468,8 +468,8 @@ describe('Span', () => {
         release: '1.0.1',
         environment: 'production',
         transaction: 'tx',
-        samplerate: '0.0000000000000145',
-        traceid: expect.any(String),
+        sample_rate: '0.0000000000000145',
+        trace_id: expect.any(String),
       });
       expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
     });
diff --git a/packages/types/src/baggage.ts b/packages/types/src/baggage.ts
@@ -1,12 +1,6 @@
-export type AllowedBaggageKeys =
-  | 'environment'
-  | 'release'
-  | 'transaction'
-  | 'userid'
-  | 'usersegment'
-  | 'samplerate'
-  | 'traceid'
-  | 'publickey';
+import { DynamicSamplingContext } from './envelope';
+
+export type AllowedBaggageKeys = keyof DynamicSamplingContext;
 
 export type BaggageObj = Partial<Record<AllowedBaggageKeys, string> & Record<string, string>>;
 
diff --git a/packages/types/src/envelope.ts b/packages/types/src/envelope.ts
@@ -9,17 +9,15 @@ import { UserFeedback } from './user';
 // Based on: https://develop.sentry.dev/sdk/envelopes/
 
 // Based on https://github.com/getsentry/relay/blob/b23b8d3b2360a54aaa4d19ecae0231201f31df5e/relay-sampling/src/lib.rs#L685-L707
-export type EventTraceContext = {
+export type DynamicSamplingContext = {
   trace_id: Transaction['traceId'];
   public_key: DsnComponents['publicKey'];
   sample_rate?: string;
   release?: string;
-  user?: {
-    id?: string;
-    segment?: string;
-  };
   environment?: string;
   transaction?: string;
+  user_id?: string;
+  user_segment?: string;
 };
 
 export type EnvelopeItemType =
@@ -74,7 +72,7 @@ export type SessionItem =
   | BaseEnvelopeItem<SessionAggregatesItemHeaders, SessionAggregates>;
 export type ClientReportItem = BaseEnvelopeItem<ClientReportItemHeaders, ClientReport>;
 
-export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: EventTraceContext };
+export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: DynamicSamplingContext };
 type SessionEnvelopeHeaders = { sent_at: string };
 type ClientReportEnvelopeHeaders = BaseEnvelopeHeaders;
 
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts
@@ -13,13 +13,13 @@ export type {
   BaseEnvelopeItemHeaders,
   ClientReportEnvelope,
   ClientReportItem,
+  DynamicSamplingContext,
   Envelope,
   EnvelopeItemType,
   EnvelopeItem,
   EventEnvelope,
   EventEnvelopeHeaders,
   EventItem,
-  EventTraceContext,
   SessionEnvelope,
   SessionItem,
   UserFeedbackItem,
