Reorganize serve flags into flagsets #2023

tstirrat15 · 2024-08-15T00:04:54Z

Description

The flags on spicedb serve --help are numerous. It makes parsing through them to figure out which ones are relevant and which ones you need pretty difficult. This is the first step towards addressing that - we organize the flags into flagsets which provides a bit of logical and visual grouping.

Changes

Gonna annotate.

Testing

Review. ./spicedb serve --help and see that all of the expected flags are present and that they are organized more nicely.

tstirrat15

See comments

tstirrat15 · 2024-08-15T00:05:16Z

pkg/cmd/datastore/datastore.go

-func RegisterDatastoreFlags(cmd *cobra.Command, opts *Config) error {
-	return RegisterDatastoreFlagsWithPrefix(cmd.Flags(), "", opts)
+func RegisterDatastoreFlags(cmd *cobra.Command, flagset *pflag.FlagSet, opts *Config) error {
+	return RegisterDatastoreFlagsWithPrefix(flagset, "", opts)


Passing in the flagset so we can get them properly grouped.

tstirrat15 · 2024-08-15T00:06:02Z

pkg/cmd/root.go

-	ot := cobraotel.New(cmd.Use)
-	ot.RegisterFlags(cmd.PersistentFlags())
-	if err := ot.RegisterFlagCompletion(cmd); err != nil {
-		return fmt.Errorf("failed to register otel flag completion: %w", err)
-	}
-
 	releases.RegisterFlags(cmd.PersistentFlags())
-	termination.RegisterFlags(cmd.PersistentFlags())
-	runtime.RegisterFlags(cmd.PersistentFlags())


This is cribbing from #2022 - these flags are really more serve-specific than root flags, so we move them up into serve so that we can group them accordingly.

I think we still need these at least for migrate commands - maybe serve-testing too?

Yeah... in #2022 they're moved into a function that registers them and then that function is reused. I can mimic that.

I went ahead and added them to migrate but I'm gonna push back on serve-testing for the time being.

tstirrat15 · 2024-08-15T00:07:13Z

pkg/cmd/serve.go

+	grpcFlagSet := nfs.FlagSet("gRPC")
 	// Flags for logging
-	cmd.Flags().BoolVar(&config.EnableRequestLogs, "grpc-log-requests-enabled", false, "logs API request payloads")
-	cmd.Flags().BoolVar(&config.EnableResponseLogs, "grpc-log-responses-enabled", false, "logs API response payloads")
+	grpcFlagSet.BoolVar(&config.EnableRequestLogs, "grpc-log-requests-enabled", false, "logs API request payloads")
+	grpcFlagSet.BoolVar(&config.EnableResponseLogs, "grpc-log-responses-enabled", false, "logs API response payloads")


This is basically the pattern you'll see - create a flagset on the flagsets with a name (and they're declared in the order they're displayed) and then attach a bunch of flags to the named flagset rather than the command's flagset.

tstirrat15 · 2024-08-15T00:07:33Z

pkg/cmd/serve.go

+	// TODO: this one should maybe not live in API?
+	apiFlags.Uint64Var(&config.MaxDatastoreReadPageSize, "max-datastore-read-page-size", 1_000, "limit on the maximum page size that we will load into memory from the datastore at one time")


Wasn't sure whether this should be api or datastore

I would call this datastore since it affects how data is loaded from the backend

tstirrat15 · 2024-08-15T00:07:40Z

pkg/cmd/serve.go

+	// TODO: should this be under datastore or api?
+	datastoreFlags.DurationVar(&config.SchemaWatchHeartbeat, "datastore-schema-watch-heartbeat", 1*time.Second, "heartbeat time on the schema watch in the datastore (if supported). 0 means to default to the datastore's minimum.")


This is datastore - this controls how spicedb pro-actively fills its cache with the schema from the backend

Actually this might fit better in the Namespace cache section, since it controls the automatic filling of that cache from Watch. But also that feature is still experimental 😓

tstirrat15 · 2024-08-15T00:08:06Z

pkg/cmd/serve.go

+	// TODO: these two could reasonably be put in either the Dispatch group or the Experimental group. Is there a preference?
+	experimentalFlags.StringToStringVar(&config.DispatchSecondaryUpstreamAddrs, "experimental-dispatch-secondary-upstream-addrs", nil, "secondary upstream addresses for dispatches, each with a name")
+	experimentalFlags.StringToStringVar(&config.DispatchSecondaryUpstreamExprs, "experimental-dispatch-secondary-upstream-exprs", nil, "map from request type (currently supported: `check`) to its associated CEL expression, which returns the secondary upstream(s) to be used for the request")


Good question - I don't have a strong preference since once these are no longer experiments we'll put them in the right place anyway. I like what you've done here in grouping the experiments together.

@josephschorr or @jzelinskie sometimes have opinions about this kind of thing though.

tstirrat15 · 2024-08-15T00:08:24Z

pkg/cmd/serve.go

+	observabilityFlags := nfs.FlagSet("Observability")
+	// Flags for observability and profiling
+	otel := cobraotel.New(cmd.Use)
+	otel.RegisterFlags(observabilityFlags)
+	runtime.RegisterFlags(observabilityFlags)


This is where the flags that used to live in the root command landed

see my other comment - I think we at least need these on migrate, and maybe others as well

ecordell

Looking good! Much easier to scan flags now.

I noticed that our examples use colors (when supported) - I don't know how hard this would be, but it might be nice to add some color to the flagset headers so that gRPC Flags and HTTP Flags etc are different colors. If there's a simple way to do that I think it would help delineate the blocks even more than now (if it's a whole big project do make that happen don't worry about it).

ecordell · 2024-08-15T03:22:49Z

pkg/cmd/datastore/datastore.go

@@ -159,8 +159,8 @@ type Config struct {
 }

 // RegisterDatastoreFlags adds datastore flags to a cobra command.
-func RegisterDatastoreFlags(cmd *cobra.Command, opts *Config) error {
-	return RegisterDatastoreFlagsWithPrefix(cmd.Flags(), "", opts)
+func RegisterDatastoreFlags(cmd *cobra.Command, flagset *pflag.FlagSet, opts *Config) error {


Suggested change

func RegisterDatastoreFlags(cmd *cobra.Command, flagset *pflag.FlagSet, opts *Config) error {

func RegisterDatastoreFlags(flagset *pflag.FlagSet, opts *Config) error {

cmd is unused now, right?

Is it possible to nest flagsets? The datastore set is pretty big and I see some natural points we could break it up; i.e. connection pooling, bootstrapping, etc

I don't think so - the hierarchy seems to be cobra.Command -> nfs.NamedFlagSets -> nfs.FlagSet, and there isn't a recursive definition in there. I could add additional categories under datastore if you think that would help, though.

ecordell · 2024-08-15T03:23:27Z

pkg/cmd/root.go

-	ot := cobraotel.New(cmd.Use)
-	ot.RegisterFlags(cmd.PersistentFlags())
-	if err := ot.RegisterFlagCompletion(cmd); err != nil {
-		return fmt.Errorf("failed to register otel flag completion: %w", err)
-	}
-
 	releases.RegisterFlags(cmd.PersistentFlags())
-	termination.RegisterFlags(cmd.PersistentFlags())
-	runtime.RegisterFlags(cmd.PersistentFlags())


I think we still need these at least for migrate commands - maybe serve-testing too?

ecordell · 2024-08-15T13:15:07Z

pkg/cmd/serve.go

+	// TODO: this one should maybe not live in API?
+	apiFlags.Uint64Var(&config.MaxDatastoreReadPageSize, "max-datastore-read-page-size", 1_000, "limit on the maximum page size that we will load into memory from the datastore at one time")


I would call this datastore since it affects how data is loaded from the backend

ecordell · 2024-08-15T13:15:43Z

pkg/cmd/serve.go

+	// TODO: should this be under datastore or api?
+	datastoreFlags.DurationVar(&config.SchemaWatchHeartbeat, "datastore-schema-watch-heartbeat", 1*time.Second, "heartbeat time on the schema watch in the datastore (if supported). 0 means to default to the datastore's minimum.")


This is datastore - this controls how spicedb pro-actively fills its cache with the schema from the backend

ecordell · 2024-08-15T13:17:32Z

pkg/cmd/serve.go

+	// TODO: these two could reasonably be put in either the Dispatch group or the Experimental group. Is there a preference?
+	experimentalFlags.StringToStringVar(&config.DispatchSecondaryUpstreamAddrs, "experimental-dispatch-secondary-upstream-addrs", nil, "secondary upstream addresses for dispatches, each with a name")
+	experimentalFlags.StringToStringVar(&config.DispatchSecondaryUpstreamExprs, "experimental-dispatch-secondary-upstream-exprs", nil, "map from request type (currently supported: `check`) to its associated CEL expression, which returns the secondary upstream(s) to be used for the request")


Good question - I don't have a strong preference since once these are no longer experiments we'll put them in the right place anyway. I like what you've done here in grouping the experiments together.

@josephschorr or @jzelinskie sometimes have opinions about this kind of thing though.

ecordell · 2024-08-15T13:22:01Z

pkg/cmd/serve.go

+	apiFlags.Uint16Var(&config.MaximumPreconditionCount, "update-relationships-max-preconditions-per-call", 1000, "maximum number of preconditions allowed for WriteRelationships and DeleteRelationships calls")
+	// TODO: this one should maybe not live in API?
+	apiFlags.Uint64Var(&config.MaxDatastoreReadPageSize, "max-datastore-read-page-size", 1_000, "limit on the maximum page size that we will load into memory from the datastore at one time")
+	apiFlags.BoolVar(&config.DisableV1SchemaAPI, "disable-v1-schema-api", false, "disables the V1 schema API")


I think we can hide this flag, I don't think there's much utility for most users.

ecordell · 2024-08-15T13:31:01Z

pkg/cmd/serve.go

+	// TODO: should this be under datastore or api?
+	datastoreFlags.DurationVar(&config.SchemaWatchHeartbeat, "datastore-schema-watch-heartbeat", 1*time.Second, "heartbeat time on the schema watch in the datastore (if supported). 0 means to default to the datastore's minimum.")


Actually this might fit better in the Namespace cache section, since it controls the automatic filling of that cache from Watch. But also that feature is still experimental 😓

pkg/cmd/serve.go

ecordell · 2024-08-15T13:36:40Z

pkg/cmd/serve.go

-	util.RegisterGRPCServerFlags(cmd.Flags(), &config.DispatchServer, "dispatch-cluster", "dispatch", ":50053", false)
-	server.MustRegisterCacheFlags(cmd.Flags(), "dispatch-cache", &config.DispatchCacheConfig, dispatchCacheDefaults)
-	server.MustRegisterCacheFlags(cmd.Flags(), "dispatch-cluster-cache", &config.ClusterDispatchCacheConfig, dispatchClusterCacheDefaults)
+	util.RegisterGRPCServerFlags(dispatchFlags, &config.DispatchServer, "dispatch-cluster", "dispatch", ":50053", false)


this is good but I think we could further break down the dispatch flags if we wanted to: dispatch server, dispatch (client) cache, dispatch (server) cache, hashring config, etc.

tstirrat15 · 2024-08-15T16:02:22Z

@ecordell re: color: I think it's just on the far side of easy. I tried https://github.com/ivanpirog/coloredcobra and it isn't aware of flagsets, and I tried fatih/color on the flagset names but that only changes the label, not the entire string output:

gRPC Flags:
^ this is colored
      ^ this is not

I think it'd be possible to do it, but it'd require more manual intervention, so I'm gonna leave it for now and work on addressing your other comments.

alecmerdler · 2024-08-15T20:08:29Z

pkg/cmd/serve.go

-	cmd.Flags().StringVar(&config.TelemetryEndpoint, "telemetry-endpoint", telemetry.DefaultEndpoint, "endpoint to which telemetry is reported, empty string to disable")
-	cmd.Flags().StringVar(&config.TelemetryCAOverridePath, "telemetry-ca-override-path", "", "TODO")
-	cmd.Flags().DurationVar(&config.TelemetryInterval, "telemetry-interval", telemetry.DefaultInterval, "approximate period between telemetry reports, minimum 1 minute")
-


You need to add this to the end in order for the flags to actually be registered with the command. This seems like an unintuitive DX for cobrautil:

nfs.AddFlagSets(cmd)

Co-authored-by: Evan Cordell <[email protected]>

ecordell

I think the changes to the otel flags are doing weird things:

On serve:

      --otel-service-name string       service name for trace data (default "serve")

Vs the previous:

      --otel-service-name string       service name for trace data (default "spicedb")

Also look at migrate:

      --otel-service-name string                     service name for trace data (default "migrate [revision]")

I also don't see --schema-prefixes-required in the new flagsets:

$ go run ./cmd/spicedb/... serve -h | grep prefixes

It was kind of tricky to review that the flags hadn't changed - I wonder if we should have a test that with a list of flags to confirm are in the output?

tstirrat15 requested a review from a team as a code owner August 15, 2024 00:04

github-actions bot added the area/CLI Affects the command line label Aug 15, 2024

tstirrat15 commented Aug 15, 2024

View reviewed changes

ecordell reviewed Aug 15, 2024

View reviewed changes

alecmerdler reviewed Aug 15, 2024

View reviewed changes

tstirrat15 and others added 10 commits August 21, 2024 10:59

Move serve-related flags up into serve

f05242e

Pass flagset into helper

df8263a

Reorganize flags into flagsets

44a2cd8

Linting

4ccbdec

Remove command passthrough

ebe3184

Add bold and blue to headers

924908b

Fix flag registration

753a680

Add pprof and termination flags for migrate

031dec2

Reorganize some flags and hide some things

9befab7

Update pkg/cmd/serve.go

0e4105b

Co-authored-by: Evan Cordell <[email protected]>

tstirrat15 force-pushed the 2022-reorganize-flags-into-flagsets branch from 2880c7f to 0e4105b Compare August 21, 2024 16:59

tstirrat15 added 2 commits August 21, 2024 14:03

Add otel flags back to migrate command

c529652

Add flag registration to the testing command

31301ee

ecordell requested changes Aug 22, 2024

View reviewed changes

tstirrat15 added 2 commits August 26, 2024 12:28

Fix up flags

fa962ad

Lint fix

06f52bf

ecordell approved these changes Aug 27, 2024

View reviewed changes

tstirrat15 added this pull request to the merge queue Aug 27, 2024

Merged via the queue into main with commit fd2978a Aug 27, 2024
22 checks passed

tstirrat15 deleted the 2022-reorganize-flags-into-flagsets branch August 27, 2024 21:20

github-actions bot locked and limited conversation to collaborators Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize serve flags into flagsets #2023

Reorganize serve flags into flagsets #2023

tstirrat15 commented Aug 15, 2024

tstirrat15 left a comment

tstirrat15 Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 Aug 15, 2024

tstirrat15 Aug 15, 2024

tstirrat15 Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

ecordell left a comment

ecordell Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

ecordell Aug 15, 2024

tstirrat15 commented Aug 15, 2024

alecmerdler Aug 15, 2024

ecordell left a comment

		// TODO: this one should maybe not live in API?
		apiFlags.Uint64Var(&config.MaxDatastoreReadPageSize, "max-datastore-read-page-size", 1_000, "limit on the maximum page size that we will load into memory from the datastore at one time")

		// TODO: should this be under datastore or api?
		datastoreFlags.DurationVar(&config.SchemaWatchHeartbeat, "datastore-schema-watch-heartbeat", 1*time.Second, "heartbeat time on the schema watch in the datastore (if supported). 0 means to default to the datastore's minimum.")

	func RegisterDatastoreFlags(cmd cobra.Command, flagset pflag.FlagSet, opts *Config) error {
	func RegisterDatastoreFlags(flagset pflag.FlagSet, opts Config) error {

Reorganize serve flags into flagsets #2023

Reorganize serve flags into flagsets #2023

Conversation

tstirrat15 commented Aug 15, 2024

Description

Changes

Testing

tstirrat15 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell 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

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

tstirrat15 commented Aug 15, 2024

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment