metrics: expose fsync latency as prometheus histogram

[coolcom200]

Previously, the fsync latency was computed using an HDR histogram
however this histogram generated too many buckets of various widths. As
a result, a prometheus histogram was chosen instead. In addition, the
metrics were being accumulated and stored on each LogWriter and then
merged into the cumulative metrics on each WAL rotation. However, with
this new promethesus histogram the writes occur to the same histogram
and are separate from the lifetime of the WAL.

Additonally, remove all the callback listeners and configurations for
the callback based approach introduced in: 5fdb3ea

[cockroach-teamcity]

This change is 

[coolcom200]

Reviewed 8 of 10 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

---

db.go line 346 at r2 (raw file):

			// Can be nil.
			metrics struct {
				fsyncLatency prometheus.Histogram

Moved this outside of the record.LogWriterMetrics since LogWriterMetrics can be added and subtracted but doing so with this histogram doesn't make sense.

---

record/log_writer_test.go line 389 at r2 (raw file):

		Name:        "",
		Help:        "",
		ConstLabels: nil,

This needs to be deleted.

Code quote:

		Namespace:   "",
		Subsystem:   "",
		Name:        "",
		Help:        "",
		ConstLabels: nil,

---

record/log_writer_test.go line 431 at r2 (raw file):

}

func ValueAtQuantileWindowed(histogram *prometheusgo.Histogram, q float64) float64 {

The ValueAtQuantileWindowed is from cockroachdb so I am not sure if this is a test that makes sense to perform the test here for checking for valid quantiles given that prometheus histograms don't support pulling these details by default.

Code quote:

	require.LessOrEqual(t, float64(syncLatency/(2*time.Microsecond)),
		ValueAtQuantileWindowed(writeTo.Histogram, 90))
	require.LessOrEqual(t, int64(syncLatency/2), int64(m.WriteThroughput.WorkDuration))
}

func ValueAtQuantileWindowed(histogram *prometheusgo.Histogram, q float64) float64 {

[jbowens]

Reviewed 2 of 10 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @coolcom200)

---

db.go line 346 at r2 (raw file):

Previously, coolcom200 (Leon Fattakhov) wrote…

Moved this outside of the record.LogWriterMetrics since LogWriterMetrics can be added and subtracted but doing so with this histogram doesn't make sense.

Ack, makes sense

---

metrics.go line 247 at r2 (raw file):

	// FsyncLatencyBuckets are prometheus histogram buckets suitable for a histogram
	// that records latencies for fsyncs.
	FsyncLatencyBuckets = prometheus.LinearBuckets(0, float64(100*time.Microsecond), 50)

is there an exponential bucket division that maintains at most 100-200u divisions between [0,5ms]? with a max of say ~10s.

---

open.go line 428 at r2 (raw file):

		logWriterConfig := record.LogWriterConfig{
			WALMinSyncInterval: d.opts.WALMinSyncInterval,
			WALFsync:           d.mu.log.metrics.fsyncLatency,

nit: how about WALFsyncLatency

---

record/log_writer.go line 455 at r2 (raw file):

		f.Lock()
		if synced {
			f.fsyncLatency.Observe(float64(syncLatency))

can we keep the nil check? eg, if synced && f.fsyncLatency != nil

it's convenient for tests that don't need the histogram, and for other uses of the LogWriter outside of Pebble that also don't need the histogram. Cockroach uses the LogWriter for its encryption-at-rest file registry

[coolcom200]

Reviewed 5 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jbowens)

---

metrics.go line 247 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

is there an exponential bucket division that maintains at most 100-200u divisions between [0,5ms]? with a max of say ~10s.

This is possible as there are the following helpful functions:

1. ExponentialBucketsRange(min, max float64, count int)
2. ExponentialBuckets(start, factor float64, count int)

With a bit of fiddling we can fine tune the amount of buckets and the sizes. The main question here is what should the start/min value be? 100us? (as in the first bucket is 0 - 100us and the rest follow the distribution)

---

record/log_writer.go line 455 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

can we keep the nil check? eg, if synced && f.fsyncLatency != nil

it's convenient for tests that don't need the histogram, and for other uses of the LogWriter outside of Pebble that also don't need the histogram. Cockroach uses the LogWriter for its encryption-at-rest file registry

Done.

---

record/log_writer_test.go line 389 at r2 (raw file):

Previously, coolcom200 (Leon Fattakhov) wrote…

This needs to be deleted.

Done.

---

record/log_writer_test.go line 431 at r2 (raw file):

Previously, coolcom200 (Leon Fattakhov) wrote…

The ValueAtQuantileWindowed is from cockroachdb so I am not sure if this is a test that makes sense to perform the test here for checking for valid quantiles given that prometheus histograms don't support pulling these details by default.

@jbowens Do you have any thoughts on this?

[jbowens]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @coolcom200)

---

metrics.go line 247 at r2 (raw file):

Previously, coolcom200 (Leon Fattakhov) wrote…

This is possible as there are the following helpful functions:

1. ExponentialBucketsRange(min, max float64, count int)
2. ExponentialBuckets(start, factor float64, count int)

With a bit of fiddling we can fine tune the amount of buckets and the sizes. The main question here is what should the start/min value be? 100us? (as in the first bucket is 0 - 100us and the rest follow the distribution)

100u is a good starting bucket, but I imagine we'll need to experiment with adjusting start/factor values to get the right distribution.

[coolcom200]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jbowens)

---

metrics.go line 247 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

100u is a good starting bucket, but I imagine we'll need to experiment with adjusting start/factor values to get the right distribution.

Decided to go with Linear Buckets for the first 0 - 5ms and then exponential buckets from 5ms onwards

[jbowens]

Reviewed 2 of 10 files at r1, 4 of 7 files at r3, 1 of 2 files at r4, 1 of 2 files at r5, all commit messages.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @coolcom200)

[coolcom200]

TFTR!