metrics: compute InternalIntervalMetrics without making calls to GetInternalIntervalMetrics

[coolcom200]

Remove dependence on the GetInternalIntervalMetrics as this method resets the interval following each call. This results in issues if two clients interact with GetInternalIntervalMetrics as they will reset the other client’s interval. Ex: Client A calls every 5 seconds and Client B calls every 10 seconds. Client B will end up getting metrics at a 5-second interval since GetInternalIntervalMetrics resets the interval on each call.

The admission.StoreMetrics are updated to remove the InternalIntervalField as these metrics need to be computed based off the current metrics admission.StoreMetrics.Metrics and the previous metrics. The storage of the previous metrics is left up to the client. Removing the field was chosen instead of setting it to a nil value as it would be confusing to return the StoreMetrics and require the client to populate the field when they compute the necessary metrics.

For the admission control use case, the flush metrics are computed as part of ioLoadListener.adjustTokens with the previous metrics being initialized in ioLoadListener.pebbleMetricsTick and stored on the ioLoadListenerState as cumFlushWriteThroughput.

Additionally, make a copy of the pebble.InternalIntervalMetrics in order to move away from using the pebble version of the interval metrics as these metrics are now the responsibility of the caller to compute.

Release note: None
Epic: CRDB-17515

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 1 of 6 files at r1, 6 of 8 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @coolcom200 and @irfansharif)

---

-- commits line 18 at r2:
nit: necessary

---

-- commits line 23 at r2:
nit: goroutine

---

pkg/util/admission/granter.go line 651 at r2 (raw file):

}

// IntervalMetrics exposes metrics about internal subsystems, that can

Let's change the name to storeInternalIntervalMetrics (also making it package private).
Also simplify the comment to
// storeInternalIntervalMetrics exposes metrics about internal subsystems in the store,
// that are useful for admission control. These represent the metrics over ...

---

pkg/util/admission/granter.go line 659 at r2 (raw file):

