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

pkg/util/admission/granter.go line 1727 at r1 (raw file):

		}
		if doLog {
			avgAdded := bytesAdded / int64(admitted)

thanks for doing this. Is this something you wanted to backport to 22.1 and 21.2?
I ask since the log statement is changing with the byte tokens PR

Big update, PTAL. I realize this may make it harder to backport later changes (if we're not backporting this one as well). If you're concerned about that let's discuss dropping this PR.

Please review carefully, I am still worried I messed something up here though the test diffs seem to indicate that I haven't.

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

pkg/util/admission/granter.go line 1727 at r1 (raw file):

I'll defer to you in terms of backporting. I'm not sure how good the test coverage here is. I don't seem to have introduced any changes (please carefully review) but there is now a large diff. Then again, not backporting may make it impossible to backport later changes. If you think that it is too risky to make a refactor of the size I'm doing here, I'm happy to hold off, though there isn't typically a better time in the cycle to do so and so it might mean the improvements here won't happen.

I think if this bakes for some time we could probably backport it to 22.1, at least if we're also backporting the bytes change. (Not sure what the plan there was).

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

pkg/util/admission/granter.go line 1626 at r3 (raw file):

	// represents what has been given out. The units is bytes.
	totalTokens     int64
	tokensAllocated int64

all of these are moved into ioLoadListenerState, yes? I like the comments and better names.

pkg/util/admission/granter.go line 1750 at r3 (raw file):

	intL0CompactedBytes         int64
	smoothedIntL0CompactedBytes float64

is this smoothed value the same as in ioLoadListenerState? Same question for the two other Smoothed* members in this struct.

pkg/util/admission/granter.go line 1812 at r3 (raw file):

	// intIngestedBytes are the bytes ingested since the last token adjustment. Bytes
	// tracked here are also tracked in intAdmittedBytes.
	intIngestedBytes := cumAdmissionStats.ingestedBytes - prev.cumAdmissionStats.ingestedBytes

s/intIngestedBytes/intIngestedAccountedBytes/

pkg/util/admission/granter.go line 1860 at r3 (raw file):

		// Keep the old smoothed value around, i.e. don't update it with a zero and
		// don't zero it out.
		// TODO(during review): why?

we've not had work flow through admission control, so we don't have any reasonable interval data to update our previous estimates.

pkg/util/admission/granter.go line 1864 at r3 (raw file):

	}

	// TODO(during review): aren't we double-penalizing here? Let's say 1GB was

I think what you are suggesting is potentially a higher fidelity model, but I maybe misunderstanding what you are saying.
We are currently taking all the unaccounted bytes that landed into L0 (ingests or flush) and spreading them evenly over what was admitted. This will then be fed into storeRequestEstimates.workByteAddition which is used by WorkQueue for both regular writes and ingests.

I don't understand the "penalize ingestions" question. The l0BytesIngestedFraction is solely based on what was reported by the WorkQueue. That is, intIngestedBytes is not from the Pebble metrics. This fraction calculation is only to minimize the error in the interval between Admit and AdmittedWorkDone: if there a 1MB ingest, we want to make a reasonable guess on how many tokens to deduct based on past observations, and then we will fix any error in AdmittedWorkDone. So this is not saying anything about what silently bypassed admission control.
Renaming intIngestedBytes to intIngestedAccountedBytes would make this clearer.

pkg/util/admission/granter.go line 1907 at r3 (raw file):

		}
	} else {
		// Under the threshold. Maintain a smoothedNumTokens based on what was

Maintain a smoothedTotalNumByteTokens ...

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

			q.mu.stats.admittedBytes += uint64(h.writeBytes)
			if h.ingestRequest {
				q.mu.stats.ingestedBytes += uint64(h.writeBytes)

ingestedAccountedBytes?
Though by definition, all the WorkQueue stats are accounted.

Dismissed @sumeerbhola from 4 discussions.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

pkg/util/admission/granter.go line 1626 at r3 (raw file):

Yep

pkg/util/admission/granter.go line 1750 at r3 (raw file):

Good catch. I accidentally duplicated some of these fields. Cleaned this

pkg/util/admission/granter.go line 1812 at r3 (raw file):

Done.

pkg/util/admission/granter.go line 1860 at r3 (raw file):

Updated with this comment.
Still, is this the right thing to do? You'd expect us to "always measure/update" but just not throttle when admission control is off. Is this just reflecting the fact that when admission control was introduced, we wanted the "off" switch to completely disable most code related to it? As is, it seems that once admission control kicks in again after a long period of being below threshold, we'll initially start out with a mostly bogus smoothed value here. I think this is fine, just trying to deeply understand the rationale here.

pkg/util/admission/granter.go line 1864 at r3 (raw file):

I hadn't understood this properly, with your explanation it makes sense.

pkg/util/admission/granter.go line 1907 at r3 (raw file):

Done.

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

If we're going to do the accounted infix (and it looks like we are), then it's better to have it everywhere. Made sure it's present in all of the storeAdmissionStats byte counters.