Add a StringList type to be used for commandline flags. #583

nilayvaish · 2020-11-30T14:44:12Z

StringList flags would used for developing the feature: add tags as
pseudo locations.
#558

codecov-io · 2020-11-30T15:12:04Z

Codecov Report

Merging #583 (acc1602) into master (cbba55b) will decrease coverage by 0.02%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##           master     #583      +/-   ##
==========================================
- Coverage   66.94%   66.92%   -0.03%     
==========================================
  Files          78       78              
  Lines       14378    14398      +20     
==========================================
+ Hits         9626     9636      +10     
- Misses       3892     3902      +10     
  Partials      860      860

Impacted Files	Coverage Δ
internal/driver/flags.go	`30.43% <45.45%> (+15.05%)`	⬆️
...c/github.com/google/pprof/internal/driver/flags.go	`30.43% <0.00%> (+15.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbba55b...acc1602. Read the comment docs.

nolanmar511 · 2020-11-30T15:30:42Z

internal/stringlist/stringlist.go

+)
+
+// StringList is an alias for slice of strings.
+type StringList []string


Could []string be used directly?

I'm not immediately seeing the additional utility of having a wrapper class here, though could be missing something.

Modifying the FlagSet to point an internal class probably isn't ideal (depending on an internal class as part of a public interface could be a bit complicating).

The new type implements the flag.Value interface. I think we cannot retain the []string type and provide the functions required by the flag package. I am open to considering other ways to achieve this, but there does not seem to be any.

nolanmar511

Thanks for this change!

Left a few initial comments about the StringList type.

nolanmar511 · 2020-11-30T15:42:51Z

driver/driver.go

@@ -94,7 +95,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) *stringlist.StringList


This is part of pprof's public interface (one passes in plugin.Options to driver.PProf; we use this internally to, among other things, change the profile fetcher / symbolizer for internal pprof).

It would be nice if this interface did not have to change. Since we use this interface internally, it will definitely make this PR simpler to land if there aren't changes to pprof's public interface.

Also, this public interface shouldn't use an internal type (github.com/google/pprof/internal/stringlist) in its signature.

I think the interface needs to change. Currently there is no difference between a StringList flag and a String flag. A StringList flag can only hold a single string for now. To support a list of strings, we need to provide a type that adds strings to a list. The new type implements the flag.Value interface to achieve this.

Also, this public interface shouldn't use an internal type (github.com/google/pprof/internal/stringlist) in its signature.

I have moved the new type to a public directory.

I think I'd need to see a design document considering this more carefully proposed change before I'd be comfortable changing the public interface. I'm not currently persuaded that the interface change is the best or only way forward; I'd really need to be convinced of that before I'd feel comfortable with a change to the interface.

As additional context, changing the interface is a really big deal. First, we don't currently version pprof. Now that golang does support versioning a library, if we change the public interface, this becomes something we ought to look into. IMO, it'd be necessary to at least fully consider versioning pprof using go modules prior to making any breaking changes to the public interface. Next, changes to the public interface can make it really hard to sync our internal copy of pprof, since it becomes possible that code used for our internal version of pprof will break. If it is concluded that changes to the public interface are necessary, I would prefer to first see changes made to our internal copy of pprof to know that nothing internal will break (a PR would still need to be sent to change code, and any changes that need to be made to our dependencies on this library would need to be done when importing pprof from GitHub).

I have updated the PR with a different implementation (suggested by lmilewski) that does not change the public interface. Please take another look.

aalexand · 2021-01-06T19:48:32Z

flagset/flagset.go

@@ -0,0 +1,48 @@
+// Copyright 2021 Google Inc. All Rights Reserved.


I like pulling this interface out to avoid duplication between driver/driver.go and internal/plugin/plugin.go. Can we make this a separate PR? I think this can go in regardless of other discussions. One thing I'm not sure about is whether it's worth a separate top-level directory - seems too shallow for that. Maybe have it as driver/flagset.go? (and "package driver").

Can we make this a separate PR?

Yes. We just need to decide on the package for flagset.go.

Maybe have it as driver/flagset.go?

I had tried adding to driver package before, but ran into an import cycle.

package github.com/google/pprof imports github.com/google/pprof/driver imports github.com/google/pprof/internal/driver imports github.com/google/pprof/internal/binutils imports github.com/google/pprof/internal/plugin imports github.com/google/pprof/driver: import cycle not allowed package github.com/google/pprof imports github.com/google/pprof/driver imports github.com/google/pprof/internal/driver imports github.com/google/pprof/internal/binutils imports github.com/google/pprof/internal/plugin imports github.com/google/pprof/driver: import cycle not allowed

Are you OK going with a new package? I could not think of any other reasonable place for this.

I'd prefer not to have a new top-level package. Actually, reading go/go-style/decisions#interfaces I wonder if we should leave things as is with two copies of the interface for now. It's duplication, but it's rather unrelated issue to this PR, so I'd tackle it separately if we ever need to.

internal/driver/flags.go

nolanmar511 · 2021-02-05T00:47:07Z

internal/driver/flags_test.go

+	"testing"
+)
+
+func TestString(t *testing.T) {


It seems a bit different (at least to the best of my knowledge and in the context of golang) to test functions on a private type in golang. Testing the public interface would probably be preferable.

But, I know that there are no tests for flag.go right now; will leave it up to @aalexand to decide what sort of testing is sufficient.

Ping to @nilayvaish and @aalexand.

I'm not sure about this testing. It may be better than not having tests, but I'm a bit skeptical about testing only a private type, rather than the publicly available functions.

AFAICT, it is not possible to test the public interface without creating a separate process. Would you be OK with the _test.go file creating a new process with flags? Otherwise, I think we need to drop the tests all together.

The two copies the FlagSet interface are identical and are required to be so. It seems better to have them in place. We put this in a public directory as the interface defined in driver/driver.go is already public.

The commit adds a new type flagset.StringList that holds a list of strings provided via an input flag. The new type implements that flag.Value interface as required by the flag package. flagset.StringList flags would used for the following feature: add tags as pseudo locations.

We introduce a new stringList type that implements the flag.Var interface. flags' internal data structures would now hold a var of this type instead of a var of type flag.String. Multiple invocations of a StringList flag would therefore be able to add more values to this flag.

aalexand · 2021-03-16T03:22:40Z

@nilayvaish @nolanmar511 Looking at this again, I'm now not sure why flag changes are needed to support -tagroot / -tagleaf. I believe those shouldn't be flags, I think they should be vars in internal/driver/config.go like -tagfocus / -tagignore and friends.

Also, one thing to watch for with list-like flags is append vs. reset semantics. E.g. in the pprof interactive shell we need a way to specify the full set of values for these flags, without ad-infinitum append behavior.

nilayvaish · 2021-03-16T19:29:08Z

@nilayvaish @nolanmar511 Looking at this again, I'm now not sure why flag changes are needed to support -tagroot / -tagleaf. I believe those shouldn't be flags, I think they should be vars in internal/driver/config.go like -tagfocus / -tagignore and friends.

We are treating tagroot and tagleaf like tagfocus and tagignore`, only the types are different. I have added another commit to show this.

