disttask: add metrics collection for dispatcher by JK1Zhang · Pull Request #47018 · pingcap/tidb

JK1Zhang · 2023-09-16T08:38:09Z

What problem does this PR solve?

Issue Number: close #47017

Problem Summary:

What is changed and how it works?

A line chart showing the change in the number of waiting/running/cancelling/reverting tasks over time
A line chart showing the waiting time of waiting tasks
A line chart showing the dispatch time of dispatching tasks

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test

Manual test

1. start TiDB in playground

cd tidb
make
tiup playground  --db 1 --db.binpath ./bin/tidb-server

2. test with mysql

mysql --comments --host 127.0.0.1 --port 4000 -u root

test cmd

drop database if exists test;
create database test;
use test;
set global tidb_enable_dist_task=1;
create table t(a bigint auto_random primary key) partition by hash(a) partitions 8;
insert into t values (), (), (), (), (), ();
insert into t values (), (), (), (), (), ();
insert into t values (), (), (), (), (), ();
insert into t values (), (), (), (), (), ();
create table t1(a bigint auto_random primary key);
insert into t1 values (), (), (), (), (), ();
insert into t1 values (), (), (), (), (), ();
insert into t1 values (), (), (), (), (), ();
insert into t1 values (), (), (), (), (), ();
create table t2(a bigint auto_random primary key) partition by hash(a) partitions 8;
insert into t2 values (), (), (), (), (), ();
insert into t2 values (), (), (), (), (), ();
insert into t2 values (), (), (), (), (), ();
insert into t2 values (), (), (), (), (), ();
create table t3(a bigint auto_random primary key);
insert into t3 values (), (), (), (), (), ();
insert into t3 values (), (), (), (), (), ();
insert into t3 values (), (), (), (), (), ();
insert into t3 values (), (), (), (), (), ();
split table t between (3) and (8646911284551352360) regions 50;
alter table t add index idx(a);
admin check index t idx;
split table t1 between (3) and (8646911284551352360) regions 50;
alter table t1 add index idx(a);
admin check index t1 idx;
split table t2 between (3) and (8646911284551352360) regions 50;
alter table t2 add index idx(a);
admin check index t idx;
split table t3 between (3) and (8646911284551352360) regions 50;
alter table t3 add index idx(a);
admin check index t3 idx;

3. grafana result

calculate in grafana (dispatching is happening too fast to see the data points )
panel "task dispatching time" metrics: "time() - tidb_disttask_dispatcher_start_time{status="dispatching"}/1000000"

calculate in dispatcher_manager.go
panel "task dispatching duration" metrics: tidb_disttask_dispatcher_duration{status="dispatching"}
Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

update

ti-chi-bot · 2023-09-16T08:38:19Z

Hi @JK1Zhang. Thanks for your PR.

I'm waiting for a pingcap 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/test-infra repository.

tiprow · 2023-09-16T08:38:35Z

Hi @JK1Zhang. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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

okJiang

need more enhancement

Co-authored-by: okJiang <jiangxianjie@pingcap.com>

okJiang · 2023-09-21T10:10:24Z

-			}
-
 			// TODO: Consider getting these tasks, in addition to the task being worked on..
 			tasks, err := dm.taskMgr.GetGlobalTasksInStates(proto.TaskStatePending, proto.TaskStateRunning, proto.TaskStateReverting, proto.TaskStateCancelling)


How about initializing the metrics with these tasks when the DispatcherManager starts?

It is precise.

For DistDDLTaskStarttimeGauge, I think it's not necessary to initialize here because it doesn't change.
For DistDDLTaskGauge, should I go through all the tasks in the tidb_global_task table?

ywqzzy · 2023-09-22T03:03:00Z

/cc @ywqzzy

ywqzzy

LGTM

Co-authored-by: EasonBall <592838129@qq.com>

D3Hunter

rest lgtm

D3Hunter · 2023-09-25T11:22:36Z

