client: KV requests with source label #34833

you06 · 2022-05-19T10:43:19Z

Signed-off-by: you06 [email protected]

What problem does this PR solve?

Issue Number: close #33963

Problem Summary:

What is changed and how it works?

Add request source when sending KV request, the source tag includes the scope and type of the statement.

Record metrics by request type and source.

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

Documentation

Release note

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

Add source labels for KV requests.

ti-chi-bot · 2022-05-19T10:43:21Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

cfzjywxk
sticnarf

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.

you06 · 2022-06-10T09:23:15Z

/run-unit-test

you06 · 2022-06-10T12:03:49Z

/run-unit-test

you06 · 2022-06-16T15:46:49Z

/run-all-tests

you06 · 2022-06-16T16:28:43Z

/run-all-tests

sre-bot · 2022-06-19T20:39:57Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/706ec0e80b30957e21ce5bc2f8c56931377a689b

cfzjywxk · 2022-06-25T01:59:19Z

@you06
Please resolve the conflicts.

you06 · 2022-06-27T12:01:22Z

@sticnarf @cfzjywxk PTAL

ddl/ddl_api.go

sticnarf · 2022-06-30T08:22:58Z

executor/grant.go

@@ -132,15 +134,15 @@ func (e *GrantExec) Next(ctx context.Context, req *chunk.Chunk) error {
 	}
 	defer func() {
 		if !isCommit {
-			_, err := internalSession.(sqlexec.SQLExecutor).ExecuteInternal(context.Background(), "rollback")
+			_, err := internalSession.(sqlexec.SQLExecutor).ExecuteInternal(ctx, "rollback")


I'm not sure if it's ok to change these background ctx to the parent ctx... Could the original intention be just allowing it even if the parent ctx is done?

Emmm, it seems many RunInNewTxn and ExecuteInternal use context in a messy way(if there is no context in the parameters, create a new one), creating a new context in many places is redundant. Anyway, all the modifies have been checked for compatibility.

sticnarf · 2022-06-30T09:57:11Z

executor/inspection_result.go

@@ -117,6 +118,7 @@ func (e *inspectionResultRetriever) retrieve(ctx context.Context, sctx sessionct
 	}
 	e.retrieved = true

+	ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnMeta)


I'm not sure whether memtable related things are always internal... If a user reads memory tables, will it come here?

But they call ExecRestrictedSQL which indicates internal usages... If they don't send requests the request source of it won't be used.

sticnarf · 2022-06-30T13:06:38Z

executor/metrics_reader.go

@@ -204,6 +205,7 @@ func (e *MetricsSummaryRetriever) retrieve(ctx context.Context, sctx sessionctx.
 	}
 	sort.Strings(tables)

+	ctx = kv.WithInternalSourceType(ctx, kv.InternalTxnOthers)


I have questions here similar to retrieving memory table...

sticnarf

LGTM

you06 · 2022-07-01T08:24:10Z

Need to update the panel ids after #34845 got merged.

you06 · 2022-07-01T09:14:02Z

@cfzjywxk @longfangsong @zyguan PTAL

cfzjywxk · 2022-07-01T09:51:45Z

/merge

ti-chi-bot · 2022-07-01T09:51:49Z

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

Commit hash: 4eaae7a40226ce720ef93f633fece4905c8deac5

Signed-off-by: you06 <[email protected]> fix tests Signed-off-by: you06 <[email protected]> fix test Signed-off-by: you06 <[email protected]> update Signed-off-by: you06 <[email protected]> update bazel Signed-off-by: you06 <[email protected]> fix test Signed-off-by: you06 <[email protected]> fix test Signed-off-by: you06 <[email protected]> update Signed-off-by: you06 <[email protected]> update client-go Signed-off-by: you06 <[email protected]> update client-go Signed-off-by: you06 <[email protected]> update grafana Signed-off-by: you06 <[email protected]> update bazel Signed-off-by: you06 <[email protected]> display by stack Signed-off-by: you06 <[email protected]> set request source for ctx of Signed-off-by: you06 <[email protected]> address comment Signed-off-by: you06 <[email protected]> address comment Signed-off-by: you06 <[email protected]> parser, ddl: support decoding binary literal in set/enum (pingcap#35822) close pingcap#31338 executor: fix a nil point when @@tidb_enable_collect_execution_info is off and cop cache is on (pingcap#35839) close pingcap#35829 ddl: support drop columns for multi-schema change (pingcap#35737) ref pingcap#14766 planner: don't push down predicates over table dual (pingcap#35669) close pingcap#35527 *: Remove some useless staleness code (pingcap#35849) close pingcap#35848 ddl: fix potential wrong column definition when setting default value (pingcap#35845) close pingcap#35846 *: add timeout for test cases (pingcap#35844) ddl: remove `onDropColumns` and `onAddColumns` (pingcap#35862) ref pingcap#14766 *: Fix Dockerfile (pingcap#35835) close pingcap#35828 server: refactor TestConnExecutionTimeout (pingcap#35865) close pingcap#35868 fix test Signed-off-by: you06 <[email protected]> fix lint Signed-off-by: you06 <[email protected]>

Signed-off-by: you06 <[email protected]>

cfzjywxk · 2022-07-01T11:25:27Z

/merge

ti-chi-bot · 2022-07-01T11:25:31Z

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

Commit hash: 706ec0e

sre-bot · 2022-07-01T12:15:13Z

TiDB MergeCI notify

✅ Well Done! New fixed [2] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	✅ all 11 tests passed	18 min	Fixed
idc-jenkins-ci-tidb/common-test	✅ all 12 tests passed	9 min 43 sec	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 35 tests passed	24 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 42 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 14 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 4 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 27 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 3 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	2 min 52 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

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

you06 force-pushed the request-source branch from ae166ef to 0552825 Compare June 13, 2022 08:38

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 Jun 13, 2022

you06 force-pushed the request-source branch from 894e17c to 4e86b6b Compare June 15, 2022 05:22

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 Jun 15, 2022

you06 force-pushed the request-source branch from b645b63 to 51b263a Compare June 19, 2022 20:30

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2022

you06 marked this pull request as ready for review June 19, 2022 21:00

you06 requested a review from a team as a code owner June 19, 2022 21:00

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 19, 2022

cfzjywxk requested review from sticnarf, TonsnakeLin, longfangsong and cfzjywxk June 23, 2022 02:37

you06 force-pushed the request-source branch from b9a639b to 741ca2b Compare June 27, 2022 07:30

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2022

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

you06 force-pushed the request-source branch from d698f4a to 26dae22 Compare June 30, 2022 04:42

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2022

you06 force-pushed the request-source branch from 26dae22 to a078e4d Compare June 30, 2022 06:26

sticnarf reviewed Jun 30, 2022

View reviewed changes

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

you06 force-pushed the request-source branch from 2180283 to af7dd0e Compare July 1, 2022 06:48

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2022

sticnarf approved these changes Jul 1, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 1, 2022

you06 requested a review from zyguan July 1, 2022 09:07

cfzjywxk approved these changes Jul 1, 2022

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 Jul 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 1, 2022

you06 added 3 commits July 1, 2022 19:21

fix test

47d60da

Signed-off-by: you06 <[email protected]>

fix duplicated panel id

706ec0e

Signed-off-by: you06 <[email protected]>

you06 force-pushed the request-source branch from 4eaae7a to 706ec0e Compare July 1, 2022 11:22

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 1, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 1, 2022

ti-chi-bot merged commit 65e0b8e into pingcap:master Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: KV requests with source label #34833

client: KV requests with source label #34833

you06 commented May 19, 2022 •

edited

Loading

ti-chi-bot commented May 19, 2022 •

edited

Loading

you06 commented Jun 10, 2022

you06 commented Jun 10, 2022

you06 commented Jun 16, 2022

you06 commented Jun 16, 2022

sre-bot commented Jun 19, 2022 •

edited

Loading

cfzjywxk commented Jun 25, 2022

you06 commented Jun 27, 2022

sticnarf Jun 30, 2022

you06 Jun 30, 2022

sticnarf Jun 30, 2022

you06 Jun 30, 2022 •

edited

Loading

sticnarf Jun 30, 2022

sticnarf left a comment

you06 commented Jul 1, 2022

you06 commented Jul 1, 2022

cfzjywxk commented Jul 1, 2022

ti-chi-bot commented Jul 1, 2022

cfzjywxk commented Jul 1, 2022

ti-chi-bot commented Jul 1, 2022

sre-bot commented Jul 1, 2022

client: KV requests with source label #34833

client: KV requests with source label #34833

Conversation

you06 commented May 19, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 19, 2022 • edited Loading

you06 commented Jun 10, 2022

you06 commented Jun 10, 2022

you06 commented Jun 16, 2022

you06 commented Jun 16, 2022

sre-bot commented Jun 19, 2022 • edited Loading

cfzjywxk commented Jun 25, 2022

you06 commented Jun 27, 2022

sticnarf Jun 30, 2022

Choose a reason for hiding this comment

you06 Jun 30, 2022

Choose a reason for hiding this comment

sticnarf Jun 30, 2022

Choose a reason for hiding this comment

you06 Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

sticnarf Jun 30, 2022

Choose a reason for hiding this comment

sticnarf left a comment

Choose a reason for hiding this comment

you06 commented Jul 1, 2022

you06 commented Jul 1, 2022

cfzjywxk commented Jul 1, 2022

ti-chi-bot commented Jul 1, 2022

cfzjywxk commented Jul 1, 2022

ti-chi-bot commented Jul 1, 2022

sre-bot commented Jul 1, 2022

TiDB MergeCI notify

you06 commented May 19, 2022 •

edited

Loading

ti-chi-bot commented May 19, 2022 •

edited

Loading

sre-bot commented Jun 19, 2022 •

edited

Loading

you06 Jun 30, 2022 •

edited

Loading