feat(FlameGraph): Keep focus whenever the profile data changes #325

grafakus · 2024-12-19T12:50:51Z

✨ Description

Related issue(s): resolves #260, related (draft) PR in Grafana grafana/grafana#98356

This PR introduces a new keepFocusOnDataChange prop to the FlameGraph component.

📖 Summary of the changes

In order to simplify the implementation of the fix, the whole @grafana/flamegraph package has been copy-pasted. Once agreed on the definitive solution, I'll update the PR in the Grafana repository.

I've commented the changes the diff tab (mainly in src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx).

🧪 How to test?

Manually:
- Go to the "Flame graph" or the "Diff flame graph" view
- Click on a flame graph node
- Click on "Focus block"
- Change the time range or click on the time picker refresh button
- The block should stay focused and the percentage should update or drop to 0 if the function didn't consume any resource for the new time range

The same should occur for the sandwich view

github-actions · 2024-12-19T12:52:57Z

Unit test coverage

Lines	Statements	Branches	Functions
	11.12% (550/4943)	7.71% (137/1776)	8.24% (120/1455)

grafakus · 2024-12-19T12:53:35Z

package.json

@@ -38,16 +38,17 @@
    "@emotion/css": "11.10.6",
    "@grafana/data": "11.3.2",
    "@grafana/faro-web-sdk": "^1.10.0",
-    "@grafana/flamegraph": "11.3.2",


Removed and added all its dependencies instead.

grafakus · 2024-12-19T12:54:23Z

src/pages/ProfilesExplorerView/components/SceneExploreServiceFlameGraph/SceneFlameGraph.tsx

@@ -14,6 +13,7 @@ import { PyroscopeLogo } from '@shared/ui/PyroscopeLogo';
 import React, { useEffect, useMemo } from 'react';
 import { Unsubscribable } from 'rxjs';

+import { FlameGraph } from '../../../../tmp/grafana-flamegraph/src/';


TS was complaining when I tried aliasing @gfrafana/flamegraph. Webpack didn't 🤷🏾‍♂️

grafakus · 2024-12-19T12:55:21Z

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx

+   * Whether or not to keep any focused item when the profile data changes.
+   */
+  keepFocusOnDataChange?: boolean;
+};


👆🏾 The change

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx

ifrost · 2025-01-07T12:34:55Z

From the feedback:

"I would assume we want to keep the state of the flame graph and other tools that exist to help understand the flame graph. Here's a quick video:"

Shouldn't we lock entire FlameGraph state when users starts drilling down? And maybe show some sort of indication that there's new data coming in and the graph needs refreshing? I found it a bit confusing that the graph tries to keep the focus also when I change the time range. The block that is in focus may not exist anymore.

grafakus · 2025-01-14T20:19:02Z

src/tmp/grafana-flamegraph/src/FlameGraph/FlameGraphMetadata.tsx

