feat(profiling): add ability to search for frames #31723

JonasBa · 2022-02-10T10:23:40Z

Still very rough - I didnt take most of the design, but the functionality is there (we need to make it more bulletproof and add some limitations). I dont want to optimize anything early, but once we have some traces that we can stress test the search with, we should have a good idea of where the bottlenecks are.

Zylphrex · 2022-02-10T16:03:30Z

static/app/components/profiling/FlamegraphZoomView.tsx

-      // where flamegraphRenderer is not null (see check on L86)
-      if (flamegraphRenderer === null) {
-        return;
+      if (flamegraphRenderer) {


If you change these to use arrow functions, I think the scoping rules ensures that flamegraphRenderer is non null after the check from above

Actually, I think my comment was wrong and why I removed it. Imagine if we at some point clear the flamegraph renderer but forget to clear the handlers, then our scheduler may still call the draw call and it would throw because flamegraphRenderer is now null

Zylphrex · 2022-02-10T16:29:21Z

static/app/components/profiling/FlamegraphZoomView.tsx

+    newScheduler.on('searchResults', (results: Record<string, FlamegraphFrame>) => {
+      newScheduler.unregisterBeforeFrameCallback(drawRectangles);
+
+      function newDrawRectangles() {
+        if (flamegraphRenderer) {
+          flamegraphRenderer.draw(results);
+        }
+      }
+      newScheduler.registerBeforeFrameCallback(newDrawRectangles);
+
+      newScheduler.onDispose(() =>
+        newScheduler.unregisterBeforeFrameCallback(newDrawRectangles)
+      );


not a big fan of this approach, would storing the results as a state be cleaner?

Yeah, I was debating between that and storing the results. The difference here is that if we pass searchresults to this effect, we end up recreating all the renderers (unless we split each renderer to it's own useEffect init), but here we instead just replace the draw call. I think we need to figure out a good sort of "swap" mechanism here, I'm not even sure about the whole canvasScheduler and how well that will scale tbh

If you have some ideas about this, I'd love to hear them. We want a system that allows us to enqueue calls in some order and replace them. Maybe the easiest way here is to just isolate each renderer to it's own effect

I took a pass over in #31639 where I lifted all the renderers out of useEffect using useMemo. In that scenario, I think if we used a ref to store the search results to avoid recreating everything, the .draw immediately after should accomplish what we want here. It's quite a refactor for an already large PR, so I'm happy to leave this as is and resolve this in a follow up.

Zylphrex · 2022-02-10T18:26:51Z

static/app/components/profiling/FlamegraphSearch.tsx

+          for (let i = 0; i < allFrames.length; i++) {
+            const frame = allFrames[i];
+
+            if (userQuery.test(frame.frame.name.trim())) {


Is this going to cause problems when the global flag is set in the regex? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test#using_test_on_a_regex_with_the_global_flag

Yes it will indeed. Wdyt would be a good way of handling it? we could disable /g flag, but we need to add a warning for it then

We could recreate the regex on every iteration or manually reset lastIndex

That's what I went with in the end, I was a bit worried we may have a large performance overhead, but then I read that it seems that regexp objects are pretty heavily optimized and cached. Not going to worry about it until it becomes a problem

Zylphrex

Mostly just missing dependencies, otherwise, looks good to me

Zylphrex · 2022-02-14T15:22:32Z

static/app/components/profiling/FlamegraphSearch.tsx

+        }
+      }
+
+      return results;


Nit: could just return at the end of the if block instead of returning in both the try and catch

Zylphrex · 2022-02-14T15:40:11Z

static/app/components/profiling/FlamegraphSearch.tsx

+      threshold: 0.3,
+      includeMatches: true,
+    });
+  }, [allFrames.length]);


Shouldn't this be allFrames?

Zylphrex · 2022-02-14T15:46:26Z

static/app/components/profiling/FlamegraphSearch.tsx

+      canvasPoolManager.dispatch('zoomIntoFrame', [frame]);
+      setSelectedNode(frame);
+    },
+    [canvasPoolManager]


Should setSelectedNode be in the dependencies here?

It's not necessary, react guarantees that the setState fn is stable between rerenders (https://reactjs.org/docs/hooks-reference.html)

Note
React guarantees that setState function identity is stable and won’t change on re-renders. This is why it’s safe to > omit from the useEffect or useCallback dependency list.

Zylphrex · 2022-02-14T15:54:29Z

static/app/components/profiling/FlamegraphSearch.tsx

+      setSearchResults(results);
+      canvasPoolManager.dispatch('searchResults', [results]);
+    },
+    [searchIndex, frames, canvasPoolManager]


This is missing setSearchResults and allFrames

Zylphrex · 2022-02-14T16:14:47Z

static/app/components/profiling/FlamegraphSearch.tsx

