Allow multiple comments with add_comment

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

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

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{},
 	}
 }
 

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.

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.

internal/driver/testdata/pprof.cpu.add_comment.2.comments

@@ -0,0 +1,2 @@
+comment1
+comment2

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

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)
 	}