Add OpenTelemetry Native Metrics Support #3637

ofekshenawa · 2025-12-03T11:26:56Z

This PR implements Observability (Metrics) for go-redis, adding comprehensive OpenTelemetry metrics support following the OpenTelemetry Database Client Semantic Conventions.

The implementation uses a Bridge Pattern to keep the core library dependency-free while providing optional, metrics instrumentation through the new extra/redisotel-native package.

What's Included:
Metric groups covering aspects of Redis operations:

Command metrics: Operation duration with retry tracking
Connection basic: Connection count and creation time
Resiliency: Errors, handoffs, timeout relaxation
Connection advanced: Wait time and use time
Pubsub metrics: Published and received messages
Stream metrics: Processing duration and maintenance notifications

Signed-off-by: Elena Kolevska <elena@kolevska.com>

…quest metric type from UpAndDownCounter to Async gauge.

ndyakov

submitting partial review

ndyakov · 2026-01-14T15:30:02Z

internal/pool/pool.go

+	// Global callback for connection state changes (set by otel package)
+	connectionStateChangeCallback func(ctx context.Context, cn *Conn, fromState, toState string)
+
+	// Global callback for connection creation time (set by otel package)
+	connectionCreateTimeCallback func(ctx context.Context, duration time.Duration, cn *Conn)
+
+	// Global callback for connection relaxed timeout changes (set by otel package)
+	// Parameters: ctx, delta (+1/-1), cn, poolName, notificationType
+	connectionRelaxedTimeoutCallback func(ctx context.Context, delta int, cn *Conn, poolName, notificationType string)
+
+	// Global callback for connection handoff (set by otel package)
+	// Parameters: ctx, cn, poolName
+	connectionHandoffCallback func(ctx context.Context, cn *Conn, poolName string)
+
+	// Global callback for error tracking (set by otel package)
+	// Parameters: ctx, errorType, cn, statusCode, isInternal, retryAttempts
+	errorCallback func(ctx context.Context, errorType string, cn *Conn, statusCode string, isInternal bool, retryAttempts int)
+
+	// Global callback for maintenance notifications (set by otel package)
+	// Parameters: ctx, cn, notificationType
+	maintenanceNotificationCallback func(ctx context.Context, cn *Conn, notificationType string)
+
+	// Global callback for connection wait time (set by otel package)
+	// Parameters: ctx, duration, cn
+	connectionWaitTimeCallback func(ctx context.Context, duration time.Duration, cn *Conn)
+
+	// Global callback for connection use time (set by otel package)
+	// Parameters: ctx, duration, cn
+	connectionUseTimeCallback func(ctx context.Context, duration time.Duration, cn *Conn)
+
+	// Global callback for connection timeouts (set by otel package)
+	// Parameters: ctx, cn, timeoutType
+	connectionTimeoutCallback func(ctx context.Context, cn *Conn, timeoutType string)
+
+	// Global callback for connection closed (set by otel package)
+	// Parameters: ctx, cn, reason, err
+	connectionClosedCallback func(ctx context.Context, cn *Conn, reason string, err error)


This global callbacks are not thread safe. Someone may set (or set nil - remove) the callback while a check on its existence is happening. In the best case scenario - we will end up having undefined behaviour, worst case - nil pointer panics. We should fix this with either a global callback mutex or atomic values here. I do think a RWMutex will be an easier and cleaner approach.

internal/pool/pool.go

internal/otel/metrics.go

maintnotifications/push_notification_handler.go

internal/pool/pool.go

extra/redisotel-native/config.go

extra/redisotel-native/redisotel.go

otel.go

maintnotifications/push_notification_handler.go

ndyakov

left two comments that can be considered for all get/set operations. other than that the thread safety looks ok if we are willing to accept that there is a time window for which getters may have returned an old value. can we make sure if we remove the telemetry, this won't break the app? In the case when the old value is nil I assume we can accept a single event not being tracked.

ndyakov · 2026-01-19T14:24:33Z

internal/pool/pool.go

+func getConnectionStateChangeCallback() func(ctx context.Context, cn *Conn, fromState, toState string) {
+	callbackMu.RLock()
+	cb := connectionStateChangeCallback
+	callbackMu.RUnlock()
+	return cb
+}


there is a slight chance that connectionStateChangeCallback can be changed after the runlick but before the use. If we agree that this timeframe is something we are willing to tolerate, this is fine.

ndyakov · 2026-01-19T14:25:31Z

internal/pool/pool.go

