diff --git a/driver/driver.go b/driver/driver.go index 5a8222f70..abc725b2b 100644 --- a/driver/driver.go +++ b/driver/driver.go @@ -94,7 +94,7 @@ type FlagSet interface { // StringList is similar to String but allows multiple values for a // single flag - StringList(name string, def string, usage string) *[]*string + StringList(name string, def []string, usage string) *[]string // ExtraUsage returns any additional text that should be printed after the // standard usage message. The extra usage message returned includes all text diff --git a/internal/driver/cli.go b/internal/driver/cli.go index d13d4d3b8..8839154a3 100644 --- a/internal/driver/cli.go +++ b/internal/driver/cli.go @@ -37,7 +37,8 @@ type source struct { Symbolize string HTTPHostport string HTTPDisableBrowser bool - Comment string + Title string + Comments []string } // parseFlags parses the command lines through the specified flags package @@ -46,13 +47,14 @@ type source struct { func parseFlags(o *plugin.Options) (*source, []string, error) { flag := o.Flagset // Comparisons. - flagDiffBase := flag.StringList("diff_base", "", "Source of base profile for comparison") - flagBase := flag.StringList("base", "", "Source of base profile for profile subtraction") + flagDiffBase := flag.StringList("diff_base", nil, "Source of base profile for comparison") + flagBase := flag.StringList("base", nil, "Source of base profile for profile subtraction") // Source options. flagSymbolize := flag.String("symbolize", "", "Options for profile symbolization") flagBuildID := flag.String("buildid", "", "Override build id for first mapping") flagTimeout := flag.Int("timeout", -1, "Timeout in seconds for fetching a profile") - flagAddComment := flag.String("add_comment", "", "Annotation string to record in the profile") + flagAddComment := flag.StringList("add_comment", nil, "Annotation string to record in the profile") + flagTitle := flag.String("title", "", "Title string to record in the profile") // CPU profile options flagSeconds := flag.Int("seconds", -1, "Length of time for dynamic profiles") // Heap profile options @@ -67,7 +69,7 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { flagTools := flag.String("tools", os.Getenv("PPROF_TOOLS"), "Path for object tool pathnames") flagHTTP := flag.String("http", "", "Present interactive web UI at the specified http host:port") - flagNoBrowser := flag.Bool("no_browser", false, "Skip opening a browswer for the interactive web UI") + flagNoBrowser := flag.Bool("no_browser", false, "Skip opening a browser for the interactive web UI") // Flags that set configuration properties. cfg := currentConfig() @@ -148,7 +150,8 @@ func parseFlags(o *plugin.Options) (*source, []string, error) { Symbolize: *flagSymbolize, HTTPHostport: *flagHTTP, HTTPDisableBrowser: *flagNoBrowser, - Comment: *flagAddComment, + Title: *flagTitle, + Comments: *flagAddComment, } if err := source.addBaseProfiles(*flagBase, *flagDiffBase); err != nil { @@ -172,7 +175,7 @@ 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 { +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") @@ -187,11 +190,11 @@ func (source *source) addBaseProfiles(flagBase, flagDiffBase []*string) error { // 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 { +func dropEmpty(list []string) []string { var l []string for _, s := range list { - if *s != "" { - l = append(l, *s) + if s != "" { + l = append(l, s) } } return l diff --git a/internal/driver/driver_test.go b/internal/driver/driver_test.go index abe3597eb..0627763fc 100644 --- a/internal/driver/driver_test.go +++ b/internal/driver/driver_test.go @@ -78,7 +78,8 @@ func TestParse(t *testing.T) { {"dot,unit=minimum", "heap_sizetags"}, {"dot,addresses,flat,ignore=[X3]002,focus=[X1]000", "contention"}, {"dot,files,cum", "contention"}, - {"comments,add_comment=some-comment", "cpu"}, + {"comments,add_comment=some-comment=", "cpu"}, + {"comments,add_comment=comment1=,add_comment=comment2=", "cpu"}, {"comments", "heap"}, {"tags", "cpu"}, {"tags,tagignore=tag[13],tagfocus=key[12]", "cpu"}, @@ -156,15 +157,17 @@ func TestParse(t *testing.T) { f.args = []string{protoTempFile.Name()} delete(f.bools, "proto") - addFlags(&f, flags) - solution := solutionFilename(tc.source, &f) + solution := "" // Apply the flags for the second pprof run, and identify name of // the file containing expected results if flags[0] == "topproto" { - addFlags(&f, flags) + addFlags(t, &f, flags) solution = solutionFilename(tc.source, &f) delete(f.bools, "topproto") f.bools["text"] = true + } else { + addFlags(t, &f, flags) + solution = solutionFilename(tc.source, &f) } // Second pprof invocation to read the profile from profile.proto @@ -238,18 +241,33 @@ func removeScripts(in []byte) []byte { } // addFlags parses flag descriptions and adds them to the testFlags -func addFlags(f *testFlags, flags []string) { +func addFlags(t *testing.T, f *testFlags, flags []string) { for _, flag := range flags { - fields := strings.SplitN(flag, "=", 2) + fields := strings.SplitN(flag, "=", 3) switch len(fields) { case 1: f.bools[fields[0]] = true case 2: + fallthrough + case 3: + flagName := fields[0] if i, err := strconv.Atoi(fields[1]); err == nil { - f.ints[fields[0]] = i + f.ints[flagName] = i } else { - f.strings[fields[0]] = fields[1] + // String flag + if len(fields) == 3 { + // Assume stringLists + if strings, found := f.stringLists[flagName]; found { + f.stringLists[flagName] = append(strings, fields[1]) + } else { + f.stringLists[flagName] = []string{fields[1]} + } + } else { + f.strings[flagName] = fields[1] + } } + default: + t.Errorf("Invalid flag: %s", flag) } } } @@ -263,6 +281,7 @@ func solutionFilename(source string, f *testFlags) string { name := []string{"pprof", strings.TrimPrefix(source, testSourceURL(8000))} name = addString(name, f, []string{"flat", "cum"}) name = addString(name, f, []string{"functions", "filefunctions", "files", "lines", "addresses"}) + name = addString(name, f, []string{"add_comment"}) name = addString(name, f, []string{"noinlines"}) name = addString(name, f, []string{"inuse_space", "inuse_objects", "alloc_space", "alloc_objects"}) name = addString(name, f, []string{"relative_percentages"}) @@ -290,6 +309,9 @@ func addString(name []string, f *testFlags, components []string) []string { if f.bools[c] || f.strings[c] != "" || f.ints[c] != 0 { return append(name, c) } + if values, found := f.stringLists[c]; found { + return append(name, c, fmt.Sprint(len(values))) + } } return name } @@ -336,16 +358,16 @@ func (f testFlags) String(s, d, c string) *string { return &d } -func (f testFlags) StringList(s, d, c string) *[]*string { - if t, ok := f.stringLists[s]; ok { +func (f testFlags) StringList(name string, def []string, usage string) *[]string { + if t, ok := f.stringLists[name]; 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 + tp := make([]string, len(t)) + for i, _ := range t { + tp[i] = t[i] } return &tp } - return &[]*string{} + return &[]string{} } func (f testFlags) Parse(func()) []string { @@ -370,6 +392,7 @@ func baseFlags() testFlags { strings: map[string]string{ "unit": "minimum", }, + stringLists: map[string][]string{}, } } diff --git a/internal/driver/fetch.go b/internal/driver/fetch.go index 3a4c061e1..dc76011f1 100644 --- a/internal/driver/fetch.go +++ b/internal/driver/fetch.go @@ -84,8 +84,14 @@ func fetchProfiles(s *source, o *plugin.Options) (*profile.Profile, error) { p.RemoveUninteresting() unsourceMappings(p) - if s.Comment != "" { - p.Comments = append(p.Comments, s.Comment) + if s.Title != "" { + p.Title = s.Title + } + + if len(s.Comments) > 0 { + for _, comment := range s.Comments { + p.Comments = append(p.Comments, comment) + } } // Save a copy of the merged profile if there is at least one remote source. diff --git a/internal/driver/flags.go b/internal/driver/flags.go index 539031916..e01547313 100644 --- a/internal/driver/flags.go +++ b/internal/driver/flags.go @@ -44,9 +44,27 @@ func (*GoFlags) String(o, d, c string) *string { return flag.String(o, d, c) } +type StringList []string + +func newStringList(val []string, p *[]string) *StringList { + *p = val + return (*StringList)(p) +} + +func (s *StringList) Set(value string) error { + *s = append(*s, value) + return nil +} + +func (s *StringList) String() string { + return "unused" +} + // StringList implements the plugin.FlagSet interface. -func (*GoFlags) StringList(o, d, c string) *[]*string { - return &[]*string{flag.String(o, d, c)} +func (*GoFlags) StringList(name string, def []string, usage string) *[]string { + s := new([]string) + flag.Var(newStringList(def, s), name, usage) + return s } // ExtraUsage implements the plugin.FlagSet interface. diff --git a/internal/driver/testdata/pprof.cpu.comments b/internal/driver/testdata/pprof.cpu.add_comment.1.comments similarity index 100% rename from internal/driver/testdata/pprof.cpu.comments rename to internal/driver/testdata/pprof.cpu.add_comment.1.comments diff --git a/internal/driver/testdata/pprof.cpu.add_comment.2.comments b/internal/driver/testdata/pprof.cpu.add_comment.2.comments new file mode 100644 index 000000000..639c501ef --- /dev/null +++ b/internal/driver/testdata/pprof.cpu.add_comment.2.comments @@ -0,0 +1,2 @@ +comment1 +comment2 diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go index 98eb1dd81..6056748f7 100644 --- a/internal/plugin/plugin.go +++ b/internal/plugin/plugin.go @@ -63,7 +63,7 @@ type FlagSet interface { // StringList is similar to String but allows multiple values for a // single flag - StringList(name string, def string, usage string) *[]*string + StringList(name string, def []string, usage string) *[]string // ExtraUsage returns any additional text that should be printed after the // standard usage message. The extra usage message returned includes all text diff --git a/profile/profile.go b/profile/profile.go index 60ef7e926..2eefd3d7e 100644 --- a/profile/profile.go +++ b/profile/profile.go @@ -38,6 +38,7 @@ type Profile struct { Mapping []*Mapping Location []*Location Function []*Function + Title string Comments []string DropFrames string @@ -545,6 +546,9 @@ func (p *Profile) NumLabelUnits() (map[string]string, map[string][]string) { // for debugging purposes. func (p *Profile) String() string { ss := make([]string, 0, len(p.Comments)+len(p.Sample)+len(p.Mapping)+len(p.Location)) + if pt := p.Title; pt != "" { + ss = append(ss, "Title: "+pt) + } for _, c := range p.Comments { ss = append(ss, "Comment: "+c) }