internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge #554

fishy · 2022-08-16T15:52:21Z

Benchmark result:

$ go test -bench .
goos: linux
goarch: amd64
pkg: github.com/reddit/baseplate.go/internal/prometheusbp
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkHighWatermarkValue-12          24221392                48.11 ns/op            0 B/op          0 allocs/op
PASS
ok      github.com/reddit/baseplate.go/internal/prometheusbp    1.221s

cc @nanassito

fishy · 2022-08-16T15:56:02Z

prometheusbp/high_watermark_gauge.go

+
+// HighWatermarkGauge implements an int64 gauge with high watermark value.
+type HighWatermarkGauge struct {
+	lock sync.RWMutex


benchmark result with RWMutex:

goos: linux goarch: amd64 pkg: github.com/reddit/baseplate.go/prometheusbp cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz BenchmarkHighWatermarkGauge-12 20724597 55.75 ns/op 0 B/op 0 allocs/op PASS ok github.com/reddit/baseplate.go/prometheusbp 1.221s

benchmark result if we use Mutex instead:

goos: linux goarch: amd64 pkg: github.com/reddit/baseplate.go/prometheusbp cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz BenchmarkHighWatermarkGauge-12 12081015 103.0 ns/op 0 B/op 0 allocs/op PASS ok github.com/reddit/baseplate.go/prometheusbp 1.351s

kylelemons · 2022-08-16T16:29:20Z

prometheusbp/high_watermark.go

+}
+
+// Inc increases the gauge value by 1.
+func (hwv *HighWatermarkValue) Inc() {


I think this should have more of the methods from Gauge:

Inc/Dec

Add/Sub

Set

(I don't really care about SetToCurrentTime)

kylelemons · 2022-08-16T16:36:15Z

prometheusbp/high_watermark.go

+	"github.com/prometheus/client_golang/prometheus"
+)
+
+// HighWatermarkValue implements an int64 gauge with high watermark value.


I think this exposes too much of this API surface and makes it too hard to use.

I would use prometheus.Histogram as a guide here. It's exposed as a single API but when collected it produces more than one metric with a fixed naming scheme.

Expose:

func NewHighWaterMarkGauge(HighWatermarkGaugeArgs) *HighWatermarkGauge

func NewHighWaterMarkGaugeVec(HighWatermarkGaugeArgs, labels) *HighWatermarkGaugeVec

type HighWatermarkGaugeVec

With / WithLabels

type HighWatermarkGauge

Add / Sub / Set

Inc / Dec

Collect

I would strongly prefer to separate the API for the value and api for the actual prometheus metrics (the 2 gauges). There's no guarantee that we would always define the same set of labels for the 2 gauges, so an API like prometheus.Histogram can be very brittle.

As for Add/Sub, I just don't really see a use case for them yet. If we do have the use case for them later we can add them when then.

There's no guarantee that we would always define the same set of labels for the 2 gauges

I would approach this from the other side. Why would we not want them to have the same labels? Unlike a histogram where you may want a counter with additional labels, we're not talking about exploding cardinality here, and it would be pretty confusing to have different labels for the instant and high watermark metrics.

kylelemons · 2022-08-16T16:37:12Z

prometheusbp/high_watermark.go

+
+// HighWatermarkGauge implements a prometheus.Collector that reports up to 2
+// gauges backed by a HighWatermarkValue.
+type HighWatermarkGauge struct {


Constructor to help making one of these? This looks like it would be a beast to construct and I feel like there are a lot of implicit / undocumented assumptions this is making that might trip users up. These types (prometheus.Desc) also are probably not super obvious for users how to construct.

So the actual gauge api is mostly just a sugar for the common use cases we have in the spec implementations. It's a very thin wrapper around the value api that's trivial to implement, so I initially did not include this part, but then thought that it's probably better to save some repetitions.

What do you think if I move the gauge api to internal and only leave the value api as public?

I think the valuable thing to export is an easy-to-use API. If this is just used for internal things, then maybe we don't expose any of it.

ok moved both to internal.

kylelemons

RC'ing to appease harold.

kylelemons · 2022-08-16T20:05:43Z

internal/prometheusbp/high_watermark_test.go

+	b.ResetTimer()
+	b.RunParallel(func(pb *testing.PB) {
+		for pb.Next() {
+			n := int(atomic.AddInt64(&counter, 1)) % len(operations)


🔕 I feel like having a goroutine-local counter in a var just above the for pb.next loop will work fine -- they don't need to rotate through the operations in lockstep for it to make sense. That will remove the atomic overhead, which is non-negligible when you're benchmarking a mutex

good point. will do.

Benchmark result: $ go test -bench . goos: linux goarch: amd64 pkg: github.com/reddit/baseplate.go/internal/prometheusbp cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz BenchmarkHighWatermarkValue-12 24221392 48.11 ns/op 0 B/op 0 allocs/op PASS ok github.com/reddit/baseplate.go/internal/prometheusbp 1.221s

fishy · 2022-08-16T20:08:24Z

internal/prometheusbp/high_watermark_test.go

+	b.ResetTimer()
+	b.RunParallel(func(pb *testing.PB) {
+		for pb.Next() {
+			n := int(atomic.AddInt64(&counter, 1)) % len(operations)


good point. will do.

fishy · 2022-08-16T20:12:15Z

internal/prometheusbp/high_watermark.go

+
+// HighWatermarkValue implements an int64 gauge with high watermark value.
+type HighWatermarkValue struct {
+	lock sync.RWMutex


Benchmark result with RWMutex:

$ go test -bench . goos: linux goarch: amd64 pkg: github.com/reddit/baseplate.go/internal/prometheusbp cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz BenchmarkHighWatermarkValue-12 25154269 47.51 ns/op 0 B/op 0 allocs/op PASS ok github.com/reddit/baseplate.go/internal/prometheusbp 1.250s

benchmark result with Mutex:

$ git diff diff --git a/internal/prometheusbp/high_watermark.go b/internal/prometheusbp/high_watermark.go index ccfa193..6f12a0e 100644 --- a/internal/prometheusbp/high_watermark.go +++ b/internal/prometheusbp/high_watermark.go @@ -8,7 +8,7 @@ import ( // HighWatermarkValue implements an int64 gauge with high watermark value. type HighWatermarkValue struct { - lock sync.RWMutex + lock sync.Mutex curr int64 max int64 } @@ -45,23 +45,23 @@ func (hwv *HighWatermarkValue) Set(v int64) { // Get gets the current gauge value. func (hwv *HighWatermarkValue) Get() int64 { - hwv.lock.RLock() - defer hwv.lock.RUnlock() + hwv.lock.Lock() + defer hwv.lock.Unlock() return hwv.curr } // Max returns the max gauge value (the high watermark). func (hwv *HighWatermarkValue) Max() int64 { - hwv.lock.RLock() - defer hwv.lock.RUnlock() + hwv.lock.Lock() + defer hwv.lock.Unlock() return hwv.max } func (hwv *HighWatermarkValue) getBoth() (curr, max int64) { - hwv.lock.RLock() - defer hwv.lock.RUnlock() + hwv.lock.Lock() + defer hwv.lock.Unlock() return hwv.curr, hwv.max } $ go test -bench . goos: linux goarch: amd64 pkg: github.com/reddit/baseplate.go/internal/prometheusbp cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz BenchmarkHighWatermarkValue-12 15068430 76.76 ns/op 0 B/op 0 allocs/op PASS ok github.com/reddit/baseplate.go/internal/prometheusbp 1.244s

konradreiche

Could we add some comments in code or the git commit that explains what a high watermark value is?

konradreiche · 2022-08-16T22:14:47Z

internal/prometheusbp/doc.go

@@ -0,0 +1,3 @@
+// Package prometheusbp provides common code used by Baseplate.go regarding


Suggested change

// Package prometheusbp provides common code used by Baseplate.go regarding

// Package prometheusbp provides common code used by baseplate.go regarding

🔕 is the official name all lowercase?

it's not :)

konradreiche · 2022-08-16T22:15:16Z

internal/prometheusbp/high_watermark.go

+
+// HighWatermarkValue implements an int64 gauge with high watermark value.
+type HighWatermarkValue struct {
+	lock sync.RWMutex


Suggested change

lock sync.RWMutex

mu sync.RWMutex

Is mu to refer to the lock more common?

why do you want to use mu to refer to lock instead of using lock to refer to lock though :)

For no good reason other than convention. I see it mostly being called mu in Go https://go.dev/tour/concurrency/9
Maybe because lock.RLock() reads a bit repetitive 🤷

lock is noun and Lock/RLock is verb :)

fishy requested a review from a team as a code owner August 16, 2022 15:52

fishy requested review from kylelemons, konradreiche and mterwill and removed request for a team August 16, 2022 15:52

fishy commented Aug 16, 2022

View reviewed changes

fishy mentioned this pull request Aug 16, 2022

feat(prom): Implement BP.spec metrics for the pool in redisbp. #553

Merged

3 tasks

fishy force-pushed the high-watermark-gauge branch from e34e775 to 1004bac Compare August 16, 2022 16:17

fishy changed the title ~~prometheusbp: Add HighWatermarkGauge~~ prometheusbp: Add HighWatermarkValue and HighWatermarkGauge Aug 16, 2022

kylelemons requested changes Aug 16, 2022

View reviewed changes

fishy requested a review from kylelemons August 16, 2022 16:45

kylelemons requested changes Aug 16, 2022

View reviewed changes

fishy requested a review from kylelemons August 16, 2022 17:43

kylelemons approved these changes Aug 16, 2022

View reviewed changes

fishy force-pushed the high-watermark-gauge branch from 5b13bae to e56de02 Compare August 16, 2022 20:11

fishy commented Aug 16, 2022

View reviewed changes

fishy changed the title ~~prometheusbp: Add HighWatermarkValue and HighWatermarkGauge~~ internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge Aug 16, 2022

konradreiche approved these changes Aug 16, 2022

View reviewed changes

fishy merged commit e837cc4 into reddit:master Aug 17, 2022

fishy deleted the high-watermark-gauge branch August 17, 2022 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge #554

internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge #554

fishy commented Aug 16, 2022 •

edited

Loading

fishy Aug 16, 2022

kylelemons Aug 16, 2022

kylelemons Aug 16, 2022

fishy Aug 16, 2022

kylelemons Aug 16, 2022

kylelemons Aug 16, 2022

fishy Aug 16, 2022 •

edited

Loading

kylelemons Aug 16, 2022

fishy Aug 16, 2022

kylelemons left a comment

kylelemons Aug 16, 2022 •

edited

Loading

fishy Aug 16, 2022

fishy Aug 16, 2022

fishy Aug 16, 2022

konradreiche left a comment

konradreiche Aug 16, 2022

fishy Aug 17, 2022

konradreiche Aug 16, 2022

fishy Aug 16, 2022

konradreiche Aug 16, 2022

fishy Aug 17, 2022

		@@ -0,0 +1,3 @@
		// Package prometheusbp provides common code used by Baseplate.go regarding

internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge #554

internal/prometheusbp: Add HighWatermarkValue and HighWatermarkGauge #554

Conversation

fishy commented Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylelemons left a comment

Choose a reason for hiding this comment

kylelemons Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradreiche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy commented Aug 16, 2022 •

edited

Loading

fishy Aug 16, 2022 •

edited

Loading

kylelemons Aug 16, 2022 •

edited

Loading