feat(utils): Introduce Baggage API #5066

AbhiPrasad · 2022-05-10T21:02:20Z

This patch introduced a basic data structure for baggage, as well as controls to create, manipulate and serialize the baggage data structure.

The baggage data structure represents key,value pairs based on the baggage spec: https://www.w3.org/TR/baggage

It is expected that users interact with baggage using the helpers methods: createBaggage, getBaggageValue, and setBaggageValue.

Internally, the baggage data structure is a tuple of length 2, separating baggage values based on if they are related to Sentry or not. If the baggage values are set/used by sentry, they will be stored in an object to be easily accessed. If they are not, they are kept as a string to be only accessed when serialized at baggage propagation time.

As a next step, let's add the baggage values to the envelope header so they can be processed and used by relay - this will allow us to do some early validations of our assumptions.

Resolves https://getsentry.atlassian.net/browse/WEB-910

github-actions · 2022-05-10T21:14:43Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	18.75 KB (-6.91% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	58.19 KB (-9.95% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.5 KB (-7.24% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	52.43 KB (-9.56% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.33 KB (-16.84% 🔽)
@sentry/browser - Webpack (minified)	61.44 KB (-24.82% 🔽)
@sentry/react - Webpack (gzipped + minified)	19.35 KB (-16.88% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	42.81 KB (-10.92% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	24.41 KB (-6.39% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	22.96 KB (-6.24% 🔽)

Lms24

Hope you don't mind me reviewing a draft but I wanted to dive in deeper into the Baggage API discussions. Overall, this looks good to me. I left some questions to better understand some aspects. Will review again when this is ready for review.

Oh, one more question: Why are we adding this to utils rather than to the tracing package? I thought that Baggage is only useful when people install tracing?
EDIT: nvm I think I figured it out: If we added it to tracing, we couldn't add the baggage header to outgoing requests without importing @sentry/tracing in other packages (e.g. browser/core?) which we don't want to do, right? To get rid of the dependency again, we'd need something similar to hubextensions to do that which adds more complexity (?)

Lms24 · 2022-05-11T09:38:26Z

packages/utils/src/baggage.ts

+export type AllowedBaggageKeys = 'environment' | 'release'; // TODO: Add remaining allowed baggage keys | 'transaction' | 'userid' | 'usersegment';
+export type Baggage = Partial<Record<AllowedBaggageKeys, string> & Record<string, string>>;


Probably a dumb question but what's the advantage of making this a record instead of creating e..g a Baggage interface with the baggage keys as properties?

A record type is just sugar over creating a interface: https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkeys-type - this just makes is so that to add additional keys we just need to add to the union type, not to the interface.

Jumping on this thread, I would vote for just making this an interface if it's not too much of an effort to change. Currently it is quite hard to read.

I agree that an interface is easier to understand. Kinda had to analyse what this type is but after reading the TS docs it makes more sense to me. Plus I wonder if we need the flexibility of the Record because I guess those properties won't change too often (might be wrong about it though).
But as long as we only need the types internally, I think we can live with it either way. If users are expected to use it, I'd vote for an interface for easier understandability.

type BaggageObj = Partial<Record<AllowedBaggageKeys, string> & Record<string, string>>;

desugars down to

type BaggageObj = { // comes from Record<string, string> [x: string]: string | undefined; // comes from Record<AllowedBaggageKeys, string> environment?: string | undefined; release?: string | undefined; }

IMO the union type makes it clear how the two object types (created by the Record type) will come together to form the final object structure. In addition, users have to use the functions to interact with baggage - it should be sufficiently abstracted from them.

packages/utils/src/baggage.ts

AbhiPrasad · 2022-05-12T19:16:10Z

This is now ready to review!

Lms24

LGTM! Just some small remarks left but then this is ready to go IMO

packages/utils/src/baggage.ts

lforst

Just want to get one train of thought in: Isn't (because of immutability) the only operation we really need something like:

// Does not mutate already existing keys.
function addValueToBaggageString(
  baggageString: string,
  key: 'sentry-environment' | 'sentry-release',
  value: string
): string;
  
// or

function addValuesToBaggageString(
  baggageString: string,
  keyValuePairs: ['sentry-environment' | 'sentry-release', string][],
): string;

What use cases did you have in mind for the baggage object? Do we ever read singular values from the string?

EDIT: Ok just noticed the mutating section in the spec. Question still is: Do we ever want to mutate?

lforst · 2022-05-13T08:47:13Z

packages/utils/src/baggage.ts

+}
+
+/** Parse a baggage header to a string */
+export function parseBaggageString(baggageString: string): Baggage {


Suggested change

export function parseBaggageString(baggageString: string): Baggage {

export function parseBaggageString(inputBaggageString: string): Baggage {

Nit: Can be anything but there bagageString is in the same scope twice.

lforst · 2022-05-13T08:49:34Z

packages/utils/src/baggage.ts

+/** Serialize a baggage object */
+export function serializeBaggage(baggage: Baggage): string {
+  return Object.keys(baggage[0]).reduce((prev, key) => {
+    const val = baggage[0][key as keyof BaggageObj] as string;


Nit: as keyof BaggageObj is not needed here

lforst · 2022-05-13T08:52:40Z

packages/utils/src/baggage.ts

+ * If they are not, they are kept as a string to be only accessed when serialized
+ * at baggage propagation time.
+ */
+export type Baggage = [BaggageObj, string];


In my opinion the readability of all of the functions in this file (and also their type definitions) suffer a lot from this being a tuple. I'd much rather have an interface with descriptive keys. But feel free to overrule me on this one!

lforst · 2022-05-13T09:06:59Z

packages/utils/test/baggage.test.ts

+    it.each([
+      ['creates an empty baggage instance', {}, [{}, '']],
+      [
+        'creates a baggage instance with initial values',
+        { environment: 'production', anyKey: 'anyValue' },
+        [{ environment: 'production', anyKey: 'anyValue' }, ''],
+      ],
+    ])('%s', (_: string, input, output) => {


This is very high up on the cleverness scale 😄 What do you think about splitting this up into separate tests for readability?

I'm a big fan of table driven tests, and I believe they lend themselves well to maintainable and extensible unit tests. Picked it up from the golang ecosystem: https://dave.cheney.net/2019/05/07/prefer-table-driven-tests.

It removes the boilerplate, and makes it easy to understand the that the setup + usage of the function is the exact same over the various scenarios. If the setup ever needs to change, just create a new test block!

AbhiPrasad · 2022-05-16T14:48:21Z

The immutability/mutability concern in the API is very valid, but considering that we are just looking to test stuff right now (and there are other open questions about this) - I'm going to go ahead and table that conversation for future iterations of the API.

For now, let's get this in and start testing with Relay!

lforst · 2022-05-16T14:54:50Z

The immutability/mutability concern in the API is very valid, but considering that we are just looking to test stuff right now (and there are other open questions about this) - I'm going to go ahead and table that conversation for future iterations of the API.

Agree, sounds good to me!

This patch introduced a basic data structure for baggage, as well as controls to create, manipulate and serialize the baggage data structure. The baggage data structure represents key,value pairs based on the baggage spec: https://www.w3.org/TR/baggage It is expected that users interact with baggage using the helpers methods: `createBaggage`, `getBaggageValue`, and `setBaggageValue`. Internally, the baggage data structure is a tuple of length 2, separating baggage values based on if they are related to Sentry or not. If the baggage values are set/used by sentry, they will be stored in an object to be easily accessed. If they are not, they are kept as a string to be only accessed when serialized at baggage propagation time. As a next step, let's add the baggage values to the envelope header so they can be processed and used by relay - this will allow us to do some early validations of our assumptions.

Lms24 reviewed May 11, 2022

View reviewed changes

feat(utils): Introduce Baggage API

c7f0099

AbhiPrasad force-pushed the v7-abhi-basic-baggage-api branch from 42d0a6f to ce42be8 Compare May 12, 2022 15:55

add tests

0a2811d

AbhiPrasad force-pushed the v7-abhi-basic-baggage-api branch from ce42be8 to 0a2811d Compare May 12, 2022 16:29

AbhiPrasad added 2 commits May 12, 2022 14:55

description

0068cb5

spelling

8310ca7

AbhiPrasad added this to the Dynamic Sampling milestone May 12, 2022

AbhiPrasad marked this pull request as ready for review May 12, 2022 19:07

Lms24 approved these changes May 13, 2022

View reviewed changes

packages/utils/src/baggage.ts Outdated Show resolved Hide resolved

packages/utils/src/baggage.ts Outdated Show resolved Hide resolved

lforst reviewed May 13, 2022

View reviewed changes

Lms24 mentioned this pull request May 13, 2022

feat(core): Send Baggage in Envelope Header #5104

Merged

address PR review

644a00b

AbhiPrasad merged commit 0a6b4f2 into 7.x May 17, 2022

AbhiPrasad deleted the v7-abhi-basic-baggage-api branch May 17, 2022 15:57

antonpirker mentioned this pull request Jun 21, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utils): Introduce Baggage API #5066

feat(utils): Introduce Baggage API #5066

AbhiPrasad commented May 10, 2022 •

edited

Loading

github-actions bot commented May 10, 2022 •

edited

Loading

Lms24 left a comment •

edited

Loading

Lms24 May 11, 2022

AbhiPrasad May 11, 2022

lforst May 11, 2022

Lms24 May 11, 2022

AbhiPrasad May 12, 2022

AbhiPrasad commented May 12, 2022

Lms24 left a comment

lforst left a comment •

edited

Loading

lforst May 13, 2022

lforst May 13, 2022

lforst May 13, 2022

lforst May 13, 2022

AbhiPrasad May 16, 2022

AbhiPrasad commented May 16, 2022

lforst commented May 16, 2022

		export type AllowedBaggageKeys = 'environment' \| 'release'; // TODO: Add remaining allowed baggage keys \| 'transaction' \| 'userid' \| 'usersegment';
		export type Baggage = Partial<Record<AllowedBaggageKeys, string> & Record<string, string>>;

	export function parseBaggageString(baggageString: string): Baggage {
	export function parseBaggageString(inputBaggageString: string): Baggage {

feat(utils): Introduce Baggage API #5066

feat(utils): Introduce Baggage API #5066

Conversation

AbhiPrasad commented May 10, 2022 • edited Loading

github-actions bot commented May 10, 2022 • edited Loading

size-limit report 📦

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented May 12, 2022

Lms24 left a comment

Choose a reason for hiding this comment

lforst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad commented May 16, 2022

lforst commented May 16, 2022

AbhiPrasad commented May 10, 2022 •

edited

Loading

github-actions bot commented May 10, 2022 •

edited

Loading

Lms24 left a comment •

edited

Loading

lforst left a comment •

edited

Loading