ref(tracing): Check and set mutability of baggage #5205

Lms24 · 2022-06-03T11:39:20Z

This PR addresses the mutability aspect of baggage. It introduces a third item to our Baggage tuple which is a mutability flag. The reason for adding this flag is to set the mutability of the baggage object when we receive one, either from incoming requests or <meta> tags in the browser. It's necessary to decide about mutability at this time because the decision depends on the sentry-trace header. Since we're lazily populating baggage when it gets sent (propagated), we have to know at that time, if we are allowed to add sentry- entries to it, or if we should just pass it along as is. The only way to cover all cases of when we're allowed to make changes is to set the flag at the incoming time and check it at population time.

Specifically, we identify and handle mutability in the following cases:

Incoming request w/ baggage that contains sentry-* items (and possibly 3rd party items):
- At incoming request time (IRT): Parse and store baggage on the span. Set it immutable
- At baggage propagation time (BPT): Propagate baggage
Incoming request w/o baggage header:
- if sentry-trace is present
  - IRT: create empty baggage object and set it to immutable
  - BPT: propagate baggage
- else,
  - IRT: create empty baggage object (to be populated later) and leave it mutable
  - BPT: populate, set it immutable, propagate it
Incoming request with baggage header that only contains 3rd party items: Same as above. In both cases, we propagate the 3rd party baggage.
Incoming request w/o any of the two headers:
- IRT: create empty baggage object (to be populated later) and leave it mutable
- BPT: populate, set it immutable, propagate it
No baggage set on the span at BPT: populate, set it immutable, propagate it

Checking for the presence of the sentry-trace header is important because if it is present, we know that the trace was already started which means that whatever handler is receiving it, cannot modify baggage anymore. Getting a sentry-trace but no baggage header, can (and will) for example occur, when the SDK that started the trace does not yet support and propagate baggage.

ref: https://getsentry.atlassian.net/browse/WEB-935

github-actions · 2022-06-03T11:45:36Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.43 KB (added)
@sentry/browser - ES5 CDN Bundle (minified)	60.21 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.24 KB (added)
@sentry/browser - ES6 CDN Bundle (minified)	53.83 KB (added)
@sentry/browser - Webpack (gzipped + minified)	20 KB (added)
@sentry/browser - Webpack (minified)	65.2 KB (added)
@sentry/react - Webpack (gzipped + minified)	20.02 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.02 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.63 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.17 KB (added)

lforst · 2022-06-07T13:33:20Z

packages/node/test/handlers.test.ts

+    expect(isBaggageEmpty(transaction.metadata?.baggage)).toBeTruthy();
+    expect(isBaggageMutable(transaction.metadata?.baggage)).toBeFalsy();


I recommend doing .toBe(true/false) instead of .toBeTruthy()/.toBeFalsy() unless we actually wanna check truthy/falsyness.

Good point, thanks! see e2ca92a

lforst · 2022-06-07T13:34:49Z

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

+  });
+});
+
+test('Should populate and propagate sentry baggage if sentry-trace header does not exist', async () => {


Much better test names! :)

lforst · 2022-06-07T13:39:59Z

packages/utils/src/baggage.ts

+export function setBaggageImmutable(baggage: Baggage): void {
+  baggage[2] = false;
+}


(FP nerdiness on) Did we evaluate here, if it's possible/feasable/sensical to return a new Baggage object/array here, instead of mutating the one we pass in? (FP nerdiness off)

Haha I agree that it would be much nicer FP-wise. I think that this applies to many of the functions in the baggage API. So if we change it, I think we should change it for all functions that mutate baggage. Which overall makes me think that this should be its own PR because we'll probably have to make quite a few changes then.

The one point I can see that makes this a little less desireable is that it increases bundles size (albeit not a lot). Not saying that it's not worth that. Just putting it out there.

@AbhiPrasad do you have thoughts on this?

I typically favour the FP approach - but I would rather stick with mutation

less bytes

we have great test coverage

don't have to go back and change stuff now

If this ends up causing bugs and confusion, then we come back and revisit.

true 2. true 3.true - let's ship it lol

lforst · 2022-06-07T13:44:10Z

packages/utils/src/baggage.ts

+  traceparentData: TraceparentData | string | false | undefined | null,
+): Baggage {
+  const baggage = parseBaggageString(rawBaggageString || '');
+  if (!isSentryBaggageEmpty(baggage) || (traceparentData && isSentryBaggageEmpty(baggage))) {


Just because I haven't quite groked yet why we set the baggage to immutable here, if there is traceparentData: Is traceparent data connected to baggage in some way? Could you quickly explain here or maybe even add a comment?

Sure, so the thing is that traceParentData is the processed/parsed sentry-trace header. sentry-trace and baggage go hand in hand which is why we have to consider both of them if we make the mutability descision (see PR description). I used traceParentData instead of the raw header string because I think it saves us a few bytes of bundle.

Your comment made me realize though that I was thinking that I should change the traceparentData param name. Will do that now.
EDIT: d97ad84

Makes sense. Thank you!

lforst · 2022-06-07T13:53:26Z

packages/utils/test/baggage.test.ts

@@ -129,4 +160,70 @@ describe('Baggage', () => {
      expect(mergeAndSerializeBaggage(baggage, headerBaggageString)).toEqual(outcome);
    });
  });
