From bd80566b11c3b41f6532f22b2ed26e0ea884e7ab Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Tue, 8 May 2018 14:12:06 -0700 Subject: [PATCH] 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)