Skip to content

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

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
asubiotto:cont
Dec 1, 2020
Merged

sql: produce mock ContentionEvents and display contention time in EXPLAIN ANALYZE#56906
craig[bot] merged 2 commits intocockroachdb:masterfrom
asubiotto:cont

Conversation

@asubiotto
Copy link
Copy Markdown
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 a team, RaduBerinde and tbg November 19, 2020 15:21
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

TFTR

bors r=RaduBerinde,tbg

@craig
Copy link
Copy Markdown
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