resource_group_client: statistic sql cpu cost #6003

HuSharp · 2023-02-16T07:54:13Z

What problem does this PR solve?

Issue Number: Ref #5851

What is changed and how does it work?

statistic tidb cpu cost

Check List

Release note

None

ti-chi-bot · 2023-02-16T07:54:14Z

[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.

ti-chi-bot · 2023-02-16T07:54:22Z

Hi @HuSharp. 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.

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: husharp <jinhao.hu@pingcap.com>

codecov · 2023-02-16T10:09:50Z

Codecov Report

Patch coverage: 82.75% and project coverage change: -0.04 ⚠️

Comparison is base (2383cf4) 74.02% compared to head (d56342f) 73.99%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6003      +/-   ##
==========================================
- Coverage   74.02%   73.99%   -0.04%     
==========================================
  Files         381      381              
  Lines       37793    37802       +9     
==========================================
- Hits        27978    27971       -7     
- Misses       7351     7364      +13     
- Partials     2464     2467       +3

Flag	Coverage Δ
unittests	`73.99% <82.75%> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
client/resource_group/controller/model.go	`64.00% <66.66%> (+0.50%)`	⬆️
pkg/mcs/resource_manager/server/manager.go	`81.76% <66.66%> (+0.99%)`	⬆️
client/resource_group/controller/config.go	`100.00% <100.00%> (ø)`
client/resource_group/controller/controller.go	`61.91% <100.00%> (+0.53%)`	⬆️
pkg/mcs/resource_manager/server/metrics.go	`100.00% <100.00%> (ø)`
pkg/errs/errs.go	`75.00% <0.00%> (-25.00%)`	⬇️
server/region_syncer/server.go	`82.96% <0.00%> (-4.40%)`	⬇️
server/schedule/labeler/rules.go	`87.50% <0.00%> (-2.28%)`	⬇️
pkg/tso/tso.go	`69.10% <0.00%> (-1.69%)`	⬇️
pkg/tso/allocator_manager.go	`62.56% <0.00%> (-1.63%)`	⬇️
... and 20 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

client/resource_group/controller/controller.go

client/resource_group/controller/model.go

nolouch

I think we need add a metrics for it.

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp · 2023-02-17T08:15:53Z

I think we need add a metrics for it.

sqlCPUMetrics have been in backgroundMetricsFlush

nolouch

Could you show the metrics panel as an example?

nolouch · 2023-02-21T03:45:30Z

client/resource_group/controller/model.go

 func (dsc *SQLCalculator) Trickle(ctx context.Context, consumption *rmpb.Consumption) {
+	delta := getSQLProcessCPUTime() - consumption.SqlLayerCpuTimeMs
+	consumption.SqlLayerCpuTimeMs = delta


I think it's an accumulative value, not a delta value. we can calculate the delta within collectRequestAndConsumption? ptal @JmPotato

Yes, this will eventually be calculated in collectRequestAndConsumption.
The reason why delta is calculated here is because it will be added here.
https://github.com/tikv/pd/pull/6003/files#diff-ac1c6525251add035bc46a2bce40ac9d39ee05a8f1b9c062657e869d10506497R499

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp · 2023-02-21T06:05:34Z

Could you show the metrics panel as an example?

@nolouch
after modify pd client in tidb and client-go
when run sysbench:

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch · 2023-03-01T12:49:57Z

client/resource_group/controller/controller.go

@@ -468,7 +483,7 @@ func newGroupCostController(
 		gc.handleRespFunc = gc.handleRawResourceTokenResponse
 	}

-	gc.mu.consumption = &rmpb.Consumption{}
+	gc.mu.consumption = &rmpb.Consumption{SqlLayerCpuTimeMs: getSQLProcessCPUTime(mainCfg.isSingleGroupByKeyspace)}


I think should make the lastRequestConsumption init with it. then collectRequestAndConsumption will calculate the delta part.

You are right, the run.consumption represents the total, not the delta

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp · 2023-03-02T02:32:52Z

metrics
RU

SQL CPU Time

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch · 2023-03-02T05:20:03Z

pkg/mcs/resource_manager/server/metrics.go

@@ -46,6 +46,14 @@ var (
 			Help:      "Bucketed histogram of the write request unit cost for all resource groups.",
 			Buckets:   prometheus.ExponentialBuckets(3, 10, 5), // 3 ~ 300000
 		}, []string{resourceGroupNameLabel})
+	sqlLayerRequestUnitCost = prometheus.NewHistogramVec(


maybe we do no need histogram.

have changed to vec

Signed-off-by: husharp <jinhao.hu@pingcap.com>

client/resource_group/controller/controller.go

Signed-off-by: husharp <jinhao.hu@pingcap.com>

JmPotato · 2023-03-02T07:50:55Z

/merge

ti-chi-bot · 2023-03-02T07:50:57Z

@JmPotato: 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-02T07:50:58Z

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

Commit hash: d56342f

JmPotato · 2023-03-02T08:09:42Z

/ok-to-test

ref tikv#5851 statistic tidb cpu cost Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test labels Feb 16, 2023

ti-chi-bot requested review from disksing and rleungx February 16, 2023 07:56

HuSharp force-pushed the sqllayer_cpu_cost branch from bdd0683 to 6476028 Compare February 16, 2023 08:45

statistic cpu

1ab14fe

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the sqllayer_cpu_cost branch from 6476028 to 1ab14fe Compare February 16, 2023 09:59

ti-chi-bot removed the do-not-merge/needs-linked-issue label Feb 16, 2023

HuSharp marked this pull request as ready for review February 17, 2023 00:27

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2023

JmPotato requested review from JmPotato, CabinfeverB and nolouch and removed request for disksing and rleungx February 17, 2023 03:50

JmPotato reviewed Feb 17, 2023

View reviewed changes

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

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

nolouch reviewed Feb 17, 2023

View reviewed changes

address comment

c2bfebd

Signed-off-by: husharp <jinhao.hu@pingcap.com>

Merge branch 'master' into sqllayer_cpu_cost

b93667e

nolouch reviewed Feb 20, 2023

View reviewed changes

nolouch reviewed Feb 21, 2023

View reviewed changes

as ms

5a8fdcc

Signed-off-by: husharp <jinhao.hu@pingcap.com>

CabinfeverB approved these changes Feb 22, 2023

View reviewed changes

nolouch removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2023

init val

f8d978a

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the sqllayer_cpu_cost branch from 4b93930 to f8d978a Compare March 1, 2023 10:43

nolouch reviewed Mar 1, 2023

View reviewed changes

init val

85bde35

Signed-off-by: husharp <jinhao.hu@pingcap.com>

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 1, 2023

update metrics

869e8e2

Signed-off-by: husharp <jinhao.hu@pingcap.com>

HuSharp force-pushed the sqllayer_cpu_cost branch from 030751b to 869e8e2 Compare March 2, 2023 03:04

nolouch reviewed Mar 2, 2023

View reviewed changes

HuSharp added 2 commits March 2, 2023 13:55

update metrics

3225252

Signed-off-by: husharp <jinhao.hu@pingcap.com>

Merge branch 'master' into sqllayer_cpu_cost

91923a0

JmPotato reviewed Mar 2, 2023

View reviewed changes

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

address comment

d56342f

Signed-off-by: husharp <jinhao.hu@pingcap.com>

nolouch approved these changes Mar 2, 2023

View reviewed changes

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

JmPotato approved these changes Mar 2, 2023

View reviewed changes

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 2, 2023

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

ti-chi-bot added ok-to-test and removed needs-ok-to-test labels Mar 2, 2023

ti-chi-bot merged commit 2ae62af into tikv:master Mar 2, 2023

HuSharp deleted the sqllayer_cpu_cost branch March 2, 2023 08:17

rleungx pushed a commit to rleungx/pd that referenced this pull request Apr 19, 2023

resource_group_client: statistic sql cpu cost (tikv#6003)

b82cb15

ref tikv#5851 statistic tidb cpu cost Signed-off-by: husharp <jinhao.hu@pingcap.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_group_client: statistic sql cpu cost #6003

resource_group_client: statistic sql cpu cost #6003

HuSharp commented Feb 16, 2023 •

edited

ti-chi-bot commented Feb 16, 2023 •

edited

ti-chi-bot commented Feb 16, 2023

codecov bot commented Feb 16, 2023 •

edited

nolouch left a comment

HuSharp commented Feb 17, 2023

nolouch left a comment

nolouch Feb 21, 2023

HuSharp Feb 21, 2023

HuSharp commented Feb 21, 2023 •

edited

nolouch Mar 1, 2023 •

edited

HuSharp Mar 1, 2023

HuSharp commented Mar 2, 2023 •

edited

nolouch Mar 2, 2023

HuSharp Mar 2, 2023 •

edited

JmPotato commented Mar 2, 2023

ti-chi-bot commented Mar 2, 2023

ti-chi-bot commented Mar 2, 2023

JmPotato commented Mar 2, 2023

resource_group_client: statistic sql cpu cost #6003

resource_group_client: statistic sql cpu cost #6003

Conversation

HuSharp commented Feb 16, 2023 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 16, 2023 • edited

ti-chi-bot commented Feb 16, 2023

codecov bot commented Feb 16, 2023 • edited

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

HuSharp commented Feb 17, 2023

nolouch left a comment

Choose a reason for hiding this comment

nolouch Feb 21, 2023

Choose a reason for hiding this comment

HuSharp Feb 21, 2023

Choose a reason for hiding this comment

HuSharp commented Feb 21, 2023 • edited

nolouch Mar 1, 2023 • edited

Choose a reason for hiding this comment

HuSharp Mar 1, 2023

Choose a reason for hiding this comment

HuSharp commented Mar 2, 2023 • edited

nolouch Mar 2, 2023

Choose a reason for hiding this comment

HuSharp Mar 2, 2023 • edited

Choose a reason for hiding this comment

JmPotato commented Mar 2, 2023

ti-chi-bot commented Mar 2, 2023

ti-chi-bot commented Mar 2, 2023

JmPotato commented Mar 2, 2023

HuSharp commented Feb 16, 2023 •

edited

ti-chi-bot commented Feb 16, 2023 •

edited

codecov bot commented Feb 16, 2023 •

edited

HuSharp commented Feb 21, 2023 •

edited

nolouch Mar 1, 2023 •

edited

HuSharp commented Mar 2, 2023 •

edited

HuSharp Mar 2, 2023 •

edited