Colorize weblist output #271

Maaarcocr · 2017-11-29T17:23:41Z

This is my first try at solving #218 (and also first time contributing here 😄)

This is how it looks:

I have added 5 new CSS classes whose background-colors are from a red to yellow gradient (please feel free to criticize my color choices)
I am not sure if I should use flatSum or cumSum in order to calculate the percentile, so if you have any opinion on this, please let me know.

I can add some basics tests for the calculatePercentile function, if needed.

Maaarcocr · 2017-11-30T10:23:13Z

As you may have noticed I have some failing tests because of the different html output for weblist. Is there a way of updating the testdata easily?

aalexand · 2017-12-01T18:43:00Z

internal/driver/testdata/pprof.cpu.flat.addresses.weblist

-<span class=line>      5</span> <span class=nop>           .          .           line5 </span>
-<span class=line>      6</span> <span class=nop>           .          .           line6 </span>
-<span class=line>      7</span> <span class=nop>           .          .           line7 </span>
+<span class=line>      3</span> <span class="nop ">           .          .           line3 </span>


Trailing space shouldn't be there.

aalexand · 2017-12-01T18:48:11Z

internal/report/source.go

@@ -347,20 +347,40 @@ func printFunctionHeader(w io.Writer, name, path string, flatSum, cumSum int64,
 		measurement.Percentage(cumSum, rpt.total))
 }

+func calculatePercentile(partial, sum int64) string {
+	percentile := partial * 100 / sum


This is not percentile.

Will change this. Misunderstood its meaning.

@aalexand I have now implemented percentile. I'm not sure if my code (in terms of style and correctness) is fine, thus I have pushed a first version without tests, to get it reviewed. I will add tests soon.

I wonder if the percentiles functions should go into a different file or if they belong where they are.

codecov-io · 2017-12-01T19:46:23Z

Codecov Report

Merging #271 (3e3a1c0) into master (3ec35eb) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
+ Coverage   67.14%   67.25%   +0.10%     
==========================================
  Files          78       78              
  Lines       14084    14130      +46     
==========================================
+ Hits         9457     9503      +46     
  Misses       3792     3792              
  Partials      835      835

Impacted Files	Coverage Δ
internal/report/source_html.go	`0.00% <ø> (ø)`
internal/report/source.go	`82.68% <100.00%> (+1.27%)`	⬆️
.../github.com/google/pprof/internal/report/source.go	`82.68% <0.00%> (+1.27%)`	⬆️

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 3ec35eb...3e3a1c0. Read the comment docs.

aalexand · 2017-12-02T06:22:02Z

Looks like test fail at formatting check.

Maaarcocr · 2017-12-02T10:41:52Z

@aalexand solved 😄 I was sleeping so I couldn't fix it straight away.

aalexand

There are a bunch of style comments but also one important one on the what we actually want to do, please look at that first (it's the longest one).

aalexand · 2017-12-02T06:35:46Z

internal/report/source.go

 		}
 		printFunctionClosing(w)
 	}
 	return nil
 }

+func getPercentileString(cumSum float64, percentiles map[float64]float64) string {


I think this function can be simplified with a for loop, there is unnecessary repetition.

aalexand · 2017-12-02T06:36:21Z

internal/report/source.go

 		for _, fn := range fnodes {
-			printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, rpt)
+			printFunctionSourceLine(w, fn, asm[fn.Info.Lineno], reader, getPercentileString(float64(fn.Cum), percentiles), rpt)


getPercentileString - this is not just string, this is CSS class name?

aalexand · 2017-12-02T06:39:38Z

internal/report/source.go

 		}
 		printFunctionClosing(w)
 	}
 	return nil
 }

+func getPercentileString(cumSum float64, percentiles map[float64]float64) string {


Using float64 as the map key is quite unusual.

aalexand · 2017-12-02T06:40:31Z

internal/report/source.go

 		}
 		printFunctionClosing(w)
 	}
 	return nil
 }

+func getPercentileString(cumSum float64, percentiles map[float64]float64) string {


I'd suggest replacing "Percentile" with "Ptile" throughout. Using the full term makes the code unnecessarily verbose for the task.

aalexand · 2017-12-02T06:41:06Z

internal/report/source.go

+	return ""
+}
+
+// calculate percentile expects cumSums to be sorted and


Doc should start with function name.

aalexand · 2017-12-03T05:00:38Z

internal/report/source_test.go

 	"github.com/google/pprof/profile"
 )