+        setOpen(false);
+      }
+    },
+    [open, setSearchResults]


Add setOpen?

Zylphrex · 2022-02-14T16:15:05Z

static/app/components/profiling/FlamegraphSearch.tsx

+        onPreviousSearchClick();
+      }
+    },
+    [onNextSearchClick, onPreviousSearchClick, setSearchResults]


Add setOpen?

JonasBa · 2022-02-14T17:20:54Z

@Zylphrex I think I'll add the react-hooks-exhaustive dependencies to our eslint config, the linting should have caught this. But good eye 👁️

github-actions · 2022-02-15T08:30:46Z

size-limit report 📦

Path	Size
src/sentry/static/sentry/dist/entrypoints/app.js	54 KB (-0.02% 🔽)
src/sentry/static/sentry/dist/entrypoints/sentry.css	68.96 KB (0%)

* feat(flamegraph): add view select menu * feat(profiling): use buttonbar * feat(profiling): add search * style(lint): Auto commit lint changes * style(lint): Auto commit lint changes * feat(profiling): decouple search fn * fix(flamegraphsearch): missing hook deps Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>

* feat(profiling): flamegraph * feat(profiling): add flamegraph renderer and tests * fix(profiling): ignore unused renderer in test * fix(profiling): ignore unused renderer in test * feat(profiling): add util hooks * ref(hook): add usememowithprevious tests * feat(profiling): zoom view * fix(flamegraphrenderer): adjust for profiles that do not start at 0 and trim text more accurately * feat(profiling): add tooltip component * fix(flamegraph): remove merge marker * test(utils): fix tests * fix(tooltip): code review * feat(tooltip): add useDevicePixelRatio hook * feat(profiling): add view select menu (#31698) * feat(flamegraph): add view select menu * feat(profiling): use buttonbar * fix(viewselectmenu): use locale * test(gridrenderer): update tests * feat(profiling): add ability to search for frames (#31723) * feat(flamegraph): add view select menu * feat(profiling): use buttonbar * feat(profiling): add search * style(lint): Auto commit lint changes * style(lint): Auto commit lint changes * feat(profiling): decouple search fn * fix(flamegraphsearch): missing hook deps Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com> * fix(flamegraphtooltip): update deps Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>

JonasBa added 3 commits February 10, 2022 09:03

feat(flamegraph): add view select menu

cebadfa

feat(profiling): use buttonbar

e1a4630

feat(profiling): add search

ac15883

JonasBa requested a review from a team as a code owner February 10, 2022 10:23

JonasBa requested a review from Zylphrex February 10, 2022 10:23

style(lint): Auto commit lint changes

1870aa3

vercel bot deployed to Preview – sentry February 10, 2022 10:24 View deployment

vercel bot deployed to Preview – storybook February 10, 2022 10:24 View deployment

style(lint): Auto commit lint changes

7ddc2f3

vercel bot deployed to Preview – sentry February 10, 2022 10:27 View deployment

vercel bot deployed to Preview – storybook February 10, 2022 10:27 View deployment

Zylphrex reviewed Feb 10, 2022

View reviewed changes

JonasBa force-pushed the jb/profiling/flamegraph-view-select branch from fff0334 to 64af0cd Compare February 11, 2022 07:27

feat(profiling): decouple search fn

9c92b29

vercel bot deployed to Preview – storybook February 14, 2022 12:32 View deployment

vercel bot deployed to Preview – sentry February 14, 2022 12:32 View deployment

JonasBa force-pushed the jb/profiling/flamegraph-view-select branch from 64af0cd to d59c70b Compare February 14, 2022 12:35

Zylphrex approved these changes Feb 14, 2022

View reviewed changes

Base automatically changed from jb/profiling/flamegraph-view-select to jb/profiling/tooltip February 15, 2022 08:18

fix(flamegraphsearch): missing hook deps

aaf8e38

vercel bot deployed to Preview – sentry February 15, 2022 08:25 View deployment

vercel bot deployed to Preview – storybook February 15, 2022 08:25 View deployment

Merge branch 'jb/profiling/tooltip' into jb/profiling/flamegraph-search

5d5a436

vercel bot deployed to Preview – sentry February 15, 2022 08:28 View deployment

vercel bot deployed to Preview – storybook February 15, 2022 08:28 View deployment

JonasBa merged this pull request into jb/profiling/tooltip Feb 15, 2022

JonasBa deleted the jb/profiling/flamegraph-search branch February 15, 2022 08:38

github-actions bot locked and limited conversation to collaborators Mar 2, 2022

Uh oh!

feat(profiling): add ability to search for frames #31723

feat(profiling): add ability to search for frames #31723

Uh oh!

Conversation

JonasBa commented Feb 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zylphrex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonasBa commented Feb 14, 2022

Uh oh!

github-actions bot commented Feb 15, 2022

size-limit report 📦

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants