From 11c9be377f4da0647508dac5aa8f3aa00c6484bd Mon Sep 17 00:00:00 2001 From: Cristina Leon Date: Fri, 28 Jul 2023 20:56:13 +0000 Subject: [PATCH 1/3] Locate: Add histogram for Memorystore request duration --- memorystore/client.go | 29 ++++++++++++++++++++++++++--- memorystore/client_test.go | 15 +++++++++++++++ metrics/metrics.go | 11 +++++++++++ metrics/metrics_test.go | 1 + 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/memorystore/client.go b/memorystore/client.go index 4e774269..4da96ba9 100644 --- a/memorystore/client.go +++ b/memorystore/client.go @@ -2,8 +2,10 @@ package memorystore import ( "encoding/json" + "time" "github.com/gomodule/redigo/redis" + "github.com/m-lab/locate/metrics" "github.com/m-lab/locate/static" ) @@ -20,20 +22,36 @@ func NewClient[V any](pool *redis.Pool) *client[V] { // Put sets a Redis Hash using the `HSET key field value` command. // If successful, it also sets a timeout on the key. func (c *client[V]) Put(key string, field string, value redis.Scanner, expire bool) error { + t := time.Now() conn := c.pool.Get() defer conn.Close() b, err := json.Marshal(value) if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "marshal error").Observe(time.Since(t).Seconds()) return err } args := redis.Args{}.Add(key).Add(field).AddFlat(string(b)) _, err = conn.Do("HSET", args...) - if expire && err == nil { - _, err = conn.Do("EXPIRE", key, static.RedisKeyExpirySecs) + if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "HSET error").Observe(time.Since(t).Seconds()) + return err } - return err + + if !expire { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "OK").Observe(time.Since(t).Seconds()) + return nil + } + + _, err = conn.Do("EXPIRE", key, static.RedisKeyExpirySecs) + if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "EXPIRE error").Observe(time.Since(t).Seconds()) + return err + } + + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "OK").Observe(time.Since(t).Seconds()) + return nil } // GetAll uses the SCAN command to iterate over all the entries in Redis @@ -42,6 +60,7 @@ func (c *client[V]) Put(key string, field string, value redis.Scanner, expire bo // return the entries if all of them are scanned successfully. // Otherwise, it will return an error. func (c *client[V]) GetAll() (map[string]V, error) { + t := time.Now() conn := c.pool.Get() defer conn.Close() @@ -51,24 +70,28 @@ func (c *client[V]) GetAll() (map[string]V, error) { for { keys, err := redis.Values(conn.Do("SCAN", iter)) if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("get", "all", "SCAN error").Observe(time.Since(t).Seconds()) return nil, err } var temp []string keys, err = redis.Scan(keys, &iter, &temp) if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("get", "all", "SCAN copy error").Observe(time.Since(t).Seconds()) return nil, err } for _, k := range temp { v, err := c.get(k, conn) if err != nil { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("get", "all", "HGETALL error").Observe(time.Since(t).Seconds()) return nil, err } values[k] = v } if iter == 0 { + metrics.LocateMemorystoreRequestDuration.WithLabelValues("get", "all", "OK").Observe(time.Since(t).Seconds()) return values, nil } } diff --git a/memorystore/client_test.go b/memorystore/client_test.go index 99799d2c..b2993f2e 100644 --- a/memorystore/client_test.go +++ b/memorystore/client_test.go @@ -77,6 +77,21 @@ func TestPut_EXPIREError(t *testing.T) { func TestPut_Success(t *testing.T) { conn, client := setUpTest[v2.HeartbeatMessage]() + hset := conn.GenericCommand("HSET").Expect(1) + err := client.Put(testdata.FakeHostname, "Registration", testdata.FakeRegistration.Registration, false) + + if conn.Stats(hset) != 1 { + t.Fatal("Put() failure, HSET command should have been called") + } + + if err != nil { + t.Errorf("Put() error: %+v, want: nil", err) + } +} + +func TestPut_SuccessWithEXPIRE(t *testing.T) { + conn, client := setUpTest[v2.HeartbeatMessage]() + hset := conn.GenericCommand("HSET").Expect(1) expire := conn.GenericCommand("EXPIRE").Expect(1) err := client.Put(testdata.FakeHostname, "Registration", testdata.FakeRegistration.Registration, true) diff --git a/metrics/metrics.go b/metrics/metrics.go index 7514d695..a365c33a 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -54,6 +54,17 @@ var ( []string{"experiment"}, ) + // LocateMemorystoreRequestDuration is a histogram that tracks the latency of + // requests from the Locate to Memorystore. + LocateMemorystoreRequestDuration = promauto.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "locate_memorystore_request_duration", + Help: "A histogram of request latency to Memorystore.", + Buckets: prometheus.LinearBuckets(0, 2, 16), // Buckets from 0 to 30 with steps of size 2. + }, + []string{"type", "field", "status"}, + ) + // ImportMemorystoreTotal counts the number of times the Locate Service has imported // the data in Memorystore. ImportMemorystoreTotal = promauto.NewCounterVec( diff --git a/metrics/metrics_test.go b/metrics/metrics_test.go index 83e789d7..91e7eb1b 100644 --- a/metrics/metrics_test.go +++ b/metrics/metrics_test.go @@ -11,6 +11,7 @@ func TestLintMetrics(t *testing.T) { AppEngineTotal.WithLabelValues("country") CurrentHeartbeatConnections.WithLabelValues("experiment").Set(0) LocateHealthStatus.WithLabelValues("experiment").Set(0) + LocateMemorystoreRequestDuration.WithLabelValues("type", "field", "status") ImportMemorystoreTotal.WithLabelValues("status") PrometheusHealthCollectionDuration.WithLabelValues("code") ServerDistanceRanking.WithLabelValues("index") From 732e54b456c1a4b60f9eae4d15a0e41df161d6ea Mon Sep 17 00:00:00 2001 From: Cristina Leon Date: Mon, 31 Jul 2023 14:52:28 +0000 Subject: [PATCH 2/3] Address comments --- memorystore/client.go | 4 ++-- metrics/metrics.go | 7 ++++--- metrics/metrics_test.go | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/memorystore/client.go b/memorystore/client.go index 4da96ba9..3c844ae9 100644 --- a/memorystore/client.go +++ b/memorystore/client.go @@ -46,11 +46,11 @@ func (c *client[V]) Put(key string, field string, value redis.Scanner, expire bo _, err = conn.Do("EXPIRE", key, static.RedisKeyExpirySecs) if err != nil { - metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "EXPIRE error").Observe(time.Since(t).Seconds()) + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", "expiration", "EXPIRE error").Observe(time.Since(t).Seconds()) return err } - metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "OK").Observe(time.Since(t).Seconds()) + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", "expiration", "OK").Observe(time.Since(t).Seconds()) return nil } diff --git a/metrics/metrics.go b/metrics/metrics.go index a365c33a..5deff029 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -58,9 +58,10 @@ var ( // requests from the Locate to Memorystore. LocateMemorystoreRequestDuration = promauto.NewHistogramVec( prometheus.HistogramOpts{ - Name: "locate_memorystore_request_duration", - Help: "A histogram of request latency to Memorystore.", - Buckets: prometheus.LinearBuckets(0, 2, 16), // Buckets from 0 to 30 with steps of size 2. + Name: "locate_memorystore_request_duration", + Help: "A histogram of request latency to Memorystore.", + Buckets: []float64{.005, .01, .025, .05, .1, .25, .5, 1, + 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30}, }, []string{"type", "field", "status"}, ) diff --git a/metrics/metrics_test.go b/metrics/metrics_test.go index 91e7eb1b..61c62546 100644 --- a/metrics/metrics_test.go +++ b/metrics/metrics_test.go @@ -11,7 +11,7 @@ func TestLintMetrics(t *testing.T) { AppEngineTotal.WithLabelValues("country") CurrentHeartbeatConnections.WithLabelValues("experiment").Set(0) LocateHealthStatus.WithLabelValues("experiment").Set(0) - LocateMemorystoreRequestDuration.WithLabelValues("type", "field", "status") + LocateMemorystoreRequestDuration.WithLabelValues("type", "command", "status") ImportMemorystoreTotal.WithLabelValues("status") PrometheusHealthCollectionDuration.WithLabelValues("code") ServerDistanceRanking.WithLabelValues("index") From 8e421277bae64d33fd2fdf3b62e63e00ecb13919 Mon Sep 17 00:00:00 2001 From: Cristina Leon Date: Mon, 31 Jul 2023 15:03:59 +0000 Subject: [PATCH 3/3] Label value --- memorystore/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/memorystore/client.go b/memorystore/client.go index 3c844ae9..2618e1f4 100644 --- a/memorystore/client.go +++ b/memorystore/client.go @@ -46,11 +46,11 @@ func (c *client[V]) Put(key string, field string, value redis.Scanner, expire bo _, err = conn.Do("EXPIRE", key, static.RedisKeyExpirySecs) if err != nil { - metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", "expiration", "EXPIRE error").Observe(time.Since(t).Seconds()) + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field, "EXPIRE error").Observe(time.Since(t).Seconds()) return err } - metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", "expiration", "OK").Observe(time.Since(t).Seconds()) + metrics.LocateMemorystoreRequestDuration.WithLabelValues("put", field+" with expiration", "OK").Observe(time.Since(t).Seconds()) return nil }