Cleanup panics in `txthrottler`, reorder for readability #12901

timvaillancourt · 2023-04-15T14:02:26Z

Description

This PR:

Removes cases of panic() in tx_throttler.go
- Panic in .NewTxThrottler() could never be hit - removed
- Panic in .Throttle() returns false instead - I feel false is acceptable if the throttler is closed
- Panic in .throttle() logs and error and returns false
Reordering of some types/funcs (unchanged) for readability, focusing on (chrono)logical ordering
Cleanup import lines

cc @ejortegau / @shlomi-noach

Related Issue(s)

Resolves #12900

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

vitess-bot · 2023-04-15T14:02:29Z

Review Checklist

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

General

Ensure that the Pull Request has a descriptive title.
If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

Is it really necessary to add this flag?
Flag names should be clear and intuitive (as far as possible)
Help text should be descriptive.
Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

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

Bug fixes

There should be at least one unit or end-to-end test.
The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

There should be some code comments as to why things are implemented the way they are.

New/Existing features

Should be documented, either by modifying the existing documentation or creating new documentation.
New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

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

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

timvaillancourt · 2023-04-15T18:17:58Z

👋 apologies for the reviewer explosion - a bad merge caused a lot of files to be changed. Undoing this did not revert the list of reviewers 🤦

go/vt/vttablet/tabletserver/txthrottler/tx_throttler.go

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

shlomi-noach

LGTM. Pre approving, feel free to shuffle code around as discussed above.

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

timvaillancourt · 2023-04-22T22:08:57Z

LGTM. Pre approving, feel free to shuffle code around as discussed above.

Thanks! I've backed out the shuffling of struct for now. I'll move that to #12902 to reduce the size of the PR 👍

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

) * Cleanup tx_throttler.go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup tx_throttler.go #2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix throttlerFactoryFunc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo if-cond consolidation Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo struct shuffling Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * prove that disabled config returns nil error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

…itessio#12901 (#83) * Cleanup panics in `txthrottler`, reorder for readability (vitessio#12901) * Cleanup tx_throttler.go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup tx_throttler.go #2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix throttlerFactoryFunc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo if-cond consolidation Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo struct shuffling Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * prove that disabled config returns nil error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * remove unused cell string --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

) * Cleanup tx_throttler.go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup tx_throttler.go #2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix throttlerFactoryFunc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo if-cond consolidation Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo struct shuffling Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * prove that disabled config returns nil error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Add basic metrics to `vttablet` transaction throttler (vitessio#12418) * Add basic stats to vttablet tx throttler Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * test new metrics Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * reorder Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * short names Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add max rate Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Move NewGaugeFunc to under conditional Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use env Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Remove env from TxThrottler struct Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix tests Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * PR suggestion Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix unit test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * reorder test vars Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix transaction throttler ignoring the initial rate (vitessio#12618) * Fix transaction throttler ignoring the initial rate This addresses the issue reported in vitessio#12549 Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Add missing override of max replication lag in `throttler.newThrottler()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Reorder functions to make diff easier to read Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix check for maxRate in `newThrottlerFromConfig()` Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix some CI pipeline issues Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comment. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix typo Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com> * Cleanup panics in `txthrottler`, reorder for readability (vitessio#12901) * Cleanup tx_throttler.go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Cleanup tx_throttler.go #2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix throttlerFactoryFunc Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo if-cond consolidation Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo struct shuffling Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * prove that disabled config returns nil error Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Improve test Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Emit per workload labels for existing per table vttablet metrics (vitessio#12394) * Emit per workload labels for existing per table vttablet metrics This adds the possibility to configure vttablet (via CLI flag) to also have a workload label for existing per table metrics (query counts, query times, query errors, query rows affected, query rows returned, query error counts). Workload can be any string that makes sense for the client application. For example, API endpoint name, controller, batch job name, application name or something else. This is usefult to be able to gain observability about how the query load is distributed across different workloads. This is achieved with two new CLI flags, namely: * `enable-per-workload-table-metrics`: whether to enable or disable per workload metric collection - disabled by default to preserve the current behavior, thus making the new feature opt-in only. * `workload-label`: a string to look for in query comments to identify the workload running the current query. The workload is obtained by parsing query comments of the form: /* ... <workload_label>=<workload_name>; ... */ For example, if vttablet is started with `--enable-per-workload-table-metrics --workload-label app_name` anda query is issued with a comment like /* ... app_name=shop; ... */ then metrics will look like ``` vttablet_query_counts{plan="Select",table="dual", workload="shop"} 15479 ``` instead of ``` vttablet_query_counts{plan="Select",table="dual"} 15479 ``` Query comment parsing only takes place if `--enable-per-workload-table-metrics` is used, as to not incur parsing performance impact if the user does not want per workload metrics. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * make linter happy Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * fix flags e2e test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments: * Obtain workload information on the vtgate instead of the vttablet, avoiding double parsing. * Treat workload name as a query directive. * Send workload name from vtgate to vttablet as ExecuteOptions. Additionally, annotate tabletserver's execution span with the workload name to also enrich traces with workload name data, in addition to metrics. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * A few fixes: 1. Rebuild some files with `make proto`. 2. Protect against nil ExecuteOptions on the tabletserver. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix flags e2e test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comments Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fixes Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix a comment Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix e2e flag test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Update JS code for protobuf changes. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix QueryEngine unit test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix e2e flag test Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Fix spurious tab in comment Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * Address PR comment Don't use dual format flag for new flags - stick with - separated ones. Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> --------- Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * remove mistaken git add Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * make vtadmin_web_proto_types Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * test unit_race test on go-version: 1.18.9 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Revert "test unit_race test on go-version: 1.18.9" This reverts commit 922e897. * CI: Misc test improvements to limit failures with various runners (vitessio#13825) Signed-off-by: Matt Lord <mattalord@gmail.com> * Fix setup order to avoid races (vitessio#13871) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Eduardo J. Ortega U. <5791035+ejortegau@users.noreply.github.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>

