resourcecontrol: propagate request source for RU metrics#1929
Open
YuhaoZhang00 wants to merge 1 commit into
Open
resourcecontrol: propagate request source for RU metrics#1929YuhaoZhang00 wants to merge 1 commit into
YuhaoZhang00 wants to merge 1 commit into
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthrough
ChangesRequest source propagation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ti-chi-bot
Bot
added
contribution
|
Welcome @YuhaoZhang00! |
ti-chi-bot
Bot
added
the
size/M
YuhaoZhang00
changed the title
YuhaoZhang00
marked this pull request as ready for review
March 31, 2026 03:00
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics/metrics.go`:
- Around line 985-1001: The counters TiKVResourceControlRUCounter and
TiKVResourceControlBypassedCounter currently hardcode Subsystem:
"resource_control" which yields wrong metric names; update their CounterOpts to
use Subsystem: subsystem and move "resource_control" into the Name field so the
metrics become tikv_client_go_resource_control_ru_total and
tikv_client_go_resource_control_bypassed_request_total (i.e., set Name for
TiKVResourceControlRUCounter to "resource_control_ru_total" and for
TiKVResourceControlBypassedCounter to
"resource_control_bypassed_request_total"), leaving the rest (Help, ConstLabels,
label names LblResourceGroup/LblSource/LblType) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd655b94-3baa-438a-a3ab-39011ae52513
📒 Files selected for processing (2)
internal/client/client_interceptor.gometrics/metrics.go
Comment on lines
+985
to
+1001
| TiKVResourceControlRUCounter = prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Namespace: namespace, | ||
| Subsystem: "resource_control", | ||
| Name: "ru_total", | ||
| Help: "Counter of resource units consumed by requests going through resource control.", | ||
| ConstLabels: constLabels, | ||
| }, []string{LblResourceGroup, LblSource, LblType}) | ||
|
|
||
| TiKVResourceControlBypassedCounter = prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Namespace: namespace, | ||
| Subsystem: "resource_control", | ||
| Name: "bypassed_request_total", | ||
| Help: "Counter of requests that bypassed resource control.", | ||
| ConstLabels: constLabels, | ||
| }, []string{LblResourceGroup, LblSource}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: default metrics use namespace=tikv and subsystem=client_go.
rg -n 'initMetrics\("tikv",\s*"client_go",\s*nil\)' metrics/metrics.go
# Expect: the new counters override that subsystem with "resource_control".
rg -n 'TiKVResourceControl(RU|Bypassed)Counter|Subsystem:\s+"resource_control"|Name:\s+"(ru_total|bypassed_request_total)"' metrics/metrics.goRepository: tikv/client-go
Length of output: 665
Change Subsystem and Name in these CounterOpts to match the tikv_client_go_* naming pattern.
With the default initMetrics("tikv", "client_go", nil), hardcoding Subsystem: "resource_control" exports tikv_resource_control_{ru,bypassed_request}_total instead of the expected tikv_client_go_resource_control_* naming. This breaks consistency with other metrics. Use Subsystem: subsystem and move resource_control into Name.
🔧 Proposed fix
TiKVResourceControlRUCounter = prometheus.NewCounterVec(
prometheus.CounterOpts{
Namespace: namespace,
- Subsystem: "resource_control",
- Name: "ru_total",
+ Subsystem: subsystem,
+ Name: "resource_control_ru_total",
Help: "Counter of resource units consumed by requests going through resource control.",
ConstLabels: constLabels,
}, []string{LblResourceGroup, LblSource, LblType})
TiKVResourceControlBypassedCounter = prometheus.NewCounterVec(
prometheus.CounterOpts{
Namespace: namespace,
- Subsystem: "resource_control",
- Name: "bypassed_request_total",
+ Subsystem: subsystem,
+ Name: "resource_control_bypassed_request_total",
Help: "Counter of requests that bypassed resource control.",
ConstLabels: constLabels,
}, []string{LblResourceGroup, LblSource})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TiKVResourceControlRUCounter = prometheus.NewCounterVec( | |
| prometheus.CounterOpts{ | |
| Namespace: namespace, | |
| Subsystem: "resource_control", | |
| Name: "ru_total", | |
| Help: "Counter of resource units consumed by requests going through resource control.", | |
| ConstLabels: constLabels, | |
| }, []string{LblResourceGroup, LblSource, LblType}) | |
| TiKVResourceControlBypassedCounter = prometheus.NewCounterVec( | |
| prometheus.CounterOpts{ | |
| Namespace: namespace, | |
| Subsystem: "resource_control", | |
| Name: "bypassed_request_total", | |
| Help: "Counter of requests that bypassed resource control.", | |
| ConstLabels: constLabels, | |
| }, []string{LblResourceGroup, LblSource}) | |
| TiKVResourceControlRUCounter = prometheus.NewCounterVec( | |
| prometheus.CounterOpts{ | |
| Namespace: namespace, | |
| Subsystem: subsystem, | |
| Name: "resource_control_ru_total", | |
| Help: "Counter of resource units consumed by requests going through resource control.", | |
| ConstLabels: constLabels, | |
| }, []string{LblResourceGroup, LblSource, LblType}) | |
| TiKVResourceControlBypassedCounter = prometheus.NewCounterVec( | |
| prometheus.CounterOpts{ | |
| Namespace: namespace, | |
| Subsystem: subsystem, | |
| Name: "resource_control_bypassed_request_total", | |
| Help: "Counter of requests that bypassed resource control.", | |
| ConstLabels: constLabels, | |
| }, []string{LblResourceGroup, LblSource}) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@metrics/metrics.go` around lines 985 - 1001, The counters
TiKVResourceControlRUCounter and TiKVResourceControlBypassedCounter currently
hardcode Subsystem: "resource_control" which yields wrong metric names; update
their CounterOpts to use Subsystem: subsystem and move "resource_control" into
the Name field so the metrics become tikv_client_go_resource_control_ru_total
and tikv_client_go_resource_control_bypassed_request_total (i.e., set Name for
TiKVResourceControlRUCounter to "resource_control_ru_total" and for
TiKVResourceControlBypassedCounter to
"resource_control_bypassed_request_total"), leaving the rest (Help, ConstLabels,
label names LblResourceGroup/LblSource/LblType) unchanged.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
YuhaoZhang00
force-pushed
the
rc/resource-control-metrics
branch
from
July 21, 2026 07:09
3ea6073 to
aff7447
Compare
|
[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 |
ti-chi-bot
Bot
added
size/S
This was referenced Jul 21, 2026
YuhaoZhang00
changed the title
This was referenced Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref tikv/pd#10634
Related PR: tikv/pd#10588
What problem does this PR solve?
pd/clientrecords RU consumption inside its resource-group controller, but client-go'sresourcecontrol.RequestInfodoes not currently propagate the TiKV request source. Without that value, the per-request-source RU metric introduced by tikv/pd#10588 cannot attribute RRU/WRU to user queries, DDL, stats, and other internal activities.What is changed and how does it work?
tikvrpc.Request.GetRequestSource()in client-go'sresourcecontrol.RequestInfofor both read and write requests.RequestSource()soRequestInfoimplements the optional request-source provider recognized bypd/client.This PR only propagates request metadata. It does not change resource-control bypass behavior, define client-go RU counters, or duplicate RU accounting in the client-go interceptor. The metric itself is owned and recorded by
pd/clientin tikv/pd#10588.The metric becomes effective after client-go consumes a
pd/clientversion containing tikv/pd#10588.Tests
Release note