Netflix / flamescope

FlameScope is a visualization tool for exploring different time ranges as Flame Graphs.
Apache License 2.0
3k stars 168 forks source link

Increase contrast and change colors on heatmap #20

Closed nhick closed 6 years ago

nhick commented 6 years ago

Lowered the midpoint of the scale to color lower values and changed scale to white->blue->red. Addresess #4

brendangregg commented 6 years ago

No, thanks, this does not address #4. Please see the description I wrote in #4.

I would not make this the default as A) it increases the (already difficult) learning curve for newcomers, who must now learn a two-hue non-linear palette by default; and B) adding more hues becomes visually distracting for some profiles (it looks bad). I'd have this as an enhance button only (if you click enhance, it will now use two hues and be non-linear). I'd also seriously look at using blue only for counts 1 & 2, for reasons I explained in #4, unless there is a better reason than that.

nhick commented 6 years ago

Got it. Wasn't sure as to default and wanted to avoid adding UI elements. @brendangregg you recommend leveraging the "Settings" modal currently commented out?

nhick commented 6 years ago

Updated to meet all requirements set forth in #4

brendangregg commented 6 years ago

Thanks, however, this changes the default palette. I don't think it should. This new default palette is lighter, and heat maps appear a bit more washed out.

nhick commented 6 years ago

Yeah, was setting it to pure red and missed reverting it for the default palette. Changing now.

brendangregg commented 6 years ago

Did you test this? When I try, they are still very different.

Before:

screen shot 2018-04-10 at 7 05 51 am

After:

screen shot 2018-04-10 at 7 06 20 am

These should be exactly the same.

nhick commented 6 years ago

Odd. Yes, these should be the same. Let me check.

nhick commented 6 years ago

The midpoint of the color scale was removed by accident + pink was set at the upper bound. All is sorted in https://github.com/Netflix/flamescope/pull/20/commits/6f785903c22012756ef890a8c2f42a67bcd61033. Sorry for that. Adapted the 'enhanced' scale to the default scale. When selected it will map colors as follows:

brendangregg commented 6 years ago

ok, thanks, works!

Is there a way you can collapse the commits so that it's just the one for this feature?

Later on we can add docs somewhere to explain why this palette is the way it is, otherwise people might be tempted to adjust it to be more artistic. We can add a different artistic ("rainbow"?) palette, but this one is for enhancing the 1 and 2 count blocks for locating single-threaded execution.

nhick commented 6 years ago

Great, thanks. Can you merge --squash on your end? That way it should collapse all commits into one when merging. If not I can close this pr and create a new one.

In terms of having more pallettes, was thinking that we could have a small UI component in the settings modal that would allow you to input the parameters (choose domain/range and assign a color to each point). However, as there are a few UI PRs still pending (I.e. #19 ) it makes sense to wait for those to be merged and then create a new PR to "unify" the UI etc. Should I create a new issue?

brendangregg commented 6 years ago

I'll need to learn how to merge --squash; What I normally do as a contributor is a git rebase -i HEAD~# to squash the commits and git push --force. Which seems to work.

Yes, we can do follow on issues and PRs to extend this in the settings modal (do we even have that exposed yet?).

spiermar commented 6 years ago

Merged. Adjusting a few things shortly.