timvaillancourt requested review from harshit-gangal, systay, shlomi-noach and rohit-nayak-ps as code owners April 15, 2023 14:02

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Apr 15, 2023

github-actions bot added this to the v17.0.0 milestone Apr 15, 2023

timvaillancourt added 2 commits April 15, 2023 16:26

Cleanup tx_throttler.go

756d966

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Cleanup tx_throttler.go #2

b15da5b

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

timvaillancourt force-pushed the tx-throttler-cleanup branch from 1489271 to 3ccbe58 Compare April 15, 2023 14:28

timvaillancourt requested review from deepthi, mattlord, ajm188, notfelineit, frouioui, GuptaManan100, rsajwani, arthurschreiber, derekperkins, vmg and dkhenry as code owners April 15, 2023 14:28

timvaillancourt force-pushed the tx-throttler-cleanup branch from 3ccbe58 to b15da5b Compare April 15, 2023 14:32

timvaillancourt added 2 commits April 15, 2023 16:40

merge main

f14087e

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Fix throttlerFactoryFunc

76c3e7f

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

This was referenced Apr 15, 2023

Skip recalculating the rate in MaxReplicationLagModule when it can't be done #12620

Merged

txthrottler: further code cleanup #12902

Merged

shlomi-noach reviewed Apr 16, 2023

View reviewed changes

Undo if-cond consolidation

8b06678

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

deepthi removed request for mattlord, derekperkins, rsajwani, ajm188, notfelineit and rohit-nayak-ps April 17, 2023 21:03

shlomi-noach reviewed Apr 18, 2023

View reviewed changes

shlomi-noach approved these changes Apr 18, 2023

View reviewed changes

Undo struct shuffling

e8a3b6c

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

timvaillancourt added 2 commits April 23, 2023 00:44

prove that disabled config returns nil error

a6add6f

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

Improve test

b86f5f9

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

timvaillancourt requested a review from shlomi-noach April 23, 2023 00:10

shlomi-noach added Type: Internal Cleanup Component: Query Serving and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says Component: Query Serving labels Apr 23, 2023

harshit-gangal approved these changes May 2, 2023

View reviewed changes

harshit-gangal merged commit 95bf3b9 into vitessio:main May 2, 2023

timvaillancourt deleted the tx-throttler-cleanup branch May 2, 2023 20:40

timvaillancourt mentioned this pull request May 12, 2023

slack-vitess-r14.0.5-dsdefense: backport txthrottler cleanup PR vitessio/vitess#12901 slackhq/vitess#83

Merged

4 tasks

timvaillancourt mentioned this pull request Apr 16, 2024

slack-vitess-r15.0.5: backport required Transaction Throttler PRs slackhq/vitess#302

Closed

4 tasks

timvaillancourt mentioned this pull request May 7, 2024

slack-vitess-r15.0.5: backport Transaction Throttler PRs, pt. 1 slackhq/vitess#335

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup panics in `txthrottler`, reorder for readability #12901

Cleanup panics in `txthrottler`, reorder for readability #12901

timvaillancourt commented Apr 15, 2023 •

edited

Loading

vitess-bot bot commented Apr 15, 2023

timvaillancourt commented Apr 15, 2023

shlomi-noach left a comment

timvaillancourt commented Apr 22, 2023

Cleanup panics in txthrottler, reorder for readability #12901

Cleanup panics in txthrottler, reorder for readability #12901

Conversation

timvaillancourt commented Apr 15, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Apr 15, 2023

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

timvaillancourt commented Apr 15, 2023

shlomi-noach left a comment

Choose a reason for hiding this comment

timvaillancourt commented Apr 22, 2023

Cleanup panics in `txthrottler`, reorder for readability #12901

Cleanup panics in `txthrottler`, reorder for readability #12901

timvaillancourt commented Apr 15, 2023 •

edited

Loading