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. |
Lms24
left a comment
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. |
Lms24
left a comment
There was a problem hiding this comment.
LGTM. Left a question about types but nothing that should be blocking this.
packages/types/src/envelope.ts
Outdated
| 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.
739eb07 to
5b88771
Compare
jan-auer
left a comment
There was a problem hiding this comment.
Compatible with Relay and G2G.
Once the test contains public_key and trace_id, I'll create a PR in Relay to add the same payload to its tests and link it back here.
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 <aprasad@sentry.io> Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Matches how this information will be captured and processed in Relay.
See #5104 for init work.
Resolves https://getsentry.atlassian.net/browse/WEB-926