From 6223938ed79ad0505f07b9a9d764af9ef4cc0963 Mon Sep 17 00:00:00 2001 From: Gabi Davar Date: Mon, 9 Sep 2024 09:45:43 +0300 Subject: [PATCH 1/3] Re-implement connected_client to have deterministic cardinality --- Makefile | 1 + docker-compose.yml | 7 +- exporter/clients.go | 285 +++++++++++++++++++++++++++++++++++---- exporter/clients_test.go | 136 ++++++++++++++----- 4 files changed, 368 insertions(+), 61 deletions(-) diff --git a/Makefile b/Makefile index 9a7cb778..50549e30 100644 --- a/Makefile +++ b/Makefile @@ -34,6 +34,7 @@ test: TEST_REDIS_URI="redis://localhost:16384" \ TEST_REDIS5_URI="redis://localhost:16383" \ TEST_REDIS6_URI="redis://localhost:16379" \ + TEST_REDIS74_URI="redis://localhost:16385" \ TEST_VALKEY7_URI="redis://localhost:16384" \ TEST_REDIS_2_8_URI="redis://localhost:16381" \ TEST_KEYDB01_URI="redis://localhost:16401" \ diff --git a/docker-compose.yml b/docker-compose.yml index 9e4202c9..56d952d9 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,4 +1,3 @@ -version: '3' services: valkey7: @@ -20,6 +19,12 @@ services: ports: - "16379:6379" + redis74: + image: redis:7.4 + command: "redis-server --protected-mode no --dbfilename dump74.rdb" + ports: + - "16385:6379" + pwd-redis5: image: redis:5 command: "redis-server --requirepass redis-password --dbfilename dump5-pwd.rdb" diff --git a/exporter/clients.go b/exporter/clients.go index 2df2c44b..2a230da6 100644 --- a/exporter/clients.go +++ b/exporter/clients.go @@ -12,17 +12,26 @@ import ( ) type ClientInfo struct { + Id, Name, User, - CreatedAt, - IdleSince, Flags, Db, - OMem, - Cmd, Host, Port, Resp string + CreatedAt, + IdleSince, + Sub, + Psub, + Ssub, + Watch, + Qbuf, + QbufFree, + Obl, + Oll, + OMem, + TotMem int64 } /* @@ -36,6 +45,8 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { return nil, false } connectedClient := ClientInfo{} + connectedClient.Ssub = -1 // mark it as missing - introduced in Redis 7.0.3 + connectedClient.Watch = -1 // mark it as missing - introduced in Redis 7.4 for _, kvPart := range strings.Split(clientInfo, " ") { vPart := strings.Split(kvPart, "=") if len(vPart) != 2 { @@ -44,6 +55,8 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { } switch vPart[0] { + case "id": + connectedClient.Id = vPart[1] case "name": connectedClient.Name = vPart[1] case "user": @@ -51,14 +64,14 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { case "age": createdAt, err := durationFieldToTimestamp(vPart[1]) if err != nil { - log.Debugf("cloud not parse age field(%s): %s", vPart[1], err.Error()) + log.Debugf("could not parse 'age' field(%s): %s", vPart[1], err.Error()) return nil, false } connectedClient.CreatedAt = createdAt case "idle": idleSinceTs, err := durationFieldToTimestamp(vPart[1]) if err != nil { - log.Debugf("cloud not parse idle field(%s): %s", vPart[1], err.Error()) + log.Debugf("could not parse 'idle' field(%s): %s", vPart[1], err.Error()) return nil, false } connectedClient.IdleSince = idleSinceTs @@ -66,10 +79,76 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { connectedClient.Flags = vPart[1] case "db": connectedClient.Db = vPart[1] + case "sub": + Sub, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'sub' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Sub = Sub + case "psub": + Psub, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'psub' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Psub = Psub + case "ssub": + Ssub, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'ssub' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Ssub = Ssub + case "watch": + Watch, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'watch' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Watch = Watch + case "qbuf": + Qbuf, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'qbuf' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Qbuf = Qbuf + case "qbuf-free": + QbufFree, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'qbuf-free' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.QbufFree = QbufFree + case "obl": + Obl, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'obl' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Obl = Obl + case "oll": + Oll, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'oll' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.Oll = Oll case "omem": - connectedClient.OMem = vPart[1] - case "cmd": - connectedClient.Cmd = vPart[1] + OMem, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'omem' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.OMem = OMem + case "tot-mem": + TotMem, err := strconv.ParseInt(vPart[1], 10, 64) + if err != nil { + log.Debugf("could not parse 'tot-mem' field(%s): %s", vPart[1], err.Error()) + return nil, false + } + connectedClient.TotMem = TotMem case "addr": hostPortString := strings.Split(vPart[1], ":") if len(hostPortString) < 2 { @@ -86,13 +165,12 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { return &connectedClient, true } -func durationFieldToTimestamp(field string) (string, error) { +func durationFieldToTimestamp(field string) (int64, error) { parsed, err := strconv.ParseInt(field, 10, 64) if err != nil { - return "", err + return 0, err } - - return strconv.FormatInt(time.Now().Unix()-parsed, 10), nil + return time.Now().Unix() - parsed, nil } func (e *Exporter) extractConnectedClientMetrics(ch chan<- prometheus.Metric, c redis.Conn) { @@ -103,30 +181,185 @@ func (e *Exporter) extractConnectedClientMetrics(ch chan<- prometheus.Metric, c } for _, c := range strings.Split(reply, "\n") { - if lbls, ok := parseClientListString(c); ok { - connectedClientsLabels := []string{"name", "created_at", "idle_since", "flags", "db", "omem", "cmd", "host"} - connectedClientsLabelsValues := []string{lbls.Name, lbls.CreatedAt, lbls.IdleSince, lbls.Flags, lbls.Db, lbls.OMem, lbls.Cmd, lbls.Host} + if info, ok := parseClientListString(c); ok { + clientInfoLabels := []string{"id", "name", "flags", "db", "host"} + clientInfoLabelsValues := []string{info.Id, info.Name, info.Flags, info.Db, info.Host} if e.options.ExportClientsInclPort { - connectedClientsLabels = append(connectedClientsLabels, "port") - connectedClientsLabelsValues = append(connectedClientsLabelsValues, lbls.Port) + clientInfoLabels = append(clientInfoLabels, "port") + clientInfoLabelsValues = append(clientInfoLabelsValues, info.Port) } - if user := lbls.User; user != "" { - connectedClientsLabels = append(connectedClientsLabels, "user") - connectedClientsLabelsValues = append(connectedClientsLabelsValues, user) + if user := info.User; user != "" { + clientInfoLabels = append(clientInfoLabels, "user") + clientInfoLabelsValues = append(clientInfoLabelsValues, user) } - if resp := lbls.Resp; resp != "" { - connectedClientsLabels = append(connectedClientsLabels, "resp") - connectedClientsLabelsValues = append(connectedClientsLabelsValues, resp) + // introduced in Redis 7.0 + if resp := info.Resp; resp != "" { + clientInfoLabels = append(clientInfoLabels, "resp") + clientInfoLabelsValues = append(clientInfoLabelsValues, resp) } - e.metricDescriptions["connected_clients_details"] = newMetricDescr(e.options.Namespace, "connected_clients_details", "Details about connected clients", connectedClientsLabels) + e.metricDescriptions["connected_client_info"] = newMetricDescr( + e.options.Namespace, + "connected_client_info", + "Details about a connected client", + clientInfoLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_info", 1.0, + clientInfoLabelsValues..., + ) + // keep the old name for backwards compatability + e.metricDescriptions["connected_clients_details"] = newMetricDescr( + e.options.Namespace, + "connected_clients_details", + "Details about a connected client", + clientInfoLabels, + ) e.registerConstMetricGauge( ch, "connected_clients_details", 1.0, - connectedClientsLabelsValues..., + clientInfoLabelsValues..., + ) + + clientBaseLabels := []string{"id", "name"} + clientBaseLabelsValues := []string{info.Id, info.Name} + + e.metricDescriptions["connected_client_output_buffer_memory_usage_bytes"] = newMetricDescr( + e.options.Namespace, + "connected_client_output_buffer_memory_usage_bytes", + "A connected client's output buffer memory usage in bytes", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_output_buffer_memory_usage_bytes", float64(info.OMem), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_total_memory_consumed_bytes"] = newMetricDescr( + e.options.Namespace, + "connected_client_total_memory_consumed_bytes", + "Total memory consumed by a client in its various buffers", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_total_memory_consumed_bytes", float64(info.TotMem), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_created_at_timestamp"] = newMetricDescr( + e.options.Namespace, + "connected_client_created_at_timestamp", + "A connected client's creation timestamp", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_created_at_timestamp", float64(info.CreatedAt), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_idle_since_timestamp"] = newMetricDescr( + e.options.Namespace, + "connected_client_idle_since_timestamp", + "A connected client's idle since timestamp", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_idle_since_timestamp", float64(info.IdleSince), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_channel_subscriptions_count"] = newMetricDescr( + e.options.Namespace, + "connected_client_channel_subscriptions_count", + "A connected client's number of channel subscriptions", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_channel_subscriptions_count", float64(info.Sub), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_pattern_matching_subscriptions_count"] = newMetricDescr( + e.options.Namespace, + "connected_client_pattern_matching_subscriptions_count", + "A connected client's number of pattern matching subscriptions", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_pattern_matching_subscriptions_count", float64(info.Psub), + clientBaseLabelsValues..., + ) + + if info.Ssub != -1 { + e.metricDescriptions["connected_client_shard_channel_subscriptions_count"] = newMetricDescr( + e.options.Namespace, + "connected_client_shard_channel_subscriptions_count", + "a connected client's number of shard channel subscriptions", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_shard_channel_subscriptions_count", float64(info.Ssub), + clientBaseLabelsValues..., + ) + } + if info.Watch != -1 { + e.metricDescriptions["connected_client_shard_channel_watched_keys"] = newMetricDescr( + e.options.Namespace, + "connected_client_shard_channel_watched_keys", + "a connected client's number of keys it's currently watching", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_shard_channel_watched_keys", float64(info.Watch), + clientBaseLabelsValues..., + ) + } + + e.metricDescriptions["connected_client_query_buffer_length_bytes"] = newMetricDescr( + e.options.Namespace, + "connected_client_query_buffer_length_bytes", + "A connected client's query buffer length in bytes (0 means no query pending)", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_query_buffer_length_bytes", float64(info.Qbuf), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_query_buffer_free_space_bytes"] = newMetricDescr( + e.options.Namespace, + "connected_client_query_buffer_free_space_bytes", + "A connected client's free space of the query buffer in bytes (0 means the buffer is full)", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_query_buffer_free_space_bytes", float64(info.QbufFree), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_output_buffer_length_bytes"] = newMetricDescr( + e.options.Namespace, + "connected_client_output_buffer_length_bytes", + "A connected client's output buffer length in bytes", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_output_buffer_length_bytes", float64(info.Obl), + clientBaseLabelsValues..., + ) + + e.metricDescriptions["connected_client_output_list_length"] = newMetricDescr( + e.options.Namespace, + "connected_client_output_list_length", + "A connected client's output list length (replies are queued in this list when the buffer is full)", + clientBaseLabels, + ) + e.registerConstMetricGauge( + ch, "connected_client_output_list_length", float64(info.Oll), + clientBaseLabelsValues..., ) } } diff --git a/exporter/clients_test.go b/exporter/clients_test.go index 203e8869..966e9a8e 100644 --- a/exporter/clients_test.go +++ b/exporter/clients_test.go @@ -2,7 +2,6 @@ package exporter import ( "os" - "strconv" "strings" "testing" "time" @@ -39,19 +38,15 @@ func TestDurationFieldToTimestamp(t *testing.T) { t.Fatalf("expected ok, but got error: %s, input: [%s]", err, tst.in) } if tst.expectedOk { - resInt64, err := strconv.ParseInt(res, 10, 64) - if err != nil { - t.Fatalf("ParseInt( %s ) err: %s", res, err) - } - if resInt64 != tst.expectedVal { - t.Fatalf("expected %d, but got: %d", tst.expectedVal, resInt64) + if res != tst.expectedVal { + t.Fatalf("expected %d, but got: %d", tst.expectedVal, res) } } } } func TestParseClientListString(t *testing.T) { - convertDurationToTimestampString := func(duration string) string { + convertDurationToTimestampInt64 := func(duration string) int64 { ts, err := durationFieldToTimestamp(duration) if err != nil { panic(err) @@ -62,29 +57,29 @@ func TestParseClientListString(t *testing.T) { tsts := []struct { in string expectedOk bool - expectedLbls ClientInfo + expectedInfo ClientInfo }{ { - in: "id=11 addr=127.0.0.1:63508 fd=8 name= age=6321 idle=6320 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=0 qbuf-free=0 obl=0 oll=0 omem=0 events=r cmd=setex", + in: "id=11 addr=127.0.0.1:63508 fd=8 name= age=6321 idle=6320 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=0 qbuf-free=0 obl=3 oll=8 omem=0 tot-mem=0 events=r cmd=setex", expectedOk: true, - expectedLbls: ClientInfo{CreatedAt: convertDurationToTimestampString("6321"), IdleSince: convertDurationToTimestampString("6320"), Flags: "N", Db: "0", OMem: "0", Cmd: "setex", Host: "127.0.0.1", Port: "63508"}, + expectedInfo: ClientInfo{Id: "11", CreatedAt: convertDurationToTimestampInt64("6321"), IdleSince: convertDurationToTimestampInt64("6320"), Flags: "N", Db: "0", Ssub: -1, Watch: -1, Obl: 3, Oll: 8, OMem: 0, TotMem: 0, Host: "127.0.0.1", Port: "63508"}, }, { - in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 events=r cmd=client", + in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: true, - expectedLbls: ClientInfo{Name: "foo", CreatedAt: convertDurationToTimestampString("5"), IdleSince: convertDurationToTimestampString("0"), Flags: "N", Db: "1", OMem: "0", Cmd: "client", Host: "127.0.0.1", Port: "64958"}, + expectedInfo: ClientInfo{Id: "14", Name: "foo", CreatedAt: convertDurationToTimestampInt64("5"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "1", Ssub: -1, Watch: -1, Qbuf: 26, QbufFree: 32742, OMem: 0, TotMem: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64959 fd=9 name= age=5 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 events=r cmd=client user=default resp=3", + in: "id=14 addr=127.0.0.1:64959 fd=9 name= age=5 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client user=default resp=3", expectedOk: true, - expectedLbls: ClientInfo{CreatedAt: convertDurationToTimestampString("5"), IdleSince: convertDurationToTimestampString("0"), Flags: "N", Db: "0", OMem: "0", Cmd: "client", Host: "127.0.0.1", Port: "64959", User: "default", Resp: "3"}, + expectedInfo: ClientInfo{Id: "14", CreatedAt: convertDurationToTimestampInt64("5"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "0", Ssub: -1, Watch: -1, Qbuf: 26, QbufFree: 32742, OMem: 0, TotMem: 0, Host: "127.0.0.1", Port: "64959", User: "default", Resp: "3"}, }, { - in: "id=40253233 addr=fd40:1481:21:dbe0:7021:300:a03:1a06:44426 fd=19 name= age=782 idle=0 flags=N db=0 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 argv-mem=10 obl=0 oll=0 omem=0 tot-mem=61466 ow=0 owmem=0 events=r cmd=client user=default lib-name=redis-py lib-ver=5.0.1 numops=9", + in: "id=40253233 addr=fd40:1481:21:dbe0:7021:300:a03:1a06:44426 fd=19 name= age=782 idle=0 flags=N db=0 sub=0 psub=0 ssub=17 watch=3 multi=-1 qbuf=26 qbuf-free=32742 argv-mem=10 obl=0 oll=0 omem=0 tot-mem=61466 ow=0 owmem=0 events=r cmd=client user=default lib-name=redis-py lib-ver=5.0.1 numops=9", expectedOk: true, - expectedLbls: ClientInfo{CreatedAt: convertDurationToTimestampString("782"), IdleSince: convertDurationToTimestampString("0"), Flags: "N", Db: "0", OMem: "0", Cmd: "client", Host: "fd40:1481:21:dbe0:7021:300:a03:1a06", Port: "44426", User: "default"}, + expectedInfo: ClientInfo{Id: "40253233", CreatedAt: convertDurationToTimestampInt64("782"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "0", Ssub: 17, Watch: 3, Qbuf: 26, QbufFree: 32742, OMem: 0, TotMem: 61466, Host: "fd40:1481:21:dbe0:7021:300:a03:1a06", Port: "44426", User: "default"}, }, { - in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=ABCDE idle=0 flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 events=r cmd=client", + in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=ABCDE idle=0 flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: false, }, { - in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=NOPE flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 events=r cmd=client", + in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=NOPE flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: false, }, { in: "", @@ -93,7 +88,7 @@ func TestParseClientListString(t *testing.T) { } for _, tst := range tsts { - lbls, ok := parseClientListString(tst.in) + info, ok := parseClientListString(tst.in) if !tst.expectedOk { if ok { t.Errorf("expected NOT ok, but got ok, input: %s", tst.in) @@ -101,8 +96,8 @@ func TestParseClientListString(t *testing.T) { continue } - if *lbls != tst.expectedLbls { - t.Errorf("TestParseClientListString( %s ) error. Given: %s Wanted: %s", tst.in, lbls, tst.expectedLbls) + if *info != tst.expectedInfo { + t.Errorf("TestParseClientListString( %s ) error. Given: %#v Wanted: %#v", tst.in, info, tst.expectedInfo) } } } @@ -120,17 +115,51 @@ func TestExportClientList(t *testing.T) { close(chM) }() - found := false + tsts := []struct { + in string + found bool + }{ + { + in: "connected_client_info", + }, { + in: "connected_clients_details", + }, { + in: "connected_client_output_buffer_memory_usage_bytes", + }, { + in: "connected_client_total_memory_consumed_bytes", + }, { + in: "connected_client_created_at_timestamp", + }, { + in: "connected_client_idle_since_timestamp", + }, { + in: "connected_client_channel_subscriptions_count", + }, { + in: "connected_client_pattern_matching_subscriptions_count", + }, { + in: "connected_client_query_buffer_length_bytes", + }, { + in: "connected_client_query_buffer_free_space_bytes", + }, { + in: "connected_client_output_buffer_length_bytes", + }, { + in: "connected_client_output_list_length", + }, + } for m := range chM { - if strings.Contains(m.Desc().String(), "connected_clients_details") { - found = true + desc := m.Desc().String() + for i := range tsts { + if strings.Contains(desc, tsts[i].in) { + tsts[i].found = true + } } } - if isExportClientList && !found { - t.Errorf("connected_clients_details was *not* found in isExportClientList metrics but expected") - } else if !isExportClientList && found { - t.Errorf("connected_clients_details was *found* in isExportClientList metrics but *not* expected") + for _, tst := range tsts { + if isExportClientList && !tst.found { + t.Errorf("%s was *not* found in isExportClientList metrics but expected", tst.in) + } else if !isExportClientList && tst.found { + t.Errorf("%s was *found* in isExportClientList metrics but *not* expected", tst.in) + } } } } @@ -152,7 +181,7 @@ func TestExportClientListInclPort(t *testing.T) { found := false for m := range chM { desc := m.Desc().String() - if strings.Contains(desc, "connected_clients_details") { + if strings.Contains(desc, "connected_client_info") { if strings.Contains(desc, "port") { found = true } @@ -160,9 +189,48 @@ func TestExportClientListInclPort(t *testing.T) { } if inclPort && !found { - t.Errorf(`connected_clients_details did *not* include "port" in isExportClientList metrics but was expected`) + t.Errorf(`connected_client_info did *not* include "port" in isExportClientList metrics but was expected`) } else if !inclPort && found { - t.Errorf(`connected_clients_details did *include* "port" in isExportClientList metrics but was *not* expected`) + t.Errorf(`connected_client_info did *include* "port" in isExportClientList metrics but was *not* expected`) + } + } +} + +func TestExportClientListRedis7(t *testing.T) { + redisSevenAddr := os.Getenv("TEST_REDIS74_URI") + e := getTestExporterWithAddrAndOptions(redisSevenAddr, Options{ + Namespace: "test", Registry: prometheus.NewRegistry(), + ExportClientList: true, + }) + + chM := make(chan prometheus.Metric) + go func() { + e.Collect(chM) + close(chM) + }() + + tsts := []struct { + in string + found bool + }{ + { + in: "connected_client_shard_channel_subscriptions_count", + }, { + in: "connected_client_shard_channel_watched_keys", + }, + } + for m := range chM { + desc := m.Desc().String() + for i := range tsts { + if strings.Contains(desc, tsts[i].in) { + tsts[i].found = true + } + } + } + + for _, tst := range tsts { + if !tst.found { + t.Errorf(`%s was *not* found in isExportClientList metrics but expected`, tst.in) } } } @@ -183,7 +251,7 @@ func TestExportClientListResp(t *testing.T) { found := false for m := range chM { desc := m.Desc().String() - if strings.Contains(desc, "connected_clients_details") { + if strings.Contains(desc, "connected_client_info") { if strings.Contains(desc, "resp") { found = true } @@ -191,6 +259,6 @@ func TestExportClientListResp(t *testing.T) { } if !found { - t.Errorf(`connected_clients_details did *not* include "resp" in isExportClientList metrics but was expected`) + t.Errorf(`connected_client_info did *not* include "resp" in isExportClientList metrics but was expected`) } } From 4f2bf38e2d926fb5989264d59dda9c6c41a46430 Mon Sep 17 00:00:00 2001 From: Gabi Davar Date: Fri, 20 Sep 2024 13:30:16 +0300 Subject: [PATCH 2/3] More tests, remove backwards compatibility metrics --- exporter/clients.go | 12 ------------ exporter/clients_test.go | 36 ++++++++++++++++++++++++++++++++---- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/exporter/clients.go b/exporter/clients.go index 2a230da6..be5bdcc8 100644 --- a/exporter/clients.go +++ b/exporter/clients.go @@ -212,18 +212,6 @@ func (e *Exporter) extractConnectedClientMetrics(ch chan<- prometheus.Metric, c clientInfoLabelsValues..., ) - // keep the old name for backwards compatability - e.metricDescriptions["connected_clients_details"] = newMetricDescr( - e.options.Namespace, - "connected_clients_details", - "Details about a connected client", - clientInfoLabels, - ) - e.registerConstMetricGauge( - ch, "connected_clients_details", 1.0, - clientInfoLabelsValues..., - ) - clientBaseLabels := []string{"id", "name"} clientBaseLabelsValues := []string{info.Id, info.Name} diff --git a/exporter/clients_test.go b/exporter/clients_test.go index 966e9a8e..cd893089 100644 --- a/exporter/clients_test.go +++ b/exporter/clients_test.go @@ -72,15 +72,45 @@ func TestParseClientListString(t *testing.T) { expectedOk: true, expectedInfo: ClientInfo{Id: "14", CreatedAt: convertDurationToTimestampInt64("5"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "0", Ssub: -1, Watch: -1, Qbuf: 26, QbufFree: 32742, OMem: 0, TotMem: 0, Host: "127.0.0.1", Port: "64959", User: "default", Resp: "3"}, }, { - in: "id=40253233 addr=fd40:1481:21:dbe0:7021:300:a03:1a06:44426 fd=19 name= age=782 idle=0 flags=N db=0 sub=0 psub=0 ssub=17 watch=3 multi=-1 qbuf=26 qbuf-free=32742 argv-mem=10 obl=0 oll=0 omem=0 tot-mem=61466 ow=0 owmem=0 events=r cmd=client user=default lib-name=redis-py lib-ver=5.0.1 numops=9", + in: "id=40253233 addr=fd40:1481:21:dbe0:7021:300:a03:1a06:44426 fd=19 name= age=782 idle=0 flags=N db=0 sub=896 psub=18 ssub=17 watch=3 multi=-1 qbuf=26 qbuf-free=32742 argv-mem=10 obl=0 oll=555 omem=0 tot-mem=61466 ow=0 owmem=0 events=r cmd=client user=default lib-name=redis-py lib-ver=5.0.1 numops=9", expectedOk: true, - expectedInfo: ClientInfo{Id: "40253233", CreatedAt: convertDurationToTimestampInt64("782"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "0", Ssub: 17, Watch: 3, Qbuf: 26, QbufFree: 32742, OMem: 0, TotMem: 61466, Host: "fd40:1481:21:dbe0:7021:300:a03:1a06", Port: "44426", User: "default"}, + expectedInfo: ClientInfo{Id: "40253233", CreatedAt: convertDurationToTimestampInt64("782"), IdleSince: convertDurationToTimestampInt64("0"), Flags: "N", Db: "0", Sub: 896, Psub: 18, Ssub: 17, Watch: 3, Qbuf: 26, QbufFree: 32742, Oll: 555, OMem: 0, TotMem: 61466, Host: "fd40:1481:21:dbe0:7021:300:a03:1a06", Port: "44426", User: "default"}, }, { in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=ABCDE idle=0 flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: false, }, { in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=NOPE flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=ERR psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=0 psub=ERR multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 ssub=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 watch=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 qbuf=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 qbuf-free=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 obl=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 oll=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 omem=ERR", + expectedOk: false, + }, { + in: "id=14 addr=127.0.0.1:64958 tot-mem=ERR", + expectedOk: false, }, { in: "", expectedOk: false, @@ -121,8 +151,6 @@ func TestExportClientList(t *testing.T) { }{ { in: "connected_client_info", - }, { - in: "connected_clients_details", }, { in: "connected_client_output_buffer_memory_usage_bytes", }, { From 4ff27955cd304007281db4c3c297cb0251190028 Mon Sep 17 00:00:00 2001 From: Gabi Davar Date: Mon, 23 Sep 2024 14:27:11 +0300 Subject: [PATCH 3/3] simplify int parsing, update tests Signed-off-by: Gabi Davar --- exporter/clients.go | 70 ++++++---------------------------------- exporter/clients_test.go | 50 ++++++++++++++++------------ 2 files changed, 40 insertions(+), 80 deletions(-) diff --git a/exporter/clients.go b/exporter/clients.go index be5bdcc8..d8d919c7 100644 --- a/exporter/clients.go +++ b/exporter/clients.go @@ -80,75 +80,25 @@ func parseClientListString(clientInfo string) (*ClientInfo, bool) { case "db": connectedClient.Db = vPart[1] case "sub": - Sub, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'sub' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Sub = Sub + connectedClient.Sub, _ = strconv.ParseInt(vPart[1], 10, 64) case "psub": - Psub, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'psub' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Psub = Psub + connectedClient.Psub, _ = strconv.ParseInt(vPart[1], 10, 64) case "ssub": - Ssub, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'ssub' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Ssub = Ssub + connectedClient.Ssub, _ = strconv.ParseInt(vPart[1], 10, 64) case "watch": - Watch, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'watch' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Watch = Watch + connectedClient.Watch, _ = strconv.ParseInt(vPart[1], 10, 64) case "qbuf": - Qbuf, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'qbuf' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Qbuf = Qbuf + connectedClient.Qbuf, _ = strconv.ParseInt(vPart[1], 10, 64) case "qbuf-free": - QbufFree, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'qbuf-free' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.QbufFree = QbufFree + connectedClient.QbufFree, _ = strconv.ParseInt(vPart[1], 10, 64) case "obl": - Obl, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'obl' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Obl = Obl + connectedClient.Obl, _ = strconv.ParseInt(vPart[1], 10, 64) case "oll": - Oll, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'oll' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.Oll = Oll + connectedClient.Oll, _ = strconv.ParseInt(vPart[1], 10, 64) case "omem": - OMem, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'omem' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.OMem = OMem + connectedClient.OMem, _ = strconv.ParseInt(vPart[1], 10, 64) case "tot-mem": - TotMem, err := strconv.ParseInt(vPart[1], 10, 64) - if err != nil { - log.Debugf("could not parse 'tot-mem' field(%s): %s", vPart[1], err.Error()) - return nil, false - } - connectedClient.TotMem = TotMem + connectedClient.TotMem, _ = strconv.ParseInt(vPart[1], 10, 64) case "addr": hostPortString := strings.Split(vPart[1], ":") if len(hostPortString) < 2 { diff --git a/exporter/clients_test.go b/exporter/clients_test.go index cd893089..0a288d77 100644 --- a/exporter/clients_test.go +++ b/exporter/clients_test.go @@ -82,35 +82,45 @@ func TestParseClientListString(t *testing.T) { in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=NOPE flags=N db=1 sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", expectedOk: false, }, { - in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=ERR psub=0 multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 sub=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Sub: 0, Ssub: -1, Watch: -1, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 fd=9 name=foo age=5 idle=0 flags=N db=1 sub=0 psub=ERR multi=-1 qbuf=26 qbuf-free=32742 obl=0 oll=0 omem=0 tot-mem=0 events=r cmd=client", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 psub=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Psub: 0, Ssub: -1, Watch: -1, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 ssub=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 ssub=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: 0, Watch: -1, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 watch=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 watch=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 qbuf=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 qbuf=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, Qbuf: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 qbuf-free=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 qbuf-free=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, QbufFree: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 obl=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 obl=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, Obl: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 oll=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 oll=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, Oll: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 omem=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 omem=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, OMem: 0, Host: "127.0.0.1", Port: "64958"}, }, { - in: "id=14 addr=127.0.0.1:64958 tot-mem=ERR", - expectedOk: false, + in: "id=14 addr=127.0.0.1:64958 tot-mem=ERR", + expectedOk: true, + expectedInfo: ClientInfo{Id: "14", Ssub: -1, Watch: -1, TotMem: 0, Host: "127.0.0.1", Port: "64958"}, }, { in: "", expectedOk: false,