feat: implement valkey and rueidis sentry instrumentation by Shikachuu · Pull Request #1282 · getsentry/sentry-go

Shikachuu · 2026-04-25T12:21:02Z

Implemented Redis and Valkey (collectively redis like) database and cache layer instrumentation.

Redis implementation is using the upstream https://github.com/redis/rueidis library, it's a faster alternative to redis-go and almost 1/1 compatible with valkey-go.

Valkey implementation using the upstream https://github.com/valkey-io/valkey-go library.

All implementations are built around the upstream library's hook system to avoid collision with otel, while still providing the leanest possible downstream interface.

All common logic has been extracted to an internal/redis folder to help us maintain feature parity between implementation.

PS: This ended up being a much larger PR that I've initially anticipated, lmk if we should split this into 2 or 3 PRs.

Issues

resolves Valkey SDK cache/db support #1268
resolves GO-136

Changelog Entry Instructions

To add a custom changelog entry, uncomment the section above. Supports:

Single entry: just write text
Multiple entries: use bullet points
Nested bullets: indent 4+ spaces

For more details: custom changelog entries

Reminders

Add GH Issue ID & Linear ID
PR title should use conventional commit style (feat:, fix:, ref:, meta:)
For external contributors: CONTRIBUTING.md, Sentry SDK development docs, Discord community

Signed-off-by: Máté Czékus <mate@picloud.hu>

Shikachuu · 2026-04-25T12:28:24Z

Also here are some sentry screenshots how the traces looks like in practice, I've managed to fix database query recognition for the db insight module.

As well as followed the practices demonstrated in other upstream Sentry SDK implementations such as laravel and nextjs, so we have the cache insight module function correctly as well, you can try them by running the examples.

We the default metric naming and labels uses the cache one, however we can change it via changing the Type field in valkey.Options eg: sentryvalkey.Options{Type: sentryvalkey.TypeDB}

Signed-off-by: Máté Czékus <mate@picloud.hu>

sentry · 2026-04-25T12:56:03Z

+		}
+		portNum, err := strconv.Atoi(p)
+		if err != nil {
+			return redis.Address{Host: h}
+		}
+		return redis.Address{Host: h, Port: portNum}
+	}


Bug: In a multi-node cluster, extractAddr non-deterministically returns a node address due to randomized map iteration, affecting observability data consistency.
_{Severity: LOW}

Suggested Fix

To ensure deterministic address extraction in cluster mode, retrieve the keys from the client.Nodes() map, sort them alphabetically, and then select the first node from the sorted list. This will provide a consistent address for observability.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: valkey/sentryvalkey.go#L216-L222 Potential issue: The `extractAddr` function iterates over the map returned by `client.Nodes()` to retrieve a node address. In Go, map iteration order is randomized. Consequently, when operating in a Redis/Valkey cluster with multiple nodes, this function will non-deterministically return the address of a different node on each execution. This behavior does not cause crashes or functional failures but results in inconsistent node addresses being reported in observability spans. The issue is confined to cluster deployments and does not affect more common single-node configurations.

Also affects:

rueidis/sentryrueidis.go:216~222

giortzisg

Hey @Shikachuu,

Thanks for taking the time. Had an initial first pass through the code and raised a few things. I want to go a bit deeper on commandPatterns on the next pass to properly validate each command, but leaving an initial review to keep things moving.

giortzisg · 2026-04-29T09:50:57Z

