client/resource_group: add client-side demand observability#10604
client/resource_group: add client-side demand observability#10604okJiang wants to merge 6 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
|
[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 |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds per-resource-group Prometheus metrics, demand RU/sec estimation, structured logging, client identity propagation, metric cleanup, limiter read concurrency, updated tests, and an indirect Go dependency. ChangesResource Group Metrics Observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RequestHandler
participant groupCostController
participant tokenCounter
participant Prometheus
RequestHandler->>groupCostController: acquire tokens and process request
groupCostController->>tokenCounter: calculate demand and consumption deltas
groupCostController->>Prometheus: observe RU, token, byte, and CPU metrics
groupCostController->>RequestHandler: return wait or response accounting
Suggested labels: Suggested reviewers: 🚥 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
the
size/XXL
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: 2
🧹 Nitpick comments (3)
client/resource_group/controller/group_controller.go (2)
307-311: 💤 Low valueMinor: collapse the throttled gauge if/else.
Trivial readability improvement; pre-existing
boolToFloat(or inline conversion) is enough.♻️ Suggested change
- if gc.isThrottled.Load() { - gc.metrics.throttledGauge.Set(1) - } else { - gc.metrics.throttledGauge.Set(0) - } + var throttled float64 + if gc.isThrottled.Load() { + throttled = 1 + } + gc.metrics.throttledGauge.Set(throttled)🤖 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 `@client/resource_group/controller/group_controller.go` around lines 307 - 311, The if/else that sets the throttled gauge can be collapsed into a single call: replace the conditional block that checks gc.isThrottled.Load() and calls gc.metrics.throttledGauge.Set(1/0) with one call to gc.metrics.throttledGauge.Set(boolToFloat(gc.isThrottled.Load())) or an inline conversion (e.g., use a conditional expression to produce 1.0/0.0) so the value is computed from gc.isThrottled.Load() and passed directly to throttledGauge.Set.
480-487: 💤 Low valueThrottled-state-change log message is ambiguous on the "exit" transition.
When
wasThrottledis true andisThrottledflips to false, the log emits the generic"throttled state changed", while the entry transition gets the descriptive"resource group entered throttled mode". Mirroring with an explicit "exited" message makes log-based alerting and debugging easier.♻️ Suggested change
- message := "[resource group controller] throttled state changed" - if isThrottled { - message = "[resource group controller] resource group entered throttled mode" - } + message := "[resource group controller] resource group exited throttled mode" + if isThrottled { + message = "[resource group controller] resource group entered throttled mode" + }🤖 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 `@client/resource_group/controller/group_controller.go` around lines 480 - 487, The current log in the throttled-state transition only emits a specific message when entering throttled mode but uses a generic "throttled state changed" on exit; update the conditional in the block that compares wasThrottled and gc.isThrottled.Load() so that when isThrottled is false it logs a clear exit message (e.g., "[resource group controller] resource group exited throttled mode") instead of the generic one, using the same log.Info call and gc.logFields(0, nil)... to keep context.client/resource_group/controller/metrics/metrics.go (1)
248-282: ⚡ Quick winCounter metrics should use
_totalsuffix instead of_sum.Plain counters must follow Prometheus naming convention with the
_totalsuffix. The metricsconsume_by_type,read_byte_sum,write_byte_sum,kv_cpu_time_ms_sum, andsql_cpu_time_ms_sumare allCounterVecinstances and should be renamed:
consume_by_type→consume_by_type_totalread_byte_sum→read_bytes_totalwrite_byte_sum→write_bytes_totalkv_cpu_time_ms_sum→kv_cpu_time_ms_totalsql_cpu_time_ms_sum→sql_cpu_time_ms_totalThe
_sumsuffix is reserved for auto-generated suffixes on histograms and summaries. Fixing this now prevents confusion with Prometheus tooling and avoids breaking changes once these metrics are consumed by dashboards or alerting rules.🤖 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 `@client/resource_group/controller/metrics/metrics.go` around lines 248 - 282, The Prometheus counter metric names use `_sum` instead of the required `_total`; update the CounterOpts.Name fields and any code that references them: change `consume_by_type` → `consume_by_type_total`, `read_byte_sum` → `read_bytes_total`, `write_byte_sum` → `write_bytes_total`, `kv_cpu_time_ms_sum` → `kv_cpu_time_ms_total`, and `sql_cpu_time_ms_sum` → `sql_cpu_time_ms_total` (these correspond to the CounterVecs declared as ReadByteCost, WriteByteCost, KVCPUCost, SQLCPUCost and the consume-by-type CounterVec), ensure you update all usages/registration and any dashboards/labels referencing those metric names to the new `_total` names to keep semantics and help text consistent.
🤖 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.
Inline comments:
In `@client/go.mod`:
- Line 29: The go.mod entry for "github.com/kylelemons/godebug v1.1.0" appears
unused; run "go mod why github.com/kylelemons/godebug" to confirm whether any
direct dependency requires it, inspect codebase for any imports of
"github.com/kylelemons/godebug" or its consumers, and if it's not needed remove
the entry by running "go mod tidy"; if it is required transitively by an
observability/library dependency, document that relationship or add the explicit
import in the dependent package so the dependency is intentional.
In `@client/resource_group/controller/group_controller.go`:
- Around line 164-170: There's a data race on tokenCounter's demand/avg fields;
protect these by adding a dedicated mutex to tokenCounter and using it wherever
those fields are read or written: acquire the mutex in calcDemandAvg (and the
called calcMovingAvgAt) before mutating demandTotalRU, demandRUPerSec,
demandRUPerSecLastRU, demandAvgLastTime; also acquire the same mutex when
reading demandRUPerSec and avgRUPerSec in logFields and when
updateAvgRequestResourcePerSec writes avgRUPerSec so all concurrent access to
these symbols (tokenCounter.demandTotalRU, demandRUPerSec, demandRUPerSecLastRU,
demandAvgLastTime, avgRUPerSec) is synchronized.
---
Nitpick comments:
In `@client/resource_group/controller/group_controller.go`:
- Around line 307-311: The if/else that sets the throttled gauge can be
collapsed into a single call: replace the conditional block that checks
gc.isThrottled.Load() and calls gc.metrics.throttledGauge.Set(1/0) with one call
to gc.metrics.throttledGauge.Set(boolToFloat(gc.isThrottled.Load())) or an
inline conversion (e.g., use a conditional expression to produce 1.0/0.0) so the
value is computed from gc.isThrottled.Load() and passed directly to
throttledGauge.Set.
- Around line 480-487: The current log in the throttled-state transition only
emits a specific message when entering throttled mode but uses a generic
"throttled state changed" on exit; update the conditional in the block that
compares wasThrottled and gc.isThrottled.Load() so that when isThrottled is
false it logs a clear exit message (e.g., "[resource group controller] resource
group exited throttled mode") instead of the generic one, using the same
log.Info call and gc.logFields(0, nil)... to keep context.
In `@client/resource_group/controller/metrics/metrics.go`:
- Around line 248-282: The Prometheus counter metric names use `_sum` instead of
the required `_total`; update the CounterOpts.Name fields and any code that
references them: change `consume_by_type` → `consume_by_type_total`,
`read_byte_sum` → `read_bytes_total`, `write_byte_sum` → `write_bytes_total`,
`kv_cpu_time_ms_sum` → `kv_cpu_time_ms_total`, and `sql_cpu_time_ms_sum` →
`sql_cpu_time_ms_total` (these correspond to the CounterVecs declared as
ReadByteCost, WriteByteCost, KVCPUCost, SQLCPUCost and the consume-by-type
CounterVec), ensure you update all usages/registration and any dashboards/labels
referencing those metric names to the new `_total` names to keep semantics and
help text consistent.
🪄 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: 9825b70a-d0c9-40d8-a4fb-978dd35ad1ef
⛔ Files ignored due to path filters (1)
client/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
client/go.modclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_group/controller/group_controller.goclient/resource_group/controller/group_controller_test.goclient/resource_group/controller/limiter.goclient/resource_group/controller/metrics/metrics.go
| return calcMovingAvgAt(gc.run.now, &counter.avgRUPerSec, &counter.avgRUPerSecLastRU, &counter.avgLastTime, new) | ||
| } | ||
|
|
||
| func (gc *groupCostController) calcDemandAvg(counter *tokenCounter, delta float64) bool { |
There was a problem hiding this comment.
calcDemandAvg is called from acquireTokens request goroutine so data race might happen.
Suggestion: accumulate demandTotalRU under gc.mu in the request path, compute the EMA in the main loop — same pattern as avgRUPerSec.
| failpoint.Inject("triggerUpdate", func() { | ||
| gc.lowRUNotifyChan <- notifyMsg{} | ||
| }) | ||
| if waitDuration > slowNotifyFilterDuration { |
There was a problem hiding this comment.
slowNotifyFilterDuration == 10 ms might be too short for token wait scenario? (this also applies to other log.Warn added in this PR)
There was a problem hiding this comment.
Agreed. A successful token wait is expected throttling behavior, so logging every successful wait over 10ms at WARN is too noisy under high QPS. #10997 removed these successful-wait WARNs on release-nextgen-202603 and has merged. I applied the equivalent change here in b8a835c and 916fcba: successful request-side and response-side wait WARNs are removed, failed-wait WARNs remain, and SuccessfulRequestDuration preserves its existing semantics.
okJiang
force-pushed
the
codex/rc-client-observability-latest
branch
2 times, most recently
from
0060aeb to
46d8a19
Compare
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
Signed-off-by: okjiang <819421878@qq.com>
okJiang
force-pushed
the
codex/rc-client-observability-latest
branch
from
16c761a to
369e6aa
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10604 +/- ##
==========================================
+ Coverage 79.22% 79.27% +0.04%
==========================================
Files 541 541
Lines 75965 76212 +247
==========================================
+ Hits 60187 60414 +227
- Misses 11531 11543 +12
- Partials 4247 4255 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Successful token waits are expected while resource control throttles requests. Logging every wait over 10ms at WARN can flood TiDB logs under high QPS. Keep the existing histogram for observability and record the accumulated retry and reservation wait duration. Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
|
Dependency update: #10997 fixed this logging issue on |
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
|
@okJiang: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: ref #10488, ref #10581
Resource control observability on the client side does not show the difference between real demand and post-throttling consumption. It is also hard to tell whether pressure is local to one TiDB instance or spread across the whole resource group.
What is changed and how does it work?
This PR adds client-side metrics and slow-path logs so we can explain demand, limiter state, and per-instance pressure before requests are throttled.
It includes:
demand_ru_per_secmetric collected beforeReserve()Check List
Tests
Manual test:
tiup playgroundsetup with 3 PD, 2 TiDB, and 3 TiKV.demand_ru_per_sec > fill_rate, higher client wait, and throttling on the hot TiDB instance.Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Improvements
Tests