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

ui: Improve showing filtered and trimmed nodes in ProfileIcicleGraph #2758

Merged
merged 7 commits into from
Mar 21, 2023

Conversation

metalmatze
Copy link
Member

If nothing gets filtered and trimmed the icicle graph now looks like this (see the line at the bottom).
icicle-raw

With trimming enabled:
icicle-trimmed

With filtering by function (for sort)
icicle-filtered

And finally with both trimming and filtering.
icicle-filtered-trimmed

@metalmatze metalmatze requested review from a team as code owners March 14, 2023 18:46
@metalmatze
Copy link
Member Author

Oh, seems like #2756 created a merge conflict. I'll rebase these tomorrow.

Other follow-ups are:

@brancz
Copy link
Member

brancz commented Mar 14, 2023

Super cool!! I think the indicating text should be above the icicle graph as it's unpredictable how large it may be so you may not even know if that text is there just because the icicle graph is too tall.

@metalmatze
Copy link
Member Author

Super cool!! I think the indicating text should be above the icicle graph as it's unpredictable how large it may be so you may not even know if that text is there just because the icicle graph is too tall.

It was there before and I moved it below the graph. It's mostly debugging information that's there when needed but nothing that usually concerns users unless they look for it.
I think we, as developers, are biased towards thinking the information is important because we think in terms of samples and filtering and trimming. It's still there, just not at the very top.

@metalmatze
Copy link
Member Author

I'm writing and updating the flamegraph_table_test.go cases now. 😌

@brancz
Copy link
Member

brancz commented Mar 15, 2023

Alright happy with trying like this. I'm not convinced if I'm honest but I was wrong about function filtering as well, so I'm very happy to give it a shot and build an opinion again after using it a bit.

Copy link
Contributor

@yomete yomete left a comment

Choose a reason for hiding this comment

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

lgtm!

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.

Lgtm!

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.

None yet

4 participants