Also, one thing to watch for with list-like flags is append vs. reset semantics. E.g. in the pprof interactive shell we need a way to specify the full set of values for these flags, without ad-infinitum append behavior.

This is an interesting point I had not considered before. I checked internal/drive/interactive.go and it seems like the command line is parsed again. TagIgnoreand TagFocus have been treated specially in the parse routing: parseCommandLine. We could do something similar for TagRoot and TagLeaf. Let me know what you think.

aalexand · 2021-03-17T04:50:21Z

internal/driver/commands.go

+	"tagleaf": helpText(
+		"Adds a pseudo leaf node for samples with tags matched by regexp",
+		"Use name=value1,value2 syntax to specify the tag and the values",
+		"for which nodes would be added.  Example: latency=1s,2s,3s."),
+	"tagroot": helpText(
+		"Adds a pseudo root node for samples with tags matched by regexp",
+		"Use name=value1,value2 syntax to specify the tag and the values",
+		"for which nodes would be added.  Example: latency=1s,2s,3s."),


Couple of questions:

The option / flag is of type []string / multi-value. The examples are of the form "foo=val1,val2" What exactly the multi-value here? Is it the comma-separated multi-values as in example, i.e. "foo=val1" and "val2" as two values which is weird? Or is that there is ability to have multiple occurrences of each of these like "-flag=foo=val1,val2 -flag=bar=val3,val4" and the documentation needs to explain this?

How are numeric tags handled, including with units like memory? Can we reuse the same value format as in tagfocus / taghide etc.? I feel that we are introducing a new way to specify a set of values and I don't see why different tag* flags can't unify on a single way to do that.

google-cla bot added the cla: yes label Nov 30, 2020

nilayvaish marked this pull request as ready for review November 30, 2020 14:55

nolanmar511 reviewed Nov 30, 2020

View reviewed changes

aalexand reviewed Jan 6, 2021

View reviewed changes

nolanmar511 reviewed Feb 5, 2021

View reviewed changes

internal/driver/flags.go Show resolved Hide resolved

nolanmar511 reviewed Feb 5, 2021

View reviewed changes

nilayvaish added 3 commits March 9, 2021 12:23

Move FlagSet interface to a common place.

4fd09e7

The two copies the FlagSet interface are identical and are required to be so. It seems better to have them in place. We put this in a public directory as the interface defined in driver/driver.go is already public.

Add pseudo tags.

b911008

aalexand reviewed Mar 17, 2021

View reviewed changes

nilayvaish closed this Apr 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a StringList type to be used for commandline flags. #583

Add a StringList type to be used for commandline flags. #583

nilayvaish commented Nov 30, 2020

codecov-io commented Nov 30, 2020 •

edited

Loading

nolanmar511 Nov 30, 2020 •

edited

Loading

nilayvaish Jan 3, 2021

nolanmar511 left a comment

nolanmar511 Nov 30, 2020

aalexand Nov 30, 2020

nilayvaish Jan 3, 2021

nolanmar511 Jan 5, 2021

nilayvaish Feb 4, 2021

aalexand Jan 6, 2021

nilayvaish Jan 7, 2021 •

edited

Loading

aalexand Jan 7, 2021

nolanmar511 Feb 5, 2021

nolanmar511 Feb 16, 2021

nilayvaish Feb 16, 2021

aalexand commented Mar 16, 2021 •

edited

Loading

nilayvaish commented Mar 16, 2021

aalexand Mar 17, 2021

		@@ -0,0 +1,48 @@
		// Copyright 2021 Google Inc. All Rights Reserved.

Add a StringList type to be used for commandline flags. #583

Add a StringList type to be used for commandline flags. #583

Conversation

nilayvaish commented Nov 30, 2020

codecov-io commented Nov 30, 2020 • edited Loading

Codecov Report

nolanmar511 Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanmar511 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilayvaish Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexand commented Mar 16, 2021 • edited Loading

nilayvaish commented Mar 16, 2021

Choose a reason for hiding this comment

codecov-io commented Nov 30, 2020 •

edited

Loading

nolanmar511 Nov 30, 2020 •

edited

Loading

nilayvaish Jan 7, 2021 •

edited

Loading

aalexand commented Mar 16, 2021 •

edited

Loading