Skip to content

feat(resourcemanager): add keyspace name label to metrics #9338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ti-chi-bot merged 3 commits into from
May 28, 2025
Merged

feat(resourcemanager): add keyspace name label to metrics #9338

ti-chi-bot merged 3 commits into from
May 28, 2025

Conversation

@JmPotato
Copy link
Member

@JmPotato JmPotato commented May 21, 2025

What problem does this PR solve?

Issue Number: ref #9296.

What is changed and how does it work?

Add keyspace name label to all resource management metrics for better observability.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

No disruption to the current Grafana panel:

image

Release note

None.

@JmPotato JmPotato requested review from lhy1024, nolouch and rleungx May 21, 2025 09:27
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2025
JmPotato
JmPotato commented May 21, 2025
View reviewed changes
@JmPotato JmPotato force-pushed the keyspace_rg_metrics branch from edca17f to e388a3b Compare May 21, 2025 13:10
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2025
@codecov
Copy link

codecov bot commented May 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.57642% with 17 lines in your changes missing coverage. Please review.

Project coverage is 76.08%. Comparing base (506cae1) to head (ff4ea54).
Report is 8 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #9338      +/-   ##
==========================================
+ Coverage   76.02%   76.08%   +0.06%     
==========================================
  Files         475      477       +2     
  Lines       74080    74256     +176     
==========================================
+ Hits        56320    56499     +179     
+ Misses      14241    14235       -6     
- Partials     3519     3522       +3
Flag Coverage Δ
unittests 76.08% <92.57%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JmPotato JmPotato force-pushed the keyspace_rg_metrics branch 7 times, most recently from 2ed8611 to b3b3469 Compare May 28, 2025 06:53
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2025
okJiang
okJiang reviewed May 28, 2025
View reviewed changes
Copy link
Member

@okJiang okJiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest lgtm

@JmPotato JmPotato force-pushed the keyspace_rg_metrics branch from b3b3469 to e23d3ee Compare May 28, 2025 07:14
JmPotato added 2 commits May 28, 2025 15:24
@JmPotato
feat(resourcemanager): add keyspace name label to metrics
33da145 
Signed-off-by: JmPotato <github@ipotato.me>
@JmPotato
refactor(resource-manager): move metrics-related code to metrics.go
55273a1 
Signed-off-by: JmPotato <github@ipotato.me>
@JmPotato JmPotato force-pushed the keyspace_rg_metrics branch from e23d3ee to 55273a1 Compare May 28, 2025 07:24
rleungx
rleungx reviewed May 28, 2025
View reviewed changes
pkg/mcs/resourcemanager/server/manager.go
case <-availableRUTicker.C:
// Prevent from holding the lock too long when there're many keyspaces and resource groups.
for _, krgm := range m.getKeyspaceResourceGroupManagers() {
keyspaceName, err := m.getKeyspaceNameByID(ctx, krgm.keyspaceID)
Copy link
Member

@rleungx rleungx May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to store the keyspace name in krgm?

Copy link
Member Author

@JmPotato JmPotato May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean keyspace manager? I tried, but encountered a circular reference problem, and considering that the resource manager microservice might be deployed in the future, it seems that we cannot fully rely on calls that may exist in another process's logic.

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 28, 2025
rleungx
rleungx reviewed May 28, 2025
View reviewed changes
@ti-chi-bot ti-chi-bot bot added the lgtm label May 28, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented May 28, 2025

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 28, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented May 28, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-05-28 07:33:05.898718793 +0000 UTC m=+4206.960736412: ☑️ agreed by okJiang.
  • 2025-05-28 07:57:54.785385293 +0000 UTC m=+5695.847402910: ☑️ agreed by rleungx.

@ti-chi-bot ti-chi-bot bot merged commit b0822db into tikv:master May 28, 2025
25 checks passed
@JmPotato JmPotato deleted the keyspace_rg_metrics branch May 28, 2025 08:01
@JmPotato JmPotato mentioned this pull request May 28, 2025
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rleungx rleungx rleungx approved these changes

@nolouch nolouch Awaiting requested review from nolouch

@lhy1024 lhy1024 Awaiting requested review from lhy1024

+1 more reviewer

@okJiang okJiang okJiang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved component/keyspace Key space. component/metrics Metrics. component/resource-control dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JmPotato @okJiang @rleungx