metrics: show Disconnection Count as a rate, split out undetermined - #70337
metrics: show Disconnection Count as a rate, split out undetermined#70337dulao5 wants to merge 3 commits into
Conversation
The Server > Disconnection Count panel plotted the raw cumulative tidb_server_disconnection_total counter, which grows unbounded on long-running clusters and stops being useful for spotting current connect/disconnect activity. Switch the ok/error series to irate(...[30s]) so the panel reflects disconnect frequency (and, transitively, connection churn / workload shape). Keep result="undetermined" as its own cumulative series on the right Y axis, since any nonzero value means a transaction was once left undecided and needs manual investigation, so it should stay an absolute "has this ever happened" counter rather than a rate. close pingcap#70336 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @dulao5. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @dulao5! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe three TiDB Grafana panels now display known disconnection results as 30-second ChangesTiDB disconnection panel updates
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/metrics/grafana/tidb.json (1)
3137-3148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the now-dead
totalseries override.The
{"alias": "total", "fill": 0, "lines": false}override no longer matches any series legend. Target A now emits{{instance}}-{{result}}and target B now emits{{instance}}-undetermined-total. Neither legend equals the literal stringtotal. Remove the override to avoid confusing future maintainers who might assume atotalseries still exists.♻️ Proposed cleanup
"seriesOverrides": [ - { - "alias": "total", - "fill": 0, - "lines": false - }, { "alias": "/-undetermined-total$/", "stack": false, "yaxis": 2 } ],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/metrics/grafana/tidb.json` around lines 3137 - 3148, Remove the dead series override with alias "total" from the seriesOverrides configuration, while preserving the "/-undetermined-total$/" override unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/metrics/grafana/tidb.json`:
- Around line 3137-3148: Remove the dead series override with alias "total" from
the seriesOverrides configuration, while preserving the "/-undetermined-total$/"
override unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 055e42f8-65ba-4ccb-96d0-f053491d7137
📒 Files selected for processing (3)
pkg/metrics/grafana/tidb.jsonpkg/metrics/nextgengrafana/tidb_with_keyspace_name.jsonpkg/metrics/nextgengrafana/tidb_worker.json
|
/ok-to-test |
|
@dulao5: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70337 +/- ##
================================================
- Coverage 76.3271% 73.3791% -2.9480%
================================================
Files 2041 2079 +38
Lines 559032 584768 +25736
================================================
+ Hits 426693 429098 +2405
- Misses 131439 155150 +23711
+ Partials 900 520 -380
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
|
Please preserve the existing target and add the new rate query separately. Replacing it would change the existing |
…series
Per review feedback, restore the original cumulative
sum(tidb_server_disconnection_total{...}) by (instance, result) target
instead of replacing it, so existing dashboard semantics and the
cumulative count view are preserved.
Add the ok/error irate(...[30s]) rate view and the undetermined
cumulative counter as new, additional series on the right Y axis
instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Updated per review feedback: restored the original The new views are now additive, on the right Y axis:
Applied consistently across all three affected dashboard files. |
The dedicated result="undetermined" query duplicated the undetermined series already produced by the original by (instance, result) cumulative target. Drop the extra query and instead route that existing series to the right Y axis via a seriesOverride alias match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Dropped the redundant |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
What problem does this PR solve?
Issue Number: close #70336
Problem Summary:
The
ServerrowDisconnection Countpanel in the TiDB Grafana dashboard plotted the raw cumulativetidb_server_disconnection_totalcounter. On a long-running production cluster this counter accumulates to very large numbers, so it stops being useful for spotting current connect/disconnect activity.What changed and how does it work?
ok/errorresult series toirate(...[30s]), so the panel reflects disconnect frequency (and, transitively, connection churn / whether the workload looks long- or short-lived).result="undetermined"as its own series, still a cumulative total, routed to the panel's right Y axis via aseriesOverridesentry (unstacked). Any nonzeroundeterminedcount means a transaction was once left in a state TiDB could not automatically resolve and needs manual investigation, so it stays an absolute counter instead of being folded into a rate.pkg/metrics/nextgengrafana/tidb_with_keyspace_name.json,pkg/metrics/nextgengrafana/tidb_worker.json) to keep all three dashboard definitions consistent.Manually verified by loading all three dashboard JSON files into a local
tiup playgroundGrafana instance and confirming the panel renders the rate view plus the separate right-axisundeterminedseries as expected.Check List
Tests
Manually loaded the three modified dashboard JSON files into a local tiup playground Grafana instance (http://127.0.0.1:3000) and confirmed the
Disconnection Countpanel renders correctly.Side effects
Documentation
Release note
Summary by CodeRabbit