Skip to content

Commit 4f15e76

Browse files
ccoVeilleccoVeille
committed
chore: remove logging.Lgr interface
Replace everything that used logging.Lgr with *logging.LoggingWithLevelI This way people can use any logger that implements a few methods instead of being forced to implement legacy Errorf, Infof, Debugf methods. This allows users to directly use slog.Logger or other structured loggers without having to write adapter code. Also, add test to custom logger for Go 1.21+
1 parent ad41c03 commit 4f15e76

20 files changed

+298
-65
lines changed

‎internal/log.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,6 @@ import (
77
"os"
88
)
99

10-
// TODO (ned): Revisit logging
11-
// Add more standardized approach with log levels and configurability
12-
1310
type Logging interface {
1411
Printf(ctx context.Context, format string, v ...interface{})
1512
}

‎internal/pool/pool.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ type Options struct {
122122
DialerRetryTimeout time.Duration
123123

124124
// Optional logger for connection pool operations.
125-
Logger logging.Lgr
125+
Logger logging.LoggerWithLevelI
126126
}
127127

128128
type lastDialErrorWrap struct {
@@ -1058,9 +1058,9 @@ func (p *ConnPool) isHealthyConn(cn *Conn, nowNs int64) bool {
10581058
return true
10591059
}
10601060

1061-
func (p *ConnPool) logger() logging.Lgr {
1061+
func (p *ConnPool) logger() *logging.LoggerWrapper {
10621062
if p.cfg != nil && p.cfg.Logger != nil {
1063-
return p.cfg.Logger
1063+
return logging.NewLoggerWrapper(p.cfg.Logger)
10641064
}
10651065
return logging.LoggerWithLevel()
10661066
}

‎logging/custom.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ func (cl *LoggerWrapper) Errorf(ctx context.Context, format string, v ...any) {
5454
legacyLoggerWithLevel.Errorf(ctx, format, v...)
5555
return
5656
}
57-
cl.logger.ErrorContext(cl.printfToStructured(ctx, format, v...))
57+
ctx, msg, args := cl.printfToStructured(ctx, format, v...)
58+
cl.logger.ErrorContext(ctx, msg, args...)
5859
}
5960

6061
// Warn is a structured warning level logging method with context and arguments.
@@ -71,7 +72,8 @@ func (cl *LoggerWrapper) Warnf(ctx context.Context, format string, v ...any) {
7172
legacyLoggerWithLevel.Warnf(ctx, format, v...)
7273
return
7374
}
74-
cl.logger.WarnContext(cl.printfToStructured(ctx, format, v...))
75+
ctx, msg, args := cl.printfToStructured(ctx, format, v...)
76+
cl.logger.WarnContext(ctx, msg, args...)
7577
}
7678

7779
// Info is a structured info level logging method with context and arguments.
@@ -98,15 +100,17 @@ func (cl *LoggerWrapper) Infof(ctx context.Context, format string, v ...any) {
98100
return
99101
}
100102

101-
cl.logger.InfoContext(cl.printfToStructured(ctx, format, v...))
103+
ctx, msg, args := cl.printfToStructured(ctx, format, v...)
104+
cl.logger.InfoContext(ctx, msg, args...)
102105
}
103106

104107
func (cl *LoggerWrapper) Debugf(ctx context.Context, format string, v ...any) {
105108
if cl == nil || cl.logger == nil {
106109
legacyLoggerWithLevel.Debugf(ctx, format, v...)
107110
return
108111
}
109-
cl.logger.DebugContext(cl.printfToStructured(ctx, format, v...))
112+
ctx, msg, args := cl.printfToStructured(ctx, format, v...)
113+
cl.logger.DebugContext(ctx, msg, args...)
110114
}
111115

112116
func (cl *LoggerWrapper) printfToStructured(ctx context.Context, format string, v ...any) (context.Context, string, []any) {
@@ -121,7 +125,7 @@ func (cl *LoggerWrapper) Enabled(ctx context.Context, level LogLevelT) bool {
121125
return level >= *cl.loggerLevel
122126
}
123127

124-
return legacyLoggerWithLevel.Enabled(ctx, level)
128+
return isLevelEnabled(ctx, cl.logger, level)
125129
}
126130

