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

Add SortBy and GroupBy actions for Icicle Graphs #3479

Merged
merged 8 commits into from
Jul 20, 2023
Merged

Conversation

metalmatze
Copy link
Member

This PR adds two additional UI components to the arrow icicle graph.
image

One is a SortBy dropdown to either sort by the function name (the default and what we currently only had) or cumulative value.
The other component is a dropdown that toggles GroupBy function name or pprof labels or no grouping at all.

All state is stored in the URL. Sharing a profile keeps the group by and sort by order.

Group By

No grouping

image

Function name (the default)

image

(pprof) Labels

image

Function and Labels

image

Sorting

Function name (the default)

This sorts alphabetically by function names:
image

Cumulative value

This sorts by cumulative value in descending order. This makes the biggest values show up to the left and the smallest to the right:
image

We want to add sorting to the icicle graph itself. This allows to not only sort by function name (the default and what's currently supported) but also sort by cumulative values (such that the highest functions are always left) and in compare mode by diff value (meaning the functions that got worse are left and better right).

This can make finding specific things even easier.
@metalmatze metalmatze requested review from a team as code owners July 19, 2023 18:13
@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Jul 19, 2023

🤖 Meticulous spotted visual differences in 210 of 229 screens tested: view and approve differences detected.

Last updated for commit 4be0494. This comment will update as new commits are pushed.

Copy link
Contributor

@manojVivek manojVivek left a comment

Choose a reason for hiding this comment

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

Great looking UI! 👏🏼

Just a couple of minor nits and the linter comments, otherwise looks good from the UI perspective.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

backend/API changes lgtm

@metalmatze
Copy link
Member Author

Thanks a lot for fixing those re-renders @manojVivek 👏

@metalmatze metalmatze merged commit c57c71f into main Jul 20, 2023
@metalmatze metalmatze deleted the ui-icicle-actions branch July 20, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants