From 5a693747b0da020aa592d6a0f22d967d7bc0b5da Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Thu, 26 Apr 2018 16:28:51 -0700 Subject: [PATCH 01/15] add -diff flag for better profile comparision --- internal/driver/cli.go | 14 ++- internal/driver/fetch.go | 7 +- internal/driver/fetch_test.go | 223 ++++++++++++++++++++++++++++++--- internal/report/report.go | 39 ++++-- internal/report/report_test.go | 119 ++++++++++++++++++ profile/profile.go | 30 +++++ profile/profile_test.go | 161 ++++++++++++++++++++++++ 7 files changed, 561 insertions(+), 32 deletions(-) diff --git a/internal/driver/cli.go b/internal/driver/cli.go index c3c22e7c..b74ab5f9 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -28,6 +28,7 @@ type source struct { ExecName string BuildID string Base []string + DiffBase bool Normalize bool Seconds int @@ -43,7 +44,8 @@ type source struct { func parseFlags(o *plugin.Options) (*source, []string, error) { flag := o.Flagset // Comparisons. - flagBase := flag.StringList("base", "", "Source for base profile for comparison") + flagBase := flag.StringList("base", "", "Source for base profile for profile subtraction") + flagDiff := flag.StringList("diff", "", "Source for base profile for comparison") // Source options. flagSymbolize := flag.String("symbolize", "", "Options for profile symbolization") flagBuildID := flag.String("buildid", "", "Override build id for first mapping") @@ -140,7 +142,15 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { Comment: *flagAddComment, } - for _, s := range *flagBase { + if len(*flagBase) > 0 && len(*flagDiff) > 0 { + return nil, nil, fmt.Errorf("only -base or -diff flag can be specified") + } + base := *flagBase + if len(*flagDiff) > 0 { + base = *flagDiff + source.DiffBase = true + } + for _, s := range base { if *s != "" { source.Base = append(source.Base, *s) } diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index 7c576de6..4c175ee7 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -57,7 +57,7 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) { }) } - p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, o.Fetch, o.Obj, o.UI) + p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, s.DiffBase, o.Fetch, o.Obj, o.UI) if err != nil { return nil, err } @@ -120,7 +120,7 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) { return p, nil } -func grabSourcesAndBases(sources, bases []profileSource, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) { +func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) { wg := sync.WaitGroup{} wg.Add(2) var psrc, pbase *profile.Profile @@ -135,6 +135,9 @@ func grabSourcesAndBases(sources, bases []profileSource, fetch plugin.Fetcher, o go func() { defer wg.Done() pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui) + if pbase != nil && isDiffBase { + pbase.AddTag("pprof::diff", "true") + } }() wg.Wait() save := savesrc || savebase diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index afb135b7..23eedf2b 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -210,13 +210,20 @@ func TestFetchWithBase(t *testing.T) { baseVars := pprofVariables defer func() { pprofVariables = baseVars }() + type WantSample struct { + values []int64 + labels map[string][]string + } + const path = "testdata/" type testcase struct { - desc string - sources []string - bases []string - normalize bool - expectedSamples [][]int64 + desc string + sources []string + bases []string + diffBases []string + normalize bool + wantSamples []WantSample + wantErrorMsg string } testcases := []testcase{ @@ -224,43 +231,206 @@ func TestFetchWithBase(t *testing.T) { "not normalized base is same as source", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, + []string{}, + false, + []WantSample{}, + "", + }, + { + "not normalized base is same as source", + []string{path + "cppbench.contention"}, + []string{path + "cppbench.contention"}, + []string{}, false, - [][]int64{}, + []WantSample{}, + "", }, { "not normalized single source, multiple base (all profiles same)", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention", path + "cppbench.contention"}, + []string{}, false, - [][]int64{{-2700, -608881724}, {-100, -23992}, {-200, -179943}, {-100, -17778444}, {-100, -75976}, {-300, -63568134}}, + []WantSample{ + { + values: []int64{-2700, -608881724}, + labels: map[string][]string{}, + }, + { + values: []int64{-100, -23992}, + labels: map[string][]string{}, + }, + { + values: []int64{-200, -179943}, + labels: map[string][]string{}, + }, + { + values: []int64{-100, -17778444}, + labels: map[string][]string{}, + }, + { + values: []int64{-100, -75976}, + labels: map[string][]string{}, + }, + { + values: []int64{-300, -63568134}, + labels: map[string][]string{}, + }, + }, + "", }, { "not normalized, different base and source", []string{path + "cppbench.contention"}, []string{path + "cppbench.small.contention"}, + []string{}, false, - [][]int64{{1700, 608878600}, {100, 23992}, {200, 179943}, {100, 17778444}, {100, 75976}, {300, 63568134}}, + []WantSample{ + { + values: []int64{1700, 608878600}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 23992}, + labels: map[string][]string{}, + }, + { + values: []int64{200, 179943}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 17778444}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 75976}, + labels: map[string][]string{}, + }, + { + values: []int64{300, 63568134}, + labels: map[string][]string{}, + }, + }, + "", }, { "normalized base is same as source", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, + []string{}, true, - [][]int64{}, + []WantSample{}, + "", }, { "normalized single source, multiple base (all profiles same)", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention", path + "cppbench.contention"}, + []string{}, true, - [][]int64{}, + []WantSample{}, + "", }, { "normalized different base and source", []string{path + "cppbench.contention"}, []string{path + "cppbench.small.contention"}, + []string{}, true, - [][]int64{{-229, -370}, {28, 0}, {57, 0}, {28, 80}, {28, 0}, {85, 287}}, + []WantSample{ + { + values: []int64{-229, -370}, + labels: map[string][]string{}, + }, + { + values: []int64{28, 0}, + labels: map[string][]string{}, + }, + { + values: []int64{57, 0}, + labels: map[string][]string{}, + }, + { + values: []int64{28, 80}, + labels: map[string][]string{}, + }, + { + values: []int64{28, 0}, + labels: map[string][]string{}, + }, + { + values: []int64{85, 287}, + labels: map[string][]string{}, + }, + }, + "", + }, + { + "not normalized diff base is same as source", + []string{path + "cppbench.contention"}, + []string{}, + []string{path + "cppbench.contention"}, + false, + []WantSample{ + { + values: []int64{2700, 608881724}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 23992}, + labels: map[string][]string{}, + }, + { + values: []int64{200, 179943}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 17778444}, + labels: map[string][]string{}, + }, + { + values: []int64{100, 75976}, + labels: map[string][]string{}, + }, + { + values: []int64{300, 63568134}, + labels: map[string][]string{}, + }, + { + values: []int64{-2700, -608881724}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + { + values: []int64{-100, -23992}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + { + values: []int64{-200, -179943}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + { + values: []int64{-100, -17778444}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + { + values: []int64{-100, -75976}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + { + values: []int64{-300, -63568134}, + labels: map[string][]string{"pprof::diff": {"true"}}, + }, + }, + "", + }, + { + "diff and base both specified", + []string{path + "cppbench.contention"}, + []string{path + "cppbench.contention"}, + []string{path + "cppbench.contention"}, + false, + []WantSample{}, + "only -base or -diff flag can be specified", }, } @@ -273,9 +443,15 @@ func TestFetchWithBase(t *testing.T) { base[i] = &s } + diffBase := make([]*string, len(tc.diffBases)) + for i, s := range tc.diffBases { + diffBase[i] = &s + } + f := testFlags{ stringLists: map[string][]*string{ "base": base, + "diff": diffBase, }, bools: map[string]bool{ "normalize": tc.normalize, @@ -289,29 +465,38 @@ func TestFetchWithBase(t *testing.T) { }) src, _, err := parseFlags(o) + if tc.wantErrorMsg != "" { + if err == nil { + t.Errorf("want error %q, got nil", tc.wantErrorMsg) + } + gotErrMsg := err.Error() + if gotErrMsg != tc.wantErrorMsg { + t.Errorf("want error %q, got error %q", tc.wantErrorMsg, gotErrMsg) + } + return + } + if err != nil { t.Fatalf("%s: %v", tc.desc, err) } p, err := fetchProfiles(src, o) - pprofVariables = baseVars + if err != nil { t.Fatal(err) } - if want, got := len(tc.expectedSamples), len(p.Sample); want != got { + if want, got := len(tc.wantSamples), len(p.Sample); want != got { t.Fatalf("want %d samples got %d", want, got) } if len(p.Sample) > 0 { for i, sample := range p.Sample { - if want, got := len(tc.expectedSamples[i]), len(sample.Value); want != got { - t.Errorf("want %d values for sample %d, got %d", want, i, got) + if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { + t.Errorf("for sample %d want values %v, got %v", i, tc.wantSamples[i], sample.Value) } - for j, value := range sample.Value { - if want, got := tc.expectedSamples[i][j], value; want != got { - t.Errorf("want value of %d for value %d of sample %d, got %d", want, j, i, got) - } + if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { + t.Errorf("for sample %d want labels %v, got %v", i, tc.wantSamples[i].labels, sample.Label) } } } diff --git a/internal/report/report.go b/internal/report/report.go index 15cadfb5..6afe9872 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -241,10 +241,10 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { for _, f := range prof.Function { f.Filename = trimPath(f.Filename, o.TrimPath, o.SourcePath) } - // Removes all numeric tags except for the bytes tag prior - // to making graph. - // TODO: modify to select first numeric tag if no bytes tag for _, s := range prof.Sample { + // Removes all numeric tags except for the bytes tag prior + // to making graph. + // TODO: modify to select first numeric tag if no bytes tag numLabels := make(map[string][]int64, len(s.NumLabel)) numUnits := make(map[string][]string, len(s.NumLabel)) for k, vs := range s.NumLabel { @@ -262,6 +262,21 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { } s.NumLabel = numLabels s.NumUnit = numUnits + + // Also remove pprof::diff=true string tag. + if values, ok := s.Label["pprof::diff"]; ok { + newVals := []string{} + for _, v := range values { + if v != "true" { + newVals = append(newVals, v) + } + } + if len(newVals) == 0 { + delete(s.Label, "pprof::diff") + } else if len(newVals) != len(values) { + s.Label["pprof::diff"] = newVals + } + } } formatTag := func(v int64, key string) string { @@ -1212,10 +1227,10 @@ func NewDefault(prof *profile.Profile, options Options) *Report { return New(prof, o) } -// computeTotal computes the sum of all sample values. This will be -// used to compute percentages. +// computeTotal computes the sum of the absolute value of all sample values. +// This will be used to compute percentages. func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 { - var div, ret int64 + var div, total, diffTotal int64 for _, sample := range prof.Sample { var d, v int64 v = value(sample.Value) @@ -1225,13 +1240,19 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i if v < 0 { v = -v } - ret += v + total += v + if sample.HasTag("pprof::diff", "true") { + diffTotal += v + } div += d } + if diffTotal > 0 { + total = diffTotal + } if div != 0 { - return ret / div + return total / div } - return ret + return total } // Report contains the data and associated routines to extract a diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 49c6e493..63c82e50 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -287,3 +287,122 @@ func TestLegendActiveFilters(t *testing.T) { } } } + +func TestComputTotal(t *testing.T) { + p1 := testProfile.Copy() + p1.Sample = []*profile.Sample{ + { + Location: []*profile.Location{testL[0]}, + Value: []int64{1, 1}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{1, 10}, + }, + { + Location: []*profile.Location{testL[4], testL[2], testL[0]}, + Value: []int64{1, 100}, + }, + } + + p2 := testProfile.Copy() + p2.Sample = []*profile.Sample{ + { + Location: []*profile.Location{testL[0]}, + Value: []int64{1, 1}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{1, -10}, + }, + { + Location: []*profile.Location{testL[4], testL[2], testL[0]}, + Value: []int64{1, 100}, + }, + } + + p3 := testProfile.Copy() + p3.Sample = []*profile.Sample{ + { + Location: []*profile.Location{testL[0]}, + Value: []int64{10000, 1}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{-10, 3}, + Label: map[string][]string{"pprof::diff": {"true"}}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{1000, -10}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{-9000, 3}, + Label: map[string][]string{"pprof::diff": {"true"}}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{-1, 3}, + Label: map[string][]string{"pprof::diff": {"true"}}, + }, + { + Location: []*profile.Location{testL[4], testL[2], testL[0]}, + Value: []int64{100, 100}, + }, + { + Location: []*profile.Location{testL[2], testL[1], testL[0]}, + Value: []int64{100, 3}, + Label: map[string][]string{"pprof::diff": {"true"}}, + }, + } + + testcases := []struct { + desc string + prof *profile.Profile + value, meanDiv func(v []int64) int64 + wantTotal int64 + }{ + { + desc: "non-diff profile, all positive values, index 1", + prof: p1, + value: func(v []int64) int64 { + return v[0] + }, + wantTotal: 3, + }, + { + desc: "non-diff profile, all positive values, index 2", + prof: p1, + value: func(v []int64) int64 { + return v[1] + }, + wantTotal: 111, + }, + { + desc: "non-diff profile, some negative values", + prof: p2, + value: func(v []int64) int64 { + return v[1] + }, + wantTotal: 111, + }, + { + desc: "diff profile, some negative values", + prof: p3, + value: func(v []int64) int64 { + return v[0] + }, + wantTotal: 9111, + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv) + if gotTotal != tc.wantTotal { + t.Errorf("want total %d, got %v", tc.wantTotal, gotTotal) + } + }) + } +} diff --git a/profile/profile.go b/profile/profile.go index 350538bf..5a6e8abe 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -674,6 +674,36 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin return strings.Join(ls, " ") } +// AddTag adds a tag with the specified key and value to all samples in the +// profile. +func (p *Profile) AddTag(key, value string) { + for _, sample := range p.Sample { + if sample.HasTag(key, value) { + continue + } + if values := sample.Label[key]; values != nil { + sample.Label[key] = append(values, value) + } else { + if sample.Label == nil { + sample.Label = make(map[string][]string) + } + sample.Label[key] = []string{value} + } + } +} + +// HasTag returns true if a sample has a tag with indicated key and value. +func (s *Sample) HasTag(key, value string) bool { + if values, ok := s.Label[key]; ok { + for _, v := range values { + if v == value { + return true + } + } + } + return false +} + // Scale multiplies all sample values in a profile by a constant. func (p *Profile) Scale(ratio float64) { if ratio == 1 { diff --git a/profile/profile_test.go b/profile/profile_test.go index 8ed67b1d..a83459a9 100644 --- a/profile/profile_test.go +++ b/profile/profile_test.go @@ -540,6 +540,39 @@ var testProfile5 = &Profile{ Mapping: cpuM, } +var testProfile6 = &Profile{ + PeriodType: &ValueType{Type: "cpu", Unit: "milliseconds"}, + Period: 1, + DurationNanos: 10e9, + SampleType: []*ValueType{ + {Type: "samples", Unit: "count"}, + }, + Sample: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value1", "value2", "value3"}, + "key2": {"value1"}, + }, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value2"}, + }, + }, + }, + Location: cpuL, + Function: cpuF, + Mapping: cpuM, +} + var aggTests = map[string]aggTest{ "precise": {true, true, true, true, 5}, "fileline": {false, true, true, true, 4}, @@ -912,6 +945,134 @@ func locationHash(s *Sample) string { return tb } +func TestHasTag(t *testing.T) { + var testcases = []struct { + desc string + sample *Sample + key string + value string + wantHasTag bool + }{ + { + desc: "empty label does not have tag", + sample: &Sample{ + Location: []*Location{cpuL[0]}, + Value: []int64{1000, 1000}, + }, + key: "key", + value: "value", + wantHasTag: false, + }, + { + desc: "label with one key and value has tag", + sample: &Sample{ + Location: []*Location{cpuL[0]}, + Value: []int64{1000, 1000}, + Label: map[string][]string{"key": {"value"}}, + }, + key: "key", + value: "value", + wantHasTag: true, + }, + { + desc: "label with one key and value does not have tag", + sample: &Sample{ + Location: []*Location{cpuL[0]}, + Value: []int64{1000, 1000}, + Label: map[string][]string{"key": {"value"}}, + }, + key: "key1", + value: "value1", + wantHasTag: false, + }, + { + desc: "label with many keys and values has tag", + sample: &Sample{ + Location: []*Location{cpuL[0]}, + Value: []int64{1000, 1000}, + Label: map[string][]string{ + "key1": {"value2", "value1"}, + "key2": {"value1", "value2", "value2"}, + "key3": {"value1", "value2", "value2"}, + }, + }, + key: "key1", + value: "value1", + wantHasTag: true, + }, + { + desc: "label with many keys and values does not have tag", + sample: &Sample{ + Location: []*Location{cpuL[0]}, + Value: []int64{1000, 1000}, + Label: map[string][]string{ + "key1": {"value2", "value1"}, + "key2": {"value1", "value2", "value2"}, + "key3": {"value1", "value2", "value2"}, + }, + }, + key: "key5", + value: "value5", + wantHasTag: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + if gotHasTag := tc.sample.HasTag(tc.key, tc.value); gotHasTag != tc.wantHasTag { + t.Errorf("sample.HasTag(%q, %q) got %v, want %v", tc.key, tc.value, gotHasTag, tc.wantHasTag) + } + }) + } +} + +func TestAddTag(t *testing.T) { + var testcases = []struct { + desc string + profile *Profile + addKey string + addVal string + wantTags []map[string][]string + }{ + { + desc: "some samples have tag already", + profile: testProfile6.Copy(), + addKey: "key1", + addVal: "value1", + wantTags: []map[string][]string{ + {"key1": {"value1"}}, + {"key1": {"value1", "value2", "value3"}, "key2": {"value1"}}, + {"key1": {"value2", "value1"}}, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + tc.profile.AddTag(tc.addKey, tc.addVal) + if got, want := len(tc.profile.Sample), len(tc.wantTags); got != want { + t.Fatalf("got %v samples, want %v samples", got, want) + } + for i, sample := range tc.profile.Sample { + wantTags := tc.wantTags[i] + if got, want := len(sample.Label), len(wantTags); got != want { + t.Errorf("got %v label keys for sample %v, want %v", got, i, want) + continue + } + for wantKey, wantValues := range wantTags { + if gotValues, ok := sample.Label[wantKey]; ok { + if !reflect.DeepEqual(gotValues, wantValues) { + t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) + } + } else { + t.Errorf("for key %s got no values, want %v", wantKey, wantValues) + } + } + } + }) + } +} + func TestNumLabelUnits(t *testing.T) { var tagFilterTests = []struct { desc string From b20456bf717b218c43d53c5bf265d76127c8583c Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 1 May 2018 14:36:40 -0700 Subject: [PATCH 02/15] address comments and fix errors which appeared in manual testing --- internal/driver/cli.go | 28 +++++++++++++++++++--------- internal/driver/fetch.go | 2 +- internal/driver/fetch_test.go | 2 +- internal/report/report.go | 25 +++++++------------------ internal/report/report_test.go | 2 +- profile/profile.go | 17 +++++------------ profile/profile_test.go | 16 ++++++++-------- 7 files changed, 42 insertions(+), 50 deletions(-) diff --git a/internal/driver/cli.go b/internal/driver/cli.go index b74ab5f9..d35c72c8 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -142,20 +142,30 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { Comment: *flagAddComment, } - if len(*flagBase) > 0 && len(*flagDiff) > 0 { - return nil, nil, fmt.Errorf("only -base or -diff flag can be specified") - } - base := *flagBase - if len(*flagDiff) > 0 { - base = *flagDiff - source.DiffBase = true + var base []string + for _, s := range *flagBase { + if *s != "" { + base = append(base, *s) + } } - for _, s := range base { + + var diff []string + for _, s := range *flagDiff { if *s != "" { - source.Base = append(source.Base, *s) + diff = append(diff, *s) } } + if len(base) > 0 && len(diff) > 0 { + return nil, nil, fmt.Errorf("-base and -diff flags cannot both be specified") + } + + source.Base = base + if len(diff) > 0 { + source.Base = diff + source.DiffBase = true + } + normalize := pprofVariables["normalize"].boolValue() if normalize && len(source.Base) == 0 { return nil, nil, fmt.Errorf("Must have base profile to normalize by") diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index 4c175ee7..ac6ba464 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -136,7 +136,7 @@ func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch defer wg.Done() pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui) if pbase != nil && isDiffBase { - pbase.AddTag("pprof::diff", "true") + pbase.SetTag("pprof::diff", []string{"true"}) } }() wg.Wait() diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index 23eedf2b..be19e3bd 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -430,7 +430,7 @@ func TestFetchWithBase(t *testing.T) { []string{path + "cppbench.contention"}, false, []WantSample{}, - "only -base or -diff flag can be specified", + "-base and -diff flags cannot both be specified", }, } diff --git a/internal/report/report.go b/internal/report/report.go index 6afe9872..251a8263 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -241,10 +241,10 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { for _, f := range prof.Function { f.Filename = trimPath(f.Filename, o.TrimPath, o.SourcePath) } + // Removes all numeric tags except for the bytes tag prior + // to making graph. + // TODO: modify to select first numeric tag if no bytes tag for _, s := range prof.Sample { - // Removes all numeric tags except for the bytes tag prior - // to making graph. - // TODO: modify to select first numeric tag if no bytes tag numLabels := make(map[string][]int64, len(s.NumLabel)) numUnits := make(map[string][]string, len(s.NumLabel)) for k, vs := range s.NumLabel { @@ -262,23 +262,12 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { } s.NumLabel = numLabels s.NumUnit = numUnits - - // Also remove pprof::diff=true string tag. - if values, ok := s.Label["pprof::diff"]; ok { - newVals := []string{} - for _, v := range values { - if v != "true" { - newVals = append(newVals, v) - } - } - if len(newVals) == 0 { - delete(s.Label, "pprof::diff") - } else if len(newVals) != len(values) { - s.Label["pprof::diff"] = newVals - } - } } + // Remove tag marking samples from the base profiles, so it does not appear + // as a nodelet in the graph view. + prof.SetTag("pprof::diff", []string{}) + formatTag := func(v int64, key string) string { return measurement.ScaledLabel(v, key, o.OutputUnit) } diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 63c82e50..50473bea 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -288,7 +288,7 @@ func TestLegendActiveFilters(t *testing.T) { } } -func TestComputTotal(t *testing.T) { +func TestComputeTotal(t *testing.T) { p1 := testProfile.Copy() p1.Sample = []*profile.Sample{ { diff --git a/profile/profile.go b/profile/profile.go index 5a6e8abe..5647a7d9 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -674,21 +674,14 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin return strings.Join(ls, " ") } -// AddTag adds a tag with the specified key and value to all samples in the +// SetTag sets the specified key to the specified value for all samples in the // profile. -func (p *Profile) AddTag(key, value string) { +func (p *Profile) SetTag(key string, value []string) { for _, sample := range p.Sample { - if sample.HasTag(key, value) { - continue - } - if values := sample.Label[key]; values != nil { - sample.Label[key] = append(values, value) - } else { - if sample.Label == nil { - sample.Label = make(map[string][]string) - } - sample.Label[key] = []string{value} + if sample.Label == nil { + sample.Label = make(map[string][]string) } + sample.Label[key] = value } } diff --git a/profile/profile_test.go b/profile/profile_test.go index a83459a9..a9914577 100644 --- a/profile/profile_test.go +++ b/profile/profile_test.go @@ -1026,30 +1026,30 @@ func TestHasTag(t *testing.T) { } } -func TestAddTag(t *testing.T) { +func TestSetTag(t *testing.T) { var testcases = []struct { desc string profile *Profile - addKey string - addVal string + setKey string + setVal []string wantTags []map[string][]string }{ { desc: "some samples have tag already", profile: testProfile6.Copy(), - addKey: "key1", - addVal: "value1", + setKey: "key1", + setVal: []string{"value1"}, wantTags: []map[string][]string{ {"key1": {"value1"}}, - {"key1": {"value1", "value2", "value3"}, "key2": {"value1"}}, - {"key1": {"value2", "value1"}}, + {"key1": {"value1"}, "key2": {"value1"}}, + {"key1": {"value1"}}, }, }, } for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { - tc.profile.AddTag(tc.addKey, tc.addVal) + tc.profile.SetTag(tc.setKey, tc.setVal) if got, want := len(tc.profile.Sample), len(tc.wantTags); got != want { t.Fatalf("got %v samples, want %v samples", got, want) } From 190ce182e07f7b1f1fda6a206c2b2a7a62d1f641 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 1 May 2018 15:40:38 -0700 Subject: [PATCH 03/15] address comments --- internal/report/report.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/report/report.go b/internal/report/report.go index 251a8263..3dac5dfa 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -1217,9 +1217,10 @@ func NewDefault(prof *profile.Profile, options Options) *Report { } // computeTotal computes the sum of the absolute value of all sample values. -// This will be used to compute percentages. +// If any samples have the tag "pprof::diff" with value "true", then the total +// will only include samples with that tag. func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 { - var div, total, diffTotal int64 + var div, total, diffDiv, diffTotal int64 for _, sample := range prof.Sample { var d, v int64 v = value(sample.Value) @@ -1232,11 +1233,13 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i total += v if sample.HasTag("pprof::diff", "true") { diffTotal += v + diffDiv += d } div += d } if diffTotal > 0 { total = diffTotal + div = diffDiv } if div != 0 { return total / div From ebd76df97f6df6c1bbc3feebce9ecd287d8a807f Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Wed, 2 May 2018 10:17:50 -0700 Subject: [PATCH 04/15] rename tags and update documentation --- doc/README.md | 36 ++++++++++++++++++++++++++++------ internal/driver/cli.go | 16 +++++++-------- internal/driver/fetch.go | 2 +- internal/driver/fetch_test.go | 20 +++++++++---------- internal/report/report.go | 6 +++--- internal/report/report_test.go | 16 +++++++-------- 6 files changed, 60 insertions(+), 36 deletions(-) diff --git a/doc/README.md b/doc/README.md index de7c393e..be3ed0dc 100644 --- a/doc/README.md +++ b/doc/README.md @@ -216,6 +216,36 @@ search for them in a directory pointed to by the environment variable * **-weblist= _regex_:** Generates a source/assembly combined annotated listing for functions matching *regex*, and starts a web browser to display it. +## Comparing profiles + +pprof can subtract one profile's samples from another in order to compare them. +For that, use the **-diff_base= _profile_** option, where *profile* is the filename +or URL for the profile to be subtracted (referred to as the base profile). When +using this mode, percentages reported in the output will be relative to the +total of the base profile. To separate samples that are part of the diff base +profile from those in the source profile, a string tag, "base"="1" is added to +all samples in the base profile. This means that if one saves the merged profile +and looks at it again with pprof, it will still be treated as a profile with a +diff base. It also means that any other samples which also have this tag will +be treated as part of the diff base profile. + +If one is interested in the relative differences between the source and base +profiles (for example, which profile has a larger percentage of CPU time used +in a particular function), it can be useful to use the **-normalize** option. +With this option, the source profile is scaled so that the total of samples in +the source profile is equal to the total of samples in the base profile prior +to merging the profiles. + +When comparing two cumulative profiles, for example two contention profiles +collected from the same program at different times, use the **-base** flag +in place of the **-diff_base** flag. When this flag is used and no negative +values appear in the merged profile when aggregated at the address-level, +which would be the case when comparing cummulative profiles collected from the +same program, percentages reported will be relative to the the difference +between the total for the source profile and the total for the base profile. +In the general case, percentages will be relative to the total of the absolute +value of all samples when aggregated at the address level. + # Fetching profiles pprof can read profiles from a file or directly from a URL over http. Its native @@ -237,11 +267,6 @@ them. This is useful to combine profiles from multiple processes of a distributed job. The profiles may be from different programs but must be compatible (for example, CPU profiles cannot be combined with heap profiles). -pprof can subtract a profile from another in order to compare them. For that, -use the **-base= _profile_** option, where *profile* is the filename or URL for the -profile to be subtracted. This may result on some report entries having negative -values. - ## Symbolization pprof can add symbol information to a profile that was collected only with @@ -286,4 +311,3 @@ the symbolization handler. * **-symbolize=demangle=templates:** Demangle, and trim function parameters, but not template parameters. - diff --git a/internal/driver/cli.go b/internal/driver/cli.go index d35c72c8..741fbbc8 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -45,7 +45,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { flag := o.Flagset // Comparisons. flagBase := flag.StringList("base", "", "Source for base profile for profile subtraction") - flagDiff := flag.StringList("diff", "", "Source for base profile for comparison") + flagDiffBase := flag.StringList("diff_base", "", "Source for diff base profile for comparison") // Source options. flagSymbolize := flag.String("symbolize", "", "Options for profile symbolization") flagBuildID := flag.String("buildid", "", "Override build id for first mapping") @@ -149,20 +149,20 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { } } - var diff []string - for _, s := range *flagDiff { + var diffBase []string + for _, s := range *flagDiffBase { if *s != "" { - diff = append(diff, *s) + diffBase = append(diffBase, *s) } } - if len(base) > 0 && len(diff) > 0 { - return nil, nil, fmt.Errorf("-base and -diff flags cannot both be specified") + if len(base) > 0 && len(diffBase) > 0 { + return nil, nil, fmt.Errorf("-base and -diff_base flags cannot both be specified") } source.Base = base - if len(diff) > 0 { - source.Base = diff + if len(diffBase) > 0 { + source.Base = diffBase source.DiffBase = true } diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index ac6ba464..5c9180f3 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -136,7 +136,7 @@ func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch defer wg.Done() pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui) if pbase != nil && isDiffBase { - pbase.SetTag("pprof::diff", []string{"true"}) + pbase.SetTag("base", []string{"1"}) } }() wg.Wait() diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index be19e3bd..4f5c82f4 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -398,39 +398,39 @@ func TestFetchWithBase(t *testing.T) { }, { values: []int64{-2700, -608881724}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, { values: []int64{-100, -23992}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, { values: []int64{-200, -179943}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, { values: []int64{-100, -17778444}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, { values: []int64{-100, -75976}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, { values: []int64{-300, -63568134}, - labels: map[string][]string{"pprof::diff": {"true"}}, + labels: map[string][]string{"base": {"1"}}, }, }, "", }, { - "diff and base both specified", + "diff_base and base both specified", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, false, []WantSample{}, - "-base and -diff flags cannot both be specified", + "-base and -diff_base flags cannot both be specified", }, } @@ -450,8 +450,8 @@ func TestFetchWithBase(t *testing.T) { f := testFlags{ stringLists: map[string][]*string{ - "base": base, - "diff": diffBase, + "base": base, + "diff_base": diffBase, }, bools: map[string]bool{ "normalize": tc.normalize, diff --git a/internal/report/report.go b/internal/report/report.go index 3dac5dfa..1515021c 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -266,7 +266,7 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { // Remove tag marking samples from the base profiles, so it does not appear // as a nodelet in the graph view. - prof.SetTag("pprof::diff", []string{}) + prof.SetTag("base", []string{}) formatTag := func(v int64, key string) string { return measurement.ScaledLabel(v, key, o.OutputUnit) @@ -1217,7 +1217,7 @@ func NewDefault(prof *profile.Profile, options Options) *Report { } // computeTotal computes the sum of the absolute value of all sample values. -// If any samples have the tag "pprof::diff" with value "true", then the total +// If any samples have the tag "base" with value "1", then the total // will only include samples with that tag. func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 { var div, total, diffDiv, diffTotal int64 @@ -1231,7 +1231,7 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i v = -v } total += v - if sample.HasTag("pprof::diff", "true") { + if sample.HasTag("base", "1") { diffTotal += v diffDiv += d } diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 50473bea..8e689aef 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -330,7 +330,7 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-10, 3}, - Label: map[string][]string{"pprof::diff": {"true"}}, + Label: map[string][]string{"base": {"1"}}, }, { Location: []*profile.Location{testL[2], testL[1], testL[0]}, @@ -339,12 +339,12 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-9000, 3}, - Label: map[string][]string{"pprof::diff": {"true"}}, + Label: map[string][]string{"base": {"1"}}, }, { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-1, 3}, - Label: map[string][]string{"pprof::diff": {"true"}}, + Label: map[string][]string{"base": {"1"}}, }, { Location: []*profile.Location{testL[4], testL[2], testL[0]}, @@ -353,7 +353,7 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{100, 3}, - Label: map[string][]string{"pprof::diff": {"true"}}, + Label: map[string][]string{"base": {"1"}}, }, } @@ -364,7 +364,7 @@ func TestComputeTotal(t *testing.T) { wantTotal int64 }{ { - desc: "non-diff profile, all positive values, index 1", + desc: "no diff base, all positive values, index 1", prof: p1, value: func(v []int64) int64 { return v[0] @@ -372,7 +372,7 @@ func TestComputeTotal(t *testing.T) { wantTotal: 3, }, { - desc: "non-diff profile, all positive values, index 2", + desc: "no diff base, all positive values, index 2", prof: p1, value: func(v []int64) int64 { return v[1] @@ -380,7 +380,7 @@ func TestComputeTotal(t *testing.T) { wantTotal: 111, }, { - desc: "non-diff profile, some negative values", + desc: "no diff base, some negative values", prof: p2, value: func(v []int64) int64 { return v[1] @@ -388,7 +388,7 @@ func TestComputeTotal(t *testing.T) { wantTotal: 111, }, { - desc: "diff profile, some negative values", + desc: "diff base, some negative values", prof: p3, value: func(v []int64) int64 { return v[0] From 92f6fbe582bc62653a1639356548c47e6f823ddd Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Wed, 2 May 2018 12:41:59 -0700 Subject: [PATCH 05/15] fix TestFetchWithBase --- internal/driver/fetch_test.go | 108 +++++++++++++++++----------------- 1 file changed, 55 insertions(+), 53 deletions(-) diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index 4f5c82f4..923a8712 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -435,72 +435,74 @@ func TestFetchWithBase(t *testing.T) { } for _, tc := range testcases { - t.Run(tc.desc, func(t *testing.T) { - pprofVariables = baseVars.makeCopy() + pprofVariables = baseVars.makeCopy() - base := make([]*string, len(tc.bases)) - for i, s := range tc.bases { - base[i] = &s - } + base := make([]*string, len(tc.bases)) + for i, s := range tc.bases { + base[i] = &s + } - diffBase := make([]*string, len(tc.diffBases)) - for i, s := range tc.diffBases { - diffBase[i] = &s - } + diffBase := make([]*string, len(tc.diffBases)) + for i, s := range tc.diffBases { + diffBase[i] = &s + } - f := testFlags{ - stringLists: map[string][]*string{ - "base": base, - "diff_base": diffBase, - }, - bools: map[string]bool{ - "normalize": tc.normalize, - }, - } - f.args = tc.sources + f := testFlags{ + stringLists: map[string][]*string{ + "base": base, + "diff_base": diffBase, + }, + bools: map[string]bool{ + "normalize": tc.normalize, + }, + } + f.args = tc.sources - o := setDefaults(&plugin.Options{ - UI: &proftest.TestUI{T: t, AllowRx: "Local symbolization failed|Some binary filenames not available"}, - Flagset: f, - }) - src, _, err := parseFlags(o) + o := setDefaults(&plugin.Options{ + UI: &proftest.TestUI{T: t, AllowRx: "Local symbolization failed|Some binary filenames not available"}, + Flagset: f, + }) + src, _, err := parseFlags(o) - if tc.wantErrorMsg != "" { - if err == nil { - t.Errorf("want error %q, got nil", tc.wantErrorMsg) - } - gotErrMsg := err.Error() - if gotErrMsg != tc.wantErrorMsg { - t.Errorf("want error %q, got error %q", tc.wantErrorMsg, gotErrMsg) - } - return + if tc.wantErrorMsg != "" { + if err == nil { + t.Errorf("%s: want error %q, got nil", tc.desc, tc.wantErrorMsg) + continue } - - if err != nil { - t.Fatalf("%s: %v", tc.desc, err) + gotErrMsg := err.Error() + if gotErrMsg != tc.wantErrorMsg { + t.Errorf("%s: want error %q, got error %q", tc.desc, tc.wantErrorMsg, gotErrMsg) } + continue + } - p, err := fetchProfiles(src, o) + if err != nil { + t.Errorf("%s: %v", tc.desc, err) + continue + } - if err != nil { - t.Fatal(err) - } + p, err := fetchProfiles(src, o) - if want, got := len(tc.wantSamples), len(p.Sample); want != got { - t.Fatalf("want %d samples got %d", want, got) - } + if err != nil { + t.Errorf("%s: %v", tc.desc, err) + continue + } + + if want, got := len(tc.wantSamples), len(p.Sample); want != got { + t.Errorf("%s: want %d samples got %d", tc.desc, want, got) + continue + } - if len(p.Sample) > 0 { - for i, sample := range p.Sample { - if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { - t.Errorf("for sample %d want values %v, got %v", i, tc.wantSamples[i], sample.Value) - } - if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { - t.Errorf("for sample %d want labels %v, got %v", i, tc.wantSamples[i].labels, sample.Label) - } + if len(p.Sample) > 0 { + for i, sample := range p.Sample { + if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { + t.Errorf("%s: for sample %d want values %v, got %v", tc.desc, i, tc.wantSamples[i], sample.Value) + } + if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { + t.Errorf("%s: for sample %d want labels %v, got %v", tc.desc, i, tc.wantSamples[i].labels, sample.Label) } } - }) + } } } From 6921532c987d99388c4b4c327b793356c4c99abc Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Wed, 2 May 2018 13:39:02 -0700 Subject: [PATCH 06/15] update docs --- doc/README.md | 68 +++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/doc/README.md b/doc/README.md index be3ed0dc..9b8b0fce 100644 --- a/doc/README.md +++ b/doc/README.md @@ -216,35 +216,9 @@ search for them in a directory pointed to by the environment variable * **-weblist= _regex_:** Generates a source/assembly combined annotated listing for functions matching *regex*, and starts a web browser to display it. -## Comparing profiles - -pprof can subtract one profile's samples from another in order to compare them. -For that, use the **-diff_base= _profile_** option, where *profile* is the filename -or URL for the profile to be subtracted (referred to as the base profile). When -using this mode, percentages reported in the output will be relative to the -total of the base profile. To separate samples that are part of the diff base -profile from those in the source profile, a string tag, "base"="1" is added to -all samples in the base profile. This means that if one saves the merged profile -and looks at it again with pprof, it will still be treated as a profile with a -diff base. It also means that any other samples which also have this tag will -be treated as part of the diff base profile. - -If one is interested in the relative differences between the source and base -profiles (for example, which profile has a larger percentage of CPU time used -in a particular function), it can be useful to use the **-normalize** option. -With this option, the source profile is scaled so that the total of samples in -the source profile is equal to the total of samples in the base profile prior -to merging the profiles. - -When comparing two cumulative profiles, for example two contention profiles -collected from the same program at different times, use the **-base** flag -in place of the **-diff_base** flag. When this flag is used and no negative -values appear in the merged profile when aggregated at the address-level, -which would be the case when comparing cummulative profiles collected from the -same program, percentages reported will be relative to the the difference -between the total for the source profile and the total for the base profile. -In the general case, percentages will be relative to the total of the absolute -value of all samples when aggregated at the address level. +Samples in a profile may have tags. These tags have a name and a value; this +value can be either numeric or a string. pprof can select samples from a +profile based on these tags using the `-tagfocus` and `-tagignore` options. # Fetching profiles @@ -267,6 +241,42 @@ them. This is useful to combine profiles from multiple processes of a distributed job. The profiles may be from different programs but must be compatible (for example, CPU profiles cannot be combined with heap profiles). + +## Comparing profiles + +pprof can subtract a profile from another, provided the profiles are of +compatible types, in order to compare them. For that, use the +**-diff_base= _profile_** option, where *profile* is the filename or URL for the +profile to be subtracted. This may result in some report entries having negative +values. Percentages in the report are relative to the total of +the diff base. + +If the merged profile is output as a protocol buffer, all samples in +the diff base profile will have a label with the key "base" and a value of "1". +If pprof is then used to look at the merged profile, it will behave as if +separate source and base profiles were passed in. + +The **-base** option can be used in the place of the **-diff_base** option, and +may be preferrable when comparing two cumulative profiles, for example two +contention profiles collected from the same program at different times. When +this flag is used and no negative values appear in the merged profile when +aggregated at the address-level, percentages reported will be relative to the +the difference between the total for the source profile and the total for the +base profile. In the general case, percentages will be relative to the total of +the absolute value of all samples when aggregated at the address level. + +The **-normalize** option can be useful for determining the relative differences +between profiles, for example, which profile has a larger percentage of CPU time +used in a particular function. With this option, the source profile is scaled so +that the total of samples in the source profile is equal to the total of samples +in the base profile prior to merging the profiles. + +This flag can be only be used when a diff base or a base profile is specified, +so for example: + + pprof -normalize -diff_base=base source + + ## Symbolization pprof can add symbol information to a profile that was collected only with From 14703a057d28096dc3d93d26bd06651e8e203689 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Thu, 3 May 2018 15:31:08 -0700 Subject: [PATCH 07/15] use pprof::bass=true to mark diff base samples --- doc/README.md | 14 +-- internal/driver/cli.go | 51 +++++---- internal/driver/fetch.go | 2 +- internal/driver/fetch_test.go | 12 +-- internal/report/report.go | 8 +- internal/report/report_test.go | 8 +- profile/profile.go | 2 +- profile/profile_test.go | 189 ++++++++++++++++++++------------- 8 files changed, 165 insertions(+), 121 deletions(-) diff --git a/doc/README.md b/doc/README.md index 9b8b0fce..39af0cb0 100644 --- a/doc/README.md +++ b/doc/README.md @@ -216,10 +216,6 @@ search for them in a directory pointed to by the environment variable * **-weblist= _regex_:** Generates a source/assembly combined annotated listing for functions matching *regex*, and starts a web browser to display it. -Samples in a profile may have tags. These tags have a name and a value; this -value can be either numeric or a string. pprof can select samples from a -profile based on these tags using the `-tagfocus` and `-tagignore` options. - # Fetching profiles pprof can read profiles from a file or directly from a URL over http. Its native @@ -248,13 +244,13 @@ pprof can subtract a profile from another, provided the profiles are of compatible types, in order to compare them. For that, use the **-diff_base= _profile_** option, where *profile* is the filename or URL for the profile to be subtracted. This may result in some report entries having negative -values. Percentages in the report are relative to the total of +values. Percentages in the report are relative to the total of the diff base. -If the merged profile is output as a protocol buffer, all samples in -the diff base profile will have a label with the key "base" and a value of "1". -If pprof is then used to look at the merged profile, it will behave as if -separate source and base profiles were passed in. +If the merged profile is output as a protocol buffer, all samples in +the diff base profile will have a label with the key "pprof::base" and a value +of "true". If pprof is then used to look at the merged profile, it will behave +as if separate source and base profiles were passed in. The **-base** option can be used in the place of the **-diff_base** option, and may be preferrable when comparing two cumulative profiles, for example two diff --git a/internal/driver/cli.go b/internal/driver/cli.go index 741fbbc8..1b3a4fbb 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -142,22 +142,31 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { Comment: *flagAddComment, } - var base []string - for _, s := range *flagBase { - if *s != "" { - base = append(base, *s) - } + if err := source.addBaseProfiles(flagBase, flagDiffBase); err != nil { + return nil, nil, err } - var diffBase []string - for _, s := range *flagDiffBase { - if *s != "" { - diffBase = append(diffBase, *s) - } + normalize := pprofVariables["normalize"].boolValue() + if normalize && len(source.Base) == 0 { + return nil, nil, fmt.Errorf("Must have base profile to normalize by") } + source.Normalize = normalize + + if bu, ok := o.Obj.(*binutils.Binutils); ok { + bu.SetTools(*flagTools) + } + return source, cmd, nil +} + +// addBaseProfiles adds the list of base profiles or diff base profiles to +// the source. This function will return an error if both base and diff base +// profiles are specified. +func (source *source) addBaseProfiles(flagBase, flagDiffBase *[]*string) error { + base := parseStringListFlag(flagBase) + diffBase := parseStringListFlag(flagDiffBase) if len(base) > 0 && len(diffBase) > 0 { - return nil, nil, fmt.Errorf("-base and -diff_base flags cannot both be specified") + return fmt.Errorf("-base and -diff_base flags cannot both be specified") } source.Base = base @@ -165,17 +174,19 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { source.Base = diffBase source.DiffBase = true } + return nil +} - normalize := pprofVariables["normalize"].boolValue() - if normalize && len(source.Base) == 0 { - return nil, nil, fmt.Errorf("Must have base profile to normalize by") - } - source.Normalize = normalize - - if bu, ok := o.Obj.(*binutils.Binutils); ok { - bu.SetTools(*flagTools) +// parseStringListFlag list takes a StringList flag, and outputs an array of non-empty +// strings associated with the flag. +func parseStringListFlag(list *[]*string) []string { + var l []string + for _, s := range *list { + if *s != "" { + l = append(l, *s) + } } - return source, cmd, nil + return l } // installFlags creates command line flags for pprof variables. diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index 5c9180f3..61d6af79 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -136,7 +136,7 @@ func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch defer wg.Done() pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui) if pbase != nil && isDiffBase { - pbase.SetTag("base", []string{"1"}) + pbase.SetTag("pprof::base", []string{"true"}) } }() wg.Wait() diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index 923a8712..b0fc51b0 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -398,27 +398,27 @@ func TestFetchWithBase(t *testing.T) { }, { values: []int64{-2700, -608881724}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, { values: []int64{-100, -23992}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, { values: []int64{-200, -179943}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, { values: []int64{-100, -17778444}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, { values: []int64{-100, -75976}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, { values: []int64{-300, -63568134}, - labels: map[string][]string{"base": {"1"}}, + labels: map[string][]string{"pprof::base": {"true"}}, }, }, "", diff --git a/internal/report/report.go b/internal/report/report.go index 1515021c..d6a8a295 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -266,7 +266,7 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { // Remove tag marking samples from the base profiles, so it does not appear // as a nodelet in the graph view. - prof.SetTag("base", []string{}) + prof.SetTag("pprof::base", []string{}) formatTag := func(v int64, key string) string { return measurement.ScaledLabel(v, key, o.OutputUnit) @@ -1217,7 +1217,7 @@ func NewDefault(prof *profile.Profile, options Options) *Report { } // computeTotal computes the sum of the absolute value of all sample values. -// If any samples have the tag "base" with value "1", then the total +// If any samples have the tag "pprof::base" with value "true", then the total // will only include samples with that tag. func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 { var div, total, diffDiv, diffTotal int64 @@ -1231,11 +1231,11 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i v = -v } total += v - if sample.HasTag("base", "1") { + div += d + if sample.HasTag("pprof::base", "true") { diffTotal += v diffDiv += d } - div += d } if diffTotal > 0 { total = diffTotal diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 8e689aef..57e0bd26 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -330,7 +330,7 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-10, 3}, - Label: map[string][]string{"base": {"1"}}, + Label: map[string][]string{"pprof::base": {"true"}}, }, { Location: []*profile.Location{testL[2], testL[1], testL[0]}, @@ -339,12 +339,12 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-9000, 3}, - Label: map[string][]string{"base": {"1"}}, + Label: map[string][]string{"pprof::base": {"true"}}, }, { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{-1, 3}, - Label: map[string][]string{"base": {"1"}}, + Label: map[string][]string{"pprof::base": {"true"}}, }, { Location: []*profile.Location{testL[4], testL[2], testL[0]}, @@ -353,7 +353,7 @@ func TestComputeTotal(t *testing.T) { { Location: []*profile.Location{testL[2], testL[1], testL[0]}, Value: []int64{100, 3}, - Label: map[string][]string{"base": {"1"}}, + Label: map[string][]string{"pprof::base": {"true"}}, }, } diff --git a/profile/profile.go b/profile/profile.go index 5647a7d9..8b4d132d 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -679,7 +679,7 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin func (p *Profile) SetTag(key string, value []string) { for _, sample := range p.Sample { if sample.Label == nil { - sample.Label = make(map[string][]string) + sample.Label = map[string][]string{} } sample.Label[key] = value } diff --git a/profile/profile_test.go b/profile/profile_test.go index a9914577..21a162ab 100644 --- a/profile/profile_test.go +++ b/profile/profile_test.go @@ -540,39 +540,6 @@ var testProfile5 = &Profile{ Mapping: cpuM, } -var testProfile6 = &Profile{ - PeriodType: &ValueType{Type: "cpu", Unit: "milliseconds"}, - Period: 1, - DurationNanos: 10e9, - SampleType: []*ValueType{ - {Type: "samples", Unit: "count"}, - }, - Sample: []*Sample{ - { - Location: []*Location{cpuL[0]}, - Value: []int64{1000}, - }, - { - Location: []*Location{cpuL[0]}, - Value: []int64{1000}, - Label: map[string][]string{ - "key1": {"value1", "value2", "value3"}, - "key2": {"value1"}, - }, - }, - { - Location: []*Location{cpuL[0]}, - Value: []int64{1000}, - Label: map[string][]string{ - "key1": {"value2"}, - }, - }, - }, - Location: cpuL, - Function: cpuF, - Mapping: cpuM, -} - var aggTests = map[string]aggTest{ "precise": {true, true, true, true, 5}, "fileline": {false, true, true, true, 4}, @@ -948,53 +915,38 @@ func locationHash(s *Sample) string { func TestHasTag(t *testing.T) { var testcases = []struct { desc string - sample *Sample + labels map[string][]string key string value string wantHasTag bool }{ { - desc: "empty label does not have tag", - sample: &Sample{ - Location: []*Location{cpuL[0]}, - Value: []int64{1000, 1000}, - }, + desc: "empty label does not have tag", + labels: map[string][]string{}, key: "key", value: "value", wantHasTag: false, }, { - desc: "label with one key and value has tag", - sample: &Sample{ - Location: []*Location{cpuL[0]}, - Value: []int64{1000, 1000}, - Label: map[string][]string{"key": {"value"}}, - }, + desc: "label with one key and value has tag", + labels: map[string][]string{"key": {"value"}}, key: "key", value: "value", wantHasTag: true, }, { - desc: "label with one key and value does not have tag", - sample: &Sample{ - Location: []*Location{cpuL[0]}, - Value: []int64{1000, 1000}, - Label: map[string][]string{"key": {"value"}}, - }, + desc: "label with one key and value does not have tag", + labels: map[string][]string{"key": {"value"}}, key: "key1", value: "value1", wantHasTag: false, }, { desc: "label with many keys and values has tag", - sample: &Sample{ - Location: []*Location{cpuL[0]}, - Value: []int64{1000, 1000}, - Label: map[string][]string{ - "key1": {"value2", "value1"}, - "key2": {"value1", "value2", "value2"}, - "key3": {"value1", "value2", "value2"}, - }, + labels: map[string][]string{ + "key1": {"value2", "value1"}, + "key2": {"value1", "value2", "value2"}, + "key3": {"value1", "value2", "value2"}, }, key: "key1", value: "value1", @@ -1002,14 +954,10 @@ func TestHasTag(t *testing.T) { }, { desc: "label with many keys and values does not have tag", - sample: &Sample{ - Location: []*Location{cpuL[0]}, - Value: []int64{1000, 1000}, - Label: map[string][]string{ - "key1": {"value2", "value1"}, - "key2": {"value1", "value2", "value2"}, - "key3": {"value1", "value2", "value2"}, - }, + labels: map[string][]string{ + "key1": {"value2", "value1"}, + "key2": {"value1", "value2", "value2"}, + "key3": {"value1", "value2", "value2"}, }, key: "key5", value: "value5", @@ -1019,7 +967,10 @@ func TestHasTag(t *testing.T) { for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { - if gotHasTag := tc.sample.HasTag(tc.key, tc.value); gotHasTag != tc.wantHasTag { + sample := &Sample{ + Label: tc.labels, + } + if gotHasTag := sample.HasTag(tc.key, tc.value); gotHasTag != tc.wantHasTag { t.Errorf("sample.HasTag(%q, %q) got %v, want %v", tc.key, tc.value, gotHasTag, tc.wantHasTag) } }) @@ -1029,31 +980,117 @@ func TestHasTag(t *testing.T) { func TestSetTag(t *testing.T) { var testcases = []struct { desc string - profile *Profile + samples []*Sample setKey string setVal []string wantTags []map[string][]string }{ { - desc: "some samples have tag already", - profile: testProfile6.Copy(), - setKey: "key1", - setVal: []string{"value1"}, + desc: "some samples have tag already", + samples: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value1", "value2", "value3"}, + "key2": {"value1"}, + }, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value2"}, + }, + }, + }, + setKey: "key1", + setVal: []string{"value1"}, wantTags: []map[string][]string{ {"key1": {"value1"}}, {"key1": {"value1"}, "key2": {"value1"}}, {"key1": {"value1"}}, }, }, + { + desc: "no samples have tags", + samples: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + }, + }, + setKey: "key1", + setVal: []string{"value1"}, + wantTags: []map[string][]string{ + {"key1": {"value1"}}, + }, + }, + { + desc: "all samples have some tags, but not key being added", + samples: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key2": {"value2"}, + }, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key3": {"value3"}, + }, + }, + }, + setKey: "key1", + setVal: []string{"value1"}, + wantTags: []map[string][]string{ + {"key1": {"value1"}, "key2": {"value2"}}, + {"key1": {"value1"}, "key3": {"value3"}}, + }, + }, + { + desc: "all samples have key being added", + samples: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value1"}, + }, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value1"}, + }, + }, + }, + setKey: "key1", + setVal: []string{"value1"}, + wantTags: []map[string][]string{ + {"key1": {"value1"}}, + {"key1": {"value1"}}, + }, + }, } for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { - tc.profile.SetTag(tc.setKey, tc.setVal) - if got, want := len(tc.profile.Sample), len(tc.wantTags); got != want { + profile := testProfile1.Copy() + profile.Sample = tc.samples + profile.SetTag(tc.setKey, tc.setVal) + if got, want := len(profile.Sample), len(tc.wantTags); got != want { t.Fatalf("got %v samples, want %v samples", got, want) } - for i, sample := range tc.profile.Sample { + for i, sample := range profile.Sample { wantTags := tc.wantTags[i] if got, want := len(sample.Label), len(wantTags); got != want { t.Errorf("got %v label keys for sample %v, want %v", got, i, want) From 49ff3461acbe363c79330f0b8ac64a213680a1d7 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Fri, 4 May 2018 10:49:23 -0700 Subject: [PATCH 08/15] update docs --- doc/README.md | 68 +++++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/doc/README.md b/doc/README.md index 39af0cb0..1fa8dffe 100644 --- a/doc/README.md +++ b/doc/README.md @@ -240,38 +240,42 @@ compatible (for example, CPU profiles cannot be combined with heap profiles). ## Comparing profiles -pprof can subtract a profile from another, provided the profiles are of -compatible types, in order to compare them. For that, use the -**-diff_base= _profile_** option, where *profile* is the filename or URL for the -profile to be subtracted. This may result in some report entries having negative -values. Percentages in the report are relative to the total of -the diff base. - -If the merged profile is output as a protocol buffer, all samples in -the diff base profile will have a label with the key "pprof::base" and a value -of "true". If pprof is then used to look at the merged profile, it will behave -as if separate source and base profiles were passed in. - -The **-base** option can be used in the place of the **-diff_base** option, and -may be preferrable when comparing two cumulative profiles, for example two -contention profiles collected from the same program at different times. When -this flag is used and no negative values appear in the merged profile when -aggregated at the address-level, percentages reported will be relative to the -the difference between the total for the source profile and the total for the -base profile. In the general case, percentages will be relative to the total of -the absolute value of all samples when aggregated at the address level. - -The **-normalize** option can be useful for determining the relative differences -between profiles, for example, which profile has a larger percentage of CPU time -used in a particular function. With this option, the source profile is scaled so -that the total of samples in the source profile is equal to the total of samples -in the base profile prior to merging the profiles. - -This flag can be only be used when a diff base or a base profile is specified, -so for example: - - pprof -normalize -diff_base=base source - +pprof can subtract one profile from another, provided the profiles are of +compatible types. pprof has two options which can be used to specify the +filename or URL for a profile to be subtracted from the source profile: + +* **-diff_base= _profile_:** useful for comparing any two profiles of compatible +types (i.e. any two heap profiles). Percentages in the output are relative to +the total of samples in the diff base profile. + +* **-base= _profile_:** useful for subtracting a cumulative profile, like a +[golang block profile](https://golang.org/doc/diagnostics.html#profiling), +from another cumulative profile collected from the same program at a later time. +When comparing cumulative profiles collected on the same profile, percentages in +the output are relative to the the difference between the total for the source +profile and the total for the base profile. + +The following flag can be used when a base profile is specified with either the +`-diff_base` or the `-base` option: + +* **-normalize**: Scales the source profile so that that the total of samples +in the source profile is equal to the total of samples in the base profile prior +to subtracting the base profile from the source profile. Useful for determining +the relative differences between profiles, for example, which profile has a +larger percentage of CPU time used in a particular function. + +When using the **-diff_base** option, some report entries may have negative +values. If the merged profile is output as a protocol buffer, all samples in the +diff base profile will have a label with the key "pprof::base" and a value of +"true". If pprof is then used to look at the merged profile, it will behave as +if separate source and base profiles were passed in. + +When using the **-base** option to subtract one cumulative profile from another +collected on the same program at a later time, percentages will be relative to +the difference between the total for the source profile and the total for +the base profile, and all values will be positive. In the general case, some +report entries may have negative values and percentages will be relative to the +total of the absolute value of all samples when aggregated at the address level. ## Symbolization From f486417078936b7bb3f8a4678003e56de9f89b9b Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 8 May 2018 13:47:06 -0700 Subject: [PATCH 09/15] update documents --- doc/README.md | 70 +++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/doc/README.md b/doc/README.md index 1fa8dffe..c76ef5d5 100644 --- a/doc/README.md +++ b/doc/README.md @@ -216,53 +216,30 @@ search for them in a directory pointed to by the environment variable * **-weblist= _regex_:** Generates a source/assembly combined annotated listing for functions matching *regex*, and starts a web browser to display it. -# Fetching profiles - -pprof can read profiles from a file or directly from a URL over http. Its native -format is a gzipped profile.proto file, but it can also accept some legacy -formats generated by [gperftools](https://github.com/gperftools/gperftools). - -When fetching from a URL handler, pprof accepts options to indicate how much to -wait for the profile. - -* **-seconds= _int_:** Makes pprof request for a profile with the specified duration - in seconds. Only makes sense for profiles based on elapsed time, such as CPU - profiles. -* **-timeout= _int_:** Makes pprof wait for the specified timeout when retrieving a - profile over http. If not specified, pprof will use heuristics to determine a - reasonable timeout. - -If multiple profiles are specified, pprof will fetch them all and merge -them. This is useful to combine profiles from multiple processes of a -distributed job. The profiles may be from different programs but must be -compatible (for example, CPU profiles cannot be combined with heap profiles). - - ## Comparing profiles pprof can subtract one profile from another, provided the profiles are of -compatible types. pprof has two options which can be used to specify the -filename or URL for a profile to be subtracted from the source profile: +compatible types (i.e. two heap profiles). pprof has two options which can be +used to specify the filename or URL for a profile to be subtracted from the +source profile: -* **-diff_base= _profile_:** useful for comparing any two profiles of compatible -types (i.e. any two heap profiles). Percentages in the output are relative to -the total of samples in the diff base profile. +* **-diff_base= _profile_:** useful for comparing two profiles. Percentages in +the output are relative to the total of samples in the diff base profile. * **-base= _profile_:** useful for subtracting a cumulative profile, like a [golang block profile](https://golang.org/doc/diagnostics.html#profiling), from another cumulative profile collected from the same program at a later time. When comparing cumulative profiles collected on the same profile, percentages in -the output are relative to the the difference between the total for the source +the output are relative to the difference between the total for the source profile and the total for the base profile. -The following flag can be used when a base profile is specified with either the -`-diff_base` or the `-base` option: - -* **-normalize**: Scales the source profile so that that the total of samples -in the source profile is equal to the total of samples in the base profile prior -to subtracting the base profile from the source profile. Useful for determining -the relative differences between profiles, for example, which profile has a -larger percentage of CPU time used in a particular function. +The **-normalize** flag can be used when a base profile is specified with either +the `-diff_base` or the `-base` option. This flag scales the source profile so +that the total of samples in the source profile is equal to the total of samples +in the base profile prior to subtracting the base profile from the source +profile. Useful for determining the relative differences between profiles, for +example, which profile has a larger percentage of CPU time used in a particular +function. When using the **-diff_base** option, some report entries may have negative values. If the merged profile is output as a protocol buffer, all samples in the @@ -277,6 +254,27 @@ the base profile, and all values will be positive. In the general case, some report entries may have negative values and percentages will be relative to the total of the absolute value of all samples when aggregated at the address level. +# Fetching profiles + +pprof can read profiles from a file or directly from a URL over http. Its native +format is a gzipped profile.proto file, but it can also accept some legacy +formats generated by [gperftools](https://github.com/gperftools/gperftools). + +When fetching from a URL handler, pprof accepts options to indicate how much to +wait for the profile. + +* **-seconds= _int_:** Makes pprof request for a profile with the specified duration + in seconds. Only makes sense for profiles based on elapsed time, such as CPU + profiles. +* **-timeout= _int_:** Makes pprof wait for the specified timeout when retrieving a + profile over http. If not specified, pprof will use heuristics to determine a + reasonable timeout. + +If multiple profiles are specified, pprof will fetch them all and merge +them. This is useful to combine profiles from multiple processes of a +distributed job. The profiles may be from different programs but must be +compatible (for example, CPU profiles cannot be combined with heap profiles). + ## Symbolization pprof can add symbol information to a profile that was collected only with From bd80566b11c3b41f6532f22b2ed26e0ea884e7ab Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 8 May 2018 14:12:06 -0700 Subject: [PATCH 10/15] address comments on code --- doc/README.md | 2 +- internal/driver/cli.go | 14 ++-- internal/driver/fetch.go | 10 +-- internal/report/report.go | 4 +- profile/profile.go | 18 +++- profile/profile_test.go | 167 ++++++++++++++++++++++++++------------ 6 files changed, 146 insertions(+), 69 deletions(-) diff --git a/doc/README.md b/doc/README.md index c76ef5d5..171dbc28 100644 --- a/doc/README.md +++ b/doc/README.md @@ -229,7 +229,7 @@ the output are relative to the total of samples in the diff base profile. * **-base= _profile_:** useful for subtracting a cumulative profile, like a [golang block profile](https://golang.org/doc/diagnostics.html#profiling), from another cumulative profile collected from the same program at a later time. -When comparing cumulative profiles collected on the same profile, percentages in +When comparing cumulative profiles collected on the same program, percentages in the output are relative to the difference between the total for the source profile and the total for the base profile. diff --git a/internal/driver/cli.go b/internal/driver/cli.go index 1b3a4fbb..4292b64b 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -142,7 +142,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { Comment: *flagAddComment, } - if err := source.addBaseProfiles(flagBase, flagDiffBase); err != nil { + if err := source.addBaseProfiles(*flagBase, *flagDiffBase); err != nil { return nil, nil, err } @@ -161,9 +161,9 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { // addBaseProfiles adds the list of base profiles or diff base profiles to // the source. This function will return an error if both base and diff base // profiles are specified. -func (source *source) addBaseProfiles(flagBase, flagDiffBase *[]*string) error { - base := parseStringListFlag(flagBase) - diffBase := parseStringListFlag(flagDiffBase) +func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { + base := dropEmpty(flagBase) + diffBase := dropEmpty(flagDiffBase) if len(base) > 0 && len(diffBase) > 0 { return fmt.Errorf("-base and -diff_base flags cannot both be specified") @@ -177,11 +177,11 @@ func (source *source) addBaseProfiles(flagBase, flagDiffBase *[]*string) error { return nil } -// parseStringListFlag list takes a StringList flag, and outputs an array of non-empty +// dropEmpty list takes a StringList flag, and outputs an array of non-empty // strings associated with the flag. -func parseStringListFlag(list *[]*string) []string { +func dropEmpty(list []*string) []string { var l []string - for _, s := range *list { + for _, s := range list { if *s != "" { l = append(l, *s) } diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index 61d6af79..7a7a1a20 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -57,12 +57,15 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) { }) } - p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, s.DiffBase, o.Fetch, o.Obj, o.UI) + p, pbase, m, mbase, save, err := grabSourcesAndBases(sources, bases, o.Fetch, o.Obj, o.UI) if err != nil { return nil, err } if pbase != nil { + if s.DiffBase { + pbase.SetLabel("pprof::base", []string{"true"}) + } if s.Normalize { err := p.Normalize(pbase) if err != nil { @@ -120,7 +123,7 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) { return p, nil } -func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) { +func grabSourcesAndBases(sources, bases []profileSource, fetch plugin.Fetcher, obj plugin.ObjTool, ui plugin.UI) (*profile.Profile, *profile.Profile, plugin.MappingSources, plugin.MappingSources, bool, error) { wg := sync.WaitGroup{} wg.Add(2) var psrc, pbase *profile.Profile @@ -135,9 +138,6 @@ func grabSourcesAndBases(sources, bases []profileSource, isDiffBase bool, fetch go func() { defer wg.Done() pbase, mbase, savebase, countbase, errbase = chunkedGrab(bases, fetch, obj, ui) - if pbase != nil && isDiffBase { - pbase.SetTag("pprof::base", []string{"true"}) - } }() wg.Wait() save := savesrc || savebase diff --git a/internal/report/report.go b/internal/report/report.go index d6a8a295..8e663d1c 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -266,7 +266,7 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { // Remove tag marking samples from the base profiles, so it does not appear // as a nodelet in the graph view. - prof.SetTag("pprof::base", []string{}) + prof.RemoveLabel("pprof::base") formatTag := func(v int64, key string) string { return measurement.ScaledLabel(v, key, o.OutputUnit) @@ -1232,7 +1232,7 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) i } total += v div += d - if sample.HasTag("pprof::base", "true") { + if sample.HasLabel("pprof::base", "true") { diffTotal += v diffDiv += d } diff --git a/profile/profile.go b/profile/profile.go index 8b4d132d..18790e00 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -674,9 +674,9 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin return strings.Join(ls, " ") } -// SetTag sets the specified key to the specified value for all samples in the +// SetLabel sets the specified key to the specified value for all samples in the // profile. -func (p *Profile) SetTag(key string, value []string) { +func (p *Profile) SetLabel(key string, value []string) { for _, sample := range p.Sample { if sample.Label == nil { sample.Label = map[string][]string{} @@ -685,8 +685,18 @@ func (p *Profile) SetTag(key string, value []string) { } } -// HasTag returns true if a sample has a tag with indicated key and value. -func (s *Sample) HasTag(key, value string) bool { +// RemoveLabel removes all labels associated with the specified key for all +// samples in the profile. +func (p *Profile) RemoveLabel(key string) { + for _, sample := range p.Sample { + if _, ok := sample.Label[key]; ok { + delete(sample.Label, key) + } + } +} + +// HasLabel returns true if a sample has a label with indicated key and value. +func (s *Sample) HasLabel(key, value string) bool { if values, ok := s.Label[key]; ok { for _, v := range values { if v == value { diff --git a/profile/profile_test.go b/profile/profile_test.go index 21a162ab..08b8f2fb 100644 --- a/profile/profile_test.go +++ b/profile/profile_test.go @@ -741,7 +741,7 @@ func TestNumLabelMerge(t *testing.T) { wantNumUnits []map[string][]string }{ { - name: "different tag units not merged", + name: "different label units not merged", profs: []*Profile{testProfile4.Copy(), testProfile5.Copy()}, wantNumLabels: []map[string][]int64{ { @@ -912,56 +912,56 @@ func locationHash(s *Sample) string { return tb } -func TestHasTag(t *testing.T) { +func TestHasLabel(t *testing.T) { var testcases = []struct { - desc string - labels map[string][]string - key string - value string - wantHasTag bool + desc string + labels map[string][]string + key string + value string + wantHasLabel bool }{ { - desc: "empty label does not have tag", - labels: map[string][]string{}, - key: "key", - value: "value", - wantHasTag: false, + desc: "empty label does not have label", + labels: map[string][]string{}, + key: "key", + value: "value", + wantHasLabel: false, }, { - desc: "label with one key and value has tag", - labels: map[string][]string{"key": {"value"}}, - key: "key", - value: "value", - wantHasTag: true, + desc: "label with one key and value has label", + labels: map[string][]string{"key": {"value"}}, + key: "key", + value: "value", + wantHasLabel: true, }, { - desc: "label with one key and value does not have tag", - labels: map[string][]string{"key": {"value"}}, - key: "key1", - value: "value1", - wantHasTag: false, + desc: "label with one key and value does not have label", + labels: map[string][]string{"key": {"value"}}, + key: "key1", + value: "value1", + wantHasLabel: false, }, { - desc: "label with many keys and values has tag", + desc: "label with many keys and values has label", labels: map[string][]string{ "key1": {"value2", "value1"}, "key2": {"value1", "value2", "value2"}, "key3": {"value1", "value2", "value2"}, }, - key: "key1", - value: "value1", - wantHasTag: true, + key: "key1", + value: "value1", + wantHasLabel: true, }, { - desc: "label with many keys and values does not have tag", + desc: "label with many keys and values does not have label", labels: map[string][]string{ "key1": {"value2", "value1"}, "key2": {"value1", "value2", "value2"}, "key3": {"value1", "value2", "value2"}, }, - key: "key5", - value: "value5", - wantHasTag: false, + key: "key5", + value: "value5", + wantHasLabel: false, }, } @@ -970,23 +970,90 @@ func TestHasTag(t *testing.T) { sample := &Sample{ Label: tc.labels, } - if gotHasTag := sample.HasTag(tc.key, tc.value); gotHasTag != tc.wantHasTag { - t.Errorf("sample.HasTag(%q, %q) got %v, want %v", tc.key, tc.value, gotHasTag, tc.wantHasTag) + if gotHasLabel := sample.HasLabel(tc.key, tc.value); gotHasLabel != tc.wantHasLabel { + t.Errorf("sample.HasLabel(%q, %q) got %v, want %v", tc.key, tc.value, gotHasLabel, tc.wantHasLabel) + } + }) + } +} + +func TestRemove(t *testing.T) { + var testcases = []struct { + desc string + samples []*Sample + removeKey string + wantLabels []map[string][]string + }{ + { + desc: "some samples have label already", + samples: []*Sample{ + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value1", "value2", "value3"}, + "key2": {"value1"}, + }, + }, + { + Location: []*Location{cpuL[0]}, + Value: []int64{1000}, + Label: map[string][]string{ + "key1": {"value2"}, + }, + }, + }, + removeKey: "key1", + wantLabels: []map[string][]string{ + {}, + {"key2": {"value1"}}, + {}, + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + profile := testProfile1.Copy() + profile.Sample = tc.samples + profile.RemoveLabel(tc.removeKey) + if got, want := len(profile.Sample), len(tc.wantLabels); got != want { + t.Fatalf("got %v samples, want %v samples", got, want) + } + for i, sample := range profile.Sample { + wantLabels := tc.wantLabels[i] + if got, want := len(sample.Label), len(wantLabels); got != want { + t.Errorf("got %v label keys for sample %v, want %v", got, i, want) + continue + } + for wantKey, wantValues := range wantLabels { + if gotValues, ok := sample.Label[wantKey]; ok { + if !reflect.DeepEqual(gotValues, wantValues) { + t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) + } + } else { + t.Errorf("for key %s got no values, want %v", wantKey, wantValues) + } + } } }) } } -func TestSetTag(t *testing.T) { +func TestSetLabel(t *testing.T) { var testcases = []struct { - desc string - samples []*Sample - setKey string - setVal []string - wantTags []map[string][]string + desc string + samples []*Sample + setKey string + setVal []string + wantLabels []map[string][]string }{ { - desc: "some samples have tag already", + desc: "some samples have label already", samples: []*Sample{ { Location: []*Location{cpuL[0]}, @@ -1010,14 +1077,14 @@ func TestSetTag(t *testing.T) { }, setKey: "key1", setVal: []string{"value1"}, - wantTags: []map[string][]string{ + wantLabels: []map[string][]string{ {"key1": {"value1"}}, {"key1": {"value1"}, "key2": {"value1"}}, {"key1": {"value1"}}, }, }, { - desc: "no samples have tags", + desc: "no samples have labels", samples: []*Sample{ { Location: []*Location{cpuL[0]}, @@ -1026,12 +1093,12 @@ func TestSetTag(t *testing.T) { }, setKey: "key1", setVal: []string{"value1"}, - wantTags: []map[string][]string{ + wantLabels: []map[string][]string{ {"key1": {"value1"}}, }, }, { - desc: "all samples have some tags, but not key being added", + desc: "all samples have some labels, but not key being added", samples: []*Sample{ { Location: []*Location{cpuL[0]}, @@ -1050,7 +1117,7 @@ func TestSetTag(t *testing.T) { }, setKey: "key1", setVal: []string{"value1"}, - wantTags: []map[string][]string{ + wantLabels: []map[string][]string{ {"key1": {"value1"}, "key2": {"value2"}}, {"key1": {"value1"}, "key3": {"value3"}}, }, @@ -1075,7 +1142,7 @@ func TestSetTag(t *testing.T) { }, setKey: "key1", setVal: []string{"value1"}, - wantTags: []map[string][]string{ + wantLabels: []map[string][]string{ {"key1": {"value1"}}, {"key1": {"value1"}}, }, @@ -1086,17 +1153,17 @@ func TestSetTag(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { profile := testProfile1.Copy() profile.Sample = tc.samples - profile.SetTag(tc.setKey, tc.setVal) - if got, want := len(profile.Sample), len(tc.wantTags); got != want { + profile.SetLabel(tc.setKey, tc.setVal) + if got, want := len(profile.Sample), len(tc.wantLabels); got != want { t.Fatalf("got %v samples, want %v samples", got, want) } for i, sample := range profile.Sample { - wantTags := tc.wantTags[i] - if got, want := len(sample.Label), len(wantTags); got != want { + wantLabels := tc.wantLabels[i] + if got, want := len(sample.Label), len(wantLabels); got != want { t.Errorf("got %v label keys for sample %v, want %v", got, i, want) continue } - for wantKey, wantValues := range wantTags { + for wantKey, wantValues := range wantLabels { if gotValues, ok := sample.Label[wantKey]; ok { if !reflect.DeepEqual(gotValues, wantValues) { t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) From 79b3df8cd7708d379dcd7012f5ecd8383706be97 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Wed, 9 May 2018 09:15:14 -0700 Subject: [PATCH 11/15] address comments --- internal/driver/cli.go | 19 ++++++++-------- internal/driver/fetch_test.go | 40 ++++++++++++++++------------------ internal/report/report_test.go | 2 +- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/internal/driver/cli.go b/internal/driver/cli.go index 4292b64b..53231786 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -15,6 +15,7 @@ package driver import ( + "errors" "fmt" "os" "strings" @@ -87,7 +88,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { usageMsgVars) }) if len(args) == 0 { - return nil, nil, fmt.Errorf("no profile source specified") + return nil, nil, errors.New("no profile source specified") } var execName string @@ -114,7 +115,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { return nil, nil, err } if cmd != nil && *flagHTTP != "" { - return nil, nil, fmt.Errorf("-http is not compatible with an output format on the command line") + return nil, nil, errors.New("-http is not compatible with an output format on the command line") } si := pprofVariables["sample_index"].value @@ -148,7 +149,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { normalize := pprofVariables["normalize"].boolValue() if normalize && len(source.Base) == 0 { - return nil, nil, fmt.Errorf("Must have base profile to normalize by") + return nil, nil, errors.New("Must have base profile to normalize by") } source.Normalize = normalize @@ -162,17 +163,15 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { // the source. This function will return an error if both base and diff base // profiles are specified. func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { - base := dropEmpty(flagBase) - diffBase := dropEmpty(flagDiffBase) + base, diffBase := dropEmpty(flagBase), dropEmpty(flagDiffBase) if len(base) > 0 && len(diffBase) > 0 { - return fmt.Errorf("-base and -diff_base flags cannot both be specified") + return errors.New("-base and -diff_base flags cannot both be specified") } source.Base = base if len(diffBase) > 0 { - source.Base = diffBase - source.DiffBase = true + source.Base, source.DiffBase = diffBase, true } return nil } @@ -271,7 +270,7 @@ func outputFormat(bcmd map[string]*bool, acmd map[string]*string) (cmd []string, for n, b := range bcmd { if *b { if cmd != nil { - return nil, fmt.Errorf("must set at most one output format") + return nil, errors.New("must set at most one output format") } cmd = []string{n} } @@ -279,7 +278,7 @@ func outputFormat(bcmd map[string]*bool, acmd map[string]*string) (cmd []string, for n, s := range acmd { if *s != "" { if cmd != nil { - return nil, fmt.Errorf("must set at most one output format") + return nil, errors.New("must set at most one output format") } cmd = []string{n, *s} } diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index b0fc51b0..e9e68e2a 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -231,25 +231,25 @@ func TestFetchWithBase(t *testing.T) { "not normalized base is same as source", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, - []string{}, + nil, false, - []WantSample{}, + nil, "", }, { "not normalized base is same as source", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, - []string{}, + nil, false, - []WantSample{}, + nil, "", }, { "not normalized single source, multiple base (all profiles same)", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention", path + "cppbench.contention"}, - []string{}, + nil, false, []WantSample{ { @@ -283,7 +283,7 @@ func TestFetchWithBase(t *testing.T) { "not normalized, different base and source", []string{path + "cppbench.contention"}, []string{path + "cppbench.small.contention"}, - []string{}, + nil, false, []WantSample{ { @@ -317,7 +317,7 @@ func TestFetchWithBase(t *testing.T) { "normalized base is same as source", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, - []string{}, + nil, true, []WantSample{}, "", @@ -326,7 +326,7 @@ func TestFetchWithBase(t *testing.T) { "normalized single source, multiple base (all profiles same)", []string{path + "cppbench.contention"}, []string{path + "cppbench.contention", path + "cppbench.contention"}, - []string{}, + nil, true, []WantSample{}, "", @@ -335,7 +335,7 @@ func TestFetchWithBase(t *testing.T) { "normalized different base and source", []string{path + "cppbench.contention"}, []string{path + "cppbench.small.contention"}, - []string{}, + nil, true, []WantSample{ { @@ -368,7 +368,7 @@ func TestFetchWithBase(t *testing.T) { { "not normalized diff base is same as source", []string{path + "cppbench.contention"}, - []string{}, + nil, []string{path + "cppbench.contention"}, false, []WantSample{ @@ -466,12 +466,12 @@ func TestFetchWithBase(t *testing.T) { if tc.wantErrorMsg != "" { if err == nil { - t.Errorf("%s: want error %q, got nil", tc.desc, tc.wantErrorMsg) + t.Errorf("%s: got nil, want error %q", tc.desc, tc.wantErrorMsg) continue } gotErrMsg := err.Error() if gotErrMsg != tc.wantErrorMsg { - t.Errorf("%s: want error %q, got error %q", tc.desc, tc.wantErrorMsg, gotErrMsg) + t.Errorf("%s: got error %q, want error %q", tc.desc, gotErrMsg, tc.wantErrorMsg) } continue } @@ -489,18 +489,16 @@ func TestFetchWithBase(t *testing.T) { } if want, got := len(tc.wantSamples), len(p.Sample); want != got { - t.Errorf("%s: want %d samples got %d", tc.desc, want, got) + t.Errorf("%s: got %d samples want %d", tc.desc, got, want) continue } - if len(p.Sample) > 0 { - for i, sample := range p.Sample { - if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { - t.Errorf("%s: for sample %d want values %v, got %v", tc.desc, i, tc.wantSamples[i], sample.Value) - } - if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { - t.Errorf("%s: for sample %d want labels %v, got %v", tc.desc, i, tc.wantSamples[i].labels, sample.Label) - } + for i, sample := range p.Sample { + if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { + t.Errorf("%s: for sample %d got values %v, want %v", tc.desc, i, sample.Value, tc.wantSamples[i]) + } + if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { + t.Errorf("%s: for sample %d got labels %v, want %v", tc.desc, i, sample.Label, tc.wantSamples[i].labels) } } } diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 57e0bd26..9a641200 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -401,7 +401,7 @@ func TestComputeTotal(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv) if gotTotal != tc.wantTotal { - t.Errorf("want total %d, got %v", tc.wantTotal, gotTotal) + t.Errorf("got total %d, want %v", gotTotal, tc.wantTotal) } }) } From ee3be39034f27f9e6274ff2e2ecf27165524fa54 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Wed, 9 May 2018 09:37:26 -0700 Subject: [PATCH 12/15] use t.Run --- internal/driver/fetch_test.go | 109 +++++++++++++++++----------------- 1 file changed, 53 insertions(+), 56 deletions(-) diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index e9e68e2a..0de63114 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -435,72 +435,69 @@ func TestFetchWithBase(t *testing.T) { } for _, tc := range testcases { - pprofVariables = baseVars.makeCopy() - - base := make([]*string, len(tc.bases)) - for i, s := range tc.bases { - base[i] = &s - } - - diffBase := make([]*string, len(tc.diffBases)) - for i, s := range tc.diffBases { - diffBase[i] = &s - } - - f := testFlags{ - stringLists: map[string][]*string{ - "base": base, - "diff_base": diffBase, - }, - bools: map[string]bool{ - "normalize": tc.normalize, - }, - } - f.args = tc.sources + t.Run(tc.desc, func(t *testing.T) { + pprofVariables = baseVars.makeCopy() + base := make([]*string, len(tc.bases)) + for i, s := range tc.bases { + base[i] = &s + } - o := setDefaults(&plugin.Options{ - UI: &proftest.TestUI{T: t, AllowRx: "Local symbolization failed|Some binary filenames not available"}, - Flagset: f, - }) - src, _, err := parseFlags(o) + diffBase := make([]*string, len(tc.diffBases)) + for i, s := range tc.diffBases { + diffBase[i] = &s + } - if tc.wantErrorMsg != "" { - if err == nil { - t.Errorf("%s: got nil, want error %q", tc.desc, tc.wantErrorMsg) - continue + f := testFlags{ + stringLists: map[string][]*string{ + "base": base, + "diff_base": diffBase, + }, + bools: map[string]bool{ + "normalize": tc.normalize, + }, } - gotErrMsg := err.Error() - if gotErrMsg != tc.wantErrorMsg { - t.Errorf("%s: got error %q, want error %q", tc.desc, gotErrMsg, tc.wantErrorMsg) + f.args = tc.sources + + o := setDefaults(&plugin.Options{ + UI: &proftest.TestUI{T: t, AllowRx: "Local symbolization failed|Some binary filenames not available"}, + Flagset: f, + }) + src, _, err := parseFlags(o) + + if tc.wantErrorMsg != "" { + if err == nil { + t.Fatalf("got nil, want error %q", tc.wantErrorMsg) + } + gotErrMsg := err.Error() + if gotErrMsg != tc.wantErrorMsg { + t.Fatalf("got error %q, want error %q", gotErrMsg, tc.wantErrorMsg) + } + return } - continue - } - - if err != nil { - t.Errorf("%s: %v", tc.desc, err) - continue - } - p, err := fetchProfiles(src, o) + if err != nil { + t.Fatal(err) + } - if err != nil { - t.Errorf("%s: %v", tc.desc, err) - continue - } + p, err := fetchProfiles(src, o) - if want, got := len(tc.wantSamples), len(p.Sample); want != got { - t.Errorf("%s: got %d samples want %d", tc.desc, got, want) - continue - } + if err != nil { + t.Fatal(err) + } - for i, sample := range p.Sample { - if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { - t.Errorf("%s: for sample %d got values %v, want %v", tc.desc, i, sample.Value, tc.wantSamples[i]) + if want, got := len(tc.wantSamples), len(p.Sample); want != got { + t.Fatalf("got %d samples want %d", got, want) } - if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { - t.Errorf("%s: for sample %d got labels %v, want %v", tc.desc, i, sample.Label, tc.wantSamples[i].labels) + + for i, sample := range p.Sample { + if !reflect.DeepEqual(tc.wantSamples[i].values, sample.Value) { + t.Errorf("for sample %d got values %v, want %v", i, sample.Value, tc.wantSamples[i]) + } + if !reflect.DeepEqual(tc.wantSamples[i].labels, sample.Label) { + t.Errorf("for sample %d got labels %v, want %v", i, sample.Label, tc.wantSamples[i].labels) + } } - } + }) } } From 562c51d8d743bc31e642f3d864a460e4e38e48b5 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Fri, 11 May 2018 15:11:35 -0700 Subject: [PATCH 13/15] address comments --- internal/driver/driver_test.go | 8 ++++++-- internal/driver/fetch_test.go | 22 ++++++---------------- internal/report/report.go | 6 +++--- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go index 309e9950..631e7e07 100644 --- a/internal/driver/driver_test.go +++ b/internal/driver/driver_test.go @@ -288,7 +288,7 @@ type testFlags struct { floats map[string]float64 strings map[string]string args []string - stringLists map[string][]*string + stringLists map[string][]string } func (testFlags) ExtraUsage() string { return "" } @@ -355,7 +355,11 @@ func (f testFlags) StringVar(p *string, s, d, c string) { func (f testFlags) StringList(s, d, c string) *[]*string { if t, ok := f.stringLists[s]; ok { - return &t + tp := make([]*string, len(t)) + for i, v := range t { + tp[i] = &v + } + return &tp } return &[]*string{} } diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index 0de63114..853afba8 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -437,20 +437,10 @@ func TestFetchWithBase(t *testing.T) { for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { pprofVariables = baseVars.makeCopy() - base := make([]*string, len(tc.bases)) - for i, s := range tc.bases { - base[i] = &s - } - - diffBase := make([]*string, len(tc.diffBases)) - for i, s := range tc.diffBases { - diffBase[i] = &s - } - f := testFlags{ - stringLists: map[string][]*string{ - "base": base, - "diff_base": diffBase, + stringLists: map[string][]string{ + "base": tc.bases, + "diff_base": tc.diffBases, }, bools: map[string]bool{ "normalize": tc.normalize, @@ -468,8 +458,8 @@ func TestFetchWithBase(t *testing.T) { if err == nil { t.Fatalf("got nil, want error %q", tc.wantErrorMsg) } - gotErrMsg := err.Error() - if gotErrMsg != tc.wantErrorMsg { + + if gotErrMsg := err.Error(); gotErrMsg != tc.wantErrorMsg { t.Fatalf("got error %q, want error %q", gotErrMsg, tc.wantErrorMsg) } return @@ -485,7 +475,7 @@ func TestFetchWithBase(t *testing.T) { t.Fatal(err) } - if want, got := len(tc.wantSamples), len(p.Sample); want != got { + if got, want := len(p.Sample), len(tc.wantSamples); got != want { t.Fatalf("got %d samples want %d", got, want) } diff --git a/internal/report/report.go b/internal/report/report.go index 8e663d1c..76db9cbf 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -264,7 +264,7 @@ func (rpt *Report) newGraph(nodes graph.NodeSet) *graph.Graph { s.NumUnit = numUnits } - // Remove tag marking samples from the base profiles, so it does not appear + // Remove label marking samples from the base profiles, so it does not appear // as a nodelet in the graph view. prof.RemoveLabel("pprof::base") @@ -1217,8 +1217,8 @@ func NewDefault(prof *profile.Profile, options Options) *Report { } // computeTotal computes the sum of the absolute value of all sample values. -// If any samples have the tag "pprof::base" with value "true", then the total -// will only include samples with that tag. +// If any samples have the label "pprof::base" with value "true", then the total +// will only include samples with that label. func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) int64 { var div, total, diffDiv, diffTotal int64 for _, sample := range prof.Sample { From 4b9834bdce6cfca6eb03909f3d4c8e6bfb9afa49 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Thu, 24 May 2018 12:44:45 -0700 Subject: [PATCH 14/15] address comments --- internal/driver/cli.go | 7 +++---- internal/driver/driver_test.go | 1 + internal/report/report_test.go | 3 +-- profile/profile.go | 17 +++++++---------- profile/profile_test.go | 4 ++-- 5 files changed, 14 insertions(+), 18 deletions(-) diff --git a/internal/driver/cli.go b/internal/driver/cli.go index 53231786..a5153e15 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -149,7 +149,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { normalize := pprofVariables["normalize"].boolValue() if normalize && len(source.Base) == 0 { - return nil, nil, errors.New("Must have base profile to normalize by") + return nil, nil, errors.New("must have base profile to normalize by") } source.Normalize = normalize @@ -164,7 +164,6 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { // profiles are specified. func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { base, diffBase := dropEmpty(flagBase), dropEmpty(flagDiffBase) - if len(base) > 0 && len(diffBase) > 0 { return errors.New("-base and -diff_base flags cannot both be specified") } @@ -176,8 +175,8 @@ func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { return nil } -// dropEmpty list takes a StringList flag, and outputs an array of non-empty -// strings associated with the flag. +// dropEmpty list takes a slice of string pointers, and outputs a slice of +// non-empty strings associated with the flag. func dropEmpty(list []*string) []string { var l []string for _, s := range list { diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go index 631e7e07..ff6afe9c 100644 --- a/internal/driver/driver_test.go +++ b/internal/driver/driver_test.go @@ -355,6 +355,7 @@ func (f testFlags) StringVar(p *string, s, d, c string) { func (f testFlags) StringList(s, d, c string) *[]*string { if t, ok := f.stringLists[s]; ok { + // convert slice of strings to slice of string pointers before returning. tp := make([]*string, len(t)) for i, v := range t { tp[i] = &v diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 9a641200..9eb435bb 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -399,8 +399,7 @@ func TestComputeTotal(t *testing.T) { for _, tc := range testcases { t.Run(tc.desc, func(t *testing.T) { - gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv) - if gotTotal != tc.wantTotal { + if gotTotal := computeTotal(tc.prof, tc.value, tc.meanDiv); gotTotal != tc.wantTotal { t.Errorf("got total %d, want %v", gotTotal, tc.wantTotal) } }) diff --git a/profile/profile.go b/profile/profile.go index 18790e00..452194b1 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -679,9 +679,10 @@ func numLabelsToString(numLabels map[string][]int64, numUnits map[string][]strin func (p *Profile) SetLabel(key string, value []string) { for _, sample := range p.Sample { if sample.Label == nil { - sample.Label = map[string][]string{} + sample.Label = map[string][]string{key: value} + } else { + sample.Label[key] = value } - sample.Label[key] = value } } @@ -689,19 +690,15 @@ func (p *Profile) SetLabel(key string, value []string) { // samples in the profile. func (p *Profile) RemoveLabel(key string) { for _, sample := range p.Sample { - if _, ok := sample.Label[key]; ok { - delete(sample.Label, key) - } + delete(sample.Label, key) } } // HasLabel returns true if a sample has a label with indicated key and value. func (s *Sample) HasLabel(key, value string) bool { - if values, ok := s.Label[key]; ok { - for _, v := range values { - if v == value { - return true - } + for _, v := range s.Label[key] { + if v == value { + return true } } return false diff --git a/profile/profile_test.go b/profile/profile_test.go index 08b8f2fb..5b299b1d 100644 --- a/profile/profile_test.go +++ b/profile/profile_test.go @@ -1033,7 +1033,7 @@ func TestRemove(t *testing.T) { for wantKey, wantValues := range wantLabels { if gotValues, ok := sample.Label[wantKey]; ok { if !reflect.DeepEqual(gotValues, wantValues) { - t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) + t.Errorf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) } } else { t.Errorf("for key %s got no values, want %v", wantKey, wantValues) @@ -1166,7 +1166,7 @@ func TestSetLabel(t *testing.T) { for wantKey, wantValues := range wantLabels { if gotValues, ok := sample.Label[wantKey]; ok { if !reflect.DeepEqual(gotValues, wantValues) { - t.Fatalf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) + t.Errorf("for key %s, got values %v, want values %v", wantKey, gotValues, wantValues) } } else { t.Errorf("for key %s got no values, want %v", wantKey, wantValues) From 9558b3c0120fb2844da21de70a6b9746100af172 Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 29 May 2018 11:24:59 -0700 Subject: [PATCH 15/15] address comments --- internal/driver/fetch_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/driver/fetch_test.go b/internal/driver/fetch_test.go index 853afba8..e67b2e9f 100644 --- a/internal/driver/fetch_test.go +++ b/internal/driver/fetch_test.go @@ -319,7 +319,7 @@ func TestFetchWithBase(t *testing.T) { []string{path + "cppbench.contention"}, nil, true, - []WantSample{}, + nil, "", }, { @@ -328,7 +328,7 @@ func TestFetchWithBase(t *testing.T) { []string{path + "cppbench.contention", path + "cppbench.contention"}, nil, true, - []WantSample{}, + nil, "", }, { @@ -429,7 +429,7 @@ func TestFetchWithBase(t *testing.T) { []string{path + "cppbench.contention"}, []string{path + "cppbench.contention"}, false, - []WantSample{}, + nil, "-base and -diff_base flags cannot both be specified", }, } @@ -466,13 +466,13 @@ func TestFetchWithBase(t *testing.T) { } if err != nil { - t.Fatal(err) + t.Fatalf("got error %q, want no error", err) } p, err := fetchProfiles(src, o) if err != nil { - t.Fatal(err) + t.Fatalf("got error %q, want no error", err) } if got, want := len(p.Sample), len(tc.wantSamples); got != want {