type IntervalMetrics struct {
	// LogWriter metrics.
	LogWriter struct {

make all of these package private

---

pkg/util/admission/granter.go line 686 at r2 (raw file):

// StoreAndIntervalMetrics is a wrapper around StoreMetrics that also contains
// the IntervalMetrics
type StoreAndIntervalMetrics struct {

storeAndIntervalMetrics since it should be package private.

---

pkg/util/admission/granter.go line 687 at r2 (raw file):

// the IntervalMetrics
type StoreAndIntervalMetrics struct {
	*StoreMetrics

StoreMetrics is being passed by value to pebbleMetricsTick. We should keep it as value and not a pointer.

But let's step back for a minute. The diff computation in StoreGrantCoordinators isn't a good place since it has to keep state about each store and all the slice index complexity (and not quite correct, since the index is not required to be stable every time we fetch the metrics).

We could move it to GrantCoordinator.pebbleMetricsTick since there is a GrantCoordinator per store. But this isn't really the role of GrantCoordinator, which isn't supposed to know about the details about store metrics. The ioLoadListener already converts cumulative to deltas for all the stuff it computes over, so that is the right place. Currently it does it in a somewhat adhoc manner and I would suggest something simpler for your change.

Define the following in io_load_listener.go (only the flush throughput for now, since that's all we use in admission control adjustTokensInner. We can add more later if needed)

// storeInternalMetrics can be used to represent both cumulative and interval state for internal store subsystems.
type storeInternalMetrics {
	// Flush loop metrics.
	Flush struct {
		// WriteThroughput is the flushing throughput.
		WriteThroughput pebble.ThroughputMetric
	}
}

type ioLoadListenerState {
     ...

     // Cumulative
     cumStoreInternalMetrics storeInternalMetrics
}

Put this early in ioLoadListernerState with all the other cumulative fields.

In ioLoadListener.pebbleMetricsTick in the initialization if-block copy this out. In the non-initialization path, do the subtraction and pass the delta as an explicit parameter to ioLoadListener.adjustTokens.

There are various tests that are currently calling pebbleMetricsTick and those tests are assuming they should pass a delta for this flush throughput, but now the callee assumes it is being given a cumulative. Luckily most of these tests were passing an empty delta, so they are not affected. The only non-superficial test change will be in TestIOLoadListener. See where that test has the following code

				var flushBytes, flushWorkSec, flushIdleSec int
				if d.HasArg("flush-bytes") {
					d.ScanArgs(t, "flush-bytes", &flushBytes)
					d.ScanArgs(t, "flush-work-sec", &flushWorkSec)
					d.ScanArgs(t, "flush-idle-sec", &flushIdleSec)
				}
				flushMetric := pebble.ThroughputMetric{
					Bytes:        int64(flushBytes),
					WorkDuration: time.Duration(flushWorkSec) * time.Second,
					IdleDuration: time.Duration(flushIdleSec) * time.Second,
				}
				im := &pebble.InternalIntervalMetrics{}
				im.Flush.WriteThroughput = flushMetric

Let's keep the testdata files unchanged, so that the testdata is still providing us interval values. So the test code needs to convert these interval values to cumulative. It can do so easily by keeping additional 3 variables cumFlushBytes, cumFlushWorkSec, cumFlushIdleSec declared above the call to datadriven.RunTest. Then add what the testcase provided to the current cumulative values and pass those in. Whenever the testcase does "init", to create a new ioLoadListener, set these cumulative values back to 0.

[coolcom200]

Reviewed 2 of 6 files at r1, 8 of 8 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @sumeerbhola)

---

pkg/util/admission/granter.go line 651 at r2 (raw file):

Previously, sumeerbhola wrote…

Let's change the name to storeInternalIntervalMetrics (also making it package private).
Also simplify the comment to
// storeInternalIntervalMetrics exposes metrics about internal subsystems in the store,
// that are useful for admission control. These represent the metrics over ...

Done.

---

pkg/util/admission/granter.go line 659 at r2 (raw file):

Previously, sumeerbhola wrote…

make all of these package private

Done.

---

pkg/util/admission/granter.go line 686 at r2 (raw file):

Previously, sumeerbhola wrote…

storeAndIntervalMetrics since it should be package private.

Done.

---

pkg/util/admission/granter.go line 687 at r2 (raw file):

Previously, sumeerbhola wrote…

StoreMetrics is being passed by value to pebbleMetricsTick. We should keep it as value and not a pointer.

But let's step back for a minute. The diff computation in StoreGrantCoordinators isn't a good place since it has to keep state about each store and all the slice index complexity (and not quite correct, since the index is not required to be stable every time we fetch the metrics).

We could move it to GrantCoordinator.pebbleMetricsTick since there is a GrantCoordinator per store. But this isn't really the role of GrantCoordinator, which isn't supposed to know about the details about store metrics. The ioLoadListener already converts cumulative to deltas for all the stuff it computes over, so that is the right place. Currently it does it in a somewhat adhoc manner and I would suggest something simpler for your change.

Define the following in io_load_listener.go (only the flush throughput for now, since that's all we use in admission control adjustTokensInner. We can add more later if needed)

// storeInternalMetrics can be used to represent both cumulative and interval state for internal store subsystems.
type storeInternalMetrics {
	// Flush loop metrics.
	Flush struct {
		// WriteThroughput is the flushing throughput.
		WriteThroughput pebble.ThroughputMetric
	}
}

type ioLoadListenerState {
     ...

     // Cumulative
     cumStoreInternalMetrics storeInternalMetrics
}

Put this early in ioLoadListernerState with all the other cumulative fields.

In ioLoadListener.pebbleMetricsTick in the initialization if-block copy this out. In the non-initialization path, do the subtraction and pass the delta as an explicit parameter to ioLoadListener.adjustTokens.

There are various tests that are currently calling pebbleMetricsTick and those tests are assuming they should pass a delta for this flush throughput, but now the callee assumes it is being given a cumulative. Luckily most of these tests were passing an empty delta, so they are not affected. The only non-superficial test change will be in TestIOLoadListener. See where that test has the following code

				var flushBytes, flushWorkSec, flushIdleSec int
				if d.HasArg("flush-bytes") {
					d.ScanArgs(t, "flush-bytes", &flushBytes)
					d.ScanArgs(t, "flush-work-sec", &flushWorkSec)
					d.ScanArgs(t, "flush-idle-sec", &flushIdleSec)
				}
				flushMetric := pebble.ThroughputMetric{
					Bytes:        int64(flushBytes),
					WorkDuration: time.Duration(flushWorkSec) * time.Second,
					IdleDuration: time.Duration(flushIdleSec) * time.Second,
				}
				im := &pebble.InternalIntervalMetrics{}
				im.Flush.WriteThroughput = flushMetric

Let's keep the testdata files unchanged, so that the testdata is still providing us interval values. So the test code needs to convert these interval values to cumulative. It can do so easily by keeping additional 3 variables cumFlushBytes, cumFlushWorkSec, cumFlushIdleSec declared above the call to datadriven.RunTest. Then add what the testcase provided to the current cumulative values and pass those in. Whenever the testcase does "init", to create a new ioLoadListener, set these cumulative values back to 0.

Oh this is a way better way to organize the code! Thank you for bringing this up I have updated it!

[sumeerbhola]

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

---

-- commits line 23 at r4:
Reminder to update the PR description to be the same as this commit.

---

pkg/util/admission/io_load_listener.go line 175 at r4 (raw file):

// store, that are useful for admission control. These represent the metrics
// over the interval of time from the last call to retrieve these metrics. These
// are not cumulative, unlike Metrics.

s/storeInternalIntervalMetrics/storeInternalMetrics

And change the comment to say something like:
These can represent either cumulative metrics, or over an interval of time. The latter is possible since the data representation here allows for subtraction between two cumulative storeInternalMetrics.

Actually, since you are calling adjustTokensInner with pebble.ThroughputMetric, just remove this wrapper struct. It is not adding any value, and we are not preserving the wrapping through the method calls. Then we simply have cumFlushWriteThroughput as a field in ioLoadListenerState.

[sumeerbhola]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @coolcom200, @irfansharif, and @sumeerbhola)

---

pkg/util/admission/io_load_listener.go line 313 at r5 (raw file):

		io.diskBW.bytesWritten = metrics.DiskStats.BytesWritten
		io.diskBW.incomingLSMBytes = cumLSMIncomingBytes
		io.setCumulativeStoreInternalIntervalMetrics(metrics)

isn't this simply io.cumFlushWriteThroughput = metrics.Flush.WriteThroughput?

[coolcom200]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif and @sumeerbhola)

---

-- commits line 23 at r4:

Previously, sumeerbhola wrote…

Reminder to update the PR description to be the same as this commit.

Done.

---

pkg/util/admission/io_load_listener.go line 175 at r4 (raw file):

Previously, sumeerbhola wrote…

s/storeInternalIntervalMetrics/storeInternalMetrics

And change the comment to say something like:
These can represent either cumulative metrics, or over an interval of time. The latter is possible since the data representation here allows for subtraction between two cumulative storeInternalMetrics.

Actually, since you are calling adjustTokensInner with pebble.ThroughputMetric, just remove this wrapper struct. It is not adding any value, and we are not preserving the wrapping through the method calls. Then we simply have cumFlushWriteThroughput as a field in ioLoadListenerState.

Done.

---

pkg/util/admission/io_load_listener.go line 313 at r5 (raw file):

Previously, sumeerbhola wrote…

isn't this simply io.cumFlushWriteThroughput = metrics.Flush.WriteThroughput?

Yes it is thank you for pointing this out! Updated!

[sumeerbhola]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif)

[coolcom200]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)