Support zoom by click in flame graph

[andjsrk]

Production | Deploy preview

Closes #969

You can zoom in/out flame graph by clicking a box(bar).

Design Questions

Initial Selection

(Implementation detail: the implementation is based on selected call node state.)
The profiler currently determines initially selected call node using some heuristics.
That feature does not only expand call tree, but also set selected call node. The selected call node is shared in tabs, so if you select a call node in call tree tab, it persists in flame graph tab. It means that, if you switch tab to flame graph, what you see is probably unexpected. See below:

Expected:

[A] [B ]
[C ][D  ] [E][F ]
[G       ][H     ]
[I                ] (root node)

Actual:

[ A          ]
[ C               ] (selected using heuristic)
[ G               ]
[ I               ] (root node)

Actually, I experienced this while implementing the feature, and I thought I made a weird bug or there is a bug in state management.
Fixing it to only expand call tree seems desirable to me, although I'm not sure if it is simple as I haven't looked into it closely.

Click on Background

Currently clicking background of graph deselects boxes(bars), leading to get back to the root, which is annoying IMO. Should we keep it selected even we click background?

Help Needed

New color style and its name

It seems that introducing new color style to contrast ascendants with descendants of the selected call node would be good. For your information, "color style" refers to:

profiler/src/utils/colors.ts

