topsql: introduce pub/sub mode by zhongzc · Pull Request #29020 · pingcap/tidb

zhongzc · 2021-10-21T09:34:34Z

What problem does this PR solve?

Make the diagnostic node dependent on TiDB instead of TiDB dependent on the diagnostic node to reverse the dependency to a normal form.

Depends on: pingcap/tipb#242
Related: tikv/tikv#11087

What is changed and how it works?

Global variable tidb_enable_top_sql doesn't affect whether to enable topsql or not on current instance directly.
Provide gRPC service tipb.TopSQLPubSubServer
- Clients can subscribe TopSQL data through this service
Modify interface ReportClient
- Rename to DataSink
- Add two methods: IsPaused, IsDown. Reporter uses them to enable/disable TopSQL.
- Add a new implementation pubSubDataSink for connecting the reporter and external subscribers

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory

Release note

Add TopSQL pub/sub gRPC service for publishing TopSQL data

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

ti-chi-bot · 2021-10-21T09:34:35Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

Details

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

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

zhongzc · 2021-10-26T04:57:43Z

/cc @crazycs520 @mornyx @breeswish

ti-chi-bot · 2021-10-26T04:57:46Z

@zhongzc: GitHub didn't allow me to request PR reviews from the following users: mornyx.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @crazycs520 @mornyx @breeswish

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.

sessionctx/variable/sysvar.go

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

util/topsql/topsql.go

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

zhongzc · 2021-12-10T13:00:56Z

/run-unit-test

sessionctx/variable/sysvar.go

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

util/topsql/reporter/reporter.go

Co-authored-by: crazycs <crazycs520@gmail.com>

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

breezewish · 2021-12-13T05:45:42Z

executor/main_test.go

 		goleak.IgnoreTopFunction("gopkg.in/natefinch/lumberjack%2ev2.(*Logger).millRun"),
+		goleak.IgnoreTopFunction("time.Sleep"),
+		goleak.IgnoreTopFunction("runtime/pprof.readProfile"),
+		goleak.IgnoreTopFunction("github.com/pingcap/tidb/util/topsql/tracecpu.(*sqlCPUProfiler).startAnalyzeProfileWorker"),


I don't think it may not be accepted to let this function leak.

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

breezewish · 2021-12-13T05:50:34Z

util/topsql/tracecpu/profile.go

+	// EnablePProfSQLCPU is true means want to keep the `sql` label. Otherwise, remove the `sql` label.
+	EnablePProfSQLCPU = atomic.NewBool(false)
+
+	// PrecisionSeconds indicates profile interval.
+	PrecisionSeconds = atomic.NewInt64(DefPrecisionSeconds)


They should be a config of sqlCPUProfiler I think, from this module's perspective.

breezewish · 2021-12-13T05:53:51Z

util/topsql/reporter/pubsub.go

+			}
+		}, nil)
+
+		<-ctx.Done()


If you are waiting goroutine to finish, why not execute it in current goroutine directly?

That spawned goroutine may block when the deadline is exceeded, in that case, this datasink should be closed.

Got it. You can add a comment to document the reason.

breezewish · 2021-12-13T05:57:30Z

util/topsql/tracecpu/profile.go

-func (sp *sqlCPUProfiler) IsEnabled() bool {
-	return variable.TopSQLEnabled() || sp.hasExportProfileTask()
+// ShouldProfile return true if it's required to profile. It exports for tests.
+func (sp *sqlCPUProfiler) ShouldProfile() bool {


Why it is not checking the enabling state?

Which enabling state?

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

ti-chi-bot · 2021-12-14T08:29:11Z

@zhongzc: PR needs rebase.

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.

zhongzc added 3 commits October 20, 2021 14:46

register pubusb

b464ff4

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

run fine

ab68337

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

globally enabled not to affect instance level enabled directly

8d6b03e

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 21, 2021

zhongzc added 3 commits October 21, 2021 17:37

tiny fix

38fccf9

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

add license

6d4a115

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

add tests & add comment

91d7143

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

ti-chi-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. labels Oct 25, 2021

zhongzc added 4 commits October 25, 2021 20:49

fix tests

8f2eae6

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

fix tests

b75478e

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

fix

30201ec

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

update tipb

9c3f723

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

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

zhongzc marked this pull request as ready for review October 26, 2021 03:32

zhongzc requested a review from a team as a code owner October 26, 2021 03:32

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 Oct 26, 2021

Merge remote-tracking branch 'origin/master' into pubsub

ba167c3

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 26, 2021

fix test

a6aa4f8

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

ti-chi-bot requested review from breezewish and crazycs520 October 26, 2021 04:57

zhongzc commented Oct 26, 2021

View reviewed changes

sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved

zhongzc added 4 commits December 10, 2021 17:40

fix unbuffered

3b72c97

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

fix InstanceEnabled

df42ffe

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

Merge branch 'master' into pubsub

7b813ff

fix init NewRemoteTopSQLReporter

2a7111f

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

mornyx reviewed Dec 10, 2021

View reviewed changes

util/topsql/topsql.go Show resolved Hide resolved

use globalTopSQLReport to judge topsql enabled

127c72e

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

zhongzc added 2 commits December 10, 2021 21:11

Merge branch 'master' into pubsub

95c7d27

Merge branch 'master' into pubsub

947ae62

crazycs520 reviewed Dec 13, 2021

View reviewed changes

sessionctx/variable/sysvar.go Show resolved Hide resolved

remove redundant EnablePProfSQLCPU

04e6a5a

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

crazycs520 reviewed Dec 13, 2021

View reviewed changes

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

crazycs520 reviewed Dec 13, 2021

View reviewed changes

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

crazycs520 reviewed Dec 13, 2021

View reviewed changes

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

zhongzc and others added 2 commits December 13, 2021 13:01

Update util/topsql/reporter/reporter.go

0ce9795

Co-authored-by: crazycs <crazycs520@gmail.com>

refactor

cc4cef3

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

breezewish reviewed Dec 13, 2021

View reviewed changes

move tracecpu out of topsql

c8a6ea5

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

breezewish reviewed Dec 13, 2021

View reviewed changes

polish

54c34e0

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>

zhongzc mentioned this pull request Dec 13, 2021

topsql: introduce datasink interface #30662

Merged

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

zhongzc closed this Dec 21, 2021

zhongzc deleted the pubsub branch December 21, 2021 09:49

Conversation

zhongzc commented Oct 21, 2021 • edited 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

Release note

Uh oh!

ti-chi-bot commented Oct 21, 2021

Uh oh!

zhongzc commented Oct 26, 2021

Uh oh!

ti-chi-bot commented Oct 26, 2021

Uh oh!

Uh oh!

Uh oh!

zhongzc commented Dec 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

breezewish Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breezewish Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

breezewish Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

zhongzc Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breezewish Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

breezewish Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

zhongzc Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhongzc commented Oct 21, 2021 •

edited

Loading

breezewish Dec 13, 2021 •

edited

Loading

zhongzc Dec 13, 2021 •

edited

Loading