Skip to content
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

Flamegraph: Add collapsing for similar items in the stack #77461

Merged
merged 16 commits into from
Nov 9, 2023

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Oct 31, 2023

closes: #77689

What is this feature?

Note: hidden behind flameGraphItemCollapsing feature flag

Sometimes stacks contain lots of levels with similar repeating items, for example long stacks of framework code that usually isn't of interest but takes up a lot of visual real estate. This PR adds collapsing of similar items, that means that instead of rendering all of the similar items we render only one and allow to expand those collapsed items on demand.

At this moment, what similar items mean is pretty simple. We collapse items if they have the same value as a parent. This usually indicates very fast wrapper code without much 'self' code which should correctly collapse some of the highly nested wrapper code but probably not all so it is something to improve in the future based on feedback.

collapsing2.mp4

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@aocenas aocenas marked this pull request as ready for review November 3, 2023 15:08
@aocenas aocenas requested review from grafanabot and a team as code owners November 3, 2023 15:08
@aocenas aocenas removed the request for review from a team November 3, 2023 15:08
@aocenas aocenas self-assigned this Nov 6, 2023
Copy link
Contributor

@joey-grafana joey-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool - few observations below 👍

  • Would it be possible to make it clearer when items are collapsed? Not sure if an icon or a different kind of UI element. Feels like it's easy to miss or not see when something is collapsed or can be collapsed. I think UX feedback could help here with this patten.
Screenshot 2023-11-07 at 09 59 15
  • It would be worth greying Expand all groups when they are already expanded. We could use a tooltip to tell the user why it's greyed out.

  • An added benefit of adding the collapse items to their own section is that we could also have description above the collapse group that specifies how many nodes this will collapse and what the label is.

  • I'm a little bit confused as to why github.com/grafana/pyroscope-go.TagWrapper and runtime/pprof.Do are linked as being collapsible?

Screenshot 2023-11-07 at 10 06 27
  • If you align text right, the collapse vertical bar is overlayed on the text, making both difficult to read/recognise.
Screenshot 2023-11-07 at 10 08 30
  • Collapse/expand all groups appears in context menu in sandwich view but appears to do nothing. No indication in flame graph of what nodes can be collapsed if any.

  • FlameGraphContainer is missing useMemo dependency

Screenshot 2023-11-07 at 09 41 49

@aocenas
Copy link
Member Author

aocenas commented Nov 7, 2023

Would it be possible to make it clearer when items are collapsed? Not sure if an icon or a different kind of UI element. Feels like it's easy to miss or not see when something is collapsed or can be collapsed. I think UX feedback could help here with this patten.

Something we will get feedback on I assume. When collapsed there is a number in front of the label. The problem is it's canvas so it's harder to design something elaborate here.

It would be worth greying Expand all groups when they are already expanded. We could use a tooltip to tell the user why it's greyed out.

Will take a look at that.

An added benefit of adding the collapse items to their own section is that we could also have description above the collapse group that specifies how many nodes this will collapse and what the label is.

Not sure what would be the benefit here, when expanded you see all the items so it's mainly about visually removing them. Whether it's exactly 10 or 11 items isn't that relevant I think. When it's collapsed then we show a number because otherwise you wouldn't know if it's 2 or 100 items so there I think it's more important. Regarding the label not sure what you mean which label.

I'm a little bit confused as to why github.com/grafana/pyroscope-go.TagWrapper and runtime/pprof.Do are linked as being collapsible?

Because they have the same value so there isn't really much code that they individually execute.

If you align text right, the collapse vertical bar is overlayed on the text, making both difficult to read/recognise.

Will take a look.

Collapse/expand all groups appears in context menu in sandwich view but appears to do nothing. No indication in flame graph of what nodes can be collapsed if any.

Yeah will remove it. This does not work in sandwich view right now. We may add it later if people think this would be usefull

FlameGraphContainer is missing useMemo dependency

Will fix

@joey-grafana
Copy link
Contributor

Btw meant to say the last UI tweak that should be made is to improve the elongating (along x axis) of items that have the horizontal collapse bar where the item is small e.g. purple & green or purple & yellow in screenshot.

Screenshot 2023-11-08 at 17 00 43

@aocenas
Copy link
Member Author

aocenas commented Nov 9, 2023

Ahh good catch. Hmm I feel like the best thing in that case would be to hide it altogether, not sure it makes sense to try to make it smaller and unreadable.

@aocenas aocenas merged commit 494a07b into main Nov 9, 2023
20 checks passed
@aocenas aocenas deleted the aocenas/flamegraph/item-collapsing branch November 9, 2023 14:31
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flamegraph: Collapse groups of node that don't add context
3 participants