Lines 30 to 33 in 3d5a1c1

	type ColorStyles = {
	readonly _selectedFillStyle: string | [string, string];
	readonly _unselectedFillStyle: string | [string, string];
	readonly _selectedTextColor: string | [string, string];

Anyway, the problem is:

• What should it be called?
• I'm not sure what exact color should be used

For name, I'm considering "uninterested": we zoom in a box(bar), so its ascendants are uninterested at this time.

Comments appreciated!

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.45%. Comparing base (8ac20f5) to head (a3cead6).
⚠️ Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/flame-graph/Canvas.tsx	92.30%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5893      +/-   ##
==========================================
+ Coverage   85.44%   85.45%   +0.01%     
==========================================
  Files         321      321              
  Lines       32061    32094      +33     
  Branches     8753     8843      +90     
==========================================
+ Hits        27393    27426      +33     
  Misses       4236     4236              
  Partials      432      432              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[andjsrk]

IDK why, but my editor did not report any formatting/linting diagnostics, so I accidentally interpreted below as "there is no error, go ahead":

$ yarn lint
$ node bin/output-fixing-commands.js run-p lint-js lint-css prettier-run
💡 You might be able to fix the error by running `yarn lint-fix`
Done in 0.27s.

I thought "The output seems it implies there is a problem... but there is no editor diagnostic at all. Ah, comes to think of it, formatters/linters usually print where the problem is, and there is none! I guess it is designed to always print that message, let's just commit it."... Well, actually Prettier and ESLint do print them, but bin/output-fixing-commands.js discards the output. Unfortunate. Maybe I could create an issue for this?

EDIT: Wait, something is weird, there is { stdio: 'inherit' } on where the process spawn. What is going on? Specifying shell: true as well resolved the problem. Without shell: true, it does not recognize what prettier is :(

[mstange]

This is cool! It'll take me a while to get used to it, but I think it's worth doing because it'll be familiar to users of SVG flame graphs.

Some things I've noticed so far:

• The "initial zoom" caused by the calltree's initial selection behavior is indeed quite confusing.
• I was missing some ambient indication showing me by how much I'm currently zoomed in (or indeed that I'm zoomed in at all).
• You're right, the ancestors of the zoomed call node have to be styled differently. This will help with the "you're probably zoomed in a bit" indication but it won't tell me by how much I'm zoomed in.
• Maybe we need a "minimap" that shows the full flame graph and a narrow rectangle for the zoomed part?
• Even if the call tree stopped doing its initial selection behavior, I think it would still be strange to select something in the call tree and then switch to the flame graph to find it zoomed to that selected call node. So I think the "flame graph zoom node" needs to be a separate state from the selected call node.
• We also need to figure out what should happen if you click a sample in the activity graph while you're looking at a (potentially zoomed) flame graph.

I don't have any concrete answers to your questions yet.

For the linting troubles, could you file a separate issue?

[mstange]

Maybe we could give the zoom node ancestor boxes a horizontal line at the bottom whose width reflects the amount of zoom? This would make them look different and also indicate how much you're zoomed in. Though if you're zoomed to 1%, the line would be 1% wide and be easy to miss. Ideally, the more you zoomed in, the more present should be the reminder that you're zoomed in. Hmm.

Also, I think the flame graph zoom node should be included in the URL, so that you can easily share the view you're looking at.

[mstange]

Ok, how about this:

1. Redefine the "selected fill color" to be the "zoom fill color", use it for the zoom node and its ancestors.
2. For the actual selected node, instead put a black or white 2px border around the box.

But for the zoom node and its ancestors, instead of filling the entire box with the zoom fill color, subdivide the box as follows:

    +------------+------------+------------------------+
25% | unselected |  zoom fill |      unselected        |
    +------------+            +------------------------+
    |                                                  |
75% |                                                  |
    |                                                  |
    +--------------------------------------------------+

and the horizontal position and width of the zoom fill "tab" at the top is based on where this box would have been located in the original unzoomed flame graph.

[andjsrk]

Maybe we need a "minimap" that shows the full flame graph and a narrow rectangle for the zoomed part?

minimap itself seems good idea, but I have no idea where to place it:

• corner? I guess it is hard to recognize in wide screens.
• center? probably it would be visually annoying.
• centered toast when clicked a node? it is inconvenient that you need to click the selected node if you want to check where you at, and maybe is bad for accessibility.

---

Even if the call tree stopped doing its initial selection behavior, I think it would still be strange to select something in the call tree and then switch to the flame graph to find it zoomed to that selected call node. So I think the "flame graph zoom node" needs to be a separate state from the selected call node.

I agree, I'll make a separate state from it.

---

We also need to figure out what should happen if you click a sample in the activity graph while you're looking at a (potentially zoomed) flame graph.

Currently, if we select a range with no activity, it displays:

The flame graph is empty for “(track name)”

Would it be confusing that applying this behavior for zoomed nodes as well?

[mstange]

minimap itself seems good idea, but I have no idea where to place it:

Let's implement the segmented fill idea first, and then we can check if we still need a minimap.

Currently, if we select a range with no activity, it displays:

Ok yes but that's not what I meant by clicking the activity graph. I mean clicking on a filled pixel in the graph (without moving your mouse). This currently changes the selected call node.

The flame graph is empty for “(track name)”

Would it be confusing that applying this behavior for zoomed nodes as well?

I'm not sure I follow. Can you share an example profile and say what steps to take to get into the state that you're suggesting a behavior change for?

Currently clicking background of graph deselects boxes(bars), leading to get back to the root, which is annoying IMO. Should we keep it selected even we click background?

I agree, let's not unset the zoom when you click outside. But we do need a way to reset the zoom. Yes you can click ancestor nodes, but what if the tree has multiple root nodes? Then there's no single root that would include the entire graph. Maybe we need an extra "Total" box at the bottom, like a synthetic "parent node" of all the roots? Then you could make it so that clicking that box resets the zoom completely.

[andjsrk]

I think the flame graph zoom node should be included in the URL, so that you can easily share the view you're looking at.

Great idea! By the way, are node indices(an IndexIntoCallNodeTable) stable so it is safe to include it in URLs?

---

But for the zoom node and its ancestors, instead of filling the entire box with the zoom fill color, subdivide the box as follows:

    +------------+------------+------------------------+
25% | unselected |  zoom fill |      unselected        |
    +------------+            +------------------------+
    |                                                  |
75% |                                                  |
    |                                                  |
    +--------------------------------------------------+

and the horizontal position and width of the zoom fill "tab" at the top is based on where this box would have been located in the original unzoomed flame graph.

I'm lost, could you elaborate on this?
Perhaps from the above, I'm confused on the below as well:

1. Redefine the "selected fill color" to be the "zoom fill color", use it for the zoom node and its ancestors.

---

I mean clicking on a filled pixel in the graph (without moving your mouse). This currently changes the selected call node.

Oh ok. Actually, I have been started using profilers for few weeks, so I'm definitely not experienced, but I think zooming it in would be better.

---

Yes you can click ancestor nodes, but what if the tree has multiple root nodes? Then there's no single root that would include the entire graph.

It is astonishing to me. I thought it is fine to assume that there is only one root node... But yeah, if so, an action is needed. Umm, for now, what comes to mind is only "Total" node, what you suggested.
EDIT: ah, I just found that multiple tracks can be selected at the same time, then multiple root is reasonable.

[mstange]

I'm lost, could you elaborate on this?

I was imagining something like this:

Before zoom:

After zoom:

Unfortunately it looks quite a bit worse than I expected. But maybe we can play with it until it looks good.

[mstange]

I think the flame graph zoom node should be included in the URL, so that you can easily share the view you're looking at.

Great idea! By the way, are node indices(an IndexIntoCallNodeTable) stable so it is safe to include it in URLs?

No they're not, but you can use the int array encoding of the call path, like we do for "focus on call node" filters.

[andjsrk]

Hello, I'm unable to commit changes due to improperly written tests.
I have no idea how I was able to commit changes before...

EDIT: The problem mentioned in #5893 (comment) makes me able to skip automated checks. As I can commit changes somehow, now this is less important.

---

profiler/src/test/components/WindowTitle.test.tsx

Lines 39 to 52 in 144ca4f

	Object.assign(profile.meta, {
	startTime: new Date('5 Nov 2024 13:00 UTC').getTime(),
	});
	const store = storeWithProfile(profile);
	store.dispatch(setDataSource('from-url'));
	render(
	<Provider store={store}>
	<WindowTitle />
	</Provider>
	);
	expect(document.title).toBe(
	'Firefox – 11/5/2024, 1:00:00 PM UTC – Firefox Profiler'
	);

This test fails on my machine because of localization:

 FAIL  src/test/components/WindowTitle.test.tsx (5.174 s)        
  ● WindowTitle › shows the profiler startTime in the window title if it is available

    expect(received).toBe(expected) // Object.is equality

    Expected: "Firefox – 11/5/2024, 1:00:00 PM UTC – Firefox Profiler"
    Received: "Firefox – 2024. 11. 5. PM 1시 0분 0초 UTC – Firefox Profiler"

      48 |     );
      49 |
    > 50 |     expect(document.title).toBe(
         |                            ^
      51 |       'Firefox – 11/5/2024, 1:00:00 PM UTC – Firefox Profiler'
      52 |     );
      53 |   });

      at Object.toBe (src/test/components/WindowTitle.test.tsx:50:28)

and pushing a commit requires tests to pass. What should I do? I'm considering opening a separate issue/PR then working it around by something like commenting out the test (but only on local environment).

[mstange]

You can skip the requirement with --no-verify