Reviewed 5 of 9 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @irfansharif)

pkg/server/server.go line 222 at r1 (raw file):

		admissionOptions.Override(opts)
	}
	admissionOptions.Settings = st

why this change to pull out settings from the options?

pkg/ts/catalog/chart_catalog.go line 3473 at r1 (raw file):

					"admission.errored.kv.0",
					"admission.errored.kv.100",
					"admission.errored.sql-kv-response.0",

is this 0 and 100 the priority values?
I am wondering why we've picked these to populate here and not the others in the admissionpb.WorkPriority enum.

pkg/util/admission/grant_coordinator.go line 158 at r1 (raw file):

	// This is IO work, so override the usesTokens value.
	opts.usesTokens = true
	// TODO(sumeer): add per-store WorkQueue state for debug.zip and db console.

this revised code is also sharing the WorkQueue metrics across all stores, yes?
This would be a good thing to fix in a followup PR.

pkg/util/admission/grant_coordinator.go line 970 at r1 (raw file):

// Prevent the linter from emitting unused warnings.
var _ = NewGrantCoordinatorSQL

why is this removed?

pkg/util/admission/work_queue.go line 1598 at r1 (raw file):

	// TODO(abaptist): This is done to pre-register stats. Need to check that we
	// getOrCreate "enough" of the priorities to be useful.
	for _, pri := range applicablePriorities {

if the lazy creation is working correctly, this eager creation seems unnecessary. Is is needed for the list in chart_catalog?

pkg/kv/kvserver/client_split_test.go line 1224 at r1 (raw file):

			// Set maxBytes to something small so we can exceed the maximum split
			// size without adding 2x64MB of data.
			const maxBytes = 1 << 17

why this change?

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

pkg/server/server.go line 222 at r1 (raw file):

There was inconsistency in the NewGrantCoordinator in terms of how it processed the settings. This wasn't strictly necessary, however, it made it harder to follow the code in terms of where the settings vs options came from. The goal was really to have more immutable options and settings coming through this code to make reasoning and testing easier. I don't mind reverting this part of the patch (or pulling into another commit).

pkg/ts/catalog/chart_catalog.go line 3473 at r1 (raw file):

These were the values that were most commonly used (0, 100 and -30). Once lazy loading is in place in the future, ideally this won't be necessary to do.

pkg/util/admission/grant_coordinator.go line 158 at r1 (raw file):

This comment seemed out of place after this chance the metrics were more obviously inserted as part of the makeStoreRequesterFunc.

pkg/util/admission/grant_coordinator.go line 970 at r1 (raw file):

Not removed, just moved up to right before the method to make it more obvious what it was referring to. The NewGrantCoordinatorSQL method will need a little work before it is enabled, and it took me a while to understand how it worked (since it was repeating metrics names) as I didn't see that it was unused.

pkg/util/admission/work_queue.go line 1598 at r1 (raw file):

lazy creation is working correctly from the metrics perspective and they get registered, unfortunately the registry does not currently handle lazy addtion (see https://cockroachlabs.slack.com/archives/C01CNRP6TSN/p1663779962687259 )

pkg/kv/kvserver/client_split_test.go line 1224 at r1 (raw file):

The two tests started failing after making the change to stats. This test initializes a system with a greatly reduced maxBytes and then attempts to create a maxBytes size value and check what happens on split. With the new stats, we push the metrics value over the 64KB limit and therefore this test fails because it also gets added to the split queue. The 1 <<16 was somewhat arbitrary, so this change simply makes it large enough so the metrics range is no longer included.

Reviewed 4 of 9 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist, @irfansharif, and @sumeerbhola)

-- commits line 7 at r1:
Please add something like
Informs #82743

pkg/server/server.go line 222 at r1 (raw file):

btw, I don't think we have a principle regarding separating immutable and mutable configuration -- we tend to pile them all into Config/Options structs. Maybe kv code is principled in this regard, in which case I'd also like to know what the idiom is. One example is base.StorageConfig which includes cluster.Settings.

But I am ok with your change.

pkg/ts/catalog/chart_catalog.go line 3473 at r1 (raw file):

So will we not see metrics for -30, -50, -100 for kv, kv-stores etc. given that the lazy registration does not work?
If yes, we should eagerly register them. One of the reasons we wanted to change is to use it when diagnosing workloads where we have user-facing stuff (typically priority 0) and various bulk operations running concurrently (and the latter will use -30 and -100).

pkg/util/admission/grant_coordinator.go line 158 at r1 (raw file):

nit: makeStoreRequesterFunc is accepting metrics created in makeStoresGrantCoordinators and not creating them itself, so we need a TODO comment about per-store metrics somewhere in StoreGrantCoordinators. See my comment below about a better place.

pkg/util/admission/grant_coordinator.go line 401 at r1 (raw file):

	// These metrics are shared across all stores and all
	// priorities across stores (even the coarser workClasses, which are a
	// mapping from priority, share the same metrics).

Comment needs updating since the "all priorities" part is stale now. Can you add a TODO here about per-store metrics?

pkg/util/admission/grant_coordinator.go line 970 at r1 (raw file):

Ack

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @sumeerbhola)

