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

docs: Make flame graph docs reflect icicle default #466

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

tjmcewan
Copy link
Contributor

Doc update to align with what appears to be a relatively new default flame graph orientation.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@godlygeek
Copy link
Contributor

Doc update to align with what appears to be a relatively new default flame graph orientation.

You'd think that, but nah - I'm pretty sure we've been using icicle orientation by default since our very first release, and this has been wrong forever 😅

Thanks for catching this! I'm going to go for a more extensive fix than you proposed, though. I've updated the first section to avoid absolute spatial references entirely, and only make references relative to the location of the root. Then, I've yanked up the section about flame graphs vs icicle graphs to right below that section, explaining the flame vs icicle distinction earlier, explaining that the root could appear in either of two positions, and making it clear that memray flamegraph produces icicle graphs by default despite its name. And I've updated the prose for the first of the two simple examples to make it clear that it's a flame graph as opposed to an icicle graph. I didn't bother for the second, since I think it's fair to assume anyone trying to read and interpret the second example flame graph will have already understood the first example.

Since I've made such extensive changes to your small original patch, I'm going to leave this for someone else to review my work.

https://godlygeek.github.io/memray/flamegraph.html shows this page with my proposed changes.

@godlygeek godlygeek changed the title Consistent spatial references given Icicle default docs: Make flame graph docs reflect icicle default Sep 25, 2023
@pablogsal pablogsal enabled auto-merge (rebase) September 26, 2023 10:15
@godlygeek godlygeek merged commit cb8c767 into bloomberg:main Sep 26, 2023
6 checks passed
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

3 participants