-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
ref(replays): Don't pre-sort Replay Network spans in ReplayReader #49194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
export function replayTimestamps( | ||
replayRecord: ReplayRecord, | ||
rrwebEvents: RecordingEvent[], | ||
rawCrumbs: ReplayCrumb[], | ||
rawSpanData: ReplaySpan[] | ||
rawErrors: any[], | ||
{ | ||
rawRRWebEvents, | ||
rawBreadcrumbs, | ||
rawNetworkSpans, | ||
rawMemorySpans, | ||
}: { | ||
rawBreadcrumbs: any[]; | ||
rawMemorySpans: any[]; | ||
rawNetworkSpans: any[]; | ||
rawRRWebEvents: any[]; | ||
} | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't include erorrs before, so that's added as an arg.
And instead of one spans array, we can accept both network and memory spans separately, so we can split attachements early and not have to recombine things.
@@ -155,9 +159,11 @@ export default class ReplayReader { | |||
) | |||
); | |||
|
|||
getNetworkSpans = memoize(() => this.sortedSpans.filter(isNetworkSpan)); | |||
getNetworkSpans = () => this.networkSpans; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be sorted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there's no need here, because we're going to sort it inside the network table, and in the timeline it will render either way. I'll double check though in both cases because of the problems that cropped up with the other PR
replayRecord.duration = duration( | ||
replayRecord.finished_at.getTime() - replayRecord.started_at.getTime() | ||
); | ||
if (localStorageWrapper.getItem('REPLAY-BACKEND-TIMESTAMPS') !== '1') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is local storage here used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to get rid of this frontend code eventually, and just read data from the backend if we can. So i wanted to create a toggle for testing
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Related to #47991