Throttler: return app name in check result, synthesize "why throttled" explanation from result #16416

shlomi-noach · 2024-07-17T09:10:55Z

Description

Followup to #15988, this PR further enhances CheckThrottlerResponse by adding the name of throttled/granted app.

Why is this needed? Isn't the app name the same as the app in CheckThrottlerRequest?

Not necessarily!

The requesting app might be vcopier:d666bbfc_169e_11ef_b0b3_0a43f95f28a3:vreplication:online-ddl. Suppose the request is throttled. Why is it being throttled? There could be many reasons and based on either of the specific identifiers. For example, someone might have issued a alter vitess_migration throttle all, in which case the throttled app will be online-ddl. So this is what we indicate back to the user.
There may not be any specific rule for the given app, but it's possible that the all app is being throttled. Either because someone actually invoked UpdateThrottlerConfig --throttle-app=all ..., or just because all is assigned specific metrics, one of which crossed its threshold. This way or another, we want to tell the user the "all" app is at "fault" here.

Check result summary

Based on that, we provide a concise summary, something like app 'online-ddl' is throttled because 'lag' exceeded its threshold of 5``, or vreplication is denied access until ....

Example actual summaries:

online-ddl is granted access
online-ddl is explicitly denied access
online-ddl is denied access due to threads_running metric value 4 exceeding threshold 2
online-ddl is denied access due to lag metric value 94.821447 exceeding threshold 5
online-ddl is denied access due to lag metric value 22.951372 exceeding threshold 5
all is explicitly denied access

This is in turn injected in _vt.vreplication and then in _vt.schema_migrations as a human readable assist for analyzing production situations.

`reason_throttled` in `_vt.vreplication`

@rohit-nayak-ps you will like this.

Looks like this:

                   id: 3
             workflow: d741953d_4503_11ef_8761_0a43f95f28a3
               source: keyspace:"commerce" shard:"0" filter:{rules:{match:"_vt_vrp_d741953d450311ef87610a43f95f28a3_20240718124708_" filter:"select `customer_id`
 as `customer_id`, `email` as `email` from `customer`" source_unique_key_columns:"customer_id" target_unique_key_columns:"customer_id" source_unique_key_target_c
olumns:"customer_id" force_unique_key:"PRIMARY"}}
                  pos: MySQL56/414befe7-4358-11ef-bc56-0a43f95f28a3:1-650
             stop_pos: NULL
              max_tps: 9223372036854775807
  max_replication_lag: 9223372036854775807
                 cell:
         tablet_types: in_order:REPLICA,PRIMARY
         time_updated: 1721306835
transaction_timestamp: 1721306834
                state: Stopped
              message: stopped for online DDL cutover
              db_name: vt_commerce
          rows_copied: 0
                 tags:
       time_heartbeat: 1721306835
        workflow_type: 5
       time_throttled: 1721306829
  component_throttled: vcopier
     reason_throttled: online-ddl is denied access due to shard/lag metric value 8.725835 exceeding threshold 5
    workflow_sub_type: 0
 defer_secondary_keys: 0
              options: {}

The same is copied to _vt.schema_migrations.

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…shot Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…etricChan -> throttleMetricChan Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…package Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…enable Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-07-17T09:10:58Z

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

Apply the release notes (needs details) label if users need to know about this change.
New features should be documented.
There should be some code comments as to why things are implemented the way they are.
There should be a comment at the top of each new or modified test to explain what the test does.

New flags

Is this flag really necessary?
Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

Protobuf changes should be wire-compatible.
Changes to _vt tables and RPCs need to be backward compatible.
RPC changes should be compatible with vitess-operator
If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
vtctl command output order should be stable and awk-able.

codecov · 2024-07-17T09:30:14Z

Codecov Report

Attention: Patch coverage is 63.96396% with 40 lines in your changes missing coverage. Please review.

Project coverage is 68.63%. Comparing base (be8f9f4) to head (0eef755).
Report is 8 commits behind head on main.

