client/resource_group: expose RU consumption by request source - #10588
client/resource_group: expose RU consumption by request source#10588YuhaoZhang00 wants to merge 12 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPer-request-source RU/WRU metrics were added: controllers now cache per-group request-source metric state, record per-request-source RU/WRU deltas to a new Prometheus counter, RequestInfo requires Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant GC as GroupController
participant Glo as GlobalController
participant P as Prometheus
Client->>GC: send request (RequestInfo with RequestSource)
GC->>GC: compute RU/WRU delta
GC->>GC: getOrCreateRequestSourceMetricsState(resource_group, request_source)
GC->>P: increment RequestSourceRUCounter{resource_group, request_source, type}
GC->>Client: respond
Note over Glo,GC: On shutdown / cleanup
Glo->>GC: trigger cleanup/tombstone
GC->>P: delete labeled series via cleanup()
GC->>Glo: remove cached request-source state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Hi @YuhaoZhang00. Thanks for your PR. I'm waiting for a tikv 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. |
ti-chi-bot
Bot
added
contribution
YuhaoZhang00
changed the title
1dd52fd to
ebd4f63
Compare
ti-chi-bot
Bot
added
size/L
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ebd4f63 to
492976a
Compare
ti-chi-bot
Bot
added
dco-signoff: yes
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10588 +/- ##
==========================================
+ Coverage 79.25% 79.31% +0.06%
==========================================
Files 541 541
Lines 76037 76154 +117
==========================================
+ Hits 60262 60405 +143
+ Misses 11534 11507 -27
- Partials 4241 4242 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
🧹 Nitpick comments (1)
client/resource_group/controller/request_source_metrics_test.go (1)
24-35: Consider increasing channel buffer or using unbuffered pattern for robustness.The channel buffer of 8 could cause the goroutine to block if the collector produces more metrics than the buffer size before the main routine starts consuming. While this is unlikely in controlled test scenarios, a more robust pattern would be to use an unbuffered channel and start consuming immediately, or increase the buffer size.
♻️ Suggested improvement for robustness
func collectorMetricCount(collector prometheus.Collector) int { - ch := make(chan prometheus.Metric, 8) + ch := make(chan prometheus.Metric, 128) go func() { collector.Collect(ch) close(ch) }() count := 0 for range ch { count++ } return count }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/resource_group/controller/request_source_metrics_test.go` around lines 24 - 35, In collectorMetricCount, avoid the fixed small buffered channel which can block if collector emits >8 metrics; change ch := make(chan prometheus.Metric, 8) to either an unbuffered channel (ch := make(chan prometheus.Metric)) so the main goroutine immediately consumes while the goroutine runs, or increase the buffer to a safely large value (e.g., 256/1024) to prevent blocking; ensure this change is applied in the collectorMetricCount function that calls collector.Collect(ch).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/resource_group/controller/request_source_metrics_test.go`:
- Around line 24-35: In collectorMetricCount, avoid the fixed small buffered
channel which can block if collector emits >8 metrics; change ch := make(chan
prometheus.Metric, 8) to either an unbuffered channel (ch := make(chan
prometheus.Metric)) so the main goroutine immediately consumes while the
goroutine runs, or increase the buffer to a safely large value (e.g., 256/1024)
to prevent blocking; ensure this change is applied in the collectorMetricCount
function that calls collector.Collect(ch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bae52b87-fccd-416c-a4c3-3b5c37206d41
📒 Files selected for processing (6)
client/resource_group/controller/global_controller.goclient/resource_group/controller/group_controller.goclient/resource_group/controller/metrics/metrics.goclient/resource_group/controller/model.goclient/resource_group/controller/request_source_metrics_test.goclient/resource_group/controller/testutil.go
|
/cc @JmPotato ptal |
|
@YuhaoZhang00: GitHub didn't allow me to request PR reviews from the following users: ptal. Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
|
/release-note-none |
ti-chi-bot
Bot
added
release-note-none
| func (mc *groupMetricsCollection) cleanupRequestSourceMetrics(resourceGroupName string) { | ||
| mc.sourceMetricsMu.Lock() | ||
| defer mc.sourceMetricsMu.Unlock() | ||
| for requestSource := range mc.sourceMetrics { |
There was a problem hiding this comment.
Will it leak if the getOrCreateRequestSourceMetrics create a new one?
There was a problem hiding this comment.
No extra leak from this cache:
1. These cached request-source metrics are cleaned up when the resource group is cleaned up (cleanupRequestSourceMetrics() called), so they do not stay around forever.
2. The request_source cardinality is also bounded in practice. In TiDB/client-go it currently comes from a small set of hardcoded values (< 100), so we do not expect it to grow uncontrollably.
If the concern is about concurrency issue, all sourceMetrics operations are wrapped by mutex locks.
|
/hold |
… RU metrics The per-source RU counter previously exported the request-side delta on onRequestWait and the response-side delta on onResponse. On a failed write AfterKVRequest emits a negative WRU via payBackWriteCost, which was dropped by the `if WRU > 0` clamp in addRequestSourceRU. The controller's own gc.mu.consumption did receive the payback, so the per-source counter permanently drifted above controller consumption proportional to the failed-write rate — biasing attribution against retry-prone sources. Defer the per-source export to response time and pass the round-trip net `count` (already built for store/global penalty bookkeeping). For a failed write, count = AfterKVRequest delta + BeforeKVRequest write cost, which yields the residual cost (per-batch base + per-replica overhead) that the controller actually charges. The counter therefore tracks gc.mu.consumption on success, failure, and read paths. Add TestFailedWriteMatchesControllerConsumption to pin the contract over mixed success/failure traffic. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: okJiang, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@YuhaoZhang00 please fix the ci |
YuhaoZhang00
changed the title
|
Cross-repository dependency: for this metric to take effect in TiDB/client-go, client-go must propagate the request source and implement |
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
/retest |
|
/hold hold temporarily for local (re)test. |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
/retest |
JmPotato
left a comment
JmPotato
left a comment
There was a problem hiding this comment.
Please address the inline lifecycle issue.
| defer s.mu.Unlock() | ||
| s.closed = true | ||
| for key := range s.items { | ||
| metrics.RequestSourceRUCounter.DeleteLabelValues( |
There was a problem hiding this comment.
RequestSourceRUCounter is process-global, so different controllers can cache the same series. Deleting it here leaves other controllers with orphaned handles whose later Add calls are no longer exported. Please make deletion ownership-aware (for example, delete only after the last owner) and add a two-controller test.
There was a problem hiding this comment.
I believe next-gen is designed to have only one active ResourceGroupsController per process in steady state (correct me if I'm wrong!).
However, a very small overlap window may still exist during bootstrap, upgrade, or controller handoff while the old controller is shutting down and the new one is starting, and I'll try to fix this.
There was a problem hiding this comment.
I understand your point: in the current way TiDB is used, a process has only a single ResourceGroupsController. However, this is not a hard constraint, and it may change in future development, which is why I have some concerns. Still, the PR can be merged as is. For this issue, I think we should leave a TODO or at least keep it in mind, as we may encounter potential pitfalls down the road.
There was a problem hiding this comment.
Will open a new issue to note this down -> #11080
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest |
4 participants
What problem does this PR solve?
Issue Number: close #10634
Tracking issue: pingcap/tidb#64339
Related PR: tikv/client-go#1929
pd/clientaccounts RU consumption in the resource-group controller, but it does not expose a breakdown by request source. This makes it difficult to attribute RU usage to user queries, DDL, stats, and other internal activities.This PR exposes RRU and WRU consumption by request source through the existing
pd/clientaccounting flow.What is changed and how does it work?
RequestSource() stringfromcontroller.RequestInfo. Requests without this provider use an emptyrequest_source.resource_manager_client_request_ru_total{resource_group, request_source, type, direction}:type:rruorwrudirection:consumeorrefundrefundvalues, so net RU isconsume - refund.This metric may create up to the following number of Prometheus time series within one client process if every request source occurs in every active resource group:
active resource groups * distinct request_source values * 2 RU types * 2 directionsMetric handles are created lazily, so only observed non-zero label combinations produce series. However, this PR does not cap the number of
request_sourcevalues; deployments with many resource groups or high-cardinality request sources may therefore create a large number of series.Dependency on client-go
This PR works independently because
RequestSource()is optional. However, attributing TiDB/client-go traffic to its actual source requires tikv/client-go#1929. Without that plumbing, requests are recorded with an emptyrequest_source.The metric becomes effective for client-go after it consumes a
pd/clientversion containing this PR.Check List
Tests:
cd client && make staticManual end-to-end test:
Real
INSERTandSELECTstatements produced:After repeating the workload, the counters increased as expected:
Requests that bypass RU accounting are intentionally absent from this metric.
Release note