127131
// LoggerWithLevelI is a logger interface with leveled logging methods.

‎logging/custom_test.go‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
//go:build go1.21
2+
3+
package logging
4+
5+
import (
6+
"bytes"
7+
"context"
8+
"encoding/json"
9+
"log/slog"
10+
"testing"
11+
)
12+
13+
// validation that [slog.Logger] implements [LoggerWithLevelI]
14+
var _ LoggerWithLevelI = &slog.Logger{}
15+
16+
var _ *LoggerWrapper = NewLoggerWrapper(&slog.Logger{})
17+
18+
func TestLoggerWrapper_slog(t *testing.T) {
19+
20+
ctx := context.Background()
21+
22+
t.Run("Debug", func(t *testing.T) {
23+
wrapped, buf := NewTestLogger(t)
24+
wrapped.Debug(ctx, "debug message", "foo", "bar")
25+
26+
checkLog(t, buf, map[string]any{
27+
"level": "DEBUG",
28+
"msg": "debug message",
29+
"foo": "bar",
30+
})
31+
})
32+
33+
t.Run("Info", func(t *testing.T) {
34+
wrapped, buf := NewTestLogger(t)
35+
wrapped.Info(ctx, "info message", "foo", "bar")
36+
37+
checkLog(t, buf, map[string]any{
38+
"level": "INFO",
39+
"msg": "info message",
40+
"foo": "bar",
41+
})
42+
})
43+
t.Run("Warn", func(t *testing.T) {
44+
wrapped, buf := NewTestLogger(t)
45+
wrapped.Warn(ctx, "warn message", "foo", "bar")
46+
47+
checkLog(t, buf, map[string]any{
48+
"level": "WARN",
49+
"msg": "warn message",
50+
"foo": "bar",
51+
})
52+
})
53+
t.Run("Error", func(t *testing.T) {
54+
wrapped, buf := NewTestLogger(t)
55+
wrapped.Error(ctx, "error message", "foo", "bar")
56+
57+
checkLog(t, buf, map[string]any{
58+
"level": "ERROR",
59+
"msg": "error message",
60+
"foo": "bar",
61+
})
62+
})
63+
64+
t.Run("Errorf", func(t *testing.T) {
65+
wrapped, buf := NewTestLogger(t)
66+
wrapped.Errorf(ctx, "%d is the answer to %s", 42, "everything")
67+
68+
checkLog(t, buf, map[string]any{
69+
"level": "ERROR",
70+
"msg": "42 is the answer to everything",
71+
})
72+
})
73+
74+
t.Run("Infof", func(t *testing.T) {
75+
wrapped, buf := NewTestLogger(t)
76+
wrapped.Infof(ctx, "%d is the answer to %s", 42, "everything")
77+
78+
checkLog(t, buf, map[string]any{
79+
"level": "INFO",
80+
"msg": "42 is the answer to everything",
81+
})
82+
})
83+
84+
t.Run("Warnf", func(t *testing.T) {
85+
wrapped, buf := NewTestLogger(t)
86+
wrapped.Warnf(ctx, "%d is the answer to %s", 42, "everything")
87+
88+
checkLog(t, buf, map[string]any{
89+
"level": "WARN",
90+
"msg": "42 is the answer to everything",
91+
})
92+
})
93+
94+
t.Run("Debugf", func(t *testing.T) {
95+
wrapped, buf := NewTestLogger(t)
96+
wrapped.Debugf(ctx, "%d is the answer to %s", 42, "everything")
97+
98+
checkLog(t, buf, map[string]any{
99+
"level": "DEBUG",
100+
"msg": "42 is the answer to everything",
101+
})
102+
})
103+
104+
t.Run("Insufficient loglevel: default", func(t *testing.T) {
105+
wrapped, buf := NewTestLoggerWithLevel(t, nil)
106+
107+
wrapped.Debug(ctx, "debug message", "foo", "bar")
108+
if buf.Len() != 0 {
109+
t.Errorf("expected no log message, got %s", buf.String())
110+
}
111+
})
112+
113+
t.Run("Insufficient loglevel: error", func(t *testing.T) {
114+
wrapped, buf := NewTestLoggerWithLevel(t, slog.LevelError)
115+
wrapped.Debug(ctx, "debug message", "foo", "bar")
116+
wrapped.Warn(ctx, "warn message", "foo", "bar")
117+
wrapped.Info(ctx, "info message", "foo", "bar")
118+
if buf.Len() != 0 {
119+
t.Errorf("expected no log message, got %s", buf.String())
120+
}
121+
wrapped.Error(ctx, "error message", "foo", "bar")
122+
123+
checkLog(t, buf, map[string]any{
124+
"level": "ERROR",
125+
"msg": "error message",
126+
"foo": "bar",
127+
})
128+
})
129+
130+
t.Run("Enabled loglevel: debug", func(t *testing.T) {
131+
132+
wrapped, buf := NewTestLoggerWithLevel(t, slog.LevelInfo)
133+
if wrapped.Enabled(ctx, LogLevelDebug) {
134+
t.Errorf("expected debug level to be disabled")
135+
}
136+
if !wrapped.Enabled(ctx, LogLevelInfo) {
137+
t.Errorf("expected info level to be enabled")
138+
}
139+
if !wrapped.Enabled(ctx, LogLevelWarn) {
140+
t.Errorf("expected warn level to be enabled")
141+
}
142+
143+
if !wrapped.Enabled(ctx, LogLevelError) {
144+
t.Errorf("expected error level to be enabled")
145+
}
146+
147+
wrapped.Debug(ctx, "debug message", "foo", "bar")
148+
if buf.Len() != 0 {
149+
t.Errorf("expected no log message, got %s", buf.String())
150+
}
151+
152+
wrapped.Info(ctx, "info message", "foo", "bar")
153+
checkLog(t, buf, map[string]any{
154+
"level": "INFO",
155+
"msg": "info message",
156+
"foo": "bar",
157+
})
158+
})
159+
}
160+
161+
func NewTestLogger(t *testing.T) (*LoggerWrapper, *bytes.Buffer) {
162+
return NewTestLoggerWithLevel(t, slog.LevelDebug)
163+
}
164+
165+
func NewTestLoggerWithLevel(t *testing.T, level slog.Leveler) (*LoggerWrapper, *bytes.Buffer) {
166+
var buf bytes.Buffer
167+
wrapped := NewLoggerWrapper(slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: level})))
168+
return wrapped, &buf
169+
}
170+
171+
func checkLog(t *testing.T, buf *bytes.Buffer, attrs map[string]any) {
172+
t.Helper()
173+
174+
res := buf.Bytes()
175+
176+
var m map[string]any
177+
err := json.Unmarshal(res, &m)
178+
if err != nil {
179+
t.Fatalf("failed to unmarshal log message: %v", err)
180+
}
181+
182+
delete(m, "time") // remove time for testing purposes
183+
184+
if len(m) != len(attrs) {
185+
t.Errorf("expected %d attributes, got %d", len(attrs), len(m))
186+
}
187+
188+
for k, expected := range attrs {
189+
v, ok := m[k]
190+
if !ok {
191+
t.Errorf("expected log to have key %s", k)
192+
continue
193+
}
194+
if v != expected {
195+
t.Errorf("expected %s to be %v, got %v", k, expected, v)
196+
}
197+
}
198+
199+
if t.Failed() {
200+
t.Logf("log message: %s", res)
201+
t.Log("time is ignored in comparison")
202+
}
203+
}