+func SetConnectionTimeoutCallback(fn func(ctx context.Context, cn *Conn, timeoutType string)) {
+	callbackMu.Lock()
+	connectionTimeoutCallback = fn
+	callbackMu.Unlock()
+}


if we are setting multiple callbacks (as we should in most cases) wouldn't it be better to have a single lock -> set all callbacks -> unlock block for them?

internal/pool/pool.go

extra/redisotel-native/config.go

elena-kolevska · 2026-01-26T23:46:12Z

extra/redisotel-native/redisotel.go

+					}
+
+					// Add pool name
+					baseAttrs = append(baseAttrs, attribute.String("db.client.connection.pool.name", poolInfo.name))


I'd prefer to see all attributes defined as constants rather than using them as strings across multiple places/files.

ndyakov

overall looks good, can merge as is, left couple of improvements that we can address

ndyakov · 2026-02-04T13:41:43Z

extra/redisotel-native/metrics.go

+func isTimeoutError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for net.Error with Timeout() method (standard way)
+	if netErr, ok := err.(net.Error); ok {
+		return netErr.Timeout()
+	}
+
+	return false
+}


this is the internal implementation that we have for detecting timeout errors:

func isTimeoutError(err error) (isTimeout bool, hasTimeoutFlag bool) { // Check for timeoutError interface (works with wrapped errors) var te timeoutError if errors.As(err, &te) { return true, te.Timeout() } // Check for net.Error specifically (common case for network timeouts) var netErr net.Error if errors.As(err, &netErr) { return true, netErr.Timeout() } return false, false }

It could be other type of error (not only net.Error) that has Timeout() method.

ndyakov · 2026-02-04T13:43:15Z

extra/redisotel-native/metrics.go

+		strings.Contains(errStr, "dns") ||
+		strings.Contains(errStr, "lookup") ||


if the actual error message is something like dns lookup let's have them together. I can see how lookup can be part of another error type.

ndyakov · 2026-02-04T13:45:09Z

extra/redisotel-native/metrics.go

+	return "other"
+}
+
+func getErrorCategoryFromType(errorType string) string {


are both getErrorCategoryFromType and getErrorCategory needed? Can we combine them since they are doing pretty much the same thing?

ndyakov · 2026-02-04T13:46:20Z

extra/redisotel-native/redisotel.go

+			)
+		}
+		connectionCreateTime, err = meter.Float64Histogram(
+			"db.client.connection.create_time",


should we have this as const?

ndyakov · 2026-02-04T13:46:26Z

extra/redisotel-native/redisotel.go

+		}
+
+		connectionRelaxedTimeout, err = meter.Int64UpDownCounter(
+			"redis.client.connection.relaxed_timeout",


should we have this as const?

ndyakov · 2026-02-04T13:48:31Z

maintnotifications/handoff_worker.go


+		// Record relaxed timeout metric (post-handoff)
+		if relaxedTimeoutCallback := pool.GetMetricConnectionRelaxedTimeoutCallback(); relaxedTimeoutCallback != nil {
+			relaxedTimeoutCallback(ctx, 1, conn, "main", "HANDOFF")


"main" as const

ndyakov · 2026-02-04T13:48:53Z

maintnotifications/handoff_worker.go

 	// successfully completed the handoff, no retry needed and no error
+	// Notify metrics: connection handoff succeeded
+	if handoffCallback := pool.GetMetricConnectionHandoffCallback(); handoffCallback != nil {
+		handoffCallback(ctx, conn, "main")


same, let's have "main" as const

ndyakov · 2026-02-04T13:49:07Z

maintnotifications/push_notification_handler.go

+
+	// Record relaxed timeout metric
+	if relaxedTimeoutCallback := pool.GetMetricConnectionRelaxedTimeoutCallback(); relaxedTimeoutCallback != nil {
+		relaxedTimeoutCallback(ctx, 1, conn, "main", "MIGRATING")


"main" as const

ndyakov · 2026-02-04T13:49:20Z

maintnotifications/push_notification_handler.go

+
+	// Record relaxed timeout metric
+	if relaxedTimeoutCallback := pool.GetMetricConnectionRelaxedTimeoutCallback(); relaxedTimeoutCallback != nil {
+		relaxedTimeoutCallback(ctx, 1, conn, "main", "FAILING_OVER")


"main" as const

ndyakov · 2026-02-04T13:50:20Z

options.go

+// generateUniqueID generates a short unique identifier for pool names.
+func generateUniqueID() string {
+	b := make([]byte, 4)
+	if _, err := rand.Read(b); err != nil {
+		return ""
+	}
+	return hex.EncodeToString(b)
+}
+


does it make sense to use global auto increment so we know which pool is it?

elena-kolevska and others added 14 commits October 24, 2025 11:18

First draft. One metric - command duration.

65d0922

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Adds connection state metrics

c17657c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

wip

9cf404c

Signed-off-by: Elena Kolevska <elena@kolevska.com>

Add phase 1 metrics

fe380b9

Add client api configurations

28292b2

remove redundent code

cf9b683

refactor: move otel-metrics example to separate module

568d889

refactor: move otel-metrics example to separate module

d60e2c9

remove internal test

b4d5dbc

Resolve merge conflicts with upstream/master

2f9ce8b

Resolve merge conflicts with upstream/master

31f2dbd

Fix linting issues

e9376db

Fix pool idle connection counter bug

990e0ec

Add OTel performance benchmarking suite

5d5dbb8

ofekshenawa marked this pull request as ready for review December 3, 2025 15:06

ofekshenawa added 2 commits December 4, 2025 16:14

Improve perf compatison script

a6ebc87

Merge remote-tracking branch 'upstream/master' into add-metrics

168f918

ofekshenawa force-pushed the add-metrics branch from 5b7ff5f to 168f918 Compare December 4, 2025 14:14

ofekshenawa added 2 commits December 10, 2025 21:36

Add phase 2,3 metrics

22a92e7

Merge remote-tracking branch 'upstream/master' into add-metrics

b8afd7f

ofekshenawa force-pushed the add-metrics branch from 0940dd5 to b8afd7f Compare December 10, 2025 19:38

ofekshenawa added 7 commits December 10, 2025 21:43

Remove local benchmark tests

89c74a5

Add new fields to tests

ed99b9d

Change db.client.connection.count and db.client.connection.pending_re…

3c09637

…quest metric type from UpAndDownCounter to Async gauge.

Merge remote-tracking branch 'upstream/master' into add-metrics

7ddfbd2

fix golangci lint

8d037b4

Add piplined commands metrics support

26e3f1d

Redesign otel init method

7ea53fe

ndyakov reviewed Jan 14, 2026

View reviewed changes

elena-kolevska requested a review from vladvildanov January 15, 2026 09:36

vladvildanov requested changes Jan 15, 2026

View reviewed changes

ndyakov reviewed Jan 15, 2026

View reviewed changes

ofekshenawa added 4 commits January 19, 2026 00:53

Address comments

44e76a1

Resolve merge conflicts with upstream/master

8eb9b55

Fix golangci-lint errors

f486b34

Fix passing nil pointer

9afcf27

ofekshenawa requested review from ndyakov and vladvildanov January 19, 2026 09:04

ndyakov reviewed Jan 19, 2026

View reviewed changes

ofekshenawa added 2 commits January 21, 2026 11:41

Add metric prefix to callbacks and atomic batch registration

4172d15

Merge remote-tracking branch 'upstream/master' into add-metrics

eefc043

ndyakov reviewed Jan 21, 2026

View reviewed changes

extra/redisotel-native/config.go Show resolved Hide resolved

Add unique name for pool.name attr

25bf09c

vladvildanov previously approved these changes Jan 23, 2026

View reviewed changes

elena-kolevska reviewed Jan 26, 2026

View reviewed changes

Fix unique pool name for pubsub

0850e55

ofekshenawa dismissed vladvildanov’s stale review via 0850e55 January 28, 2026 14:27

ofekshenawa and others added 3 commits January 28, 2026 18:06

Refactor all hardcoded OTel attribute strings to use constants

6bee66c

Refactor all hardcoded OTel attribute strings to use constants

e537ce3

Merge branch 'master' into add-metrics

31ff773

ndyakov approved these changes Feb 4, 2026

View reviewed changes

		strings.Contains(errStr, "dns") \|\|
		strings.Contains(errStr, "lookup") \|\|

Add OpenTelemetry Native Metrics Support #3637

Are you sure you want to change the base?

Add OpenTelemetry Native Metrics Support #3637

Uh oh!

Conversation

ofekshenawa commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndyakov left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndyakov 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!

Uh oh!

Uh oh!

elena-kolevska Jan 26, 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!

ndyakov 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ofekshenawa commented Dec 3, 2025 •

edited

Loading

elena-kolevska Jan 26, 2026 •

edited

Loading