feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

Lms24 · 2022-06-22T08:32:19Z

This PR adds several additional Dynamic Sampling Context items to be collected, populated and propagated via baggage or sent via envelope headers. The propagated/sent data now conforms with the specs proposed in getsentry/develop#613.

With this PR, the following fields were added to DSC:

Note that trace id and public key were already previously sent with envelope headers but the SDK would send its own values instead of the trace's values in case it is not the head-of-trace SDK. This PR also fixes that, in the sense that all trace envelope header data items are directly taken from the DSC we either received from an incoming baggage header or from the one we populate instead.

In addition to these fields, this PR makes the following changes:

store final baggage in event.sdkProcessingMetadata (was previously stored in event.contexts.baggage which was wrong, caused a bug and caused the baggage to even be sent to Sentry (which we do not want in this form).
fix a bug where populated DSC (which was set immutable) would not be stored on the span. This bug would have affected data consistency across multiple outgoing requests in one transaction, where the second/third/etc outgoing requests would potentially still find a mutable baggage object and populate it again
rename _getBaggageDataWithSentryValues to _populateBaggageDataWithSentryValues because it better describes the purpose of the function

There are still more open tasks to address to improve DSC propagation (e.g. new handling of 3rd party baggage, adjusting naming to DSC rather than baggage) but this will be done in a follow-up PR.

Fixes #5290

…ge and envelope headers

github-actions · 2022-06-22T08:40:09Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.39 KB (added)
@sentry/browser - ES5 CDN Bundle (minified)	59.95 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.18 KB (added)
@sentry/browser - ES6 CDN Bundle (minified)	53.57 KB (added)
@sentry/browser - Webpack (gzipped + minified)	19.96 KB (added)
@sentry/browser - Webpack (minified)	65.02 KB (added)
@sentry/react - Webpack (gzipped + minified)	19.98 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified)	44.25 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.79 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.26 KB (added)

lforst

Looking good! One minor thing and one I'm not entirely sure on.

lforst · 2022-06-23T08:18:39Z

packages/core/src/envelope.ts

@@ -101,27 +100,24 @@ function createEventEnvelopeHeaders(
  tunnel: string | undefined,
  dsn: DsnComponents,
 ): EventEnvelopeHeaders {
-  const baggage = event.contexts && (event.contexts.baggage as BaggageObj);
-  const { environment, release, transaction, userid, usersegment } = baggage || {};
+  const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;


Yup, having this in the sdkProcessingMetadata makes so much more sense.

lforst · 2022-06-23T08:27:49Z

packages/tracing/src/span.ts

+    const { publicKey } = (client && client.getDsn()) || {};
+
+    const metadata = this.transaction && this.transaction.metadata;
+    const sampelRate = metadata && metadata.transactionSampling && metadata.transactionSampling.rate;


s/sampelRate/sampleRate/ :D

Whoops 😅

lforst · 2022-06-23T08:41:07Z

packages/hub/src/scope.ts

+    // Since we're storing dynamic sampling context data in the event.sdkProcessingMetadata
+    // field We have to re-apply it after we applied the Scope's field.
+    // (This is because we're storing this data on the span and not on the scope)
+    const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage;
+    event.sdkProcessingMetadata = this._sdkProcessingMetadata || {};
+    event.sdkProcessingMetadata.baggage = baggage;


I'm wondering. This already seemed kinda sketchy before you changed it. Did this line just overwrite/wipe the entire metadata from before? Would it be an option here to just merge them instead (like we do with the breadcrumbs and basically everything else (extra, tags, user, ...) above)?

Like this for example:

event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, ...this._sdkProcessingMetadata }; // We probably want this ordering

Wdyt?

Did this line just overwrite/wipe the entire metadata from before?

Yes, everything that was on event.sdkProcessingMetadata.

I had the same feeling when writing this hack. Decided to go with it anyway because I thought that there was a reason for why we do this.
However, I just tried your suggestion and our tests still seem to pass, with it in, so I'll take it. Thanks :D

lforst · 2022-06-23T08:45:59Z

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

+        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-samplerate=1,' +
+          'sentry-publickey=public,sentry-traceid=',


Doesn't really matter here and it's very subjective but I personally avoid breaking up strings this way, simply because it's less "searchable". No action required though - just wanted to get this thought out ^^

Yup but I also don't like super long strings that heavily exceed the characters-per-line limit. Very annoying when working with split editor windows 😅

lforst · 2022-06-23T09:33:11Z

packages/tracing/src/span.ts

@@ -367,15 +374,46 @@ export class Span implements SpanInterface {
   *
   * @returns modified and immutable baggage object
   */
-  private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage {
+  private _populateBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage {
+    // Because of a cicular dependency, we cannot import the Transaction class here, hence the type casts


As discussed: Root cause of the circular dependency is, that we have some of the Dynamic Sampling Context functionality on the Span class, while it should probably all live on the Transaction class.

We will clean this up in a follow-up PR so no required action on this for now.

Almost done with it already. Will make a PR soon

Simplify applyToEvent logic

feat(tracing): Add additional dynamic sampling context items to bagga…

a550b9d

…ge and envelope headers

Lms24 marked this pull request as draft June 22, 2022 08:33

Lms24 changed the title ~~feat(tracing): Add additional dynamic sampling context items to bagggae and envelope headers~~ feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers Jun 22, 2022

marandaneto mentioned this pull request Jun 22, 2022

Bump Sentry JavaScript 7.3.1 getsentry/sentry-react-native#2306

Merged

8 tasks

Lms24 added 2 commits June 22, 2022 14:47

add other missing DSC values + adjust tests

cd78621

fix populated DSC not being stored on span

a47cb46

Lms24 marked this pull request as ready for review June 22, 2022 13:24

Lms24 requested review from AbhiPrasad and lforst June 22, 2022 13:24

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

Lms24 mentioned this pull request Jun 22, 2022

[DSC] include tracesSampleRate in Dynamic Sampling Context #5290

Closed

handle exponential sample rate notation

8054398

lforst reviewed Jun 23, 2022

View reviewed changes

Apply code review suggestions

3f40e64

Simplify applyToEvent logic

Lms24 requested a review from lforst June 23, 2022 10:55

lforst approved these changes Jun 23, 2022

View reviewed changes

Lms24 merged commit ea9ddaf into master Jun 23, 2022

Lms24 deleted the lms-dsc-all-the-entries branch June 23, 2022 12:40

Lms24 mentioned this pull request Jun 23, 2022

ref(tracing): Move getBaggage() from Span to Transaction class #5299

Merged

marandaneto mentioned this pull request Jun 27, 2022

Dynamic Sampling baggage header changes getsentry/team-mobile#23

Closed

6 tasks

pelov mentioned this pull request Jul 26, 2022

Exception "Locale 'fullwide' is not well-formed" under Internet Explorer 11 #5466

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

Lms24 commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

lforst left a comment

lforst Jun 23, 2022

lforst Jun 23, 2022

Lms24 Jun 23, 2022

lforst Jun 23, 2022

Lms24 Jun 23, 2022

lforst Jun 23, 2022

Lms24 Jun 23, 2022

lforst Jun 23, 2022 •

edited

Lms24 Jun 23, 2022

		'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-samplerate=1,' +
		'sentry-publickey=public,sentry-traceid=',

feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

feat(tracing): Add additional Dynamic Sampling Context items to baggage and envelope headers #5292

Conversation

Lms24 commented Jun 22, 2022 • edited

github-actions bot commented Jun 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lforst Jun 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Jun 22, 2022 •

edited

github-actions bot commented Jun 22, 2022 •

edited

lforst Jun 23, 2022 •

edited