topsql: introduce stmtstats and sql execution count

[mornyx]

What is changed and how it works?

• Introduce package topsql/stmtstats for count various indicators of statement execution.
• Implement SQL Execution Count based on topsql/stmtstats.

Note:

• This PR only includes the collection and aggregation of stmtstats, the code related to the report will be proposed as a separate PR when topsql: introduce pub/sub mode #29020 merged.

Related PR

• introduce RPC interceptor mechanism tikv/client-go#389

Check List

Tests

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

Side effects

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

Release note

None

[ti-chi-bot]

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

[mornyx]

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

[crazycs520]

LGTM

[breezewish]

No logic problems. Optional advices given.

[breezewish]

Please unify it to call it GetSQLDigest()

[mornyx]

In fact, there is no GetSQLDigest() method. The difference between XxxDigest() and GetXxxDigest() is that GetXxxDigest() only returns internal fields, while XxxDigest() comes with some initialization by sync.Once.

[breezewish]

You can try to further craft the digest part in the future PRs by moving this into parser (which could be a refactor touching a lot of code base):

// in parser:

// note: this definition makes it possible to be used directly as a map key
// while still must be created explicitly using `DigestFromHash`.
type Digest struct {
	binaryHash string
}

func DigestFromHash(hash []byte) Digest {
    // ...
}

func (d Digest) HexString() string {
    // ...
}

func (d Digest) BinaryBytes() []byte {
    // ...
}

As a reference, the current code base is:

type Digest struct {
	b   []byte
	str string
}

func NewDigest(b []byte) *Digest {
	return &Digest{
		b:   b,
		str: hex.EncodeToString(b),
	}
}

where you can see there are many improvement possibilities.

cc @crazycs520

[breezewish]

Generally I think there are other possible improvements for the current public API design and concepts. For example, there are many "Collector" now, and it would be better to use some different and more precise naming to reduce understanding difficulty. Also, "register an StatementStats" is something purely internal. There are possibilities to polish the API surface (comments and names) to make it a "feature mechanism", like (just an example):

NewStatementStats creates a new StatementStats. At any time, you can query and aggregate all StatementStats that has not been marked as finished by using a StatsGatherer.

More over, by consulting the Prometheus design, maybe a design like this could be possible as well:

var DefaultStatsRegistry ...
var DefaultStatsGatherer ...

func NewStats() ...
func MustRegister() ...

Anyway, the current design is great. Looking forward to see some design improvements in future PRs if you like!

[breezewish]

It's good that this easy-to-happen mistake is explicitly warned.. But is it possible to avoid making this mistake at all by changing some internal implementations? For example, by reducing some pointers? Or by making some copies? Or by clearing some merged data?

[mornyx]

I think this looks ok, we only have one place to use Merge:

for stat in stats {
     total.Merge(stat.Take())
}

And some code will rely on pointer behavior instead of value copy.

[breezewish]

How about making this Take logic by default? In this way you can prevent future developers (or even yourself) to make mistakes.

[mornyx]

What is Making this Take logic by default? Do you mean that we no longer provide a public Merge method, and only merge internally in the place of stats.Take() to avoid future misuse?

[mornyx]

This PR will be merged soon, but we can continue to discuss here, this point can be optimized in the next PR.

[crazycs520]

/merge

[ti-chi-bot]

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

Commit hash: d6dcca9

[crazycs520]

/merge

[mornyx]

/run-all-tests

[mornyx]

/run-check_dev_2

[crazycs520]

/merge

[ti-chi-bot]

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

[mornyx]

/run-check_dev_2

[crazycs520]

/merge