+func TestCalculatePercentiles(t *testing.T) {
+	for _, testCase := range []struct {
+		fnodes graph.Nodes


What does "f" in "fnodes" stand for here?

aalexand · 2017-12-03T05:00:54Z

internal/report/source_test.go

+func TestCalculatePercentiles(t *testing.T) {
+	for _, testCase := range []struct {
+		fnodes graph.Nodes
+		output map[float64]float64


output -> want

aalexand · 2017-12-03T05:03:27Z

internal/report/source.go

+		return ""
+	}
+	switch {
+	case cumSum >= percentiles[80]:


Why did you choose the specific percentiles of 10, 20, 40, 60 and 80?

Looking at the whole PR, I think that we should start with something simpler.

Let's start with just two colors: pink and red.

Percentiles which are calculated off unique set built of float64 numbers look weird, frankly, so not sure it was a good idea. I think the easier to understand thing is to base the coloring on the max value among of the source lines.

Color as pink values that are > 80% of the max value.

Color as red values that are > 95% of the max value.

Leave all other values as white.

I have implemented these changes. The code now calculates the percentile on the array of cum values, instead of a set. I'm not using float64s anymore, instead I use int64. This last change meant that I had to implement the Sort Interface for int64.

aalexand · 2017-12-03T05:07:05Z

internal/report/source.go

+	return cumSums[int64(rank)]
+}
+
+func getSetOfCumValues(fnodes graph.Nodes) []float64 {


This should explain what this function is doing and why. E.g. the choice of deduplicating the different values before computing the percentile values is non-obvious, we should clearly explain the tactics and document the rationale.

aalexand · 2017-12-05T00:37:51Z

internal/report/source_test.go

 	"github.com/google/pprof/profile"
 )

+func TestCalculatePercentiles(t *testing.T) {
+	for _, testCase := range []struct {


I think there should be at least these additional test cases:

Duplicate values.

All equal values.

aalexand · 2017-12-15T23:46:35Z

@Maaarcocr Could you address the PR comments?

Maaarcocr · 2017-12-16T16:07:42Z

@aalexand Sorry for the delay. I've been ill and I had a lot of coursework in the meanwhile, I will address the comments either today or tomorrow.

Maaarcocr · 2017-12-18T13:27:44Z

@aalexand I've changed the code based on your suggested changes.

I was wondering if I should change the background-color in a different way, such as:
In getPtileCSSClassName I could instead produce a style string and we wouldnt not use classes anymore. This way, if we want to add more percentiles we would not need to manually change the HTML.

Maaarcocr · 2018-04-09T08:39:11Z

Hi! I would love to get this merged if it is still something that the project need 😄

zchee · 2021-03-18T00:10:39Z

@aalexand any progress this PR...? would be great feature I think.

aalexand · 2021-03-18T00:28:10Z

@zchee the branch is out of date so the author will need to rebase it first.

zchee · 2021-03-18T00:29:48Z

@aalexand Ah, your correct.

@Maaarcocr could you rebase it?

Maaarcocr · 2021-04-14T14:49:01Z

Sorry for the very delayed answer. Will try to rebase when I have some free time, hopefully this weekend!

zdyj3170101136 · 2022-11-07T11:50:55Z

still not merged?

Colorize weblist output

b64b875

googlebot added the cla: yes label Nov 29, 2017

Maaarcocr added 2 commits December 1, 2017 18:11

updating testdata

3c1cc88

getting rid of percentile_0

f20b0fa

Maaarcocr mentioned this pull request Dec 1, 2017

Easy testdata update for internal/driver tests. #274

Closed

aalexand reviewed Dec 1, 2017

View reviewed changes

Actually calculating percentiles. Missing tests.

54536e0

Maaarcocr force-pushed the master branch from 8a7b495 to 54536e0 Compare December 1, 2017 22:17

added tests

2393a99

formatted

7f5dfe6

aalexand reviewed Dec 5, 2017

View reviewed changes

Maaarcocr closed this Dec 18, 2017

Maaarcocr reopened this Dec 18, 2017

Maaarcocr force-pushed the master branch from 45c060c to b97a9e8 Compare December 18, 2017 13:39

review suggestions implemented

f8123b3

Maaarcocr force-pushed the master branch from b97a9e8 to f8123b3 Compare December 18, 2017 19:17

aalexand requested a review from rauls5382 May 30, 2018 18:27

Merge branch 'master' into master

3e3a1c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorize weblist output #271

Colorize weblist output #271

Maaarcocr commented Nov 29, 2017 •

edited

Maaarcocr commented Nov 30, 2017

aalexand Dec 1, 2017

aalexand Dec 1, 2017

Maaarcocr Dec 1, 2017

Maaarcocr Dec 1, 2017 •

edited

codecov-io commented Dec 1, 2017 •

edited

aalexand commented Dec 2, 2017

Maaarcocr commented Dec 2, 2017

aalexand left a comment

aalexand Dec 2, 2017

aalexand Dec 2, 2017

aalexand Dec 2, 2017

aalexand Dec 2, 2017

aalexand Dec 2, 2017

aalexand Dec 3, 2017

aalexand Dec 3, 2017

aalexand Dec 3, 2017

Maaarcocr Dec 18, 2017

aalexand Dec 3, 2017

aalexand Dec 5, 2017

aalexand commented Dec 15, 2017

Maaarcocr commented Dec 16, 2017

Maaarcocr commented Dec 18, 2017

Maaarcocr commented Apr 9, 2018

zchee commented Mar 18, 2021

aalexand commented Mar 18, 2021

zchee commented Mar 18, 2021

Maaarcocr commented Apr 14, 2021

zdyj3170101136 commented Nov 7, 2022

Colorize weblist output #271

Are you sure you want to change the base?

Colorize weblist output #271

Conversation

Maaarcocr commented Nov 29, 2017 • edited

Maaarcocr commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maaarcocr Dec 1, 2017 • edited

Choose a reason for hiding this comment

codecov-io commented Dec 1, 2017 • edited

Codecov Report

aalexand commented Dec 2, 2017

Maaarcocr commented Dec 2, 2017

aalexand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Dec 15, 2017

Maaarcocr commented Dec 16, 2017

Maaarcocr commented Dec 18, 2017

Maaarcocr commented Apr 9, 2018

zchee commented Mar 18, 2021

aalexand commented Mar 18, 2021

zchee commented Mar 18, 2021

Maaarcocr commented Apr 14, 2021

zdyj3170101136 commented Nov 7, 2022

Maaarcocr commented Nov 29, 2017 •

edited

Maaarcocr Dec 1, 2017 •

edited

codecov-io commented Dec 1, 2017 •

edited