Files	Patch %	Lines
go/vt/vttablet/tabletserver/throttle/client.go	55.55%	8 Missing ⚠️
go/vt/vttablet/tabletserver/throttle/throttler.go	75.00%	8 Missing ⚠️
.../vt/vttablet/tabletserver/throttle/check_result.go	68.42%	6 Missing ⚠️
go/vt/vttablet/onlineddl/executor.go	0.00%	4 Missing ⚠️
go/vt/binlog/binlogplayer/binlog_player.go	0.00%	2 Missing ⚠️
go/vt/vttablet/tabletmanager/rpc_throttler.go	0.00%	2 Missing ⚠️
.../vt/vttablet/tabletmanager/vreplication/vcopier.go	33.33%	2 Missing ⚠️
.../vt/vttablet/tabletmanager/vreplication/vplayer.go	33.33%	2 Missing ⚠️
...vttablet/tabletmanager/vreplication/vreplicator.go	0.00%	2 Missing ⚠️
go/vt/vttablet/tabletserver/gc/tablegc.go	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16416      +/-   ##
==========================================
- Coverage   68.65%   68.63%   -0.02%     
==========================================
  Files        1550     1551       +1     
  Lines      199412   199515     +103     
==========================================
+ Hits       136900   136931      +31     
- Misses      62512    62584      +72

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…the Summary(), populate ThrottledReason added in proto, then in turn populate new _vt.vreplication.reason_throttled Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…n_throttled Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rohit-nayak-ps

This is brilliant!

I will enhance the throttler reporting added to vtadmin in #16308 to display the reason once this is merged.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

mattlord

Thank you! This is a nice enhancement. ❤️

mattlord · 2024-07-25T17:48:32Z

go/vt/vttablet/tabletserver/throttle/check_result.go

@@ -85,14 +91,32 @@ func (c *CheckResult) IsOK() bool {
 return c.StatusCode == http.StatusOK
 }