+
+// UpdateMetricsForDisptchTask update metrics when a task is added
+func UpdateMetricsForDisptchTask(task *proto.Task) {
+	DistTaskGauge.WithLabelValues(task.Type, WaitingStatus).Set(float64(300))


what does 300 means?

useless , deleted

Co-authored-by: EasonBall <592838129@qq.com>

Co-authored-by: D3Hunter <jujj603@gmail.com>

Benjamin2037

LGTM

ti-chi-bot · 2023-09-25T13:57:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, okJiang

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~~ [okJiang]

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 · 2023-09-25T13:57:38Z

[LGTM Timeline notifier]

Timeline:

2023-09-25 13:44:40.795615584 +0000 UTC m=+282470.513957800: ☑️ agreed by Benjamin2037.
2023-09-25 13:57:37.628290687 +0000 UTC m=+283247.346632904: ☑️ agreed by okJiang.

JK1Zhang and others added 5 commits September 11, 2023 13:59

Merge pull request #1 from pingcap/master

812e0d8

update

add DistTaskDispatcherGauge

7c8808b

add duration

98a0c25

duration

9fc809c

add metrics/disttask.go

80c0eff

ti-chi-bot Bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 16, 2023

ti-chi-bot Bot added the needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. label Sep 16, 2023

okJiang changed the title ~~disttask: add metrics collection for dispatcher~~ [WIP]disttask: add metrics collection for dispatcher Sep 18, 2023

ti-chi-bot Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2023

okJiang reviewed Sep 18, 2023

View reviewed changes

JK1Zhang and others added 5 commits September 18, 2023 14:47

Update disttask/framework/dispatcher/dispatcher_manager.go

bbe7f07

Co-authored-by: okJiang <jiangxianjie@pingcap.com>

Update metrics/disttask.go

75ab579

Co-authored-by: okJiang <jiangxianjie@pingcap.com>

resolve all comments

2e9f3b0

add json

a39c10c

add json

fa240db

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 Sep 20, 2023

add json

caeb50b

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. do-not-merge/needs-tests-checked labels Sep 21, 2023

okJiang reviewed Sep 21, 2023

View reviewed changes

resolve comments

374420e

ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 22, 2023

JK1Zhang and others added 9 commits September 22, 2023 15:56

add deps

4c45da7

add comments

922626c

add comments

17ffb08

fix bazel

8d47ca2

fix bazel

ee932fc

add label for expr in grafana

f895c82

fix conflicts

aa9e7ac

Merge branch 'master' into metrics-dev

2552616

fix conflicts

f491a41

ywqzzy reviewed Sep 25, 2023

View reviewed changes

Comment thread metrics/grafana/tidb.json Outdated

ywqzzy reviewed Sep 25, 2023

View reviewed changes

Update metrics/grafana/tidb.json

1f1e002

Co-authored-by: EasonBall <592838129@qq.com>

D3Hunter reviewed Sep 25, 2023

View reviewed changes

ywqzzy reviewed Sep 25, 2023

View reviewed changes

Comment thread metrics/grafana/tidb.json Outdated

JK1Zhang and others added 4 commits September 25, 2023 21:07

Update metrics/grafana/tidb.json

ce76c75

Co-authored-by: EasonBall <592838129@qq.com>

Update metrics/disttask.go

3a8b8c8

Co-authored-by: D3Hunter <jujj603@gmail.com>

Update metrics/grafana/tidb.json

68ca7a0

Co-authored-by: D3Hunter <jujj603@gmail.com>

Update disttask.go

fee895d

okJiang reviewed Sep 25, 2023

View reviewed changes

Comment thread metrics/grafana/tidb.json Outdated

JK1Zhang added 2 commits September 25, 2023 21:27

merge dispatcher row into Dist Execute Framework

5560887

a typo

9411a4d

Benjamin2037 approved these changes Sep 25, 2023

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 25, 2023

okJiang approved these changes Sep 25, 2023

View reviewed changes

ti-chi-bot Bot added the lgtm label Sep 25, 2023

ti-chi-bot Bot added approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 25, 2023

ti-chi-bot Bot merged commit 84fe6be into pingcap:master Sep 25, 2023

Conversation

JK1Zhang commented Sep 16, 2023 • edited by okJiang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Manual test

1. start TiDB in playground

2. test with mysql

3. grafana result

Release note

Uh oh!

ti-chi-bot Bot commented Sep 16, 2023

Uh oh!

tiprow Bot commented Sep 16, 2023

Uh oh!

okJiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

okJiang Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

okJiang Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

JK1Zhang Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ywqzzy commented Sep 22, 2023

Uh oh!

Uh oh!

ywqzzy left a comment

Choose a reason for hiding this comment

Uh oh!

D3Hunter left a comment

Choose a reason for hiding this comment

Uh oh!

D3Hunter Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

JK1Zhang Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Benjamin2037 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Sep 25, 2023

Uh oh!

ti-chi-bot Bot commented Sep 25, 2023

[LGTM Timeline notifier]

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

JK1Zhang commented Sep 16, 2023 •

edited by okJiang

Loading