Skip to content

Conversation

@SerjKol80
Copy link
Contributor

What problem does this PR solve?

Issue Number: Close #9855

What is changed and how does it work?

Add 'store' label to metric pd_cluster_status.

Check List

Tests

  • Unit test

Code changes

  • Metrics only

Side effects

  • metric pd_cluster_status now needs to be aggregated in metic system across all stores if you need value across whole cluster.

Related changes
N/A

Release note

metric "pd_cluster_status" now has additional label "store" containing ID of the store.

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Oct 31, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Oct 31, 2025

Hi @SerjKol80. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@ti-chi-bot ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 31, 2025
@rleungx rleungx requested a review from bufferflies November 3, 2025 05:40
@rleungx
Copy link
Member

rleungx commented Nov 3, 2025

/ok-to-test

@ti-chi-bot ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 3, 2025
ObserveHotStat(store, storesStats)
}
stats := storeStats.stats
tikvStats := stats.engineStatistics[core.EngineTiKV]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add a new test for it?

Copy link
Contributor Author

@SerjKol80 SerjKol80 Nov 4, 2025

Choose a reason for hiding this comment

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

@lhy1024
Since we don't have aggregation logic anymore, then the test was removed for that part.
So, the only thing that might be tested in a new logic is if actual metric is emitted. I looked through existing tests and didn't find any tests validating actual metric emission. Thus, no new tests.

Name: "status",
Help: "Status of the cluster.",
}, []string{"type", "engine"})
}, []string{"type", "engine", "store"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update metrics/grafana/pd.json?

Copy link
Contributor Author

@SerjKol80 SerjKol80 Nov 4, 2025

Choose a reason for hiding this comment

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

@lhy1024
I don't think so. All queries for pd_cluster_status metrics already aggregate that metric with sum(). Thus, there should no be any changes there. The description of that metrics should be updated, but I wasn't able to find doc describing metrics in this repo.

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 5, 2025
@SerjKol80
Copy link
Contributor Author

@lhy1024
Thank you. Would you initiate the merge. It looks like I don't have permission.

@lhy1024
Copy link
Contributor

lhy1024 commented Nov 6, 2025

@bufferflies PTAL

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

ti-chi-bot bot commented Nov 11, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-11-05 07:29:51.714334856 +0000 UTC m=+255241.157364725: ☑️ agreed by lhy1024.
  • 2025-11-11 23:30:21.105077587 +0000 UTC m=+831270.548107466: ☑️ agreed by Tema.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 12, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bufferflies, lhy1024, Tema

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:
  • OWNERS [bufferflies,lhy1024]

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

@Tema
Copy link
Contributor

Tema commented Nov 12, 2025

@lhy1024 @bufferflies could you please approve 4 pending workflows to run ^

@SerjKol80 SerjKol80 force-pushed the sergey-kolosov/metric-labels-master branch from 4fca504 to 4a4d487 Compare November 12, 2025 18:59
@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 Nov 12, 2025
@SerjKol80
Copy link
Contributor Author

/retest

@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.68%. Comparing base (75bdf39) to head (fc8d6df).
⚠️ Report is 69 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9898      +/-   ##
==========================================
+ Coverage   78.58%   78.68%   +0.10%     
==========================================
  Files         494      495       +1     
  Lines       66411    66457      +46     
==========================================
+ Hits        52187    52294     +107     
+ Misses      10440    10379      -61     
  Partials     3784     3784              
Flag Coverage Δ
unittests 78.68% <95.91%> (+0.10%) ⬆️

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.

@SerjKol80 SerjKol80 force-pushed the sergey-kolosov/metric-labels-master branch from 4a4d487 to 750b9be Compare November 12, 2025 21:17
@SerjKol80
Copy link
Contributor Author

/retest

@lhy1024
Copy link
Contributor

lhy1024 commented Nov 13, 2025

/test pull-unit-test-next-gen

"fmt"
"strconv"

"github.com/pingcap/kvproto/pkg/metapb"
Copy link
Contributor

Choose a reason for hiding this comment

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

plz fix ci

@SerjKol80
Copy link
Contributor Author

/retest

Signed-off-by: Sergey Kolosov <sergey.kolosov@airbnb.com>
@SerjKol80 SerjKol80 force-pushed the sergey-kolosov/metric-labels-master branch from 750b9be to fc8d6df Compare November 13, 2025 19:50
@SerjKol80
Copy link
Contributor Author

/retest

@ti-chi-bot ti-chi-bot bot merged commit 697cbd3 into tikv:master Nov 13, 2025
29 checks passed
@SerjKol80 SerjKol80 deleted the sergey-kolosov/metric-labels-master branch November 13, 2025 20:43
SerjKol80 added a commit to SerjKol80/tidb-pd that referenced this pull request Nov 13, 2025
close tikv#9855

Add 'store' label to metric pd_cluster_status.

Signed-off-by: Sergey Kolosov <sergey.kolosov@airbnb.com>

Co-authored-by: Sergey Kolosov <sergey.kolosov@airbnb.com>
(cherry picked from commit 697cbd3)
Signed-off-by: Sergey Kolosov <sergey.kolosov@airbnb.com>
JmPotato pushed a commit to JmPotato/pd that referenced this pull request Dec 3, 2025
close tikv#9855

Add 'store' label to metric pd_cluster_status.

Signed-off-by: Sergey Kolosov <sergey.kolosov@airbnb.com>

Co-authored-by: Sergey Kolosov <sergey.kolosov@airbnb.com>
@ti-chi-bot ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Dec 12, 2025
ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Dec 12, 2025
close tikv#9855

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #10055.
But this PR has conflicts, please resolve them!

ti-chi-bot bot pushed a commit that referenced this pull request Dec 15, 2025
…10055)

close #9855

Add 'store' label to metric pd_cluster_status.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
Signed-off-by: 童剑 <1045931706@qq.com>

Co-authored-by: Sergey Kolosov <47006793+SerjKol80@users.noreply.github.com>
Co-authored-by: 童剑 <1045931706@qq.com>
@ti-chi-bot ti-chi-bot bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Jan 8, 2026
ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Jan 8, 2026
close tikv#9855

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-7.5: #10136.
But this PR has conflicts, please resolve them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved contribution This PR is from a community contributor. dco-signoff: yes Indicates the PR's author has signed the dco. lgtm needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add 'store' label to metric pd_cluster_status.

6 participants