ref(replay): refactor `replayerStepper` to be called less #74540

michellewzhang · 2024-07-18T23:04:20Z

relates to https://github.com/getsentry/team-replay/issues/450
TLDR: big experimental refactor of replayerStepper and all the functions relating to it. following up with a slightly better implementation (ref(replay): refactor replayerStepper calls to be memoized #74606), but this PR gives context behind it all.

current state of things:
we have distinct replayerStepper calls in the breadcrumbs + memory tabs. in total, we could be calling it 3 times because it's used by
(1) the breadcrumbs tab to render the HTML snippet,
(2) for hydration error text diffs,
(3) and the DOM nodes chart.

the plan:
improve things by calling replayerStepper once (or at least less) and memoize the results. side note: it was hard to combine the hydration error text diff replayerStepper call with the others due to the types, so we had to keep that call separate.

the question:
is it faster to run one replayerStepper instance, and collect all the data at one time (iterate over the frames exactly once)? this means the speed of any tab will always be limited by the slower call, since we're going through ALL the data at once, even some that we might not need yet. Like this:

sentry/static/app/utils/replays/replayReader.tsx

Lines 363 to 366 in 70c9054

    
           visitFrameCallbacks: { 
        
             extractDomNodes, 
        
             countDomNodes: countDomNodes(this.getRRWebMutations()), 
        
           },

or is it better to continue to have 2 stepper instances that iterate over 2 different sets of frames (breadcrumbs loops over hundreds of frames, and DOM node count iterates over thousands, which means the speeds could be drastically different). in this situation, each tab would handle their own array of necessary frames.

this PR:
this PR tried to explore a refactor where replayerStepper is called once for the two DOM node actions (counting, for the memory chart, and extracting, for the breadcrumbs HTML).

results:
in this PR, changes were made to the stepper so it could accept a list of callbacks, and return all types of data in one shot, so we only need one hidden replayer instance on the page for the DOM node functions. unscientifically, it seemed like loading breadcrumbs + memory took the same amount of time as loading the memory chart. (see video below -- as soon as the breadcrumbs tab is done loading, the memory tab is already done).

Screen.Recording.2024-07-19.at.11.49.24.AM.mov

this makes sense because we’re iterating over thousands of frames for the DOM nodes chart, so that’s the bottleneck. this means the breadcrumbs tab loads slower.

compared to that is the approach where we have 1 stepper instance for breadcrumbs, and one for DOM node count, each iterating over their own lists (will make a separate PR for that one shortly: #74606). this approach showed breadcrumb data on the screen about 2x as fast as the approach above. therefore 2 stepper instances on the screen is better for users, especially since breadcrumbs tab is more popular than the memory tab.

michellewzhang · 2024-07-19T18:34:00Z

static/app/views/replays/detail/memoryPanel/index.tsx

 import DomNodesChart from 'sentry/views/replays/detail/memoryPanel/domNodesChart';
 import MemoryChart from 'sentry/views/replays/detail/memoryPanel/memoryChart';

-function useCountDomNodes({replay}: {replay: null | ReplayReader}) {


moved to its own file,useCountDomNodes.tsx

michellewzhang · 2024-07-19T18:43:36Z

static/app/utils/replays/extractHtml.tsx

-export default function extractDomNodes({
-  frames,
-  rrwebEvents,
-  startTimestampMs,
-}: Args): Promise<Map<ReplayFrame, Extraction>> {
-  return replayerStepper({
-    frames,
-    rrwebEvents,
-    startTimestampMs,
-    shouldVisitFrame: frame => {
-      const nodeId = getNodeId(frame);
-      return nodeId !== undefined && nodeId !== -1;
-    },
-    onVisitFrame: (frame, collection, replayer) => {
-      const mirror = replayer.getMirror();
-      const nodeId = getNodeId(frame);
-      const html = extractHtml(nodeId as number, mirror);
-      collection.set(frame as ReplayFrame, {
-        frame,
-        html,
-        timestamp: frame.timestampMs,
-      });
-    },
-  });
-}


moved to replayReader.tsx

michellewzhang · 2024-07-19T18:45:22Z

static/app/utils/replays/countDomNodes.tsx

moved to replayReader.tsx

michellewzhang · 2024-07-19T18:45:51Z

static/app/utils/replays/extractPageHtml.tsx

moved to useExtractPageHtml.tsx

michellewzhang · 2024-07-19T18:47:13Z

static/app/utils/replays/hooks/useExtractedDomNodes.tsx

renamed to useExtractDomNodes.tsx

michellewzhang · 2024-07-19T18:47:24Z

static/app/utils/replays/hooks/useExtractedPageHtml.tsx

renamed to useExtractPageHtml.tsx

- closes getsentry/team-replay#450 - this is the branch off from #74540, the experimental refactor of `replayerStepper` - TLDR: memoize the `replayerStepper` calls so that we remember the results and don't have to reload the data every time. **current state of things**: we have distinct `replayerStepper` calls in the breadcrumbs + memory tabs. in total, we could be calling it 3 times because it's used by (1) the breadcrumbs tab to render the HTML snippet, (2) for hydration error text diffs, (3) and the DOM nodes chart. **the plan**: improve things by calling `replayerStepper` once (or at least less) and memoize the results. side note: it was hard to combine the hydration error text diff `replayerStepper` call with the others due to the types, so we had to keep that call separate. **the question**: is it faster to run one `replayerStepper` instance, and collect all the data at one time (iterate over the frames exactly once)? this means the speed of any tab will always be limited by the slower call, since we're going through ALL the data at once, even some that we might not need yet. like this: https://github.com/getsentry/sentry/blob/70c90544f39dcc3d87dea8c85c0a7669c9585403/static/app/utils/replays/replayReader.tsx#L363-L366 or is it better to continue to have 2 stepper instances that iterate over 2 different sets of frames (breadcrumbs loops over hundreds of frames, and DOM node count iterates over thousands, which means the speeds could be drastically different). in this situation, each tab would handle their own array of necessary frames. **the experiments**: the first PR (#74540) tried to explore a refactor where `replayerStepper` is called once for the two DOM node actions (counting, for the memory chart, and extracting, for the breadcrumbs HTML). changes were made to the stepper so it could accept a list of callbacks, and return all types of data in one shot, so we only need one hidden `replayer` instance on the page for the DOM node functions. unscientifically, it seemed like loading breadcrumbs + memory took the same amount of time as loading the memory chart. (see video below -- as soon as the breadcrumbs tab is done loading, the memory tab is already done). https://github.com/user-attachments/assets/2c882bcb-c8df-409c-8e2a-69d75c279735 this makes sense because we’re iterating over thousands of frames for the DOM nodes chart, so that’s the bottleneck. this means the breadcrumbs tab loads slower. **this PR**: compared to that is the approach where we have 1 stepper instance for breadcrumbs, and one for DOM node count, each iterating over their own lists (which is what i'm doing in this PR). this approach showed breadcrumb data on the screen about 2x as fast as the approach above. therefore 2 stepper instances on the screen is better for users, especially since breadcrumbs tab is more popular than the memory tab. the video below demonstrates the memoization in action. once the breadcrumbs or memory tabs are loaded, switching back to them does not cause re-loading, because the results are cached. notice that the breadcrumbs tab loading is not dependent on the loading of the DOM tab, unlike the video above where the breadcrumbs tab has to "wait" for the memory tab. (i'm also on slower wifi in this clip below) https://github.com/user-attachments/assets/be71add1-63b5-4308-9d26-356d544980ef

ref(replay): refactor replayerStepper to be called once

7782d24

vercel bot deployed to Preview July 18, 2024 23:07 View deployment

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 19, 2024

♻️ ref

e31f109

vercel bot deployed to Preview July 19, 2024 06:08 View deployment

♻️ consolidate & standardize hooks

ec15b19

vercel bot deployed to Preview July 19, 2024 06:20 View deployment

vercel bot deployed to Preview July 19, 2024 06:36 View deployment

🚚 rename files

f3154c6

michellewzhang force-pushed the mz/replayer-ref branch from 758cdd2 to f3154c6 Compare July 19, 2024 06:38

vercel bot deployed to Preview July 19, 2024 06:40 View deployment

♻️ some types

af73b5a

vercel bot deployed to Preview July 19, 2024 06:52 View deployment

🏷️ types

873e5c9

vercel bot deployed to Preview July 19, 2024 07:00 View deployment

♻️ ref so it works

38867b2

michellewzhang changed the title ~~ref(replay): refactor replayerStepper to be called once~~ ref(replay): refactor replayerStepper to be called less Jul 19, 2024