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

Lms24 · 2022-06-02T09:04:04Z

Following #5133, this PR adds the sentry-environment and sentry-release values to the baggage HTTP headers of outgoing requests. We add these values to baggage as late as possible with the rationale that other baggage values (to be added in the future) will not be available right away, e.g. when intercepting baggage from incoming requests.

As a result, the flow described below happens, when the first call to span.getBaggage is made (e.g. in callbacks of instrumented outgoing requests):

In span.getBaggage, check if there is baggage present in the span (in which case it was intercepted from incoming headers/meta tags) and it is empty (or only includes 3rd party data) OR if there is no baggage yet
In both of these cases, populate the baggage with Sentry data (and leave 3rd party content untouched). Else do nothing
Add this baggage to outgoing headers (and merge with possible 3rd party header)

Furthermore, the PR adds and improves tests to check correct handling and propagation of baggage

This PR does not yet sync the Http header baggage information with the one that's sent in the trace envelope header. This will be addressed in a follow-up PR.

EDIT: As discussed with the team, this PR does not fully address all (im)mutability cases. Will open up a separate PR soon to fix this.

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

…aggage headers

github-actions · 2022-06-02T09:10:26Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.43 KB (-3.53% 🔽)
@sentry/browser - ES5 CDN Bundle (minified)	60.2 KB (-6.83% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.23 KB (-3.32% 🔽)
@sentry/browser - ES6 CDN Bundle (minified)	53.82 KB (-7.15% 🔽)
@sentry/browser - Webpack (gzipped + minified)	19.99 KB (-13.96% 🔽)
@sentry/browser - Webpack (minified)	65.19 KB (-20.22% 🔽)
@sentry/react - Webpack (gzipped + minified)	20.02 KB (-14.01% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified)	43.95 KB (-8.54% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.56 KB (-1.98% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.11 KB (-1.52% 🔽)

lforst

Minor remarks. Good tests!!

lforst · 2022-06-03T12:55:26Z

packages/tracing/src/span.ts

@@ -302,7 +311,14 @@ export class Span implements SpanInterface {
   * @inheritdoc
   */
  public getBaggage(): Baggage | undefined {
-    return this.transaction && this.transaction.metadata.baggage;
+    const existingBaggage = this.transaction && this.transaction.metadata.baggage;
+


Suggested change

// Add sentry baggage data to baggage, only when baggage is either empty or does not exist yet

Took me a while to get what's happening here. Wdyt about adding a comment like this to speed things up for future readers?

Agreed, a note would be good. I slightly rewrote this part (and the condition on when to modify baggage) in the follow-up PR (#5205). Added a note there (50e0d19) instead because adding it here would require it to be rewritten.

lforst · 2022-06-03T12:56:16Z

packages/tracing/src/span.ts

@@ -334,6 +350,24 @@ export class Span implements SpanInterface {
      trace_id: this.traceId,
    });
  }
+
+  /**
+   *


I think we forgot to add a comment here ^^

Yes, good catch, thanks! Same thing though as above, I made modifications in #5205 (2a95a39) and added the JSdoc there.

lforst · 2022-06-03T13:03:15Z

packages/tracing/src/span.ts

+   */
+  private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage {
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
+    const hub: Hub = ((this.transaction as any) && (this.transaction as any)._hub) || getCurrentHub();


To me this seems a bit dangerous. I was wondering if we can get away with only having getCurrentHub() here, because in a node context, we'll get the right hub anyways because of domains, and in a browser context we only have one proper hub anyways (?).

I'll let you make the final call on this though. It's probably a bit dangerous but should also be fine.

I think it should be fine if we take the Hub from the transaction. When creating a Transaction, users can pass in a hub as an option in the TransactionContext. Also for example when calling hub.startTransaction. People who use a multi-hub/client setup (which comes with its own set of issues) do this sometimes.

Do you think it's dangerous because there might be a "wrong" hub in the transaction or because of the casts?

Because of the casts.

lforst · 2022-06-03T13:06:31Z

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

    },
  });
 });

-test('Should assign `baggage` header which contains sentry trace baggage data of an outgoing request.', async () => {
+test('Should assign `baggage` header which contains sentry trace baggage data to an outgoing request.', async () => {


Can we say in this test title explaining, that we don't overwrite baggage that's already on the outgoing request?

Yes, thanks for bringing this up. Changed this title and the one below it
(this makes me think that I should add more tests covering different mutability cases here in #5205)

lforst · 2022-06-03T13:09:59Z

packages/tracing/test/span.test.ts

+
+  describe('getBaggage and _getBaggageWithSentryValues', () => {
+    beforeEach(() => {
+      hub.getClient()!.getOptions = () => {


Do you think it's necessary to add a cleanup for this? Wanna avoid flakeyness in the future.

I was thinking the same thing when I wrote the tests here but then I saw that at the top of this file there already is a beforeEach function. This means that the modifications on the hub from my beforeSend, are reverted as soon as we leave the closure and move to other tests.

Got it. Seems good.

Lms24 added 2 commits June 2, 2022 10:34

feat(tracing): add propagation of environment and release values in b…

97fbd3a

…aggage headers

adjust node integration tests

296f4c2

Lms24 added 2 commits June 2, 2022 11:19

fix node unit test type errors

d243198

Improve node and tracing tests

8e78e7c

Lms24 marked this pull request as ready for review June 2, 2022 11:11

Lms24 requested review from lforst, AbhiPrasad and lobsterkatie June 2, 2022 11:11

Lms24 mentioned this pull request Jun 3, 2022

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

Merged

lforst approved these changes Jun 3, 2022

View reviewed changes

change test descriptions (node integration tests)

dd44ae7

lforst approved these changes Jun 3, 2022

View reviewed changes

Lms24 merged commit 1456b9c into master Jun 7, 2022

Lms24 deleted the lms-baggage-env-release branch June 7, 2022 08:02

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

vladanpaunovic mentioned this pull request Aug 30, 2022

Populate and propagate dynamic sampling context when Python is head of trace getsentry/sentry-python#1587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Lms24 commented Jun 2, 2022 •

edited

github-actions bot commented Jun 2, 2022 •

edited

lforst left a comment

lforst Jun 3, 2022

Lms24 Jun 3, 2022

lforst Jun 3, 2022

Lms24 Jun 3, 2022

lforst Jun 3, 2022

Lms24 Jun 3, 2022 •

edited

lforst Jun 3, 2022

lforst Jun 3, 2022

Lms24 Jun 3, 2022

Lms24 Jun 3, 2022

lforst Jun 3, 2022

Lms24 Jun 3, 2022

lforst Jun 3, 2022



	// Add sentry baggage data to baggage, only when baggage is either empty or does not exist yet

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

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

Conversation

Lms24 commented Jun 2, 2022 • edited

github-actions bot commented Jun 2, 2022 • edited

size-limit report 📦

lforst left a 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

Choose a reason for hiding this comment

Lms24 Jun 3, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Jun 2, 2022 •

edited

github-actions bot commented Jun 2, 2022 •

edited

Lms24 Jun 3, 2022 •

edited