feat: Span Streaming - #1400
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
This looks mostly good to me, thanks for taking this on!
Two comments we should address:
- the timing issue when merging startup and pageload spans
- do we still need users to set
spanStreamingIntegration()?
All other comments are nits/recommendations, so feel free to disregard them/follow-up on in later PRs.
| } | ||
|
|
||
| /** | ||
| * Normalizes profile_chunk envelope items and returns the modified envelope |
There was a problem hiding this comment.
l: This just modifies the span envelope, right?
| * Normalizes profile_chunk envelope items and returns the modified envelope | |
| * Normalizes span envelope items and returns the modified envelope |
|
|
||
| let modifiedEnvelope = createEnvelope(modifiedHeader); | ||
| let isSpanContainer = false; | ||
| let transactionOrigin: string | undefined; |
There was a problem hiding this comment.
l: I'd recommend we rename this to segmentOrigin so that we don't conflate the "new" spans with the old transaction concept. Would also apply to the consumer of this function. Obviously just logaf-l, so feel free to disregard.
| // Attributes on the streamed pageload segment that describe the renderer segment itself (its | ||
| // identity and SDK metadata) rather than the measurements and trace metadata (Web Vitals, | ||
| // connection/device info, etc.) that should be merged onto the startup span. | ||
| const NON_INHERITED_SEGMENT_ATTRIBUTES = new Set([ |
There was a problem hiding this comment.
l: We can use @sentry/conventions/attributes to avoid the hard-coded strings and get notified about attribute deprecations. Not directly related to span streaming but IMHO a small win over the hard coded string list here. However, we still need to keep the list itself around so it's not a huge win... feel free to disregard or do as a follow-up. Just a recommendation.
| processSpan(span) { | ||
| span.name = normalizeUrlToBase(span.name, app.getAppPath()); | ||
| }, | ||
| processSegmentSpan(span, client) { |
There was a problem hiding this comment.
I think we can get rid of the processSegmentSpan hook here: All spans go through processSpan including segment spans.
| debug: true, | ||
| tracesSampleRate: 1.0, | ||
| traceLifecycle: 'stream', | ||
| integrations: [Sentry.spanStreamingIntegration()], |
There was a problem hiding this comment.
l: I think we don't need to set spanStreamingIntegration() explicitly anymore, right? All clients except for BrowserClient now add the integration, depending on traceLifecycle.
(If you want to keep the test as-is to cover users still explicitly adding spanStreamingIntegration, that's fine, too! no objections!)
| * Merges spans streamed from the renderer (when `traceLifecycle: 'stream'` is used) into the | ||
| * startup span, mirroring {@link applyRendererSpansAndMeasurements} for the streamed span format. | ||
| */ | ||
| function applyStreamedRendererSpans(parentSpan: Span, spans: SerializedStreamedSpan[], endTimestamp: number): number { |
There was a problem hiding this comment.
m/h: What would happen if the pageload span tree gets sent in multiple envelopes? With streaming we have no guarantee that we wait until the pageload segment span ends. So the browser SDK could send child spans in one envelope, and only later more spans (including the segment) in another. We flush every 5 seconds by default, so this could happen for long-running pageload traces.
IIUC, here we stitch up the pageload segment span and its children into the startup span trace (parentSpan). So previously sent child spans might reference the wrong pageload parent span, since we start a new one here, no?
I might be missing something though that makes this not become an issue 😅. If it is indeed an issue, I wonder if we could buffer incoming span envelopes until we get the pageload segment. Then, we ignore/forward all buffered spans that are not part of the pageload trace as-is, and then merge all pageload-trace spans together before we hand them to applyStreamedRendererSpans. WDYT?
2 participants
Tests span streaming mode and ensures and
startupTracingIntegrationworks when this mode is enabled.