Very nice code!

One thing is that we could make this more simple/efficient if we don't keep track of individual keys. If there's a chance we won't actually ever show the keys, we should decide now.. (CC @jordanlewis)

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

pkg/sql/contention/registry.go, line 89 at r2 (raw file):

	// this index have spent contended.
	cumulativeContentionTime time.Duration
	// orderedKeyMap is an LRU cache that keeps track of up to

LRU is questionable here.. Should we keep the "top" txns (in terms of the total duration), rather than LRU? (can be a TODO)

pkg/sql/contention/registry.go, line 168 at r2 (raw file):

}

// Registry is an object that keeps track of aggregated contention information.

[nit] consider moving the struct to the top of the file, to serve as a kind of file-level doc.

pkg/sql/contention/registry.go, line 227 at r2 (raw file):

		writeChild(prefix, fmt.Sprintf("cumulative contention time: %s\n", v.cumulativeContentionTime))
		writeChild(prefix, fmt.Sprintf("keys:\n"))
		keyPrefix := prefix + "\t"

[nit] Tabs can lead to inconsistent alignment, why not just spaces?

pkg/sql/contention/testdata/contention_registry, line 4 at r2 (raw file):

----

evcheck tableid=1 indexid=1 key=key id=a duration=1

Very cool tests!

TFTRs!

One thing is that we could make this more simple/efficient if we don't keep track of individual keys. If there's a chance we won't actually ever show the keys, we should decide now..

Agreed cc @awoods187 @Annebirzin , what do you think? Should we keep track of individual keys for contention or do we prefer to just show the index for privacy?

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

pkg/sql/contention/registry.go, line 89 at r2 (raw file):

I chose LRU because I think it's more important to show a current contention view rather than the largest contention problems since contention began. My thinking is that a user cares a lot more about what the current largest hotspot is. If we were to evict based on the shortest duration txns, we could get in a state where we had an index with a ton of contention, the contention problem was fixed, but the index still remains in the contention registry and won't be evicted until relatively more contention happens on many other keys.

pkg/sql/contention/registry.go, line 227 at r2 (raw file):

Done.

pkg/sql/contention/testdata/contention_registry, line 4 at r2 (raw file):

Thanks!

The values seem reasonable. We will probably need to do some benchmarking at some point to understand impact.

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

pkg/sql/contention/registry.go, line 89 at r2 (raw file):

That makes sense, not sure what I was thinking (I might have thought about it as if it was a per-statement structure, but even there LRU makes more sense).