/assign @wojtek-t

/assign @MikeSpreitzer

Extra latency should not be part of width.
Before we make a big divergence between counting requests and counting seats in the metrics, we should finish adding all the seat-counting metrics.
It would be easier to impose the extra latency in apf_filter.go than inside the QueueSet. The one advantage I see for imposing extra latency inside the QueueSet is the ability to have a divergence between the request-couting metrics and the seat-counting metrics.
A sleep in the request-handling goroutine will not get the desired behavior, which is to get a response to the client before the extra latency expires.
A PassiveClock does not have a sleep method.
Remember that PassiveClock is designed to facilitate testing, and QueueSet is tested with a FakeEventClock.
Remember that we have two clock packages.

#94738 - two clock packages with divergent history

To do a testable sleep we would promote EventClock into the clock package and add a Sleep method, which the fake one implements like the existing ClockWait function in queueset_test.go.

/triage accepted

The unit test is failing, I am going to investigate it...

Rather than a checksum test, I recommend updating the behavioral tests.
Add width and duration padding to uniformClient.
Update the calculation of the expected results.
Add scenarios with width other than 1 and with non-zero duration padding.

demands[i] = float64(nThreads) * float64(uc.execDuration) / float64(uc.thinkDuration+uc.execDuration)

would become

demands[i] = float64(nThreads) * float64(uc.width) * float64(uc.execDuration) / float64(max(uc.thinkDuration, uc.padDuration )+ uc.execDuration)

Rather than a checksum test, I recommend updating the behavioral tests.

this test is a behavioral test, but at a unit level, so if there is a regression due to a bug in finishRequestLocked this test will alert us by failing.
The behavioral test you are referring to is exercising the code from a much higher level. So I think this test in this PR is essential.

I agree that we should update the behavioral tests with variable work estimates. Do we want to do it in a follow-up PR? I am trying to expedite this to unblock #103539. Otherwise, happy to update the behavioral tests in this PR.

newTestableQueueSetFactory is intended to be the start of how a test func creates a queueSet.
Note that when doing goroutine counting, you need to use a promiseFactoryFactory that participates.

@MikeSpreitzer do you mean this initialization pattern?

	clk, counter := testeventclock.NewFake(now, 0, nil)
	qsf := newTestableQueueSetFactory(clk, countingPromiseFactoryFactory(counter))
	qCfg := fq.QueuingConfig{
		Name:             "TestTotalRequestsExecutingWithPanic",
		DesiredNumQueues: 0,
		RequestWaitLimit: 15 * time.Second,
	}
	qsc, err := qsf.BeginConstruction(qCfg, newObserverPair(clk))
	if err != nil {
		t.Fatal(err)
	}
	qs := qsc.Complete(fq.DispatchingConfig{ConcurrencyLimit: 1})
	counter.Add(1) // account for the goroutine running this test

@MikeSpreitzer this test exercises finishRequestLocked (a unit at a much lower level), not qs.StartRequest - so i don't think we need to use the above pattern, it adds unnecessary steps for the test:

                        now := time.Now()
			clk, counter := testeventclock.NewFake(now, 0, nil)
			qsf := newTestableQueueSetFactory(clk, countingPromiseFactoryFactory(counter))
			qCfg := fq.QueuingConfig{
				DesiredNumQueues: 1,
				QueueLengthLimit: 8,
				HandSize:         1,
			}
			qsc, err := qsf.BeginConstruction(qCfg, newObserverPair(clk))
			if err != nil {
				t.Fatal(err)
			}
			o := qsc.Complete(fq.DispatchingConfig{ConcurrencyLimit: 1})
			counter.Add(1) // account for the goroutine running this test

			qs, ok := o.(*queueSet)
			if !ok {
				t.Fatal("Not a queueSet")
			}

			queue := qs.queues[0]
			r := &request{
				qs:           qs,
				queue:        queue,
				workEstimate: test.workEstimate,
			}

                        ...

			qs.finishRequestLocked(r)
			counter.Add(-1) // completion of main activity of goroutine running this test

I am fine making this change if we want to stick to creating a fully initialized QueueSet in every unit test (as opposed to initialize what is needed), thoughts?

Yes, that's the initialization pattern I meant.

I suppose "checksum test" means "test of a small unit". My point is that tests of the whole queueset unit are necessary and sufficient. I am not a fan of maintaining tests of smaller units. If you want to put in a test of finishRequestLocked with non-zero duration padding in this PR and switch to whole-package testing in a follow-on PR, that is OK with me. I wiill prepare that follow-on.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MikeSpreitzer, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/LGTM

Thanks!

I do not want to hold up this improvement due to wrangling over the test; that can be worked in follow-up.

Thanks @MikeSpreitzer, I will be working on the follow-on PR.