Open asherkin opened 3 years ago
Merging #203 (4caf2f3) into master (8fd0289) will decrease coverage by
0.68%
. The diff coverage is75.60%
.
@@ Coverage Diff @@
## master #203 +/- ##
==========================================
- Coverage 90.08% 89.40% -0.69%
==========================================
Files 17 17
Lines 2239 2312 +73
==========================================
+ Hits 2017 2067 +50
- Misses 222 245 +23
Impacted Files | Coverage Δ | |
---|---|---|
src/collapse/mod.rs | 52.63% <0.00%> (-14.04%) |
:arrow_down: |
src/collapse/common.rs | 59.66% <50.00%> (-0.61%) |
:arrow_down: |
src/flamegraph/color/palettes.rs | 98.64% <86.66%> (-0.89%) |
:arrow_down: |
src/collapse/guess.rs | 69.23% <100.00%> (-8.82%) |
:arrow_down: |
src/collapse/matcher.rs | 100.00% <100.00%> (ø) |
|
src/collapse/perf.rs | 95.45% <100.00%> (-1.91%) |
:arrow_down: |
src/flamegraph/color/mod.rs | 84.07% <100.00%> (+0.10%) |
:arrow_up: |
src/flamegraph/mod.rs | 97.54% <100.00%> (+1.08%) |
:arrow_up: |
src/collapse/sample.rs | 90.90% <0.00%> (-5.06%) |
:arrow_down: |
... and 10 more |
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 43ed780...16e2494. Read the comment docs.
Just out of interest, could you post a flamegraph with this color mode enabled? Would be interesting to see what it looks like!
Just out of interest, could you post a flamegraph with this color mode enabled? Would be interesting to see what it looks like!
For posterity:
That's really neat! I almost wonder if we should make this the default behavior for the Hot
palette, and then have a LegacyHot
that only uses red as today. @jasonrhansen @bcmyers @itamarst @AnderEnder what do you think?
@jonhoo I wouldn't be opposed to that idea.
The output definitely looks useful. I would suggest documenting the heuristics, so people know what to expect.
@asherkin I'm pretty happy making this the default. Could you update the PR so that Hot
uses your new heuristic, and then also document that behavior for the hot color scheme? We'll also then want to add a LegacyHot
color scheme that is what Hot
does today.
Oh, and as in the other PR, could you please update the changelog at the end?
@jonhoo Apologies for the late response, I caught the virus that's been going around and have been out of action for a few weeks.
Would love to make this the default! I think the only concern I'd have with that myself is the massively reduced colour range of "red" used here compared to the "hot" palette. I only used these colours as they're what the java one was using, how would you feel about me crafting specifically a new default using these annotations? I'm thinking "hot" for the default (so the output will be the same as currently for anything not-annotated), something muted like greys for the kernel frames, and something bright (probably stick with the green/blue) for the jit/inline frames?
Ouch, I'm sorry to hear that. Glad to have you back with us!
Absolutely, that sounds like an excellent idea :tada:
I've implemented the proposed new colour scheme and I think it's looking good. The default has changed to be the new annotated palette - let me know if you would prefer that to still be called "hot" from a CLI perspective and I'll make that change, right now I've kept it as its own new thing (internally it'll never be totally straightforward due to BasicPalette vs MultiPalette anyway). I'll update the changelog once that's cleared up.
I could do with some pointers regarding where you'd like documentation related to the annotation behaviour - the adding of annotations and the details around that are handled (and documented) on the stackcollapse side, so I'm not totally sure what's wanted over here on the flamegraph end.
EDIT: Whoops, need to sort the tests.
Nice! How about we call the old palette legacy
, and leave the new one as hot
?
As for documentation, I think the thing to do is mention in the stack collapsing docs how we detect JIT/kernel things, and then in the flamegraph docs how we expect names to be annotated in order to highlight them as JIT/kernel. How does that sound? I think we'll likely end up with a full API makeover (including docs) at some point in the future when we land a proper API as well :sweat_smile:
The "java" and "js" colour palettes have a very useful feature of being able to highlight which frames are jitted or kernel functions based on the annotations added by the stackcollapse programs. Unfortunately they also make a number of assumptions based on the function names which are very specific to those languages, and cause nonsensical results with other runtimes.
This PR adds a new generic "annotated" colour palette that only uses the function name annotations, and can thus be used regardless of the runtime being profiled.