Isn't this result backwards?

It feels like this test should run 3 things:

1. TPCC with no concurrent load
2. TPCC with lots of concurrent load via tpch (to the point that it fails and we see efficiency in the 60% or worse range)
3. Same load as 2) but with the background QoS

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

I tried higher load, and can change the test to do this again. However, with background QoS I sometimes get much worse performance, so I would have to widen the allowed regression percentage so the test isn't failing all the time.. What would the first test show? Wouldn't it always be faster than tests 2 and 3?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

I don't think the result is backwards. Giving higher admission control priority to TPCC than TPCH should allow it to run faster.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

If admission control worked as intended, the idea is that test 3 should come close to test 1 and test 2 should do poorly

Yeah, so far I am having a hard time finding a test that shows precisely this point, so I'm using the test to record the current level of performance and make sure we don't regress any further.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

Ah, sorry, I was looking at the latency profiles. At the end of the day, 96.88 vs 98.11 feels in the noise for a 3 minute run. I have a feeling that load-balancing variability could be the culprit. Both are clearly passing runs. Do we know that any admission control throttling kicked in? It'd be nice to validate that the delay over the time interval was non-zero. We have some other roachtest which validate metrics

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

No worries, OK, I'll increase the load and runtime. Any pointers on the other roachtests you're referring to is appreciated. Thanks.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

I don't see a predefined metric.Metadata for admission control throttling. I guess a new one is needed.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

These are cockroach metrics. see like

You can look at /_status/vars to find metrics too. admission_wait_durations_kv_stores_sum may be interesting

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @yuzefovich)

The admission.wait* metrics will show whether overload is being achieved, the overload page in the admin is good too.

Reviewed 2 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

Thanks. The latest code push shows my attempt to look up these metrics (see printAdmissionMetrics), though I am not seeing any entries in the response, at least on a local cluster. I am running again now on a cloud cluster, but if you see anything I'm doing wrong in the code, please let me know.

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

Nice work! I have quite a few nits (sorry) and a suggestion.

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

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 36 at r3 (raw file):

	Warehouses      int
	Concurrency     int
	OlapConcurrency int

nit: s/Olap/OLAP/ and s/Tpcc/TPCC/ throughout the file - I think we tend to either capitalize all letters in an acronym or none.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 42 at r3 (raw file):

}

// run sets up TPCC then runs both TPCC plus TPCH OLAP queries involving the

nit: probably s/involving the/involving them/.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 46 at r3 (raw file):

// quality of service, then with OLAP queries using `regular` transaction
// quality of service.  The results are logged and compared. The test fails if
// using `background` QoS doesn't result in a tpmC score from TPCC which is at

nit: s/which is at least/at least/.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 60 at r3 (raw file):

	noThrottleTpmC = make([]float64, 0, s.numRuns)
	noThrottleTpccEfficiency = make([]float64, 0, s.numRuns)
	var tpmCQos, tpmCNoQoS float64

nit: s/Qos/QoS/ throughout the file for consistency.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 61 at r3 (raw file):

	noThrottleTpccEfficiency = make([]float64, 0, s.numRuns)
	var tpmCQos, tpmCNoQoS float64
	var efficiencyCQos, efficiencyNoQoS float64

nit: s/CQos/QoS/.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 67 at r3 (raw file):

		crdbNodes, workloadNode := s.setupDatabases(ctx, t, c)

		s.runTpcc(ctx, t, c, crdbNodes, workloadNode, histogramsPath, false)

nit: it's good to add an inline comment for constant arguments like false /* useBackgroundQoS */.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 135 at r3 (raw file):

	if percentImprovement < maxAllowedRegression {
		ttycolor.Stdout(ttycolor.Red)
		failMessage :=

nit: seems like this and the next lines should be squashed.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 207 at r3 (raw file):

	response := mustGetMetrics(t, adminURL, start, end, admissionMetrics)

	// Drop the first two minutes of datapoints as a "ramp-up" period.

nit: seems like a leftover?

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 256 at r3 (raw file):

) {
	m := c.NewMonitor(ctx, crdbNodes)
	// Kick off TPC-H with concurrency

nit: missing period.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go, line 394 at r3 (raw file):

			CPUs:            4,
			Concurrency:     32,
			OlapConcurrency: 64,

This concurrency might be a bit too high since it might push the cluster into the OOM territory. I'd probably drop it to 32. Or you had to bump it up to see the difference? If so, it might be better to only use TPCH Q1 and also do SET CLUSTER SETTING sql.distsql.temp_storage.hash_agg.enabled = false; - Q1 will be burning up CPU while staying within memory limits.

Thanks for the input. Right now I'm blocked a bit because printAdmissionMetrics is not able to retrieve metrics from the cluster. Any advice on what looks wrong in this code is appreciated.

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

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 36 at r3 (raw file):

Done

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 42 at r3 (raw file):

Removed involving the

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 46 at r3 (raw file):

It sounds better as is.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 60 at r3 (raw file):

Sure. I was trying to follow our Go naming conventions because I thought I got review comments before indicating not to capitalize letters other than the first of an acronym. I have made this change.

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 61 at r3 (raw file):

Done

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 67 at r3 (raw file):

Done

Code quote:

runTpcc(

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 135 at r3 (raw file):

Done

Code quote:

ttycolor.Stdout(ttycolor.Red)

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 207 at r3 (raw file):

I don't know if I need this code yet. I am having trouble getting any metrics back at all, so I don't know if I need this code. I'll leave it in commented-out for now.

Code quote:

Drop the first two minutes of datapoints as a

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 256 at r3 (raw file):

Added

Code quote:

Kick off TPC-H with concurrency

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 394 at r3 (raw file):

Thanks for the advice, I'll try out different things. Right now the blocker for me is I'm unable to get any metrics at all in printAdmissionMetrics.

Hm, I don't have any experience with those, so hopefully others can chime in.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msirek and @sumeerbhola)

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 60 at r3 (raw file):

I'm a big fan of consistency, and my nit came from the fact we had Qos and NoQoS previously - either Qos or QoS seems ok to me, but once picked, it should be used throughout the whole file.

Thanks. I'm talking with Tommy today, maybe he'll know the answer.

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

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 60 at r3 (raw file):

Good point, thanks.

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

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 64 at r4 (raw file):

	var baselineTpmC, baselineEfficiency float64

	for i := 0; i < s.numRuns; i++ {

numRuns is possibly redundant with roachtest --count

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 84 at r4 (raw file):

		startNoQoS := timeutil.Now()
		s.runTPCCAndOLAPQueries(ctx, t, c, crdbNodes, workloadNode, histogramsPath, false /* useBackgroundQoS */)

Do we want to run 3 workloads back to back? We could also run 3 separate tests and not worry about having a pass fail. The semantics of restarting a workload are unknown to me, like does it try to insert the same records on subsequent runs and get a bunch of collisions? Consider also just leaving tpcc running the whole time and do a couple minutes
w/o tpch, a couple minutes w/ noqos, a couple minutes w/ qos and then a washout period at the end again with no tpch. Not sure how to get instantaneous tpmc metrics but maybe they can be copied and analyzed while the test is running?

pkg/cmd/roachtest/tests/qos_tpcc_background_olap.go line 392 at r4 (raw file):

		{
			CPUs:            4,
			Concurrency:     32,

I think in order for admission control queuing to occur you need sustained goroutines per cpu to be over 40. Check the overload page in the admin. I'd guess concurrency needs to be higher.