+
+  describe('parseBaggageSetMutability', () => {
+    it.each([


I put this in abhi's original PR but at what point can I convince you to write non-table tests here? :D This is so unwieldy

Btw no action required here!

I do remember the comment ;D

So I think that for some of the functions I could change it because the table has a length of 2. So in that case I think it could be changed to two individual tests.
However, for the parseBaggageSetMutability strings, I prefer the parametrized/tabled tests because I would essentially just copy the same code over and over again for different input. So this spares us a little duplicated code.
I do agree that at first this style of test writing is a little harder to read in the first place but I don't think that the alternative (i.e. 6 very similar/identical tests) isn't better either.

On a side note: Having worked a lot with testing during my Master's thesis, I don't like parametrized tests because they're harder to parse and to handle than single-input unit tests if you write testing tools. But that's about it. Generally I think, parametrized tests are a good thing to have, also as a first step if we ever want to try out more sophisticated testing approaches. I'm thinking of e.g. property-based testing where we would just define predicates how the SUT should behave and the input is generated randomly. But yeah, just some random thoughts on testing there, pretty off-topic 😅

Sounds good! Thanks for the insight ^^

Lms24 changed the title ~~ref(tracing): Ensure baggage (im)mutability~~ ref(tracing): Check and set mutability of baggage Jun 3, 2022

Lms24 mentioned this pull request Jun 3, 2022

feat(tracing): Propagate environment and release values in baggage Http headers #5193

Merged

Lms24 force-pushed the lms-baggage-immutability branch 4 times, most recently from f2d7de9 to df74f42 Compare June 7, 2022 09:38

Lms24 marked this pull request as ready for review June 7, 2022 09:47

Lms24 requested review from AbhiPrasad, lforst and lobsterkatie June 7, 2022 12:16

Lms24 mentioned this pull request Jun 7, 2022

ref(tracing): Sync baggage data in Http and envelope headers #5218

Merged

lforst approved these changes Jun 7, 2022

View reviewed changes

Lms24 force-pushed the lms-baggage-immutability branch from d97ad84 to dd445e0 Compare June 7, 2022 15:40

lforst approved these changes Jun 7, 2022

View reviewed changes

Lms24 force-pushed the lms-baggage-immutability branch from dd445e0 to 688269e Compare June 8, 2022 11:42

vladanpaunovic assigned Lms24 Jun 10, 2022

Lms24 added 12 commits June 14, 2022 10:35

ref(tracing): Check mutability state of incoming baggage headers

b0927a9

modify Span.getBaggage() to only mutate when flag allows it

66a9b4c

improve and add baggage-related tests

e2021b4

fix tests (and bug) in serverless package

2fbdf0b

possibly fix last serverless test

a8f9b6a

next try

1e7cb72

add note about getBaggage() baggage modification

06b7acf

Add JSDoc for _getBaggageWithSentryValues function

7144fc3

add some more node integration tests

0845bf2

rename mutability helper functions

61d3c94

change expect calls from truthy/falsy to true/false checks

89a0d9b

rename parseBaggageSetMutability parameter (sentryTraceData)

2e76e36

Lms24 force-pushed the lms-baggage-immutability branch from 688269e to 2e76e36 Compare June 14, 2022 08:35

Lms24 merged commit 7309b5d into master Jun 14, 2022

Lms24 deleted the lms-baggage-immutability branch June 14, 2022 08:51

AbhiPrasad added this to the Dynamic Sampling milestone Jun 14, 2022

antonpirker mentioned this pull request Jun 21, 2022

Add Dynamic Sampling Context + Baggage Spec getsentry/develop#611

Closed

		expect(isBaggageEmpty(transaction.metadata?.baggage)).toBeTruthy();
		expect(isBaggageMutable(transaction.metadata?.baggage)).toBeFalsy();

Uh oh!

ref(tracing): Check and set mutability of baggage #5205

ref(tracing): Check and set mutability of baggage #5205

Uh oh!

Conversation

Lms24 commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lms24 commented Jun 3, 2022 •

edited

Loading

github-actions bot commented Jun 3, 2022 •

edited

Loading

Lms24 Jun 7, 2022 •

edited

Loading

Lms24 Jun 7, 2022 •

edited

Loading