ref: Rename baggage env header to trace #5128
Conversation
size-limit report 📦
|
| expect(envHeader.baggage).toBeDefined(); | ||
| expect(envHeader.baggage).toEqual( | ||
| expect(envHeader.trace).toBeDefined(); | ||
| expect(envHeader.trace).toEqual( |
There was a problem hiding this comment.
The envelope header is independent of baggage or tracestate encoding. It is directly a JSON object with all the keys relevant to Sentry. See: https://develop.sentry.dev/sdk/performance/trace-context/#envelope-headers
Instead of serializing baggage, please filter for sentry-* keys and directly add them as object to the header.
|
@AbhiPrasad after applying the above suggestion and before merging, I'd like to take this example payload and also check it into Relay's unit test suite for verification, just to make sure we're not missing anything. |
There was a problem hiding this comment.
We need to adjust unit tests for the baggage env header entry creation, too: https://github.com/getsentry/sentry-javascript/blob/250ec10d2577bc98bf2688627873e7e1077e781a/packages/core/test/lib/envelope.test.ts
After reading the Slack thread about the changes, I think we want to send the BaggageObj (i.e. our entries) instead of the serialized baggage string, right? Do we want to make the change in this PR or a follow-up one?
|
We can make these changes in this PR! I'll amend the PR title, description, and JIRA ticket. |
packages/types/src/envelope.ts
Outdated
| @@ -56,7 +56,7 @@ export type SessionItem = | |||
| | BaseEnvelopeItem<SessionAggregatesItemHeaders, SessionAggregates>; | |||
| export type ClientReportItem = BaseEnvelopeItem<ClientReportItemHeaders, ClientReport>; | |||
|
|
|||
| export type EventEnvelopeHeaders = { event_id: string; sent_at: string; baggage?: string }; | |||
| export type EventEnvelopeHeaders = { event_id: string; sent_at: string; trace?: Record<string, unknown> }; | |||
There was a problem hiding this comment.
I first thought, "why not use the BaggageObj type here" but then I realized that we can't because we declared the baggage types in @sentry/utils. For my PR I decided to move them (i.e. Baggage and BaggageObj) to @sentry/types and export them from there because I needed them in the types package. WDYT is this a good idea? If yes, we can either move them already in this PR or make the change afterwards. If not, I'll have to make some adjustments in my PR 😅
There was a problem hiding this comment.
Yes, we can move the types, good idea.
| release: '1.0.0', | ||
| environment: 'prod', | ||
| user: { id: 'bob', segment: 'segmentA' }, | ||
| transaction: 'TX', |
There was a problem hiding this comment.
Thanks, this schema looks correct now based on the reference struct. However, we currently also expect the trace_id and the public_key of the DSN where the trace originated. Is this just missing in this test?
There was a problem hiding this comment.
It's missing in the test, can add!
Matches how this information will be captured and processed in Relay.
AbhiPrasad
force-pushed
the
v7-abhi-baggage-to-trace
branch
from
739eb07 to
5b88771
Compare
ensure that trace_id and public_key are sent in trace header
This patch renames the tracing data envelope header from `baggage` to `trace`. Additionally, it now includes the `trace_id` and `public_key` fields as required by Relay Co-authored-by: Abhijeet Prasad <[email protected]> Co-authored-by: Lukas Stracke <[email protected]>
Matches how this information will be captured and processed in Relay.
See #5104 for init work.
Resolves https://getsentry.atlassian.net/browse/WEB-926