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

kvserver: very rough prototype for storage load metrics #79031

Closed
wants to merge 1 commit into from

Conversation

tbg
Copy link
Member

@tbg tbg commented Mar 30, 2022

#65414 (comment)

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)


pkg/kv/kvserver/replica_evaluate.go, line 279 at r1 (raw file):

			ctx, readWriter, rec, ms, baHeader, args, reply, ui)
		{
			newWriteSize := writeBatchSize()

the cost of this should be low, but we could also conditionally call this if the command was a RW command.


pkg/kv/kvserver/replica_evaluate.go, line 305 at r1 (raw file):

				// TODO: do we want to expose an EWMA of these per-req breakdowns on the
				// (*Replica).State method? This could power a richer "hot ranges"
				// dashboard, where you could for example find ranges that are "hot for

+1 to per range stats.
Maybe showing (1min, 10min, 1h) aggregations on a hot ranges page per node.
I'm a fan of lightweight state inspection pages per node which show info in a tabular form (here it would have rangeid, range span, 1min rate, 10min rate, 1h rate) columns and one can choose to sort by any of the columns. It is super easy for a developer to add and doesn't require a more heavyweight project. This is basically inspectz pages and jstable in google-land #66772


pkg/storage/mvcc.go, line 2505 at r1 (raw file):

) (MVCCScanResult, error) {
	iter := newMVCCIterator(reader, timestamp.IsEmpty(), IterOptions{LowerBound: key, UpperBound: endKey})
	// TODO: is this better than KeyBytes+ValBytes?

maybe do both?
there is also a question about whether we should be counting (BlockBytes-BlockBytesInCache).
It may be too much to do metrics for each, but if we did the lightweight state inspection pages (as mentioned earlier), we could do all.


pkg/storage/mvcc.go, line 2517 at r1 (raw file):

	res, err := mvccScanToBytes(ctx, iter, key, endKey, timestamp, opts)
	res.ReadBytes = int64(iter.Stats().Stats.InternalStats.BlockBytes)
	res.ReadBytes++ // HACK: BlockBytes seems to always be zero at least in unit tests, maybe something about the in-mem engine?

possibly because we are not doing enough writes to flush the memtable. One can print engine.Metrics().Metrics.String() in a couple of tests to confirm.

@tbg tbg closed this Aug 8, 2022
@sumeerbhola
Copy link
Collaborator

@tbg I am curious why this was closed -- are granular "storage load metrics" subsumed by some other PR (also mentioned in #65414)?

@tbg
Copy link
Member Author

tbg commented Aug 10, 2022

I closed this because I am not planning to work on it anytime soon. #65414 is the best tracking issue that I know. I don't think anyone is planning to pick it up, though. I agree that there is a gap here and we should consider escalating whether there is something we can still do for 22.2. I'll bring it up with the KV/storage/KVObs folks.

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.

3 participants