+}
+
+//nolint:revive // ctx position is dictated by rueidishook.Hook interface.
+func (h *sentryHook) DoMulti(client rueidis.Client, ctx context.Context, multi ...rueidis.Completed) []rueidis.RedisResult {


is there a way to wrap what redis actually executes here? Creating a single span here seems misleading and duration would probably be inaccurate.

Also we don't really have a pipeline span convention, so I feel like this is not exactly what we want.

ahh so you mean every single call should be it's fully own span in a sense of there should be 0 difference between a DoMulti w 5 commands and calling Do 5 times?

Yes that was the idea, I feel it's easier to understand and trace through the logic, because each Do is a separate command. Conceptually it makes sense to me to have a multi span that spawns 5 more spans for each Do. Might be wrong though so feel free to correct me here.

Actually, since this is a single network call, so a single span is the correct approach. So we can keep it as is.

giortzisg · 2026-04-29T10:29:27Z

+
+	result := client.Do(span.Context(), cmd)
+
+	redis.FinishSpan(span, h.typ, cmd.IsReadOnly(), result.Error(), rueidis.IsRedisNil(result.Error()), respSize(result))


Does rueidis.IsRedisNil cover every case for a cache miss?

RESP3 protocol treats all classic cache miss scenarios like missing key etc. nil responses, every other error would be either be a non-cache related command's logical error or network error afaik.

i might be wrong here 🤔

giortzisg · 2026-04-29T10:40:00Z

+		if isReadOnly {
+			return "cache.get"
+		}
+		return "cache.put"


it seems too broad to default to only those two here.

fair point! there were multiple logical issues w my impl. if we compare it to python (it was way less strict), let me summarize, what has been done in: 64d532f

i've refactored the whole instrumentation and operation logic to match the python implementation! first of all spanOp is now a fully exhaustive switch

besides that commands that has nothing to do w caching like ECHO or PING etc. aren't getting any spans in TypeCache mode to 1:1 match the Python implementation

tests were also refactored to match this new mentality

cache.flush was completely missing, so it has been implemented too

Signed-off-by: Máté Czékus <mate@picloud.hu>

cursor · 2026-05-01T15:01:00Z

+	pattern, known := commandPatterns[cmdName]
+	if !known {
+		// Unknown commands: best-effort, return first arg as key.
+		return []string{cmds[1]}


Unknown commands in cache mode leak PII

High Severity

ExtractKeys returns the first argument of unknown commands as a key. In cache mode, this value flows into the span description (via spanDescription) and the cache.key span data (via setSpanData), both sent to Sentry. The first argument of an unknown command could contain sensitive data (passwords, tokens, etc.), causing PII to leak. For unknown commands, ExtractKeys returning a non-empty slice also prevents spanOp from returning "", so the command gets instrumented instead of being skipped. Returning nil instead would both prevent PII leakage and correctly skip instrumentation.

Additional Locations (2)

internal/redis/span.go#L136-L140

internal/redis/span.go#L170-L173

^{Reviewed by Cursor Bugbot for commit ff5ba75. Configure here.}

This sounds legit and I agree that for unknown spans it's better to skip instrumenting @Shikachuu

Signed-off-by: Máté Czékus <mate@picloud.hu>

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit c820c0d. Configure here.}

cursor · 2026-05-01T15:21:55Z

+		return redis.Address{Host: h, Port: portNum}
+	}
+	return redis.Address{}
+}


Non-deterministic map iteration in extractAddr for clusters

Medium Severity

extractAddr iterates over client.Nodes() (a map[string]Client) with range and returns on the first iteration. Go map iteration order is non-deterministic, so in a cluster setup with multiple nodes, repeated calls to extractAddr will randomly return different node addresses. Since extractAddr is called on every Do, DoCache, DoMulti, DoMultiCache, and Receive invocation, the server.address / network.peer.address span metadata becomes inconsistent and unreliable across spans for the same client, which could confuse users and skew observability data.

Additional Locations (1)

valkey/sentryvalkey.go#L183-L197

^{Reviewed by Cursor Bugbot for commit c820c0d. Configure here.}

codecov · 2026-05-20T10:57:12Z

Codecov Report

❌ Patch coverage is 77.96610% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.35%. Comparing base (1ce3436) to head (32cecbe).
⚠️ Report is 118 commits behind head on master.

Files with missing lines	Patch %	Lines
rueidis/sentryrueidis.go	55.10%	42 Missing and 2 partials ⚠️
valkey/sentryvalkey.go	55.10%	42 Missing and 2 partials ⚠️
internal/redis/span.go	98.06%	2 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (1ce3436) and HEAD (32cecbe). Click for more details.