‎logging/legacy.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,6 @@ func (l *legacyLoggerAdapter) Enabled(ctx context.Context, level LogLevelT) bool
9393

9494
var legacyLoggerWithLevel = &legacyLoggerAdapter{}
9595

96-
func LoggerWithLevel() Lgr {
97-
return legacyLoggerWithLevel
96+
func LoggerWithLevel() *LoggerWrapper {
97+
return NewLoggerWrapper(legacyLoggerWithLevel)
9898
}

‎logging/level.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
//go:build go1.21
2+
3+
package logging
4+
5+
import (
6+
"context"
7+
"log/slog"
8+
)
9+
10+
// TODO(ndyakov): simplify in v10 align when levels will be aligned with slog.Level
11+
var levelMap = map[LogLevelT]slog.Level{
12+
LogLevelDebug: slog.LevelDebug,
13+
LogLevelInfo: slog.LevelInfo,
14+
LogLevelWarn: slog.LevelWarn,
15+
LogLevelError: slog.LevelError,
16+
}
17+
18+
type SlogLoggerEnabler interface {
19+
Enabled(ctx context.Context, level slog.Level) bool
20+
}
21+
22+
func isLevelEnabled(ctx context.Context, logger LoggerWithLevelI, level LogLevelT) bool {
23+
sl, ok := logger.(SlogLoggerEnabler)
24+
if !ok {
25+
// unknown logger type, fall back to legacy logger
26+
return legacyLoggerWithLevel.Enabled(ctx, level)
27+
}
28+
29+
// map our [LogLevelT] to [slog.Level]
30+
// TODO(ndyakov): simplify in v10 align when levels will be aligned with slog.Level
31+
slogLevel, ok := levelMap[level]
32+
if !ok {
33+
// unknown level, assume enabled
34+
return true
35+
}
36+
return sl.Enabled(ctx, slogLevel)
37+
38+
}

‎logging/level_pre121.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
//go:build !go1.21
2+
3+
package logging
4+
5+
import "context"
6+
7+
func isLevelEnabled(ctx context.Context, logger LoggerWithLevelI, level LogLevelT) bool {
8+
// unknown logger type, fall back to legacy logger
9+
return legacyLoggerWithLevel.Enabled(ctx, level)
10+
}

‎logging/logging.go‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -89,22 +89,3 @@ func (l *filterLogger) Printf(ctx context.Context, format string, v ...interface
8989
return
9090
}
9191
}
92-
93-
// Lgr is a logger interface with leveled logging methods.
94-
// It is implemented by LoggerWrapper and legacyLoggerAdapter.
95-
// If you would like to use `slog.Logger` from the standard library, you can use LoggerWrapper.
96-
//
97-
// logger := slog.New(slog.NewTextHandler(os.Stderr))
98-
// db := redis.NewClient(&redis.Options{
99-
// Logger: logging.NewLoggerWrapper(logger),
100-
// })
101-
//
102-
// This will NOT handle all logging at the moment, singe there is still a global logger in use.
103-
// that will be deprecated in the future.
104-
type Lgr interface {
105-
Errorf(ctx context.Context, format string, v ...any)
106-
Warnf(ctx context.Context, format string, v ...any)
107-
Infof(ctx context.Context, format string, v ...any)
108-
Debugf(ctx context.Context, format string, v ...any)
109-
Enabled(ctx context.Context, level LogLevelT) bool
110-
}

‎maintnotifications/circuit_breaker.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,9 @@ func (cb *CircuitBreaker) GetStats() CircuitBreakerStats {
194194
}
195195
}
196196

197-
func (cb *CircuitBreaker) logger() logging.Lgr {
197+
func (cb *CircuitBreaker) logger() *logging.LoggerWrapper {
198198
if cb.config != nil && cb.config.Logger != nil {
199-
return cb.config.Logger
199+
return logging.NewLoggerWrapper(cb.config.Logger)
200200
}
201201
return logging.LoggerWithLevel()
202202
}
@@ -351,9 +351,9 @@ func (cbm *CircuitBreakerManager) Reset() {
351351
})
352352
}
353353

354-
func (cbm *CircuitBreakerManager) logger() logging.Lgr {
354+
func (cbm *CircuitBreakerManager) logger() *logging.LoggerWrapper {
355355
if cbm.config != nil && cbm.config.Logger != nil {
356-
return cbm.config.Logger
356+
return logging.NewLoggerWrapper(cbm.config.Logger)
357357
}
358358
return logging.LoggerWithLevel()
359359
}

0 commit comments

Comments
 (0)