topsql: refactor reporter and add reporting part of stmtstats #30954

mornyx · 2021-12-22T17:22:20Z

What is changed and how it works?

In this PR (#30277), we introduced the stmtstats framework, but the reporting part is missing. This PR completes this part by:

Refactor topsql/reporter for flexibility.
Implement stmtstats.Collector for reporter.RemoteTopSQLReporter.
Register reporter.RemoteTopSQLReporter as stmtstats.Collector.
Add reporting of stmtstats.

Related PR

topsql: introduce sql exec count and sql duration tipb#250

Check List

Tests

Unit test
Integration test

Release note

None

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot · 2021-12-22T17:22:22Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

breeswish
crazycs520

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.

Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx · 2021-12-23T14:35:35Z

/run-all-tests

mornyx · 2021-12-23T14:36:57Z

PTAL, Thanks~
/cc @crazycs520 @zhongzc @breeswish

util/topsql/reporter/reporter.go

breezewish

The current design is mixing the architecture and the business metrics. Could you make them isolated? A good design should be some architecture that, when we want to add a new metrics, reporter/* does not need any changes. Could you design for this direction?

Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx · 2021-12-26T13:34:12Z

The current design is mixing the architecture and the business metrics. Could you make them isolated? A good design should be some architecture that, when we want to add a new metrics, reporter/* does not need any changes. Could you design for this direction?

I have refactored the entire topsql/reporter to decouple the "data logic"(datamodel.go) and "report logic"(reporter.go). In the future, if we need to add new indicators, we only need to add the corresponding lines of code to the protobuf conversion part in datamodel.go without modifying reporter.go.

Signed-off-by: mornyx <mornyx.z@gmail.com>

sre-bot · 2021-12-26T13:37:51Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/114e53be775bc0e739cfc12e90891beab61cbb83

Signed-off-by: mornyx <mornyx.z@gmail.com>

util/topsql/reporter/datamodel.go

dianqihanwangzi · 2021-12-27T04:15:48Z

/run-check_dev_2

Signed-off-by: mornyx <mornyx.z@gmail.com>

util/topsql/reporter/datamodel.go

util/topsql/reporter/reporter.go

util/topsql/reporter/datasink.go

zhongzc · 2021-12-28T07:38:34Z

rest LGTM

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot · 2021-12-28T07:49:36Z

@zhongzc: 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:

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.

go.mod

util/topsql/reporter/reporter.go

util/topsql/reporter/datamodel.go

breezewish · 2021-12-28T14:59:16Z

util/topsql/reporter/datamodel.go

+		r.tsIndex[timestamp] = len(r.tsItems)
+		r.tsItems = append(r.tsItems, newItem)
+	}
+	r.totalCPUTimeMs += uint64(cpuTimeMs)


This calculation may be incorrect when duplicated ts is met, as you will replace the value instead of add the value in branch L183.

Replacement occurs only when the original value is zero.

I still prefer to do a correct calculation here, since this function is indeed calculated incorrectly, it's just the caller has not entered this path yet. The overall result is fine only because the bug is not triggered, but the bug exists.

You have two ways to make this function correct:

It only accepts the call and update the total when current value=0. For other cases, the array is not overwritten, and total is not updated.

It accepts all calls and always update the total according to a delta of the current value.

Your opinion is reasonable, I changed the "replace" behavior to "add/merge" behavior, so method names are changed back to appendXxx.

util/topsql/reporter/datamodel.go

Signed-off-by: mornyx <mornyx.z@gmail.com>

breezewish

the rest LGTM

util/topsql/reporter/datamodel.go

Signed-off-by: mornyx <mornyx.z@gmail.com>

crazycs520

LGTM

crazycs520 · 2021-12-29T05:09:52Z

/merge

ti-chi-bot · 2021-12-29T05:09:55Z

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

Commit hash: c0524ef

mornyx · 2021-12-29T05:38:45Z

/run-check_dev_2

mornyx · 2021-12-29T06:01:58Z

/run-check_dev_2

zhongzc · 2021-12-29T06:39:50Z

/run-mysql-test

mornyx added 2 commits December 23, 2021 01:11

Add reporting for sql exec count and sql duration

8e99bd7

Signed-off-by: mornyx <mornyx.z@gmail.com>

Replace go.mod

8eaada2

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot added 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. labels Dec 22, 2021

Upgrade tipb

3381e6c

Signed-off-by: mornyx <mornyx.z@gmail.com>

ti-chi-bot requested review from breezewish, crazycs520 and zhongzc December 23, 2021 14:36

breezewish reviewed Dec 23, 2021

View reviewed changes

util/topsql/reporter/reporter.go Outdated Show resolved Hide resolved

util/topsql/reporter/reporter.go Outdated Show resolved Hide resolved

util/topsql/reporter/reporter.go Outdated Show resolved Hide resolved

breezewish reviewed Dec 23, 2021

View reviewed changes

Refactor topsql/reporter

8113667

Signed-off-by: mornyx <mornyx.z@gmail.com>

mornyx changed the title ~~topsql: add reporting part of stmtstats~~ topsql: refactor reporter and add reporting part of stmtstats Dec 26, 2021

Add comment

435b3da

Signed-off-by: mornyx <mornyx.z@gmail.com>

Fix implicit memory aliasing in for loop

ec2e155

Signed-off-by: mornyx <mornyx.z@gmail.com>

breezewish reviewed Dec 27, 2021

View reviewed changes

util/topsql/reporter/datamodel.go Outdated Show resolved Hide resolved

mornyx added 2 commits December 27, 2021 12:23

Upgrade tipb, use TopSQLRecordItem list

abf988c

Signed-off-by: mornyx <mornyx.z@gmail.com>

Get timestamp in reporter instead of pass; Upgrade tipb

e080499

Signed-off-by: mornyx <mornyx.z@gmail.com>

zhongzc reviewed Dec 28, 2021

View reviewed changes

util/topsql/reporter/datamodel.go Outdated Show resolved Hide resolved

zhongzc reviewed Dec 28, 2021

View reviewed changes

util/topsql/reporter/reporter.go Outdated Show resolved Hide resolved

zhongzc reviewed Dec 28, 2021

View reviewed changes

util/topsql/reporter/reporter.go Outdated Show resolved Hide resolved

zhongzc reviewed Dec 28, 2021

View reviewed changes

util/topsql/reporter/datasink.go Outdated Show resolved Hide resolved

mornyx added 2 commits December 28, 2021 15:45

Resolve comments

620f10c

Signed-off-by: mornyx <mornyx.z@gmail.com>

Avoid sort

baef9d6

Signed-off-by: mornyx <mornyx.z@gmail.com>

zhongzc approved these changes Dec 28, 2021

View reviewed changes

Merge branch 'master' into stmtstats

c8b24dd

breezewish reviewed Dec 28, 2021

View reviewed changes

mornyx added 3 commits December 29, 2021 01:08

Resolve comments

7fb1c80

Signed-off-by: mornyx <mornyx.z@gmail.com>

Remove useless test

93824c1

Signed-off-by: mornyx <mornyx.z@gmail.com>

Use add or merge instead of replace

36a0b56

Signed-off-by: mornyx <mornyx.z@gmail.com>

breezewish approved these changes Dec 29, 2021

View reviewed changes

util/topsql/reporter/datamodel.go Show resolved Hide resolved

util/topsql/reporter/datamodel.go Show resolved Hide resolved

util/topsql/reporter/datamodel.go Show resolved Hide resolved

util/topsql/reporter/datamodel.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 29, 2021

mornyx added 3 commits December 29, 2021 11:33

Resolve

32d746c

Signed-off-by: mornyx <mornyx.z@gmail.com>

Merge branch 'master' into stmtstats

4854b97

Rename test

2af4950

Signed-off-by: mornyx <mornyx.z@gmail.com>

crazycs520 approved these changes Dec 29, 2021

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 Dec 29, 2021

Merge branch 'master' into stmtstats

c0524ef

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 29, 2021

Merge branch 'master' into stmtstats

114e53b

ti-chi-bot merged commit 7e2d12c into pingcap:master Dec 29, 2021

mornyx deleted the stmtstats branch December 29, 2021 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

topsql: refactor reporter and add reporting part of stmtstats #30954

topsql: refactor reporter and add reporting part of stmtstats #30954

mornyx commented Dec 22, 2021 •

edited

Loading

ti-chi-bot commented Dec 22, 2021 •

edited

Loading

mornyx commented Dec 23, 2021

mornyx commented Dec 23, 2021

breezewish left a comment

mornyx commented Dec 26, 2021

sre-bot commented Dec 26, 2021 •

edited

Loading

dianqihanwangzi commented Dec 27, 2021

zhongzc commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

breezewish Dec 28, 2021 •

edited

Loading

mornyx Dec 28, 2021

breezewish Dec 29, 2021 •

edited

Loading

mornyx Dec 29, 2021 •

edited

Loading

breezewish left a comment

crazycs520 left a comment

crazycs520 commented Dec 29, 2021

ti-chi-bot commented Dec 29, 2021

mornyx commented Dec 29, 2021

mornyx commented Dec 29, 2021

zhongzc commented Dec 29, 2021

topsql: refactor reporter and add reporting part of stmtstats #30954

topsql: refactor reporter and add reporting part of stmtstats #30954

Conversation

mornyx commented Dec 22, 2021 • edited Loading

What is changed and how it works?

Related PR

Check List

Release note

ti-chi-bot commented Dec 22, 2021 • edited Loading

mornyx commented Dec 23, 2021

mornyx commented Dec 23, 2021

breezewish left a comment

Choose a reason for hiding this comment

mornyx commented Dec 26, 2021

sre-bot commented Dec 26, 2021 • edited Loading

dianqihanwangzi commented Dec 27, 2021

zhongzc commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

breezewish Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

mornyx Dec 28, 2021

Choose a reason for hiding this comment

breezewish Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

mornyx Dec 29, 2021 • edited Loading

Choose a reason for hiding this comment

breezewish left a comment

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

crazycs520 commented Dec 29, 2021

ti-chi-bot commented Dec 29, 2021

mornyx commented Dec 29, 2021

mornyx commented Dec 29, 2021

zhongzc commented Dec 29, 2021

mornyx commented Dec 22, 2021 •

edited

Loading

ti-chi-bot commented Dec 22, 2021 •

edited

Loading

sre-bot commented Dec 26, 2021 •

edited

Loading

breezewish Dec 28, 2021 •

edited

Loading

breezewish Dec 29, 2021 •

edited

Loading

mornyx Dec 29, 2021 •

edited

Loading