-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dispatch tests and metrics flag #561
Conversation
d1c9359
to
30273cf
Compare
internal/datastore/mysql/options.go
Outdated
connMaxIdleTime: defaultConnMaxIdleTime, | ||
connMaxLifetime: defaultConnMaxLifetime, | ||
splitAtUsersetCount: defaultUsersetBatchSize, | ||
enablePrometheusStats: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we want to pull a constant just like every other option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
// Prometheus metrics are enabled by default. | ||
func EnablePrometheusStats() Option { | ||
// Prometheus metrics are disabled by default. | ||
func WithEnablePrometheusStats(enablePrometheusStats bool) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nit: perhaps WithPrometheusStats
would be easier to reason about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vacillated back and forth on that, but I decided to be explicit
@@ -104,6 +104,7 @@ func RegisterDatastoreFlags(cmd *cobra.Command, opts *Config) { | |||
cmd.Flags().DurationVar(&opts.RequestHedgingInitialSlowValue, "datastore-request-hedging-initial-slow-value", 10*time.Millisecond, "initial value to use for slow datastore requests, before statistics have been collected") | |||
cmd.Flags().Uint64Var(&opts.RequestHedgingMaxRequests, "datastore-request-hedging-max-requests", 1_000_000, "maximum number of historical requests to consider") | |||
cmd.Flags().Float64Var(&opts.RequestHedgingQuantile, "datastore-request-hedging-quantile", 0.95, "quantile of historical datastore request time over which a request will be considered slow") | |||
cmd.Flags().BoolVar(&opts.EnableDatastoreMetrics, "datastore-prometheus-metrics", true, "set to false to disabled prometheus metrics from the datastore") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not seem to align with the default in the individual stores? e.g. https://github.com/authzed/spicedb/pull/561/files#diff-1f9d83984c96749a94659b71f4b5b8366b0b98e11e71923100983d110cd7b3a3R50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It should be enabled by default from the CLI, but disabled by default if you just instantiate the datastore directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, it's intentional 👍🏻
3321b45
to
c379d14
Compare
c379d14
to
69c1d6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
69c1d6f
to
684f4a1
Compare
|
684f4a1
to
662b246
Compare
662b246
to
12cb0d1
Compare
No description provided.