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

Filter out lines which begin with '# ' in inferno-flamegraph #239

Merged
merged 3 commits into from
May 8, 2022

Conversation

mrob95
Copy link
Contributor

@mrob95 mrob95 commented May 1, 2022

implements #237

also fixes #238 by moving the check for empty/ignored lines out of merge and into from_lines. I think this makes more sense as the block in from_lines which handles opt.reverse_stack_order pre-processes the samples before calling merge, and seems to assume that the lines it deals with are not empty. I'm not very experienced with rust though, so please let me know if there is a better way!

I've added an empty line to the bad-lines test file as a regression test for #238, and a couple of test files for input from the austin profiler.

@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #239 (8d4cf04) into master (29fb054) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   87.17%   87.21%   +0.04%     
==========================================
  Files          18       18              
  Lines        2526     2527       +1     
==========================================
+ Hits         2202     2204       +2     
+ Misses        324      323       -1     
Impacted Files Coverage Δ
src/flamegraph/merge.rs 87.23% <ø> (+0.77%) ⬆️
src/flamegraph/mod.rs 87.03% <100.00%> (+0.13%) ⬆️

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 29fb054...8d4cf04. 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.

Just one small nit and then I think we're good to land this. Thanks for taking the time to fix the issue too!

src/flamegraph/mod.rs Outdated Show resolved Hide resolved
@jonhoo jonhoo merged commit 9925700 into jonhoo:master May 8, 2022
@jonhoo
Copy link
Owner

jonhoo commented May 8, 2022

Thanks! I'll fix the MSRV issue separately — that's not caused by your change.

@jonhoo
Copy link
Owner

jonhoo commented May 8, 2022

Published as 0.11.3 🎉

@mrob95
Copy link
Contributor Author

mrob95 commented May 8, 2022

Awesome! Thanks 😊

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.

flamegraph panics on empty lines with --reverse set
2 participants