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)