resource_manager/client: introduce RU and Request metrics #6170

CabinfeverB · 2023-03-15T16:00:25Z

What problem does this PR solve?

Issue Number: close #6136

What is changed and how does it work?

Check List

Tests

Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2023-03-15T16:00:29Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

nolouch · 2023-03-16T06:42:23Z

client/resource_group/controller/controller.go

-	requestUnitConfigPath  = "resource_group/ru_config"
-	maxRetry               = 3
-	maxNotificationChanLen = 200
+	requestUnitConfigPath        = "resource_group/ru_config"


need consist with #6063

This will be resolved automatically after merging #6063

nolouch · 2023-03-16T07:00:22Z

client/resource_group/controller/controller.go

+			resourceGroupTokenRequestCounter.WithLabelValues(name).Inc()
+			// RU info.
+			if consumption.RRU != 0 {
+				rruMetrics.Observe(consumption.RRU)


Does it's duplicated with the server side?

@JmPotato @BornChanger Do u think it is necessary to count RU consumption of tidb instances?

may do not need in current.

I didn't really think about the need to view RU consumption at the instance level, so not keeping a record is okay for me.

ti-chi-bot · 2023-03-16T15:27:51Z

@CabinfeverB: PR needs rebase.

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/test-infra repository.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2023-03-17T10:10:23Z

Codecov Report

Patch coverage: 77.00% and project coverage change: +0.03 🎉

Comparison is base (6eb4410) 74.54% compared to head (964a37c) 74.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6170      +/-   ##
==========================================
+ Coverage   74.54%   74.57%   +0.03%     
==========================================
  Files         393      394       +1     
  Lines       38527    38644     +117     
==========================================
+ Hits        28719    28819     +100     
- Misses       7268     7285      +17     
  Partials     2540     2540

Flag	Coverage Δ
unittests	`74.57% <77.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
client/errs/errno.go	`100.00% <ø> (ø)`
pkg/mcs/meta_storage/server/grpc_service.go	`59.61% <0.00%> (ø)`
pkg/mcs/resource_manager/server/grpc_service.go	`67.77% <0.00%> (ø)`
pkg/storage/endpoint/key_path.go	`93.33% <ø> (ø)`
server/api/server.go	`100.00% <ø> (ø)`
client/tso_stream.go	`73.49% <31.25%> (-5.38%)`	⬇️
client/keyspace_client.go	`63.63% <33.33%> (ø)`
client/meta_storage_client.go	`68.47% <50.00%> (ø)`
client/resource_manager_client.go	`70.29% <50.00%> (ø)`
client/resource_group/controller/limiter.go	`69.93% <60.00%> (ø)`
... and 18 more

... and 16 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

nolouch · 2023-03-20T03:13:31Z

client/resource_group/controller/metrics.go

+const (
+	namespace             = "resource_manager_client"
+	requestSubsystem      = "request"
+	ruSubsystem           = "resource_unit"


remove this one.

nolouch · 2023-03-20T03:13:44Z

ptal @JmPotato

HuSharp · 2023-03-20T03:23:25Z

client/resource_group/controller/metrics.go

+			Help:      "",
+		}, []string{resourceGroupNameLabel})
+
+	successfulTokenRequestDuration = prometheus.NewHistogram(


Maybe add some comments on why "success" is a "Histogram" and "failure" is a "counter".

nolouch · 2023-03-20T03:31:43Z

client/resource_group/controller/metrics.go

+			Help:      "",
+		}, []string{resourceGroupNameLabel})
+
+	failedRequestCounter = prometheus.NewCounterVec(


better use label about failed and success.

I don't think failed requests need to be recorded, so failedRequestCounter is Counter.

JmPotato · 2023-03-20T03:49:50Z

client/resource_group/controller/controller.go

@@ -192,6 +197,7 @@ func (c *ResourceGroupsController) Start(ctx context.Context) {
 		for {
 			select {
 			case <-c.loopCtx.Done():
+				c.resetMetrics()


Do we need to reset resourceGroupStatusGauge for each resource group when it's deleted from the client?

Yes, I do it in cleanUpResourceGroup

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

HuSharp

rest LGTM!

HuSharp · 2023-03-20T06:05:44Z

client/resource_group/controller/metrics.go

+			Subsystem: requestSubsystem,
+			Name:      "success",
+			Buckets:   prometheus.ExponentialBuckets(0.001, 4, 8), // 0.001 ~ 40.96
+			Help:      "Bucketed histogram of wait duration of successfult request.",


successful not successfult

ti-chi-bot · 2023-03-20T06:05:56Z

@HuSharp: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

rest LGTM!

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 ti-community-infra/tichi repository.

JmPotato

The rest LGTM.

client/resource_group/controller/controller.go

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch · 2023-03-20T06:32:23Z

/merge

ti-chi-bot · 2023-03-20T06:32:24Z

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-20T06:32:25Z

This pull request has been accepted and is ready to merge.

Commit hash: a62c7cd

ti-chi-bot · 2023-03-20T06:34:51Z

@CabinfeverB: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-21T06:33:29Z

In response to a cherrypick label: new pull request created to branch release-7.0: #6197.

close tikv#6136 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

close tikv#6136 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

) close #6136, ref #6170 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Signed-off-by: lhy1024 <admin@liudos.us> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Co-authored-by: lhy1024 <admin@liudos.us>

…tikv#6197) close tikv#6136, ref tikv#6170 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Signed-off-by: lhy1024 <admin@liudos.us> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Co-authored-by: lhy1024 <admin@liudos.us> Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB added 3 commits March 15, 2023 15:51

client metrics

b7dae8f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

client metrics

41ce95b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge master

07820e8

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the release-note-none label Mar 15, 2023

ti-chi-bot requested review from JmPotato and rleungx March 15, 2023 16:00

nolouch reviewed Mar 16, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2023

merge master

bba7c35

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2023

address comment

efd0f44

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Mar 20, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 20, 2023

HuSharp reviewed Mar 20, 2023

View reviewed changes

nolouch reviewed Mar 20, 2023

View reviewed changes

JmPotato reviewed Mar 20, 2023

View reviewed changes

address comment

1f64f33

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Mar 20, 2023

View reviewed changes

HuSharp approved these changes Mar 20, 2023

View reviewed changes

JmPotato approved these changes Mar 20, 2023

View reviewed changes

client/resource_group/controller/controller.go Outdated Show resolved Hide resolved

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 20, 2023

address comment

a62c7cd

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 20, 2023

ti-chi-bot added 2 commits March 20, 2023 14:32

Merge branch 'master' into resource-manager/client_metrics

0433e92

Merge branch 'master' into resource-manager/client_metrics

964a37c

ti-chi-bot merged commit 220dbed into tikv:master Mar 20, 2023
18 checks passed

CabinfeverB added the needs-cherry-pick-release-7.0 label Mar 21, 2023

ti-chi-bot mentioned this pull request Mar 21, 2023

resource_manager/client: introduce RU and Request metrics (#6170) #6197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager/client: introduce RU and Request metrics #6170

resource_manager/client: introduce RU and Request metrics #6170

CabinfeverB commented Mar 15, 2023 •

edited

ti-chi-bot commented Mar 15, 2023 •

edited

nolouch Mar 16, 2023

CabinfeverB Mar 16, 2023

nolouch Mar 16, 2023

CabinfeverB Mar 16, 2023

nolouch Mar 16, 2023

JmPotato Mar 17, 2023

ti-chi-bot commented Mar 16, 2023

codecov bot commented Mar 17, 2023 •

edited

nolouch left a comment

nolouch Mar 20, 2023

nolouch commented Mar 20, 2023

HuSharp Mar 20, 2023

nolouch Mar 20, 2023

CabinfeverB Mar 20, 2023

JmPotato Mar 20, 2023

CabinfeverB Mar 20, 2023

nolouch left a comment

HuSharp left a comment

HuSharp Mar 20, 2023 •

edited

ti-chi-bot commented Mar 20, 2023

JmPotato left a comment

nolouch commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 21, 2023

resource_manager/client: introduce RU and Request metrics #6170

resource_manager/client: introduce RU and Request metrics #6170

Conversation

CabinfeverB commented Mar 15, 2023 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 16, 2023

codecov bot commented Mar 17, 2023 • edited

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Mar 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

HuSharp Mar 20, 2023 • edited

Choose a reason for hiding this comment

ti-chi-bot commented Mar 20, 2023

JmPotato left a comment

Choose a reason for hiding this comment

nolouch commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 20, 2023

ti-chi-bot commented Mar 21, 2023

CabinfeverB commented Mar 15, 2023 •

edited

ti-chi-bot commented Mar 15, 2023 •

edited

codecov bot commented Mar 17, 2023 •

edited

HuSharp Mar 20, 2023 •

edited