Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: produce mock ContentionEvents and display contention time in EXPLAIN ANALYZE #56906

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

asubiotto
Copy link
Contributor

Please take a look at individual commits for details

Release note: None

Closes #56612

@tbg could you take a look at the first commit which defines a ContentionEvent protobuf? It's close to what's described in #55583 (minus some fields that can be added later).

@asubiotto asubiotto requested review from tbg, RaduBerinde and a team November 19, 2020 15:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First commit looks good!

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: This is very cool!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @RaduBerinde)


pkg/sql/colexec/op_creation.go, line 81 at r2 (raw file):

type NewColOperatorResult struct {
	Op              colexecbase.Operator
	KVReader        execinfra.KVReader

Nice change :)


pkg/sql/colfetcher/colbatch_scan.go, line 135 at r2 (raw file):

// GetCumulativeContentionTime is part of the execinfra.KVReader interface.
func (s *ColBatchScan) GetCumulativeContentionTime() time.Duration {
	var totalContentionTime time.Duration

I found that sometimes (maybe in error cases, or other early termination situation), s.rf.fetcher might be nil here. I'm adding a check in GetBytesRead in 56929, we should check here too.


pkg/sql/execinfrapb/component_stats.proto, line 92 at r2 (raw file):

  // ContentionTime is the cumulative time a KV request spent contending with
  // other transactions. This time is a subset of KVTime above.

[nit] s/is a subset/accounts for a portion of

@asubiotto
Copy link
Contributor Author

TFTRs! The CI failure should be fixed by #56951. Will wait for that to rebase, address comments, and update this PR.

@asubiotto asubiotto requested a review from a team as a code owner November 30, 2020 15:28
This commit adds a simple ContentionEvent protobuf. This protobuf is not yet
emitted by KV although the idea is that it will be. In the meantime, the SQL
Execution engine can use this message to generate mock contention events in
order to build higher-level observability infrastructure around contention.

Release note: None
This commit also implements mock contention event generation when either a
testing knob or cluster setting is set. The former is useful for programmable
tests, while the latter will be useful for observing changes to the DB console
(e.g. global contention view).

Release note: None (new behavior is only enabled in tests)
@asubiotto
Copy link
Contributor Author

TFTR

bors r=RaduBerinde,tbg

@craig
Copy link
Contributor

craig bot commented Dec 1, 2020

Build succeeded:

@craig craig bot merged commit 5faa952 into cockroachdb:master Dec 1, 2020
@asubiotto asubiotto deleted the cont branch December 1, 2020 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rowexec/colexec: add cumulative contention time to EXPLAIN ANALYZE
4 participants