Tablet throttler: multi-metric support #15988
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,7 +27,11 @@ import ( | |||||
|
|
||||||
| topodatapb "vitess.io/vitess/go/vt/proto/topodata" | ||||||
| vtctldatapb "vitess.io/vitess/go/vt/proto/vtctldata" | ||||||
| "vitess.io/vitess/go/vt/proto/vttime" | ||||||
| "vitess.io/vitess/go/vt/topo/topoproto" | ||||||
| "vitess.io/vitess/go/vt/vttablet/tabletserver/throttle" | ||||||
| "vitess.io/vitess/go/vt/vttablet/tabletserver/throttle/base" | ||||||
| "vitess.io/vitess/go/vt/vttablet/tabletserver/throttle/throttlerapp" | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
|
|
@@ -37,33 +41,61 @@ var ( | |||||
| Short: "Update the tablet throttler configuration for all tablets in the given keyspace (across all cells)", | ||||||
| DisableFlagsInUseLine: true, | ||||||
| Args: cobra.ExactArgs(1), | ||||||
| PreRunE: validateUpdateThrottlerConfig, | ||||||
| RunE: commandUpdateThrottlerConfig, | ||||||
| } | ||||||
| CheckThrottler = &cobra.Command{ | ||||||
| Use: "CheckThrottler [--app-name <name>] <tablet alias>", | ||||||
| Short: "Issue a throttler check on the given tablet.", | ||||||
| Example: "CheckThrottler --app-name online-ddl zone1-0000000101", | ||||||
| DisableFlagsInUseLine: true, | ||||||
| Args: cobra.ExactArgs(1), | ||||||
| RunE: commandCheckThrottler, | ||||||
| } | ||||||
|
|
||||||
| GetThrottlerStatus = &cobra.Command{ | ||||||
| Use: "GetThrottlerStatus <tablet alias>", | ||||||
| Short: "Get the throttler status for the given tablet.", | ||||||
| Example: "GetThrottlerStatus zone1-0000000101", | ||||||
| DisableFlagsInUseLine: true, | ||||||
| Args: cobra.ExactArgs(1), | ||||||
| RunE: commandGetThrottlerStatus, | ||||||
| } | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
| updateThrottlerConfigOptions vtctldatapb.UpdateThrottlerConfigRequest | ||||||
| throttledAppRule topodatapb.ThrottledAppRule | ||||||
| unthrottledAppRule topodatapb.ThrottledAppRule | ||||||
| throttledAppDuration time.Duration | ||||||
|
|
||||||
| checkThrottlerOptions vtctldatapb.CheckThrottlerRequest | ||||||
| requestHeartbeats bool | ||||||
| ) | ||||||
|
|
||||||
| func validateUpdateThrottlerConfig(cmd *cobra.Command, args []string) error { | ||||||
| if updateThrottlerConfigOptions.MetricName != "" && !cmd.Flags().Changed("threshold") { | ||||||
| return fmt.Errorf("--metric-name flag requires --threshold flag. Set threshold to 0 to disable the metric threshold configuration") | ||||||
| } | ||||||
| if cmd.Flags().Changed("app-name") && updateThrottlerConfigOptions.AppName == "" { | ||||||
| return fmt.Errorf("--app-name must not be empty") | ||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func commandUpdateThrottlerConfig(cmd *cobra.Command, args []string) error { | ||||||
| keyspace := cmd.Flags().Arg(0) | ||||||
| cli.FinishedParsing(cmd) | ||||||
|
|
||||||
| updateThrottlerConfigOptions.CustomQuerySet = cmd.Flags().Changed("custom-query") | ||||||
| updateThrottlerConfigOptions.Keyspace = keyspace | ||||||
|
|
||||||
| if throttledAppRule.Name != "" { | ||||||
| throttledAppRule.ExpiresAt = protoutil.TimeToProto(time.Now().Add(throttledAppDuration)) | ||||||
| updateThrottlerConfigOptions.ThrottledApp = &throttledAppRule | ||||||
| } else if unthrottledAppRule.Name != "" { | ||||||
| unthrottledAppRule.ExpiresAt = &vttime.Time{} // zero | ||||||
| updateThrottlerConfigOptions.ThrottledApp = &unthrottledAppRule | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -74,9 +106,67 @@ func commandUpdateThrottlerConfig(cmd *cobra.Command, args []string) error { | |||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func commandCheckThrottler(cmd *cobra.Command, args []string) error { | ||||||
| alias, err := topoproto.ParseTabletAlias(cmd.Flags().Arg(0)) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| cli.FinishedParsing(cmd) | ||||||
| if _, err := base.ScopeFromString(checkThrottlerOptions.Scope); err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
Comment on lines
+116
to
+118
There was a problem hiding this comment. Minor, but this feels like something we would do before we mark parsing as finished as we're ensuring the passed value is valid right? There was a problem hiding this comment. The flags won't be assigned before |
||||||
| resp, err := client.CheckThrottler(commandCtx, &vtctldatapb.CheckThrottlerRequest{ | ||||||
| TabletAlias: alias, | ||||||
| AppName: checkThrottlerOptions.AppName, | ||||||
| Scope: checkThrottlerOptions.Scope, | ||||||
| SkipRequestHeartbeats: !requestHeartbeats, | ||||||
| OkIfNotExists: checkThrottlerOptions.OkIfNotExists, | ||||||
| }) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| data, err := cli.MarshalJSON(resp) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| fmt.Printf("%s\n", data) | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func commandGetThrottlerStatus(cmd *cobra.Command, args []string) error { | ||||||
| alias, err := topoproto.ParseTabletAlias(cmd.Flags().Arg(0)) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| cli.FinishedParsing(cmd) | ||||||
|
|
||||||
| resp, err := client.GetThrottlerStatus(commandCtx, &vtctldatapb.GetThrottlerStatusRequest{ | ||||||
| TabletAlias: alias, | ||||||
| }) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| data, err := cli.MarshalJSON(resp) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
|
|
||||||
| fmt.Printf("%s\n", data) | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func init() { | ||||||
| // UpdateThrottlerConfig | ||||||
| UpdateThrottlerConfig.Flags().BoolVar(&updateThrottlerConfigOptions.Enable, "enable", false, "Enable the throttler") | ||||||
| UpdateThrottlerConfig.Flags().BoolVar(&updateThrottlerConfigOptions.Disable, "disable", false, "Disable the throttler") | ||||||
| UpdateThrottlerConfig.Flags().StringVar(&updateThrottlerConfigOptions.MetricName, "metric-name", "", "name of the metric for which we apply a new threshold (requires --threshold). If empty, the default (either 'lag' or 'custom') metric is used.") | ||||||
| UpdateThrottlerConfig.Flags().Float64Var(&updateThrottlerConfigOptions.Threshold, "threshold", 0, "threshold for the either default check (replication lag seconds) or custom check") | ||||||
| UpdateThrottlerConfig.Flags().StringVar(&updateThrottlerConfigOptions.CustomQuery, "custom-query", "", "custom throttler check query") | ||||||
| UpdateThrottlerConfig.Flags().BoolVar(&updateThrottlerConfigOptions.CheckAsCheckSelf, "check-as-check-self", false, "/throttler/check requests behave as is /throttler/check-self was called") | ||||||
|
|
@@ -87,6 +177,19 @@ func init() { | |||||
| UpdateThrottlerConfig.Flags().Float64Var(&throttledAppRule.Ratio, "throttle-app-ratio", throttle.DefaultThrottleRatio, "ratio to throttle app (app specififed in --throttled-app)") | ||||||
| UpdateThrottlerConfig.Flags().DurationVar(&throttledAppDuration, "throttle-app-duration", throttle.DefaultAppThrottleDuration, "duration after which throttled app rule expires (app specififed in --throttled-app)") | ||||||
| UpdateThrottlerConfig.Flags().BoolVar(&throttledAppRule.Exempt, "throttle-app-exempt", throttledAppRule.Exempt, "exempt this app from being at all throttled. WARNING: use with extreme care, as this is likely to push metrics beyond the throttler's threshold, and starve other apps") | ||||||
| UpdateThrottlerConfig.Flags().StringVar(&updateThrottlerConfigOptions.AppName, "app-name", "", "app name for which to assign metrics (requires --app-metrics)") | ||||||
|
There was a problem hiding this comment. Just FYI, you can create flag groups so that cobra enforces mutual inclusion/exclusion etc. https://github.com/spf13/cobra/blob/main/site/content/user_guide.md#flag-groups |
||||||
| UpdateThrottlerConfig.Flags().StringSliceVar(&updateThrottlerConfigOptions.AppCheckedMetrics, "app-metrics", nil, "metrics to be used when checking the throttler for the app (requires --app-name). Empty to restore to default metrics") | ||||||
|
There was a problem hiding this comment. Nit, but noting the inconsistent use of sentence capitalization here and for metric-name. There was a problem hiding this comment. I don't see the inconsistency? There was a problem hiding this comment.
Suggested change
I think it's the |
||||||
| UpdateThrottlerConfig.MarkFlagsMutuallyExclusive("unthrottle-app", "throttle-app") | ||||||
| UpdateThrottlerConfig.MarkFlagsRequiredTogether("app-name", "app-metrics") | ||||||
|
|
||||||
| Root.AddCommand(UpdateThrottlerConfig) | ||||||
| // Check Throttler | ||||||
| CheckThrottler.Flags().StringVar(&checkThrottlerOptions.AppName, "app-name", throttlerapp.VitessName.String(), "app name to check") | ||||||
| CheckThrottler.Flags().StringVar(&checkThrottlerOptions.Scope, "scope", base.UndefinedScope.String(), "check scope ('shard', 'self' or leave empty for per-metric defaults)") | ||||||
| CheckThrottler.Flags().BoolVar(&requestHeartbeats, "request-heartbeats", false, "request heartbeat lease") | ||||||
| CheckThrottler.Flags().BoolVar(&checkThrottlerOptions.OkIfNotExists, "ok-if-not-exists", false, "return OK even if metric does not exist") | ||||||
| Root.AddCommand(CheckThrottler) | ||||||
|
|
||||||
| // GetThrottlerStatus | ||||||
| Root.AddCommand(GetThrottlerStatus) | ||||||
| } | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 and somewhat a personal preference, I think, but IMO it's worth moving all of these checks to the
PreRunEhook for the command. I think that makes it clearer that this is part of the pre-check validations done for the command before we actually execute it.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.
Ah, I wasn't aware of
PreRunE. Looking into.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.
Done!