log error message when negative samples appear in non-diff_base profiles #391

nolanmar511 · 2018-06-22T22:41:14Z

This is the last change needed for improving diff mode, so fixes #323.

codecov-io · 2018-06-22T22:54:40Z

Codecov Report

Merging #391 into master will increase coverage by 0.02%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   67.67%   67.69%   +0.02%     
==========================================
  Files          36       36              
  Lines        7535     7540       +5     
==========================================
+ Hits         5099     5104       +5     
  Misses       2031     2031              
  Partials      405      405

Impacted Files	Coverage Δ
internal/driver/webui.go	`58.47% <0%> (ø)`	⬆️
internal/driver/driver.go	`68.92% <0%> (ø)`	⬆️
internal/report/report.go	`31.77% <90.9%> (+0.48%)`	⬆️

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 ef43755...ad55e97. Read the comment docs.

aalexand · 2018-06-27T00:57:15Z

internal/report/report.go

 	if diffTotal > 0 {
 		total = diffTotal
 		div = diffDiv
+	} else if negSamples {
+		ui.PrintErr("Negative sample values appeared in the profile.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.")


It's a bit strange that printing out this user interface messages goes down as deeply as to this code that computes the total. As well as that report.New has to take the UI now. I think it would be better to make computeTotal return a boolean (in addition to the int64 like now) indicating whether negative values were seen, store that in the report object, and print the message in Generate() making that function take UI.

nolanmar511 · 2018-06-27T23:44:50Z

PTAL

aalexand · 2018-06-27T23:47:47Z

internal/report/report_test.go

@@ -213,6 +238,41 @@ var testProfile = &profile.Profile{
 	Mapping:  testM,
 }

+var testProfile2 = &profile.Profile{


I think it would be easier to massage testProfile where needed in the test to produce this profile. As far as I can tell, it's just negating the value in one of the samples.

Done.
I negated all samples for simplicity, but this works.

aalexand · 2018-06-27T23:48:17Z

internal/report/report_test.go

 				t.Errorf("got total %d, want %v", gotTotal, tc.wantTotal)
 			}
+			if gotUnexpNegSamples != tc.wantUnexpNegSamples {
+				t.Errorf("got  unexpected negative samples %v, want %v", gotUnexpNegSamples, tc.wantUnexpNegSamples)


Nit: double space after "got"?

aalexand · 2018-06-27T23:53:24Z

internal/report/report_test.go

+					OutputFormat: Dot,
+					CallTree:     true,
+					Symbol:       regexp.MustCompile(`.`),
+					TrimPath:     "/some/path",


What do you specify TrimPath in this test for?

Removed this.

aalexand · 2018-06-27T23:53:58Z

internal/report/report_test.go

+			rpt: New(
+				testProfile2.Copy(),
+				&Options{
+					OutputFormat: Dot,


For this test, is Dot format really necessary? Which kind of issues would it help to catch?

Switched to list

aalexand · 2018-06-27T23:54:52Z

internal/report/report.go

@@ -1202,8 +1206,8 @@ func New(prof *profile.Profile, o *Options) *Report {
 		}
 		return measurement.ScaledLabel(v, o.SampleUnit, o.OutputUnit)
 	}
-	return &Report{prof, computeTotal(prof, o.SampleValue, o.SampleMeanDivisor),
-		o, format}
+	total, unexpNegSamples := computeTotal(prof, o.SampleValue, o.SampleMeanDivisor)


unexpNegSamples -> hasNegativeSamples, here and in other places.

Done.
Not sure if this could be confusing, because -diff_base profiles will have negative samples, but this will be false.

I see. Well, maybe leave the old name then. Up to you.

Switching back to old name.

aalexand · 2018-06-27T23:56:59Z

internal/report/report.go

-func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) error {
+func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error {
+	if rpt.unexpNegSamples {
+		ui.PrintErr("Negative sample values appeared in the profile.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.")


It would be good to shorten the message a bit (without any loss of information), but I couldn't come up with a good version...

I'm thinking about it still... not sure if I'll come up with something.

aalexand · 2018-06-27T23:57:37Z

internal/report/report.go

+	total           int64
+	options         *Options
+	formatValue     func(int64) string
+	unexpNegSamples bool


Maybe move it up to be after "total" since these are related.

nolanmar511 · 2018-07-09T16:16:59Z

PTAL

aalexand · 2018-07-09T18:19:37Z

internal/report/report.go

-func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) error {
+func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error {
+	if rpt.unexpNegSamples {
+		ui.PrintErr("Profile has negative sample values.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.")


Could we check if -base flag was actually used and print this warning (or this part of the warning) only in that case?

It's possible, but I don't think it's trivial.

I see a few possible options:

Call report.Generate() with a boolean indicating if -base was used to specify a base profile. This requires also passing that boolean into interactive() and generateReport() or extending plugin.Options with a field for specifying if there is a -base profile.

Add a boolean to profile.Profile to track if a base profile was specified with the base.

Start tracking -base somehow in pprofVariables (this seems undesirable, because I think specifying a base profile from the interactive mode would fail silently).

Any other ideas or preferences?

(There's also the smaller complication of what to name a boolean indicating the a profile has a "base: profile but not a "diff base" profile)

I wonder if it would make more sense to print out this warning in fetchProfiles around where pbase and s.DiffBase are checked. The complication is that at that point the sample index is not known, but we could print a warning if any of the metrics has a negative sample and mention all such metrics in the message. The advantage of placing it there is that the message would be printed only once as opposed to for every report in the interactive mode. What do you think?

I'm happy with that solution.
It may lead to the message printing out when people are using -base to look at cumulative profiles, but I think that'll be okay.

That's a good point, we don't want the message to be printed out when someone is doing pprof -sample_index=alloc_space profile1.pb.gz -base profile2.pb.gz.

nolanmar511 · 2018-07-09T20:29:09Z

PTAL at response to comment.

aalexand · 2018-07-17T16:52:25Z

internal/report/report.go

-func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) error {
+func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error {
+	if rpt.unexpNegSamples {
+		ui.PrintErr("Profile has negative sample values.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.")


Maybe shorten the message a bit to have it one line: "Profile has negative values. If using -base, consider using -diff_base instead."

aalexand · 2018-07-17T16:53:08Z

internal/report/report.go

@@ -1246,20 +1254,23 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i
 	if diffTotal > 0 {
 		total = diffTotal
 		div = diffDiv
+		// negative samples are expected in diff base profiles.


Nit: "Negative" (capitalize documentation sentences)

nolanmar511 · 2018-07-19T14:32:50Z

PTAL

aalexand · 2018-07-19T17:02:42Z

internal/report/report.go

-func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool) error {
+func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error {
+	if rpt.unexpNegSamples {
+		ui.PrintErr("Profile has negative values. If using -base, consider using -diff_base instead.")


Sorry, let's do one more tweak to clarify why someone would need to consider using -diff_base instead. How about

Profile has negative values, percentage values may be incorrect. If using -base, consider using -diff_base instead.

Edited the comment, I had a mistake there. The right version is Profile has negative values, percentage values may be incorrect. If using -base, consider using -diff_base instead..

nolanmar511 · 2018-07-23T20:15:48Z

PTAL

nolanmar511 · 2018-07-26T18:27:05Z

Closing because we do not want to submit this.
The message currently appears in situations when we would not want the message to appear.

aalexand · 2018-07-27T06:46:27Z

Actually closing the PR. Also to clarify, the exact reason for why the change shouldn't go in is golang/go#26638.

googlebot added the cla: yes label Jun 22, 2018

aalexand changed the title ~~log error message when negatice samples appear in non-diff_base profiles~~ log error message when negative samples appear in non-diff_base profiles Jun 27, 2018

aalexand reviewed Jun 27, 2018

View reviewed changes

nolanmar511 force-pushed the neg-warn branch 2 times, most recently from c1f2196 to 9b1cf87 Compare June 27, 2018 23:43

aalexand reviewed Jun 27, 2018

View reviewed changes

nolanmar511 force-pushed the neg-warn branch 3 times, most recently from 234e004 to c789498 Compare June 28, 2018 17:54

nolanmar511 force-pushed the neg-warn branch from c789498 to f688f27 Compare July 9, 2018 16:16

nolanmar511 force-pushed the neg-warn branch from f688f27 to ada0333 Compare July 9, 2018 16:21

aalexand reviewed Jul 9, 2018

View reviewed changes

nolanmar511 force-pushed the neg-warn branch from ada0333 to dc6bb2a Compare July 12, 2018 17:44

nolanmar511 added 2 commits July 17, 2018 09:40

log error message when negative samples appear in non-diff_base profiles

d64e368

address comments

68e5354

aalexand reviewed Jul 17, 2018

View reviewed changes

nolanmar511 force-pushed the neg-warn branch from 2e09821 to 5cbdd38 Compare July 19, 2018 14:23

aalexand reviewed Jul 19, 2018

View reviewed changes

address comments

12208ab

nolanmar511 force-pushed the neg-warn branch from 5cbdd38 to 12208ab Compare July 19, 2018 17:06

nolanmar511 added 2 commits July 23, 2018 13:13

update error message

ab929e8

Merge branch 'master' into neg-warn

ad55e97

aalexand closed this Jul 27, 2018

log error message when negative samples appear in non-diff_base profiles #391

log error message when negative samples appear in non-diff_base profiles #391

Conversation

nolanmar511 commented Jun 22, 2018 • edited Loading

codecov-io commented Jun 22, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented Jun 27, 2018

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

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

nolanmar511 commented Jul 9, 2018

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

nolanmar511 commented Jul 9, 2018

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

nolanmar511 commented Jul 19, 2018

aalexand Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 commented Jul 23, 2018

nolanmar511 commented Jul 26, 2018

aalexand commented Jul 27, 2018

nolanmar511 commented Jun 22, 2018 •

edited

Loading

codecov-io commented Jun 22, 2018 •

edited

Loading

aalexand Jul 19, 2018 •

edited

Loading