metrics: add keyspace label to show tso request details #9778
metrics: add keyspace label to show tso request details #9778bufferflies wants to merge 3 commits intotikv:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
[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
the
size/L
bufferflies
force-pushed
the
metrics/keyspace
branch
from
88e7e33 to
b597a74
Compare
bufferflies
force-pushed
the
metrics/keyspace
branch
from
b597a74 to
41ce060
Compare
Signed-off-by: 童剑 <1045931706@qq.com>
bufferflies
force-pushed
the
metrics/keyspace
branch
from
41ce060 to
d5786ac
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9778 +/- ##
==========================================
- Coverage 76.88% 76.80% -0.08%
==========================================
Files 485 486 +1
Lines 77372 77586 +214
==========================================
+ Hits 59485 59592 +107
- Misses 14270 14346 +76
- Partials 3617 3648 +31
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
There was a problem hiding this comment.
Pull Request Overview
This PR adds keyspace label support to TSO request metrics to provide better observability by enabling users to distinguish TSO requests across different keyspaces. The change modifies the metrics collection system to include keyspace names in TSO request duration tracking.
Key changes:
- Add keyspace name tracking throughout the TSO client pipeline
- Update metrics to include keyspace_name label for TSO request duration
- Refactor RequestDuration metric to be exported and include keyspace information
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/servicediscovery/service_discovery.go | Add keyspaceName field and parameter to service discovery |
| client/metrics/metrics.go | Export RequestDuration metric and add keyspace_name label |
| client/inner_client.go | Add keyspaceName field and pass to TSO client initialization |
| client/constants/constants.go | Add NullKeyspaceName constant for keyspace-agnostic operations |
| client/clients/tso/stream_test.go | Update test mocks to include keyspaceName parameter |
| client/clients/tso/stream.go | Add keyspaceName to stream builders and TSO stream |
| client/clients/tso/dispatcher_test.go | Update test setup with keyspaceName parameter |
| client/clients/tso/client.go | Add keyspaceName field to TSO client and pass through stream creation |
| client/client.go | Add keyspace name resolution logic and initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // case, `Recv` may return an error while no request is pending. | ||
| if hasReq { | ||
| metrics.RequestFailedDurationTSO.Observe(latencySeconds) | ||
| successObserver().Observe(latencySeconds) |
There was a problem hiding this comment.
The success and failed observers are swapped. When there's an error, it should use the failed observer, not the success observer.
| if keyspaceID == constants.NullKeyspaceID { | ||
| return nil | ||
| } | ||
| metas, err := c.GetAllKeyspaces(clientCtx, keyspaceID, 1) |
There was a problem hiding this comment.
Can we introduce a separate GetKeyspaceNameByID call? Fetching all keyspaces seems too heavy.
There was a problem hiding this comment.
Yes, it should add a new api to fetch the keyspace name easily and simply. I will do it in next PR
| metrics.EstimateTSOLatencyGauge.WithLabelValues(s.streamID).Set(micros * 1e-6) | ||
| } | ||
| successObserver := sync.OnceValue(func() prometheus.Observer { | ||
| return metrics.RequestDuration.WithLabelValues("tso", s.keyspaceName) |
There was a problem hiding this comment.
For metrics such as batch size, should we also introduce the keyspace name label?
Signed-off-by: 童剑 <1045931706@qq.com>
bufferflies
force-pushed
the
metrics/keyspace
branch
from
1c6fa70 to
b28df15
Compare
| metrics.EstimateTSOLatencyGauge.WithLabelValues(s.streamID).Set(micros * 1e-6) | ||
| } | ||
| successObserver := sync.OnceValue(func() prometheus.Observer { | ||
| return metrics.RequestDuration.WithLabelValues("tso", s.keyspaceName) |
There was a problem hiding this comment.
What if we have thousands of keyspaces?
There was a problem hiding this comment.
The metrics report in the TIDB server, one TIDB server can only have one keyspace name. And the metrics agent collects this into different tenants.
|
@bufferflies: The following tests 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. |
What problem does this PR solve?
Issue Number: Close #9780, ref #9707
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note