-- commits line 7 at r1:

Done

pkg/ts/catalog/chart_catalog.go line 3468 at r1 (raw file):

Done

pkg/ts/catalog/chart_catalog.go line 3473 at r1 (raw file):

Done, I added some additional stats, I am attempting to avoid overloading the number of stats. Having a few missed stats isn't too bad because they will fall into the "default" bucket.

pkg/util/admission/grant_coordinator.go line 158 at r1 (raw file):

Done

pkg/util/admission/grant_coordinator.go line 401 at r1 (raw file):

Done

pkg/util/admission/grant_coordinator.go line 970 at r1 (raw file):

Done

pkg/util/admission/work_queue.go line 1526 at r1 (raw file):

Done

pkg/util/admission/work_queue.go line 1537 at r1 (raw file):

No - the lazy registration today does not handle either the internal or prometheus database. I hope we can remove the pre-registration in the future, but I didn't dive into that code now.

pkg/util/admission/work_queue.go line 1598 at r1 (raw file):

We don't currently pre-register everything so this shouldn't be fatal. I want to remove pre-registration in the future, but didn't want to try and do this as part of this commit. I kept LoadOrStore for that reason.

pkg/util/admission/granter_test.go line 124 at r1 (raw file):

Done

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andrewbaptist, @irfansharif, and @sumeerbhola)

pkg/util/admission/work_queue.go line 1529 at r3 (raw file):

	Admitted        *metric.Counter
	Errored         *metric.Counter
	WaitDurationSum *metric.Counter

You are removing this because we don't find it useful, yes?
My original motivation was to be able to look at mean latency, but it is not possible to compute delta(waitDurationSum)/delta(requested) in our internal metrics system, though it it possible with Prometheus.
I'll leave it to @irfansharif and your judgement on whether this is useful or not.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @sumeerbhola)

pkg/util/admission/work_queue.go line 1529 at r3 (raw file):

This is automatically computed as part of the WaitDurations histogram, and the values are the same. So this was a redundant state. I'm not sure about the internal metrics, but I assume they could use the sum metric. Prometheus can definitely use the histogram counter to get the sum

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andrewbaptist, @irfansharif, and @sumeerbhola)

pkg/ts/catalog/chart_catalog.go line 3463 at r4 (raw file):

					"admission.admitted.elastic-cpu",
					"admission.errored.elastic-cpu",
					"admission.admitted.elastic-cpu.bulk",

Optional nit: add the "-pri" suffix so it's even clearer.

pkg/util/admission/work_queue.go line 1529 at r3 (raw file):

The sum is missing, but the avg is computed already. So this is redundant and can be removed.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/util/admission/work_queue.go line 1529 at r3 (raw file):

The histogram reporting is limited to work that actually waited in the queue. So if a large fraction did not wait in the queue, it will overreport the latency percentiles. We should fix that by reporting everything. I did not do this when writing the first version of this code, because I was worried about lock contention overhead in the histogram and did not have time to investigate how much of a problem it really was.