+// Summary returns a human-readable summary of the check result
+func (c *CheckResult) Summary() string {


Any reason not to use String() so that CheckResult implements the Stringer interface?

I'm ambivalent. There can only be one String() function, and I'm not sure what the expected implementation should be. Summary() does not capture all of the CheckResult's fields. Should String() capture all fields? Should String() be human readable or machine readable?

Gonna keep this with Summary(). We can always easily change it to String() later.

mattlord · 2024-07-25T17:50:40Z

go/vt/vttablet/tabletserver/throttle/check_result.go

+func (c *CheckResult) Summary() string {
+ switch c.StatusCode {
+ case http.StatusOK:
+ return fmt.Sprintf("%v is granted access", c.AppName)


Super nitty, but IMO better to use %s with strings like AppName.

mattlord · 2024-07-25T17:52:41Z

go/vt/vttablet/tabletserver/throttle/client.go

+var (
+ throttleTicks int64
+ throttleInit sync.Once
+ emptyCheckResult = &CheckResult{}


Any reason for this not to be a const?

golang does nto permit this as a const

mattlord · 2024-07-25T17:57:27Z

go/vt/vttablet/tabletserver/throttle/throttler_test.go

@@ -418,6 +418,7 @@ func TestApplyThrottlerConfigMetricThresholds(t *testing.T) {
 assert.EqualValues(t, 0.3, checkResult.Value) // self lag value
 assert.EqualValues(t, http.StatusOK, checkResult.StatusCode)
 assert.Len(t, checkResult.Metrics, 1)
+ assert.Contains(t, checkResult.Summary(), "test is granted access")


IIRC, the "test" value in all of these asserts is from the const test app name. Nitty, but better to use the const everywhere it applies. Similar for the other app names which are based on existing consts/vars.

mattlord · 2024-07-25T18:00:37Z

go/vt/vttablet/tabletserver/throttle/throttler_test.go


 // continuing previous test, we had 3 throttled apps. "all" is a new app being throttled.
 assert.Equal(t, 4, throttler.throttledApps.ItemCount())
 })
- //
+ // //


Looks like we added an extraneous comment indicator.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…" explanation from result (vitessio#16416) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added 27 commits July 11, 2024 09:46

rename variable mysqlMetricThresholds to metricThresholds

5fb9a6e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename function mysqlMetricThresholdsSnapshot to metricThresholdsSnap…

c6d7047

…shot Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename *Interval variables away from 'mysql'

9eb59b3

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename *Interval variables away from 'mysql'

9f2716a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename aggregateMySQLProbes -> aggregateMetricResults. Cleanup tests

685441e

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename MySQLThrottleMetrics -> ThrottleMetrics. Rename mysqlThrottleM…

dd11de1

…etricChan -> throttleMetricChan Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

renamed files

c9ac150

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename file

dd3868f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

eliminatye throttle/mysql package; move everything under throttle/base

2886bf3

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Remove unused type

f6e8a55

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove ctx argument

1ea1b51

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

rename aggregateMetricResults -> aggregateTabletMetricResults

1bddcc0

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shuffle code around, moving aggregateTabletMetricResults into 'base' …

ec62bbe

…package Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove ctx where unused

c82ea9a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

terminology: remove 'MySQL' where irrelevant. Add where relevant

3423780

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

terminology: remove 'MySQL' where irrelevant

1f584a1

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

renamed getMySQLStoreMetric -> getScopedMetric

85994ed

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

comment wording

2492d6d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

tests: explicitly disabling throttler so as to ensure it does not re-…

d7c63c8

…enable Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

CheckThrottlerResponse: include the matched app name

114405f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

CheckThrottler: return matched app name

692f7d5

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adding missing files

090f96d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adding missing files

8c20483

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into throttler-cleanup-types-and-names

d913132

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

update copyright year

a8eeea7

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

resolved conflict

f6fda40

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into throttler-response-app

ca9b878

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work labels Jul 17, 2024

shlomi-noach added 10 commits July 18, 2024 09:21

Merge branch 'main' into throttler-response-app

ff04023

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove unused httpClient

eb0635d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

adding Summary in CheckThrottlerResponse

032c87a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

evaluate and populate metric result Summary

3527745

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

apply matchedApp on indirect throttling. Add testing

32d1f3c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

proto: add ThrottledReason in VStreamRowsResponse and in VEvent

0f48902

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ThrottlerClient returns CheckResult. Used in vreplication to extract …

af99924

…the Summary(), populate ThrottledReason added in proto, then in turn populate new _vt.vreplication.reason_throttled Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

copy _vt.vreplication.reason_throttled to _vt.schema_migrations.reaso…

b31a552

…n_throttled Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

test 'reason_throttled' in endtoend

313675c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

fix expected message

5a3aea7

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review July 18, 2024 13:38

shlomi-noach requested review from deepthi, harshit-gangal, mattlord, systay, rohit-nayak-ps and timvaillancourt as code owners July 18, 2024 13:38

shlomi-noach requested a review from a team July 18, 2024 13:40

shlomi-noach mentioned this pull request Jul 18, 2024

RFC: tablet throttler multi-metrics #15624

Open

14 tasks

rohit-nayak-ps approved these changes Jul 22, 2024

View reviewed changes

Merge branch 'main' into throttler-response-app

f66da96

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested a review from a team July 25, 2024 08:20

mattlord approved these changes Jul 25, 2024

View reviewed changes

shlomi-noach added 3 commits July 28, 2024 08:06

string format

2cd72f1

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

remove extraneous comment indicator.

d6e3f45

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

formalize expected check result summary based on app name

0eef755

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit e341f23 into vitessio:main Jul 28, 2024
132 checks passed

shlomi-noach deleted the throttler-response-app branch July 28, 2024 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throttler: return app name in check result, synthesize "why throttled" explanation from result #16416

Throttler: return app name in check result, synthesize "why throttled" explanation from result #16416

shlomi-noach commented Jul 17, 2024 •

edited

Loading

vitess-bot bot commented Jul 17, 2024

codecov bot commented Jul 17, 2024 •

edited

Loading

rohit-nayak-ps left a comment

mattlord left a comment

mattlord Jul 25, 2024

shlomi-noach Jul 28, 2024

shlomi-noach Jul 28, 2024

mattlord Jul 25, 2024

shlomi-noach Jul 28, 2024

mattlord Jul 25, 2024

shlomi-noach Jul 28, 2024

mattlord Jul 25, 2024

shlomi-noach Jul 28, 2024

mattlord Jul 25, 2024

shlomi-noach Jul 28, 2024

Throttler: return app name in check result, synthesize "why throttled" explanation from result #16416

Throttler: return app name in check result, synthesize "why throttled" explanation from result #16416

Conversation

shlomi-noach commented Jul 17, 2024 • edited Loading

Description

Check result summary

reason_throttled in _vt.vreplication

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jul 17, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jul 17, 2024 • edited Loading

Codecov Report

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

mattlord 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

shlomi-noach commented Jul 17, 2024 •

edited

Loading

`reason_throttled` in `_vt.vreplication`

codecov bot commented Jul 17, 2024 •

edited

Loading