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 Rust language coloring #211

Merged
merged 6 commits into from May 6, 2021
Merged

Conversation

zthompson47
Copy link
Contributor

This PR is part of an effort to provide more coloring options for cargo-flamegraph in flamegraph-rs/flamegraph#19.

It looks for Rust functions in the stack (i.e. contains "::"), and applies separate color palettes for Rust system functions and user functions.

@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #211 (484f718) into master (534eb03) will increase coverage by 0.14%.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
+ Coverage   89.48%   89.63%   +0.14%     
==========================================
  Files          17       17              
  Lines        2312     2353      +41     
==========================================
+ Hits         2069     2109      +40     
- Misses        243      244       +1     
Impacted Files Coverage Δ
src/flamegraph/color/mod.rs 83.43% <50.00%> (-0.44%) ⬇️
src/flamegraph/color/palettes.rs 99.39% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 534eb03...484f718. Read the comment docs.

Copy link
Owner

@jonhoo jonhoo 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 a great addition, thank you! I left some notes inline.

src/flamegraph/color/palettes.rs Outdated Show resolved Hide resolved
src/flamegraph/color/palettes.rs Outdated Show resolved Hide resolved
src/flamegraph/color/palettes.rs Outdated Show resolved Hide resolved
src/flamegraph/color/palettes.rs Outdated Show resolved Hide resolved
@zthompson47
Copy link
Contributor Author

Thanks for the review and your time! I can start another revision tomorrow morning or so, and I left some responses up above.

@zthompson47
Copy link
Contributor Author

I pushed another commit. I hope that's the right way to go at this point, but I thought it would be most useful to have my latest code up here for review. I'm still kind of new to the collaboration end of this process so I'm hoping I didn't confuse things by introducing new changes at this stage.

@jonhoo
Copy link
Owner

jonhoo commented May 3, 2021

Not at all — the right way to go is to continuously improve the PR incrementally until we get to it being ready to land :)

@jonhoo
Copy link
Owner

jonhoo commented May 5, 2021

This looks great, now we just need to get the tests to pass!

jonhoo
jonhoo previously approved these changes May 5, 2021
@zthompson47
Copy link
Contributor Author

Excellent! I just tried to merge with upstream/master, and it looks like there are still errors in the tests.

Ok, just realized that my test was failing and fixed it.

FWIW, there were some already-broken tests on my system before this feature which I attributed to not have the right profiling software installed on my end.

@jonhoo jonhoo merged commit 4128ae1 into jonhoo:master May 6, 2021
@jonhoo
Copy link
Owner

jonhoo commented May 6, 2021

Looks great, thanks!

@jonhoo
Copy link
Owner

jonhoo commented May 6, 2021

Released in 0.10.5 🎉

@zthompson47
Copy link
Contributor Author

Thanks so much for your help on this!

@jonhoo
Copy link
Owner

jonhoo commented May 6, 2021

Thanks for sticking with it!

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

2 participants