From 134ee02d294e50f9488abe63c9a7ac15395d60f3 Mon Sep 17 00:00:00 2001 From: davidpavlovschi Date: Fri, 31 Jul 2026 00:21:49 +0300 Subject: [PATCH] fix(logging): fall back to the slog default logger in LogHolder LogHolder.Logger() documented "If nil, returns slog.Default()" but returned slog.New(slog.DiscardHandler) with a "Should never be reached" comment, so any LogHolder whose SetLogger was never called silently dropped every record. SetLogger's comment claimed it "sets the default logger" for a nil handler while it also stored a discard handler. Return slog.Default() when no logger is set, and treat SetLogger(nil) as clearing the logger so the same default fallback applies. Fix both doc comments to match. The Helm CLI is unaffected because pkg/cmd/root.go injects its own handler, but SDK consumers now get logs on the standard slog default. Closes #31433 Signed-off-by: davidpavlovschi --- internal/logging/logging.go | 10 +++-- internal/logging/logging_test.go | 71 ++++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 674e2db34..611530159 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -104,18 +104,20 @@ type LogHolder struct { logger atomic.Pointer[slog.Logger] } -// Logger returns the logger for the LogHolder. If nil, returns slog.Default(). +// Logger returns the logger for the LogHolder. When no logger has been set, +// it returns slog.Default() so log records are not silently dropped. func (l *LogHolder) Logger() *slog.Logger { if lg := l.logger.Load(); lg != nil { return lg } - return slog.New(slog.DiscardHandler) // Should never be reached + return slog.Default() } -// SetLogger sets the logger for the LogHolder. If nil, sets the default logger. +// SetLogger sets the logger for the LogHolder. A nil handler clears the logger, +// so Logger() falls back to slog.Default() again. func (l *LogHolder) SetLogger(newHandler slog.Handler) { if newHandler == nil { - l.logger.Store(slog.New(slog.DiscardHandler)) // Assume nil as discarding logs + l.logger.Store(nil) return } l.logger.Store(slog.New(newHandler)) diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index 1d6eaf852..4fd23e638 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -43,14 +43,75 @@ func TestLogHolder_Logger(t *testing.T) { assert.Contains(t, buf.String(), "test message") }) - t.Run("should return discard - defaultlogger when no handler is set", func(t *testing.T) { + t.Run("should return the slog default logger when no handler is set", func(t *testing.T) { holder := &LogHolder{} logger := holder.Logger() - assert.Equal(t, slog.Handler(slog.DiscardHandler), logger.Handler()) + assert.Equal(t, slog.Default(), logger) }) } +// TestLogHolder_DefaultHandler covers where a LogHolder sends records for each +// way of (not) configuring it, against a capture handler installed with +// slog.SetDefault. +func TestLogHolder_DefaultHandler(t *testing.T) { + tests := []struct { + name string + setup func(holder *LogHolder, ownHandler slog.Handler) + wantDefault bool // record lands in the slog default buffer + wantOwn bool // record lands in the handler set on the holder + }{ + { + name: "fresh holder logs through the slog default", + setup: func(_ *LogHolder, _ slog.Handler) {}, + wantDefault: true, + }, + { + name: "handler set on the holder wins over the slog default", + setup: func(holder *LogHolder, ownHandler slog.Handler) { + holder.SetLogger(ownHandler) + }, + wantOwn: true, + }, + { + name: "nil handler falls back to the slog default", + setup: func(holder *LogHolder, ownHandler slog.Handler) { + holder.SetLogger(ownHandler) + holder.SetLogger(nil) + }, + wantDefault: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defaultBuf := &bytes.Buffer{} + previous := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(defaultBuf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + ownBuf := &bytes.Buffer{} + ownHandler := slog.NewTextHandler(ownBuf, &slog.HandlerOptions{Level: slog.LevelDebug}) + + holder := &LogHolder{} + tt.setup(holder, ownHandler) + + holder.Logger().Info("test message") + + if tt.wantDefault { + assert.Contains(t, defaultBuf.String(), "test message") + } else { + assert.Empty(t, defaultBuf.String()) + } + if tt.wantOwn { + assert.Contains(t, ownBuf.String(), "test message") + } else { + assert.Empty(t, ownBuf.String()) + } + }) + } +} + func TestLogHolder_SetLogger(t *testing.T) { t.Run("sets logger with valid handler", func(t *testing.T) { holder := &LogHolder{} @@ -66,15 +127,17 @@ func TestLogHolder_SetLogger(t *testing.T) { assert.Equal(t, handler, logger.Handler()) }) - t.Run("sets discard logger with nil handler", func(t *testing.T) { + t.Run("clears the logger with nil handler", func(t *testing.T) { holder := &LogHolder{} + buf := &bytes.Buffer{} + holder.SetLogger(slog.NewTextHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})) holder.SetLogger(nil) logger := holder.Logger() assert.NotNil(t, logger) - assert.Equal(t, slog.Handler(slog.DiscardHandler), logger.Handler()) + assert.Equal(t, slog.Default(), logger) }) t.Run("can replace existing logger", func(t *testing.T) {