+    }
+
+    if (focusedItem) {
+      const percentValue = totalTicks > 0 ? Math.round(10000 * (focusedItem.item.value / totalTicks)) / 100 : 0;


A small change to prevent NaN or 0 values to appear. A tooltip has also been added to remind which function is focused, as well as some minor CSS alignment changes below.

grafakus · 2025-01-14T20:19:13Z

src/tmp/grafana-flamegraph/src/FlameGraph/FlameGraphMetadata.tsx

+    verticalAlign: 'text-bottom',
+    margin: theme.spacing(0, 0.5),
+  }),
+  metadata: css({


This fixes the alignment

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx

grafakus · 2025-01-14T20:47:28Z

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx

+    setSandwichItem(undefined);
+  }, [setSandwichItem]);
+
+  useEffect(() => {


The change.

grafakus · 2025-01-14T20:47:58Z

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx

+      setRangeMin(item.start / totalViewTicks);
+      setRangeMax((item.start + item.value) / totalViewTicks);
+    } else {
+      setFocusedItemData({


Edge case when the function is not present in the data received:

ifrost

Works great!

My only doubt is about FlameGraphCanvas vs FlameGraphContainer responsibilities. Canvas sets range when user clicks while container sets range to keep the focus. Also container is responsible for resetting range when focus is reset. Just a gut feeling but maybe setting range based on focused item should be fully controlled by the container 🤔 otherwise we end up with similar code in both:

setRangeMin(clickedItemData.item.start / totalViewTicks);
            setRangeMax((clickedItemData.item.start + clickedItemData.item.value) / totalViewTicks);
            onItemFocused(clickedItemData);

Anyway, probably something to tackle separately. Good stuff!

ifrost · 2025-01-15T09:54:39Z

What's the plan?

keep this branch open
apply changes to Flame Graph in https://github.com/grafana/grafana/pull/98356/files
merge https://github.com/grafana/grafana/pull/98356/files
update and merge this branch
?

grafakus · 2025-01-15T10:36:32Z

My only doubt is about FlameGraphCanvas vs FlameGraphContainer responsibilities.
Just a gut feeling but maybe setting range based on focused item should be fully controlled by the container 🤔 otherwise we end up with similar code in both
Anyway, probably something to tackle separately

Thanks for the testing and review! I agree to your suggestion, it seems a good improvement to the code. We have a couple of issues for the flame graph package in our backlog, we will have many opportunities to refactor and clarify the responsibilities of the components in the future.

src/tmp/grafana-flamegraph/src/FlameGraph/FlameGraph.tsx

grafakus · 2025-01-15T12:32:23Z

What's the plan?

keep this branch open

apply changes to Flame Graph in https://github.com/grafana/grafana/pull/98356/files

merge https://github.com/grafana/grafana/pull/98356/files

update and merge this branch
?

I've updated grafana/grafana#98356 and will ping Joey and André to see if they want to have a look at it. Then I'd suggest to merge this PR so the functionality does not depend on Grafana's release cycle and has a chance to hit production faster.

grafakus · 2025-01-17T13:11:23Z

src/tmp/grafana-flamegraph/src/FlameGraph/FlameGraph.tsx

-          <Icon className={styles.sandwichMarkerIcon} name={'arrow-down'} />
+  let canvas = null;
+
+  if (levelsCallers?.length) {


This is made to prevent runtime errors (as seen in prod) when either:

levelsCallers[levelsCallers.length - 1] is undefined

levels[0] is undefined

feat(FlameGraph): Keep focus whenever the profile data changes

c3941bf

github-actions bot added the feat label Dec 19, 2024

grafakus self-assigned this Dec 19, 2024

grafakus commented Dec 19, 2024

View reviewed changes

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx Outdated Show resolved Hide resolved

grafakus requested review from adrapereira and joey-grafana December 19, 2024 12:56

feat: Keep sandwhich

54fca79

grafakus commented Dec 19, 2024

View reviewed changes

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx Outdated Show resolved Hide resolved

grafakus mentioned this pull request Dec 19, 2024

Flame graph view: (auto)refreshing should preserve UI context #260

Open

grafakus requested a review from ifrost December 26, 2024 11:12

grafakus removed request for adrapereira and joey-grafana January 14, 2025 17:42

fix: Keep item focused at all times + cosmetic UI changes

efa05d6

grafakus commented Jan 14, 2025

View reviewed changes

src/tmp/grafana-flamegraph/src/FlameGraphContainer.tsx Outdated Show resolved Hide resolved

grafakus added 2 commits January 14, 2025 21:44

fix: Prevent too many renders

9078f02

feat: Add tooltip on focus pill

b3bb997

grafakus commented Jan 14, 2025

View reviewed changes

ifrost previously approved these changes Jan 15, 2025

View reviewed changes

ifrost mentioned this pull request Jan 15, 2025

FlameGraph: Add prop to keep focused items when the profile data changes grafana/grafana#98356

Open

3 tasks

fix(FlameGraph): Fix fatal runtime error when in sandwich view

29d3650

grafakus dismissed ifrost’s stale review via 29d3650 January 15, 2025 11:03

grafakus commented Jan 15, 2025

View reviewed changes

src/tmp/grafana-flamegraph/src/FlameGraph/FlameGraph.tsx Outdated Show resolved Hide resolved

grafakus requested a review from ifrost January 15, 2025 12:33

grafakus added 2 commits January 16, 2025 17:56

fix: Keep sandwich view

2d4a91a

fix: Edge case with sandwich view

56d07b6

grafakus commented Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(FlameGraph): Keep focus whenever the profile data changes #325

feat(FlameGraph): Keep focus whenever the profile data changes #325

grafakus commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

grafakus Dec 19, 2024

grafakus Dec 19, 2024

grafakus Dec 19, 2024 •

edited

Loading

ifrost commented Jan 7, 2025

grafakus Jan 14, 2025 •

edited

Loading

grafakus Jan 14, 2025 •

edited

Loading

grafakus Jan 14, 2025

grafakus Jan 14, 2025 •

edited

Loading

ifrost left a comment

ifrost commented Jan 15, 2025

grafakus commented Jan 15, 2025 •

edited

Loading

grafakus commented Jan 15, 2025 •

edited

Loading

grafakus Jan 17, 2025

feat(FlameGraph): Keep focus whenever the profile data changes #325

Are you sure you want to change the base?

feat(FlameGraph): Keep focus whenever the profile data changes #325

Conversation

grafakus commented Dec 19, 2024 • edited Loading

✨ Description

📖 Summary of the changes

🧪 How to test?

github-actions bot commented Dec 19, 2024 • edited Loading

Unit test coverage

grafakus Dec 19, 2024

Choose a reason for hiding this comment

grafakus Dec 19, 2024

Choose a reason for hiding this comment

grafakus Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

ifrost commented Jan 7, 2025

grafakus Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

grafakus Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

grafakus Jan 14, 2025

Choose a reason for hiding this comment

grafakus Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

ifrost left a comment

Choose a reason for hiding this comment

ifrost commented Jan 15, 2025

grafakus commented Jan 15, 2025 • edited Loading

grafakus commented Jan 15, 2025 • edited Loading

grafakus Jan 17, 2025

Choose a reason for hiding this comment

grafakus commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024 •

edited

Loading

grafakus Dec 19, 2024 •

edited

Loading

grafakus Jan 14, 2025 •

edited

Loading

grafakus Jan 14, 2025 •

edited

Loading

grafakus Jan 14, 2025 •

edited

Loading

grafakus commented Jan 15, 2025 •

edited

Loading

grafakus commented Jan 15, 2025 •

edited

Loading