improve and add baggage-related tests

Author: Lms24

packages/nextjs/src/utils/withSentry.ts

@@ -7,7 +7,6 @@ import {
   logger,
   objectify,
   parseAndFreezeBaggageIfNecessary,
-  parseBaggageString,
   stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import * as domain from 'domain';

packages/node/test/handlers.test.ts

@@ -3,7 +3,7 @@ import * as sentryHub from '@sentry/hub';
 import { Hub } from '@sentry/hub';
 import { Transaction } from '@sentry/tracing';
 import { Baggage, Runtime } from '@sentry/types';
-import { SentryError } from '@sentry/utils';
+import { isBaggageEmpty, isBaggageFrozen, SentryError } from '@sentry/utils';
 import * as http from 'http';
 import * as net from 'net';
 
@@ -368,7 +368,9 @@ describe('tracingHandler', () => {
     expect(transaction.traceId).toEqual('12312012123120121231201212312012');
     expect(transaction.parentSpanId).toEqual('1121201211212012');
     expect(transaction.sampled).toEqual(false);
-    expect(transaction.metadata?.baggage).toBeUndefined();
+    expect(transaction.metadata?.baggage).toBeDefined();
+    expect(isBaggageEmpty(transaction.metadata?.baggage)).toBeTruthy();
+    expect(isBaggageFrozen(transaction.metadata?.baggage)).toBeTruthy();
   });
 
   it("pulls parent's data from tracing and baggage headers on the request", () => {
@@ -386,7 +388,7 @@ describe('tracingHandler', () => {
     expect(transaction.parentSpanId).toEqual('1121201211212012');
     expect(transaction.sampled).toEqual(false);
     expect(transaction.metadata?.baggage).toBeDefined();
-    expect(transaction.metadata?.baggage).toEqual([{ version: '1.0', environment: 'production' }, ''] as Baggage);
+    expect(transaction.metadata?.baggage).toEqual([{ version: '1.0', environment: 'production' }, '', true] as Baggage);
   });
 
   it("pulls parent's baggage (sentry + third party entries) headers on the request", () => {
@@ -402,6 +404,7 @@ describe('tracingHandler', () => {
     expect(transaction.metadata?.baggage).toEqual([
       { version: '1.0', environment: 'production' },
       'dogs=great,cats=boring',
+      true,
     ] as Baggage);
   });
 

packages/tracing/src/browser/browsertracing.ts

@@ -1,6 +1,6 @@
 import { Hub } from '@sentry/hub';
 import { EventProcessor, Integration, Transaction, TransactionContext } from '@sentry/types';
-import { getGlobalObject, logger, parseAndFreezeBaggageIfNecessary, parseBaggageString } from '@sentry/utils';
+import { getGlobalObject, logger, parseAndFreezeBaggageIfNecessary } from '@sentry/utils';
 
 import { IS_DEBUG_BUILD } from '../flags';
 import { startIdleTransaction } from '../hubextensions';

packages/tracing/src/span.ts

@@ -4,8 +4,10 @@ import { Baggage, Hub, Primitive, Span as SpanInterface, SpanContext, Transactio
 import {
   createBaggage,
   dropUndefinedKeys,
+  freezeBaggage,
   isBaggageEmpty,
   isBaggageFrozen,
+  isSentryBaggageEmpty,
   setBaggageValue,
   timestampWithMs,
   uuid4,
@@ -313,8 +315,10 @@ export class Span implements SpanInterface {
   public getBaggage(): Baggage | undefined {
     const existingBaggage = this.transaction && this.transaction.metadata.baggage;
 
-    const canModifyBaggage = !(existingBaggage && isBaggageFrozen(existingBaggage));
-    const finalBaggage = canModifyBaggage ? this._getBaggageWithSentryValues(existingBaggage) : existingBaggage;
+    const finalBaggage =
+      !existingBaggage || (!isBaggageFrozen(existingBaggage) && isSentryBaggageEmpty(existingBaggage))
+        ? this._getBaggageWithSentryValues(existingBaggage)
+        : existingBaggage;
 
     return isBaggageEmpty(finalBaggage) ? undefined : finalBaggage;
   }
@@ -364,6 +368,8 @@ export class Span implements SpanInterface {
     environment && setBaggageValue(baggage, 'environment', environment);
     release && setBaggageValue(baggage, 'release', release);
 
+    freezeBaggage(baggage);
+
     return baggage;
   }
 }

packages/tracing/test/browser/browsertracing.test.ts

@@ -1,7 +1,13 @@
 import { BrowserClient } from '@sentry/browser';
 import { Hub, makeMain } from '@sentry/hub';
-import type { BaggageObj, BaseTransportOptions, ClientOptions } from '@sentry/types';
-import { getGlobalObject, InstrumentHandlerCallback, InstrumentHandlerType } from '@sentry/utils';
+import type { Baggage, BaggageObj, BaseTransportOptions, ClientOptions } from '@sentry/types';
+import {
+  getGlobalObject,
+  InstrumentHandlerCallback,
+  InstrumentHandlerType,
+  isBaggageEmpty,
+  isSentryBaggageEmpty,
+} from '@sentry/utils';
 import { JSDOM } from 'jsdom';
 
 import {
@@ -228,7 +234,7 @@ describe('BrowserTracing', () => {
           parentSpanId: 'b6e54397b12a2a0f',
           parentSampled: true,
           metadata: {
-            baggage: [{ release: '2.1.14' }, 'foo=bar'],
+            baggage: [{ release: '2.1.14' }, 'foo=bar', true],
           },
         }),
         expect.any(Number),
@@ -390,7 +396,9 @@ describe('BrowserTracing', () => {
 
         const headerContext = extractTraceDataFromMetaTags();
 
-        expect(headerContext).toBeUndefined();
+        expect(headerContext).toBeDefined();
+        expect(headerContext?.metadata?.baggage).toBeDefined();
+        expect(isBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBeTruthy();
       });
 
       it('does not crash if the baggage header is malformed', () => {
@@ -406,12 +414,14 @@ describe('BrowserTracing', () => {
         expect(baggage && baggage[1]).toBeDefined();
       });
 
-      it("returns undefined if the header isn't there", () => {
+      it("returns default object if the header isn't there", () => {
         document.head.innerHTML = '<meta name="dogs" content="12312012123120121231201212312012-1121201211212012-0">';
 
         const headerContext = extractTraceDataFromMetaTags();
 
-        expect(headerContext).toBeUndefined();
+        expect(headerContext).toBeDefined();
+        expect(headerContext?.metadata?.baggage).toBeDefined();
+        expect(isBaggageEmpty(headerContext?.metadata?.baggage as Baggage)).toBeTruthy();
       });
     });
 
@@ -448,7 +458,7 @@ describe('BrowserTracing', () => {
         expect(baggage[1]).toEqual('foo=bar');
       });
 
-      it('adds Sentry baggage data to pageload transactions if not present in meta tags', () => {
+      it('does not add Sentry baggage data to pageload transactions if sentry-trace data is present but passes on 3rd party baggage', () => {
         // make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one
         document.head.innerHTML =
           '<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">' +
@@ -465,8 +475,7 @@ describe('BrowserTracing', () => {
         expect(transaction.parentSpanId).toEqual('1121201211212012');
         expect(transaction.sampled).toBe(false);
         expect(baggage).toBeDefined();
-        expect(baggage[0]).toBeDefined();
-        expect(baggage[0]).toEqual({ environment: 'production', release: '1.0.0' });
+        expect(isSentryBaggageEmpty(baggage)).toBeTruthy();
         expect(baggage[1]).toBeDefined();
         expect(baggage[1]).toEqual('foo=bar');
       });

packages/tracing/test/span.test.ts

@@ -424,11 +424,11 @@ describe('Span', () => {
       expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
     });
 
-    test('add Sentry baggage data to baggage if Sentry content is empty', () => {
+    test('add Sentry baggage data to baggage if Sentry content is empty and baggage is mutable', () => {
       const transaction = new Transaction(
         {
           name: 'tx',
-          metadata: { baggage: createBaggage({}, '') },
+          metadata: { baggage: createBaggage({}, '', false) },
         },
         hub,
       );

packages/utils/test/baggage.test.ts

@@ -1,8 +1,14 @@
+import { Baggage } from '@sentry/types';
+
 import {
   createBaggage,
+  freezeBaggage,
   getBaggageValue,
+  isBaggageEmpty,
+  isBaggageFrozen,
   isSentryBaggageEmpty,
   mergeAndSerializeBaggage,
+  parseAndFreezeBaggageIfNecessary,
   parseBaggageString,
   serializeBaggage,
   setBaggageValue,
@@ -11,15 +17,23 @@ import {
 describe('Baggage', () => {
   describe('createBaggage', () => {
     it.each([
-      ['creates an empty baggage instance', {}, [{}, '']],
+      ['creates an empty baggage instance', {}, [{}, '', false]],
       [
         'creates a baggage instance with initial values',
         { environment: 'production', anyKey: 'anyValue' },
-        [{ environment: 'production', anyKey: 'anyValue' }, ''],
+        [{ environment: 'production', anyKey: 'anyValue' }, '', false],
       ],
     ])('%s', (_: string, input, output) => {
       expect(createBaggage(input)).toEqual(output);
     });
+
+    it('creates a baggage instance and marks it immutable if explicitly specified', () => {
+      expect(createBaggage({ environment: 'production', anyKey: 'anyValue' }, '', true)).toEqual([
+        { environment: 'production', anyKey: 'anyValue' },
+        '',
+        true,
+      ]);
+    });
   });
 
   describe('getBaggageValue', () => {
@@ -40,6 +54,12 @@ describe('Baggage', () => {
     it.each([
       ['sets a baggage item', createBaggage({}), 'environment', 'production'],
       ['overwrites a baggage item', createBaggage({ environment: 'development' }), 'environment', 'production'],
+      [
+        'does not set a value if the passed baggage item is immutable',
+        createBaggage({ environment: 'development' }, '', true),
+        'environment',
+        'development',
+      ],
     ])('%s', (_: string, baggage, key, value) => {
       setBaggageValue(baggage, key, value);
       expect(getBaggageValue(baggage, key)).toEqual(value);
@@ -98,10 +118,21 @@ describe('Baggage', () => {
     });
   });
 
+  describe('isBaggageEmpty', () => {
+    it.each([
+      ['returns true if the entire baggage tuple is empty', createBaggage({}), true],
+      ['returns false if the Sentry part of baggage is not empty', createBaggage({ release: '10.0.2' }), false],
+      ['returns false if the 3rd party part of baggage is not empty', createBaggage({}, 'foo=bar'), false],
+      ['returns false if both parts of baggage are not empty', createBaggage({ release: '10.0.2' }, 'foo=bar'), false],
+    ])('%s', (_: string, baggage, outcome) => {
+      expect(isBaggageEmpty(baggage)).toEqual(outcome);
+    });
+  });
+
   describe('isSentryBaggageEmpty', () => {
     it.each([
-      ['returns true if the modifyable part of baggage is empty', createBaggage({}), true],
-      ['returns false if the modifyable part of baggage is not empty', createBaggage({ release: '10.0.2' }), false],
+      ['returns true if the Sentry part of baggage is empty', createBaggage({}), true],
+      ['returns false if the Sentry part of baggage is not empty', createBaggage({ release: '10.0.2' }), false],
     ])('%s', (_: string, baggage, outcome) => {
       expect(isSentryBaggageEmpty(baggage)).toEqual(outcome);
     });
@@ -129,4 +160,70 @@ describe('Baggage', () => {
       expect(mergeAndSerializeBaggage(baggage, headerBaggageString)).toEqual(outcome);
     });
   });
+
+  describe('parseAndFreezeBaggageIfNecessary', () => {
+    it.each([
+      [
+        'returns an empty, mutable baggage object if both params are undefined',
+        undefined,
+        undefined,
+        [{}, '', false] as Baggage,
+      ],
+      [
+        'returns an empty, immutable baggage object if sentry-trace header data is defined',
+        undefined,
+        {},
+        [{}, '', true] as Baggage,
+      ],
+      [
+        'returns an empty, immutable baggage object if sentry-trace header data is a string',
+        undefined,
+        '123',
+        [{}, '', true] as Baggage,
+      ],
+      [
+        'returns a non-empty, mutable baggage object if sentry-trace is not defined and only 3rd party baggage items are passed',
+        'foo=bar',
+        undefined,
+        [{}, 'foo=bar', false] as Baggage,
+      ],
+      [
+        'returns a non-empty, immutable baggage object if sentry-trace is not defined and Sentry baggage items are passed',
+        'sentry-environment=production,foo=bar',
+        undefined,
+        [{ environment: 'production' }, 'foo=bar', true] as Baggage,
+      ],
+      [
+        'returns a non-empty, immutable baggage object if sentry-trace is defined',
+        'foo=bar',
+        {},
+        [{}, 'foo=bar', true] as Baggage,
+      ],
+    ])(
+      '%s',
+      (_: string, baggageString: string | undefined, sentryTraceData: any | string | undefined, result: Baggage) => {
+        expect(parseAndFreezeBaggageIfNecessary(baggageString, sentryTraceData)).toEqual(result);
+      },
+    );
+  });
+
+  describe('isBaggageFrozen', () => {
+    it.each([
+      ['returns true if the baggage was set immutable', true],
+      ['returns false if the baggage was set mutable', false],
+    ])('%s', (_: string, outcome) => {
+      const baggage: Baggage = [{}, '', outcome];
+      expect(isBaggageFrozen(baggage)).toEqual(outcome);
+    });
+  });
+
+  describe('freezeBaggage', () => {
+    it.each([
+      ['sets baggage immutable', [{}, '', false] as Baggage],
+      ['does not do anything when baggage is already immutable', [{}, '', true] as Baggage],
+    ])('%s', (_: string, baggage: Baggage) => {
+      freezeBaggage(baggage);
+      expect(baggage[2]).toEqual(true);
+    });
+  });
 });