HEAD has 8 uploads less than BASE

Flag BASE (1ce3436) HEAD (32cecbe)

9 1

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   86.04%   80.35%   -5.70%     
==========================================
  Files          62       84      +22     
  Lines        6090     8148    +2058     
==========================================
+ Hits         5240     6547    +1307     
- Misses        635     1328     +693     
- Partials      215      273      +58

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

giortzisg · 2026-05-20T11:20:08Z

+	// Pub/Sub
+	"SUBSCRIBE":    {repeating: []argRole{roleKey}},
+	"UNSUBSCRIBE":  {repeating: []argRole{roleKey}},
+	"PSUBSCRIBE":   {repeating: []argRole{roleKey}},
+	"PUNSUBSCRIBE": {repeating: []argRole{roleKey}},
+	"PUBLISH":      {fixed: []argRole{roleKey, roleValue}},


these should be under roleField right?

giortzisg

Left some more comments around scrubbing

giortzisg · 2026-05-20T11:32:20Z

+	parentSpan := redis.StartPipelineSpan(ctx, h.typ, redis.DBSystemRedis, cmdsSlice, addr)
+	defer redis.FinishIfNotNil(parentSpan)
+
+	results := client.DoMulti(redis.SpanContext(ctx, parentSpan), multi...)
+
+	hasError := false
+	for i, cmd := range multi {
+		cmds := cmd.Commands()
+		childSpan := redis.StartChildSpan(parentSpan, h.typ, redis.DBSystemRedis, cmds, cmd.IsReadOnly(), addr)
+		err := results[i].Error()
+		isNil := rueidis.IsRedisNil(err)
+		if err != nil && !isNil {
+			hasError = true
+		}
+		redis.FinishSpan(childSpan, h.typ, cmd.IsReadOnly(), err, isNil, respSize(results[i]))
+		redis.FinishIfNotNil(childSpan)
+	}


Currently in this part of the code, child spans would start after the redis client already finished, so timing would be incorrect here. Also as mentioned in #1282 (comment), I think it's better to have a single multi span, since it better illustrates what actually happened in the code. I need to get back to you for what span data we should set, because it might be nice to have something like cache.commands: ["SET", "GET", "DEL"].

giortzisg · 2026-05-20T11:36:21Z

