feat(replay): Add traces_by_timestamp to replay event#18048
Open
feat(replay): Add traces_by_timestamp to replay event#18048
traces_by_timestamp to replay event#18048Conversation
Contributor
size-limit report 📦
|
Contributor
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Member
Author
|
@sentry review |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
packages/replay-internal/src/coreHandlers/handleAfterSendEvent.ts
Outdated
Show resolved
Hide resolved
packages/replay-internal/test/integration/coreHandlers/handleAfterSendEvent.test.ts
Show resolved
Hide resolved
Member
Author
|
@sentry review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (event.contexts?.trace?.trace_id && replayContext.traceIds.size < 100) { | ||
| replayContext.traceIds.add(event.contexts.trace.trace_id); | ||
| if (event.contexts?.trace?.trace_id && event.start_timestamp && replayContext.traceIds.length < 100) { | ||
| replayContext.traceIds.push([event.start_timestamp, event.contexts.trace.trace_id]); |
There was a problem hiding this comment.
Bug: Timestamp Handling Ignores Valid Zero Value
The event.start_timestamp check in handleTransactionEvent uses a truthy evaluation, which causes trace IDs to not be recorded for transactions with a start_timestamp of 0. Although rare, 0 is a valid Unix epoch timestamp.
| initialUrl: this._context.initialUrl, | ||
| errorIds: Array.from(this._context.errorIds), | ||
| traceIds: Array.from(this._context.traceIds), | ||
| traceIds: this._context.traceIds, |
There was a problem hiding this comment.
Bug: Shared array reference corrupts replay context
In _popEventContext(), the traceIds array is assigned by reference without creating a copy. When _clearContext() is subsequently called, it only creates a new array via slice(-1) if the length is greater than 1. For arrays with length 0 or 1, both the returned context and this._context share the same array reference. This means subsequent modifications to this._context.traceIds will affect the already-returned event context, potentially corrupting replay event data. The fix should copy the array: traceIds: [...this._context.traceIds] or traceIds: this._context.traceIds.slice().
chargome
reviewed
Nov 10, 2025
| if (this._context.traceIds.length === 0) { | ||
| const currentTraceId = getCurrentScope().getPropagationContext().traceId; | ||
| if (currentTraceId) { | ||
| this._context.traceIds.push([-1, currentTraceId]); |
Member
There was a problem hiding this comment.
Could we put a comment in here explaining the -1 ?
In order to support moving our custom rrweb events to EAP, we need to send timestamps w/ trace ids so that we can identify which trace the event belongs to. In order to avoid breaking changes, we should not change the current type of trace_ids field in the replay event, instead we add a new field traces_by_timestamp
billyvg
force-pushed
the
billy/replay-802-replay-sdk-needs-to-send-timestamps-w-trace-ids
branch
from
1985467 to
84b12da
Compare
Contributor
Codecov Results 📊Generated by Codecov Action |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to support moving our custom rrweb events to EAP, we need to send timestamps w/ trace ids so that we can identify which trace the event belongs to.
In order to avoid breaking changes, we should not change the current type of
trace_idsfield in the replay event, instead we add a new fieldtraces_by_timestampw/ type[transaction.start_timestamp, traceId][].Previously, we would clear all trace ids after each replay segment. so if there is not a new transaction, segments would not have a trace id associated at all. I've changed this so that we always keep the most recent trace id in between segments.
Also previously, in order to associate a replay with a trace, we wait until after a
type: transactionevent is sent successfully. it's possible that the replay integration is loaded after this event is sent and never gets associated with any trace ids. in this case, I've changed it so that we take the trace id from current scope and propagation context, and I use -1 for the timestamp here, but could also change to use current ts.Closes #19201 (added automatically)