feat: refactor logging to use a logger with severity levels

Author: ccoVeille

internal/log.go

@@ -77,3 +77,39 @@ func (l LogLevelT) InfoOrAbove() bool {
 func (l LogLevelT) DebugOrAbove() bool {
 	return l >= LogLevelDebug
 }
+
+// LoggerWithLevel is a logger interface with leveled logging methods
+type LoggerWithLevel interface {
+	Infof(ctx context.Context, format string, v ...interface{})
+	Warnf(ctx context.Context, format string, v ...interface{})
+	Debugf(ctx context.Context, format string, v ...interface{})
+	Errorf(ctx context.Context, format string, v ...interface{})
+}
+
+// LegacyLoggerAdapter is a logger that implements LoggerWithLevel interface
+// using the global [Logger] and [LogLevel] variables.
+type LegacyLoggerAdapter struct{}
+
+func (l *LegacyLoggerAdapter) Infof(ctx context.Context, format string, v ...interface{}) {
+	if LogLevel.InfoOrAbove() {
+		Logger.Printf(ctx, format, v...)
+	}
+}
+
+func (l *LegacyLoggerAdapter) Warnf(ctx context.Context, format string, v ...interface{}) {
+	if LogLevel.WarnOrAbove() {
+		Logger.Printf(ctx, format, v...)
+	}
+}
+
+func (l *LegacyLoggerAdapter) Debugf(ctx context.Context, format string, v ...interface{}) {
+	if LogLevel.DebugOrAbove() {
+		Logger.Printf(ctx, format, v...)
+	}
+}
+
+func (l LegacyLoggerAdapter) Errorf(ctx context.Context, format string, v ...interface{}) {
+	Logger.Printf(ctx, format, v...)
+}
+
+var LegacyLoggerWithLevel LoggerWithLevel = &LegacyLoggerAdapter{}

internal/pool/pool.go

@@ -117,7 +117,7 @@ type Options struct {
 	DialerRetryTimeout time.Duration
 
 	// Optional logger for connection pool operations.
-	Logger internal.Logging
+	Logger internal.LoggerWithLevel
 }
 
 type lastDialErrorWrap struct {
@@ -226,7 +226,7 @@ func (p *ConnPool) checkMinIdleConns() {
 						p.idleConnsLen.Add(-1)
 
 						p.freeTurn()
-						p.logf(context.Background(), "addIdleConn panic: %+v", err)
+						p.logger().Errorf(context.Background(), "addIdleConn panic: %+v", err)
 					}
 				}()
 
@@ -382,7 +382,7 @@ func (p *ConnPool) dialConn(ctx context.Context, pooled bool) (*Conn, error) {
 		return cn, nil
 	}
 
-	p.logf(ctx, "redis: connection pool: failed to dial after %d attempts: %v", maxRetries, lastErr)
+	p.logger().Errorf(ctx, "redis: connection pool: failed to dial after %d attempts: %v", maxRetries, lastErr)
 	// All retries failed - handle error tracking
 	p.setLastDialError(lastErr)
 	if atomic.AddUint32(&p.dialErrorsNum, 1) == uint32(p.cfg.PoolSize) {
@@ -455,7 +455,7 @@ func (p *ConnPool) getConn(ctx context.Context) (*Conn, error) {
 
 	for {
 		if attempts >= getAttempts {
-			p.logf(ctx, "redis: connection pool: was not able to get a healthy connection after %d attempts", attempts)
+			p.logger().Errorf(ctx, "redis: connection pool: was not able to get a healthy connection after %d attempts", attempts)
 			break
 		}
 		attempts++
@@ -482,12 +482,12 @@ func (p *ConnPool) getConn(ctx context.Context) (*Conn, error) {
 		if hookManager != nil {
 			acceptConn, err := hookManager.ProcessOnGet(ctx, cn, false)
 			if err != nil {
-				p.logf(ctx, "redis: connection pool: failed to process idle connection by hook: %v", err)
+				p.logger().Errorf(ctx, "redis: connection pool: failed to process idle connection by hook: %v", err)
 				_ = p.CloseConn(cn)
 				continue
 			}
 			if !acceptConn {
-				p.logf(ctx, "redis: connection pool: conn[%d] rejected by hook, returning to pool", cn.GetID())
+				p.logger().Errorf(ctx, "redis: connection pool: conn[%d] rejected by hook, returning to pool", cn.GetID())
 				p.Put(ctx, cn)
 				cn = nil
 				continue
@@ -512,7 +512,7 @@ func (p *ConnPool) getConn(ctx context.Context) (*Conn, error) {
 		// this should not happen with a new connection, but we handle it gracefully
 		if err != nil || !acceptConn {
 			// Failed to process connection, discard it
-			p.logf(ctx, "redis: connection pool: failed to process new connection conn[%d] by hook: accept=%v, err=%v", newcn.GetID(), acceptConn, err)
+			p.logger().Errorf(ctx, "redis: connection pool: failed to process new connection conn[%d] by hook: accept=%v, err=%v", newcn.GetID(), acceptConn, err)
 			_ = p.CloseConn(newcn)
 			return nil, err
 		}
@@ -706,7 +706,7 @@ func (p *ConnPool) popIdle() (*Conn, error) {
 
 	// If we exhausted all attempts without finding a usable connection, return nil
 	if attempts > 1 && attempts >= maxAttempts && int32(attempts) >= p.poolSize.Load() {
-		p.logf(context.Background(), "redis: connection pool: failed to get a usable connection after %d attempts", attempts)
+		p.logger().Errorf(context.Background(), "redis: connection pool: failed to get a usable connection after %d attempts", attempts)
 		return nil, nil
 	}
 
@@ -723,7 +723,7 @@ func (p *ConnPool) Put(ctx context.Context, cn *Conn) {
 		// Peek at the reply type to check if it's a push notification
 		if replyType, err := cn.PeekReplyTypeSafe(); err != nil || replyType != proto.RespPush {
 			// Not a push notification or error peeking, remove connection
-			p.logf(ctx, "Conn has unread data (not push notification), removing it")
+			p.logger().Errorf(ctx, "Conn has unread data (not push notification), removing it")
 			p.Remove(ctx, cn, err)
 		}
 		// It's a push notification, allow pooling (client will handle it)
@@ -736,7 +736,7 @@ func (p *ConnPool) Put(ctx context.Context, cn *Conn) {
 	if hookManager != nil {
 		shouldPool, shouldRemove, err = hookManager.ProcessOnPut(ctx, cn)
 		if err != nil {
-			p.logf(ctx, "Connection hook error: %v", err)
+			p.logger().Errorf(ctx, "Connection hook error: %v", err)
 			p.Remove(ctx, cn, err)
 			return
 		}
@@ -838,7 +838,7 @@ func (p *ConnPool) removeConn(cn *Conn) {
 		// this can be idle conn
 		for idx, ic := range p.idleConns {
 			if ic.GetID() == cid {
-				p.logf(context.Background(), "redis: connection pool: removing idle conn[%d]", cid)
+				p.logger().Errorf(context.Background(), "redis: connection pool: removing idle conn[%d]", cid)
 				p.idleConns = append(p.idleConns[:idx], p.idleConns[idx+1:]...)
 				p.idleConnsLen.Add(-1)
 				break
@@ -954,7 +954,7 @@ func (p *ConnPool) isHealthyConn(cn *Conn, now time.Time) bool {
 			if replyType, err := cn.rd.PeekReplyType(); err == nil && replyType == proto.RespPush {
 				// For RESP3 connections with push notifications, we allow some buffered data
 				// The client will process these notifications before using the connection
-				p.logf(context.Background(), "push: conn[%d] has buffered data, likely push notifications - will be processed by client", cn.GetID())
+				p.logger().Infof(context.Background(), "push: conn[%d] has buffered data, likely push notifications - will be processed by client", cn.GetID())
 				return true // Connection is healthy, client will handle notifications
 			}
 			return false // Unexpected data, not push notifications, connection is unhealthy
@@ -965,10 +965,10 @@ func (p *ConnPool) isHealthyConn(cn *Conn, now time.Time) bool {
 	return true
 }
 
-func (p *ConnPool) logf(ctx context.Context, format string, args ...any) {
+func (p *ConnPool) logger() internal.LoggerWithLevel {
 	if p.cfg.Logger != nil {
-		p.cfg.Logger.Printf(ctx, format, args...)
-	} else {
-		internal.Logger.Printf(ctx, format, args...)
+		return p.cfg.Logger
 	}
+
+	return &internal.LegacyLoggerAdapter{}
 }

maintnotifications/circuit_breaker.go

@@ -102,9 +102,7 @@ func (cb *CircuitBreaker) Execute(fn func() error) error {
 			if cb.state.CompareAndSwap(int32(CircuitBreakerOpen), int32(CircuitBreakerHalfOpen)) {
 				cb.requests.Store(0)
 				cb.successes.Store(0)
-				if internal.LogLevel.InfoOrAbove() {
-					cb.logf(context.Background(), logs.CircuitBreakerTransitioningToHalfOpen(cb.endpoint))
-				}
+				cb.logger().Infof(context.Background(), logs.CircuitBreakerTransitioningToHalfOpen(cb.endpoint))
 				// Fall through to half-open logic
 			} else {
 				return ErrCircuitBreakerOpen
@@ -144,17 +142,13 @@ func (cb *CircuitBreaker) recordFailure() {
 	case CircuitBreakerClosed:
 		if failures >= int64(cb.failureThreshold) {
 			if cb.state.CompareAndSwap(int32(CircuitBreakerClosed), int32(CircuitBreakerOpen)) {
-				if internal.LogLevel.WarnOrAbove() {
-					cb.logf(context.Background(), logs.CircuitBreakerOpened(cb.endpoint, failures))
-				}
+				cb.logger().Warnf(context.Background(), logs.CircuitBreakerOpened(cb.endpoint, failures))
 			}
 		}
 	case CircuitBreakerHalfOpen:
 		// Any failure in half-open state immediately opens the circuit
 		if cb.state.CompareAndSwap(int32(CircuitBreakerHalfOpen), int32(CircuitBreakerOpen)) {
-			if internal.LogLevel.WarnOrAbove() {
-				cb.logf(context.Background(), logs.CircuitBreakerReopened(cb.endpoint))
-			}
+			cb.logger().Warnf(context.Background(), logs.CircuitBreakerReopened(cb.endpoint))
 		}
 	}
 }
@@ -176,9 +170,7 @@ func (cb *CircuitBreaker) recordSuccess() {
 		if successes >= int64(cb.maxRequests) {
 			if cb.state.CompareAndSwap(int32(CircuitBreakerHalfOpen), int32(CircuitBreakerClosed)) {
 				cb.failures.Store(0)
-				if internal.LogLevel.InfoOrAbove() {
-					cb.logf(context.Background(), logs.CircuitBreakerClosed(cb.endpoint, successes))
-				}
+				cb.logger().Infof(context.Background(), logs.CircuitBreakerClosed(cb.endpoint, successes))
 			}
 		}
 	}
@@ -202,12 +194,11 @@ func (cb *CircuitBreaker) GetStats() CircuitBreakerStats {
 	}
 }
 
-func (cb *CircuitBreaker) logf(ctx context.Context, format string, args ...interface{}) {
+func (cb *CircuitBreaker) logger() internal.LoggerWithLevel {
 	if cb.config != nil && cb.config.Logger != nil {
-		cb.config.Logger.Printf(ctx, format, args...)
-	} else {
-		internal.Logger.Printf(ctx, format, args...)
+		return cb.config.Logger
 	}
+	return internal.LegacyLoggerWithLevel
 }
 
 // CircuitBreakerStats provides statistics about a circuit breaker
@@ -333,8 +324,8 @@ func (cbm *CircuitBreakerManager) cleanup() {
 	}
 
 	// Log cleanup results
-	if len(toDelete) > 0 && internal.LogLevel.InfoOrAbove() {
-		cbm.logf(context.Background(), logs.CircuitBreakerCleanup(len(toDelete), count))
+	if len(toDelete) > 0 {
+		cbm.logger().Infof(context.Background(), logs.CircuitBreakerCleanup(len(toDelete), count))
 	}
 
 	cbm.lastCleanup.Store(now.Unix())
@@ -360,10 +351,9 @@ func (cbm *CircuitBreakerManager) Reset() {
 	})
 }
 
-func (cbm *CircuitBreakerManager) logf(ctx context.Context, format string, args ...interface{}) {
+func (cbm *CircuitBreakerManager) logger() internal.LoggerWithLevel {
 	if cbm.config != nil && cbm.config.Logger != nil {
-		cbm.config.Logger.Printf(ctx, format, args...)
-	} else {
-		internal.Logger.Printf(ctx, format, args...)
+		return cbm.config.Logger
 	}
+	return internal.LegacyLoggerWithLevel
 }

maintnotifications/config.go

@@ -130,7 +130,7 @@ type Config struct {
 	MaxHandoffRetries int
 
 	// Logger is an optional custom logger for maintenance notifications.
-	Logger internal.Logging
+	Logger internal.LoggerWithLevel
 }
 
 func (c *Config) IsEnabled() bool {
@@ -315,10 +315,9 @@ func (c *Config) ApplyDefaultsWithPoolConfig(poolSize int, maxActiveConns int) *
 		result.CircuitBreakerMaxRequests = c.CircuitBreakerMaxRequests
 	}
 
-	if internal.LogLevel.DebugOrAbove() {
-		internal.Logger.Printf(context.Background(), logs.DebugLoggingEnabled())
-		internal.Logger.Printf(context.Background(), logs.ConfigDebug(result))
-	}
+	c.logger().Debugf(context.Background(), logs.DebugLoggingEnabled())
+	c.logger().Debugf(context.Background(), logs.ConfigDebug(result))
+
 	return result
 }
 
@@ -370,6 +369,13 @@ func (c *Config) applyWorkerDefaults(poolSize int) {
 	}
 }
 
+func (c *Config) logger() internal.LoggerWithLevel {
+	if c.Logger != nil {
+		return c.Logger
+	}
+	return internal.LegacyLoggerWithLevel
+}
+
 // DetectEndpointType automatically detects the appropriate endpoint type
 // based on the connection address and TLS configuration.
 //