+		b.WriteByte(' ')
+
+		role := roleForPosition(i, fixedLen, pattern)
+		if role == roleKey || (sendDefaultPII && role == roleField) {


This misses scrubbing keys like session:<sess-id>, where the key also holds PII.

giortzisg · 2026-05-20T11:58:11Z

+	"SET":         {fixed: []argRole{roleKey, roleValue}, tailRole: roleField},
+	"SETNX":       {fixed: []argRole{roleKey, roleValue}},
+	"SETEX":       {fixed: []argRole{roleKey, roleField, roleValue}},
+	"PSETEX":      {fixed: []argRole{roleKey, roleField, roleValue}},


I feel that this approach is not really extendable and quite error prone. Brainstorming a bit for a potential improvement:

var commandPatterns = map[string]commandSpec{ "GET": read(firstKey), "MGET": read(allKeys), "SET": write(firstKey), "AUTH": noop(), // skip command } type pattern struct { op cacheOp // roleKeys would be the current command key pattern with firstKey, allKeys, noKeys, etc roleKeys func(args []string) []string } func read(keys func([]string) []string) commandSpec { return pattern{op: cacheGet, keys: keys} } func write(keys func([]string) []string) commandSpec { return pattern{op: cachePut, keys: keys} } // and then scrubbing becomes extendable // scrub gets called with // scrub(cmds, commandPatterns[cmd[0]], PII) func scrub(cmds []string, roles []argRole, sendPII bool) string { for each cmd { switch { case role == roleKey: b.WriteString(scrubKey(arg, sendPII)) case role == roleField && sendPII: b.WriteString(arg) default: b.WriteByte('?') } } return b.String() }

One more thing here, we would also need to implement a ScrubKey for scrubbing keys like session:<sess-id> to session:?. Mentioned #1282 (comment)

Shikachuu added 7 commits April 25, 2026 11:16

feat: init valkey-go insturmentation hooks

52c143e

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: init rueidis instrumentation hooks

61c0ef4

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: init internal redis instrumentation and constructors

9c22cb6

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: add redis instrumentation logic for both db and cache

72faf7a

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: implement rueidis hook instrumentation

eb58af2

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: implement valkey-go hook instrumentation

6c236eb

Signed-off-by: Máté Czékus <mate@picloud.hu>

chore: add examples for both rueidis and valkey

7b55587

Signed-off-by: Máté Czékus <mate@picloud.hu>

sentry Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread valkey/sentryvalkey.go Outdated

Comment thread valkey/sentryvalkey.go

cursor Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread rueidis/sentryrueidis.go Outdated

Comment thread internal/redis/types.go Outdated

Comment thread internal/redis/scrub.go Outdated

Shikachuu added 2 commits April 25, 2026 14:35

fix: remove unused timeout field, it's handled by sentry client

3ba8f89

Signed-off-by: Máté Czékus <mate@picloud.hu>

fix: scrub all params for ZADD syntax

600bd93

Signed-off-by: Máté Czékus <mate@picloud.hu>

sentry Bot reviewed Apr 25, 2026

View reviewed changes

cursor Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread internal/redis/scrub.go Outdated

giortzisg reviewed Apr 29, 2026

View reviewed changes

chore: add separate craft entries to rueidis and valkey

8b0b7a0

Signed-off-by: Máté Czékus <mate@picloud.hu>

sentry Bot reviewed May 1, 2026

View reviewed changes

Comment thread rueidis/sentryrueidis.go Outdated

feat: handle SendDefaultPII flag, extend test cases for scrub

2ae115d

Signed-off-by: Máté Czékus <mate@picloud.hu>

cursor Bot reviewed May 1, 2026

View reviewed changes

Comment thread rueidis/sentryrueidis.go

feat: handle flush type of operations

ca1a51d

Signed-off-by: Máté Czékus <mate@picloud.hu>

sentry Bot reviewed May 1, 2026

View reviewed changes

Comment thread internal/redis/span.go

Shikachuu added 2 commits May 1, 2026 15:48

refactor: make spanOp exhaustive, rewrite to match python in cache mode

64d532f

Signed-off-by: Máté Czékus <mate@picloud.hu>

feat: create integration tests for Hooks

ff5ba75

Signed-off-by: Máté Czékus <mate@picloud.hu>

cursor Bot reviewed May 1, 2026

View reviewed changes

feat: remove receive/receive multi instrumentation bc of limits

c820c0d

Signed-off-by: Máté Czékus <mate@picloud.hu>

cursor Bot reviewed May 1, 2026

View reviewed changes

Shikachuu requested a review from giortzisg May 18, 2026 11:34

Merge branch 'getsentry:master' into feat/rueidis-valkey

32cecbe

giortzisg reviewed May 20, 2026

View reviewed changes


		result := client.Do(span.Context(), cmd)

		redis.FinishSpan(span, h.typ, cmd.IsReadOnly(), result.Error(), rueidis.IsRedisNil(result.Error()), respSize(result))

Conversation

Shikachuu commented Apr 25, 2026

Issues

Uh oh!

Uh oh!

Uh oh!

Shikachuu commented Apr 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sentry Bot Apr 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giortzisg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giortzisg May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot May 1, 2026

Choose a reason for hiding this comment

Unknown commands in cache mode leak PII

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot May 1, 2026

Choose a reason for hiding this comment

Non-deterministic map iteration in extractAddr for clusters

Uh oh!

codecov Bot commented May 20, 2026

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giortzisg left a comment

Choose a reason for hiding this comment

Uh oh!

giortzisg May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giortzisg May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

giortzisg May 20, 2026 •

edited

Loading

Non-deterministic map iteration in `extractAddr` for clusters

giortzisg May 20, 2026 •

edited

Loading

giortzisg May 20, 2026 •

edited

Loading