Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jun 17, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jun 7, 2022

This PR is the continuation of our efforts of propagating baggage data along outgoing Http Requests and to Relay in envelope headers. It syncs the content of the trace envelope header with the baggage Http header we propagate with outgoing requests. This means that both headers will from now on have the same baggage contents (the envelope header has additional fields that are not part of Baggage).

This also means that from now on, the trace envelope header conforms with the immutability requirements set on baggage. Both headers will call Span.getBaggage() to get the baggage data. This method in turn checks if it is allowed to add values to the baggage (see #5205 for more details`). If it is, it will gather all values that are available at that time and mark baggage immutable for potential future calls. This ensures that both headers will get the same data and send consistent data to the next SDK (via outgoing requests) and Relay.

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.35 KB (added)
@sentry/browser - ES5 CDN Bundle (minified) 59.85 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.15 KB (added)
@sentry/browser - ES6 CDN Bundle (minified) 53.49 KB (added)
@sentry/browser - Webpack (gzipped + minified) 19.92 KB (added)
@sentry/browser - Webpack (minified) 64.85 KB (added)
@sentry/react - Webpack (gzipped + minified) 19.95 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified) 43.9 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.52 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.04 KB (added)

@Lms24 Lms24 force-pushed the lms-sync-env-header branch from c680f33 to e04d1b1 Compare June 7, 2022 15:37
@Lms24 Lms24 force-pushed the lms-sync-env-header branch from e04d1b1 to 16877b9 Compare June 14, 2022 09:19
@Lms24 Lms24 self-assigned this Jun 14, 2022
@Lms24 Lms24 marked this pull request as ready for review June 14, 2022 12:54
@Lms24 Lms24 requested review from AbhiPrasad and lobsterkatie June 14, 2022 12:54
@AbhiPrasad AbhiPrasad added this to the Dynamic Sampling milestone Jun 14, 2022
Comment on lines 508 to 510
normalized.contexts = {};
normalized.contexts.trace = event.contexts.trace;
normalized.contexts.baggage = event.contexts.baggage;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if line 508 is a problem. I thought that contexts could hold arbitrary data 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup contexts can hold arbitrary data!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this line shouldn't exist, right? I checked and it was added in #5171 to normalize trace context data but IIUC what's going on here, we're accidentally erasing all other contexts except the ones we're adding back (which is why I added line 510).

Copy link
Member Author

@Lms24 Lms24 Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the line for the time being in eebeb6b (happy to revisit this if it's a problem). I checked, the other elements in normalized.contexts are normalized a few lines above:

...(event.contexts && {
contexts: normalize(event.contexts, depth, maxBreadth),
}),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have that one for trace because of

// event.contexts.trace stores information about a Transaction. Similarly,
// event.spans[] stores information about child Spans. Given that a
// Transaction is conceptually a Span, normalization should apply to both
// Transactions and Spans consistently.
// For now the decision is to skip normalization of Transactions and Spans,
// so this block overwrites the normalized event to add back the original
// Transaction information prior to normalization.
.

That said, perhaps it's time to rethink that.

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few small questions/suggestions, but otherwise looks good!

@AbhiPrasad
Copy link
Member

As was indicated in #5171 (comment), this will fix #5261 as per the comment here: #5261 (comment)

@Lms24 Lms24 merged commit dfe7d61 into master Jun 17, 2022
@Lms24 Lms24 deleted the lms-sync-env-header branch June 17, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants