feat(replay): Add traces_by_timestamp to replay event
#18048
Conversation
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
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
size-limit report 📦
|
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.
|
|
@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
|
@sentry review |
…-timestamps-w-trace-ids
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| trace_ids: uniqueTraceIds, | ||
| traces_by_timestamp: traceIds | ||
| .filter(([_ts, traceId]) => uniqueTraceIds.includes(traceId)) | ||
| .map(([ts, traceId]) => [ts, traceId]), |
There was a problem hiding this comment.
Bug: Redundant Filter Causes Unnecessary Performance Overhead
The filter for traces_by_timestamp is redundant. Since uniqueTraceIds is derived directly from traceIds, every trace ID in traceIds will always be present in uniqueTraceIds. This makes the filter a no-op, always returning true, and introduces unnecessary O(n²) performance overhead.
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.