tx throttler: remove unused topology watchers #14412

deepthi · 2023-10-31T16:01:36Z

Description

We had topology watchers in TxThrottler even though they are no longer needed or used. Healthcheck already encapsulates a topology watcher per cell. Cleaning this up in preparation for the actual work needed to fix #14277.
I have also rewritten most of the comments in healthcheck.go because a lot of them were obsolete or incorrect.

Related Issue(s)

#14277

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

Signed-off-by: deepthi <deepthi@planetscale.com>

vitess-bot · 2023-10-31T16:01:39Z

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.

dbussink · 2023-10-31T20:30:15Z

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

@@ -204,8 +199,7 @@ func NewTxThrottler(env tabletenv.Env, topoServer *topo.Server) TxThrottler {
 config: config,
 topoServer: topoServer,
 throttlerRunning: env.Exporter().NewGauge(TxThrottlerName+"Running", "transaction throttler running state"),
- topoWatchers: env.Exporter().NewGaugesWithSingleLabel(TxThrottlerName+"TopoWatchers", "transaction throttler topology watchers", "cell"),
- healthChecksReadTotal: env.Exporter().NewCountersWithMultiLabels(TxThrottlerName+"HealthchecksRead", "transaction throttler healthchecks read",
+ topoWatchers: env.Exporter().NewGaugesWithSingleLabel(TxThrottlerName+"TopoWatchers", "DEPRECATED: transaction throttler topology watchers", "cell"), healthChecksReadTotal: env.Exporter().NewCountersWithMultiLabels(TxThrottlerName+"HealthchecksRead", "transaction throttler healthchecks read",


Looks like a newline here was removed which makes the diff a bit confusing?

Also, what is the general value / policy around metrics deprecations? Is it useful / expected for folks to have empty metrics? Does that have value over removing the metric?

I'll add back the newline. I went back and forth between deleting the metric versus deprecating. We need to deprecate so that people's existing metrics pipelines don't break without warning.

I've added a line in the release notes for these.

We need to deprecate so that people's existing metrics pipelines don't break without warning.

But doesn't a metric that flatlines break things anyway? I think it's also possible to argue that having a metric go to zero is more confusing / problematic than removing it as a signal to the user that the value is (no longer) useful? I think this is tricky anyway, we can argue both ways but I don't think this is as clear cut as say a command line flag that is deprecated?

Honestly I don't know if anyone is even using this metric. It's not much effort to keep it deprecated and delete it in the next release, so I ended up choosing that as the "safer" approach.

Signed-off-by: deepthi <deepthi@planetscale.com>

GuptaManan100

LGTM

…r: remove unused topology watchers (vitessio#3578) * backport of 3563 * fix conflicts Signed-off-by: deepthi <deepthi@planetscale.com> --------- Signed-off-by: deepthi <deepthi@planetscale.com> Co-authored-by: deepthi <deepthi@planetscale.com>

Signed-off-by: deepthi <deepthi@planetscale.com>

…pt. 3 + ci fixes (#351) * txthrottler: add metrics for topoWatcher and healthCheckStreamer (vitessio#13153) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Replace deprecated `github.com/golang/mock` with `go.uber.org/mock` (vitessio#13512) Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Per workload TxThrottler metrics (vitessio#13526) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * tx throttler: healthcheck all cells if `--tx-throttler-healthcheck-cells` is undefined (vitessio#12477) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> * Add dry-run/monitoring-only mode for TxThrottler (vitessio#13604) 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> * `txthrottler`: remove `txThrottlerConfig` struct, rely on `tabletenv` (vitessio#13624) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * tx throttler: remove unused topology watchers (vitessio#14412) Signed-off-by: deepthi <deepthi@planetscale.com> * tx_throttler: delete topo watcher metric instead of deprecating (vitessio#14445) Signed-off-by: deepthi <deepthi@planetscale.com> * TxThrottler: dont throttle unless lag (vitessio#14789) Signed-off-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> * go test -v Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * add mutex to flaky parseFlags() Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * revert tweaks for flaky tests Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * fix protojson err Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * make vtadmin_web_proto_types Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * remove debug t.Logf(...) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> Signed-off-by: Shlomi Noach <2607934+shlomi-noach@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> Signed-off-by: deepthi <deepthi@planetscale.com> Co-authored-by: Eng Zer Jun <engzerjun@gmail.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Eduardo J. Ortega U <5791035+ejortegau@users.noreply.github.com> Co-authored-by: Deepthi Sigireddi <deepthi@planetscale.com>

deepthi added 2 commits October 19, 2023 19:37

healthcheck: edit comments for correctness

9b04f1d

Signed-off-by: deepthi <deepthi@planetscale.com>

tx_throttler: remove unused topology watchers

fecbcf5

Signed-off-by: deepthi <deepthi@planetscale.com>

deepthi requested review from harshit-gangal, systay, shlomi-noach, rohit-nayak-ps and frouioui as code owners October 31, 2023 16:01

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Oct 31, 2023

deepthi removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Oct 31, 2023

github-actions bot added this to the v19.0.0 milestone Oct 31, 2023

deepthi added Component: Query Serving Type: Internal Cleanup labels Oct 31, 2023

dbussink reviewed Oct 31, 2023

View reviewed changes

deepthi added 2 commits October 31, 2023 15:33

tx_throttler: formatting

3fa9841

Signed-off-by: deepthi <deepthi@planetscale.com>

release notes: document deprecated metrics

3f40609

Signed-off-by: deepthi <deepthi@planetscale.com>

dbussink approved these changes Nov 1, 2023

View reviewed changes

GuptaManan100 approved these changes Nov 1, 2023

View reviewed changes

GuptaManan100 merged commit 3924566 into vitessio:main Nov 1, 2023
115 checks passed

GuptaManan100 deleted the ds-cleanup-tw branch November 1, 2023 14:00

This was referenced Nov 1, 2023

txthrottler: filter topology watcher by shard #13062

Closed

Feature Request: filter topology by shard in vttablet txthrottler #13068

Closed

This was referenced Nov 2, 2023

tx_throttler: remove topo watchers metric #14444

Merged

tx_throttler: delete topo watcher metric instead of deprecating #14445

Merged

timvaillancourt pushed a commit to slackhq/vitess that referenced this pull request May 16, 2024

tx throttler: remove unused topology watchers (vitessio#14412)

65333f6

Signed-off-by: deepthi <deepthi@planetscale.com>

timvaillancourt mentioned this pull request May 16, 2024

slack-vitess-r15.0.5: backport required Transaction Throttler PRs, pt. 3 + ci fixes slackhq/vitess#351

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tx throttler: remove unused topology watchers #14412

tx throttler: remove unused topology watchers #14412

deepthi commented Oct 31, 2023 •

edited

Loading

vitess-bot bot commented Oct 31, 2023

dbussink Oct 31, 2023

deepthi Oct 31, 2023

deepthi Oct 31, 2023

dbussink Nov 1, 2023

deepthi Nov 1, 2023

GuptaManan100 left a comment

tx throttler: remove unused topology watchers #14412

tx throttler: remove unused topology watchers #14412

Conversation

deepthi commented Oct 31, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Oct 31, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

dbussink Oct 31, 2023

Choose a reason for hiding this comment

deepthi Oct 31, 2023

Choose a reason for hiding this comment

deepthi Oct 31, 2023

Choose a reason for hiding this comment

dbussink Nov 1, 2023

Choose a reason for hiding this comment

deepthi Nov 1, 2023

Choose a reason for hiding this comment

GuptaManan100 left a comment

Choose a reason for hiding this comment

deepthi commented Oct 31, 2023 •

edited

Loading