-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Tablet throttler: multi-metric support #15988
Changes from 172 commits
b40945f
f100aea
3fa8b23
187963c
70726f8
5f7b978
106efab
bc3ae15
94d0f93
eb84e80
2f61a42
2aa3647
98df99b
0d49400
f749a39
e8b9df8
848fcf7
fb27a3a
c6b164c
aca248c
afe407a
e284d2d
d783bca
ac1982e
294b39f
073e6f4
a7e0e68
4946477
0c34e16
dec1881
044b818
a7901e3
b6c5367
becd655
bff2ed8
f9f5f8e
8548c88
81ddd5c
84bdc68
bb9713c
1d96ae1
747d151
65f5529
1d37b05
9e2ae21
e31047d
4763bec
7e514b7
4e1012a
9f237f6
0b0811f
6827f4a
803c73a
e370ca5
869943a
4185c53
c299b42
9835f9c
fa28d9c
7a6fe75
7de790f
ef48a2d
97e07ea
26b8825
347dd56
d1921e4
880c14a
30612fe
d3142ad
c802f7b
28e1120
b41efa2
b793ecc
b01d081
a506ffa
0ffd206
159b573
17fa059
cb16a8b
93825b0
ebaf173
a3dc70b
d037924
d95ec66
f718dd8
1224a92
64ef430
5003b0c
b3a8e2f
88f7431
0e02a22
24239f2
2746661
0509de4
cbbdfd4
c47bc5f
c9f0a24
ef0650c
6b7accc
dbf3b93
cc5c31b
1e0d70b
e45ac0c
7d12a94
c3b90e8
fd6ef65
48b08fa
4e58ada
17b1a2e
da6ecf5
841d2a6
bae3b90
39171b4
cc8806f
663dcd3
b0f08de
a46e6ce
d08a04c
b5eca7c
07611ab
1cee554
f799e8c
1ce84f5
af7d347
e922d3c
d825c1f
a63cdef
ed8a74b
d94de0f
80f0f37
d75c404
269c69d
9ba160d
8bae917
616154e
c940e0c
4de5550
4e15dd3
27514e1
9ae7caa
1a09d25
14af07d
d72b1e3
76d3f38
a499d7a
5b1561d
14e5ee8
e4aa405
0633dd8
de5443b
c6ce707
9b40469
978354d
8f96886
318da0c
9a5cfec
da56591
bb60e89
03238e0
2d981cb
aeb5b65
f98fb83
8fea8bf
b01fb81
3c838a0
1c76866
8e9d0aa
4c00b91
231efcc
e1364b0
28dd1d2
997b7d6
71e8a59
ac556d8
36b1f53
ca44839
6853576
c617c75
3cbfb9e
c8bf456
3057dc4
b4d9f6c
615781a
3e71054
9f0e712
199ca16
b845f2c
2ab1924
39abd35
ee4f831
36bba11
1d6f0bc
029c443
d0972d3
2880182
9ac893e
2499169
2f587ce
886a6d0
56bae93
ca3dc8e
9624a29
d487993
8e798aa
ff365e9
2ba62c8
8e8271c
63dd98e
77aa2d9
7025dbf
18141ac
6d8091e
c9bde7d
cd04223
2b8f780
ef5ea1f
f9fc8df
74b7fdc
f901615
8d05c97
8d69d4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 ( | ||||||
|
@@ -39,13 +43,32 @@ var ( | |||||
Args: cobra.ExactArgs(1), | ||||||
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 | ||||||
) | ||||||
|
||||||
func commandUpdateThrottlerConfig(cmd *cobra.Command, args []string) error { | ||||||
|
@@ -56,14 +79,24 @@ func commandUpdateThrottlerConfig(cmd *cobra.Command, args []string) error { | |||||
return fmt.Errorf("throttle-app and unthrottle-app are mutually exclusive") | ||||||
} | ||||||
|
||||||
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") != cmd.Flags().Changed("app-metrics") { | ||||||
return fmt.Errorf("--app-name and --app-metrics must be set together") | ||||||
} | ||||||
if cmd.Flags().Changed("app-name") && updateThrottlerConfigOptions.AppName == "" { | ||||||
return fmt.Errorf("--app-name must not be empty") | ||||||
} | ||||||
|
||||||
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 = protoutil.TimeToProto(time.Now()) | ||||||
unthrottledAppRule.ExpiresAt = &vttime.Time{} // zero | ||||||
updateThrottlerConfigOptions.ThrottledApp = &unthrottledAppRule | ||||||
} | ||||||
|
||||||
|
@@ -74,9 +107,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The flags won't be assigned before |
||||||
resp, err := client.CheckThrottler(commandCtx, &vtctldatapb.CheckThrottlerRequest{ | ||||||
TabletAlias: alias, | ||||||
AppName: checkThrottlerOptions.AppName, | ||||||
Scope: checkThrottlerOptions.Scope, | ||||||
SkipRequestHeartbeats: checkThrottlerOptions.SkipRequestHeartbeats, | ||||||
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). Leave empty for v19 default metric/threshold") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's true. I'll edit all occurrences. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meanwhile clarified comment and fixed typo. |
||||||
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") | ||||||
|
@@ -88,5 +179,17 @@ func init() { | |||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, but noting the inconsistent use of sentence capitalization here and for metric-name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the inconsistency? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think it's the |
||||||
|
||||||
Root.AddCommand(UpdateThrottlerConfig) | ||||||
// Check Throttler | ||||||
CheckThrottler.Flags().StringVar(&checkThrottlerOptions.AppName, "app-name", throttlerapp.VitessName.String(), "app to identify as") | ||||||
shlomi-noach marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
CheckThrottler.Flags().StringVar(&checkThrottlerOptions.Scope, "scope", base.UndefinedScope.String(), "check scope ('shard', 'self' or leave empty for per-metric defaults)") | ||||||
CheckThrottler.Flags().BoolVar(&checkThrottlerOptions.SkipRequestHeartbeats, "skip-heartbeats", false, "skip renewing heartbeat lease") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this should default to false and only be there to explicitly/intentionally generate on-demand heartbeats for testing, debugging, etc. as otherwise the user may be unintentionally and unknowingly affecting the related behaviors in Vitess via a status check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I've changed |
||||||
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) | ||||||
} |
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
PreRunE
hook 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!