From ec1d9a6b4b7ce070eff34bd09e7cbd95ce1e2412 Mon Sep 17 00:00:00 2001 From: Miccah Date: Fri, 20 Feb 2026 11:03:16 -0800 Subject: [PATCH] Refactor log package (#4734) * Replace logConfig with zapcore.Core * Add SyncFunc type and tests * Change Sentry functions to accept a *sentry.Client This allows the caller to do the error handling. * Rename newCoreConfig to newCore * Remove redundant WithCore option * Update doc comments --- pkg/log/log.go | 123 ++++++++++++-------------------------------- pkg/log/log_test.go | 123 +++++++++++++++++++++++++++++++------------- 2 files changed, 120 insertions(+), 126 deletions(-) diff --git a/pkg/log/log.go b/pkg/log/log.go index ad208c371..59d832d5c 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -16,62 +16,31 @@ import ( "go.uber.org/zap/zapcore" ) -type logConfig struct { - core zapcore.Core - cleanup func() error - err error -} +type SyncFunc func() error type SinkOption func(*sinkConfig) -// New creates a new log object with the provided configurations. If no sinks -// are provided, a no-op sink will be used. Returns the logger and a cleanup +// New creates a new log object with the provided sinks. If no sinks are +// provided, a no-op sink will be used. Returns the logger and a cleanup // function that should be executed before the program exits. -func New(service string, configs ...logConfig) (logr.Logger, func() error) { - return NewWithCaller(service, false, configs...) +func New(service string, cores ...zapcore.Core) (logr.Logger, SyncFunc) { + return NewWithCaller(service, false, cores...) } -// NewWithCaller creates a new logger named after the specified service with the provided sink configurations. If -// addCaller is true, call site information will be attached to each emitted log message. (This behavior can be disabled -// on a per-sink basis using WithSuppressCaller.) -func NewWithCaller(service string, addCaller bool, configs ...logConfig) (logr.Logger, func() error) { - var cores []zapcore.Core - var cleanupFuncs []func() error - - // create cores for the logger - for _, config := range configs { - if config.err != nil { - continue - } - cores = append(cores, config.core) - if config.cleanup != nil { - cleanupFuncs = append(cleanupFuncs, config.cleanup) - } - } +// NewWithCaller creates a new logger named after the specified service with +// the provided sinks. If addCaller is true, call site information will be +// attached to each emitted log message. (This behavior can be disabled on a +// per-sink basis using WithSuppressCaller.) +func NewWithCaller(service string, addCaller bool, cores ...zapcore.Core) (logr.Logger, SyncFunc) { // create logger zapLogger := zap.New(zapcore.NewTee(cores...), zap.WithCaller(addCaller)) - cleanupFuncs = append(cleanupFuncs, zapLogger.Sync) logger := zapr.NewLogger(zapLogger).WithName(service) - // report the errors we encountered in the configs - for _, config := range configs { - if config.err != nil { - logger.Error(config.err, "error configuring logger") - } - } - - return logger, firstErrorFunc(cleanupFuncs...) + return logger, zapLogger.Sync } -// WithSentry adds sentry integration to the logger. This configuration may -// fail, in which case, sentry will not be added and execution will continue -// normally. -func WithSentry(opts sentry.ClientOptions, tags map[string]string) logConfig { - client, err := sentry.NewClient(opts) - if err != nil { - return logConfig{err: err} - } - +// WithSentry adds sentry integration to the logger. +func WithSentry(client *sentry.Client, tags map[string]string) zapcore.Core { // create sentry core cfg := zapsentry.Configuration{ Tags: tags, @@ -81,16 +50,14 @@ func WithSentry(opts sentry.ClientOptions, tags map[string]string) logConfig { } core, err := zapsentry.NewCore(cfg, zapsentry.NewSentryClientFromClient(client)) if err != nil { - return logConfig{err: err} + // NewCore should never fail because NewSentryClientFromClient + // never returns an error and NewCore only returns an error if + // the zapsentry.Configuration is invalid, which would indicate + // a programmer error. + panic(err) } - return logConfig{ - core: core, - cleanup: func() error { - sentry.Flush(5 * time.Second) - return nil - }, - } + return core } type sinkConfig struct { @@ -102,8 +69,8 @@ type sinkConfig struct { } // WithJSONSink adds a JSON encoded output to the logger. -func WithJSONSink(sink io.Writer, opts ...SinkOption) logConfig { - return newCoreConfig( +func WithJSONSink(sink io.Writer, opts ...SinkOption) zapcore.Core { + return newCore( zapcore.NewJSONEncoder(defaultEncoderConfig()), zapcore.Lock(zapcore.AddSync(sink)), globalLogLevel, @@ -112,8 +79,8 @@ func WithJSONSink(sink io.Writer, opts ...SinkOption) logConfig { } // WithConsoleSink adds a console-style output to the logger. -func WithConsoleSink(sink io.Writer, opts ...SinkOption) logConfig { - return newCoreConfig( +func WithConsoleSink(sink io.Writer, opts ...SinkOption) zapcore.Core { + return newCore( zapcore.NewConsoleEncoder(defaultEncoderConfig()), zapcore.Lock(zapcore.AddSync(sink)), globalLogLevel, @@ -135,15 +102,10 @@ func defaultEncoderConfig() zapcore.EncoderConfig { return conf } -// WithCore adds any user supplied zap core to the logger. -func WithCore(core zapcore.Core) logConfig { - return logConfig{core: core} -} - // AddSentry initializes a sentry client and extends an existing // logr.Logger with the hook. -func AddSentry(l logr.Logger, opts sentry.ClientOptions, tags map[string]string) (logr.Logger, func() error, error) { - return AddSink(l, WithSentry(opts, tags)) +func AddSentry(l logr.Logger, client *sentry.Client, tags map[string]string) (logr.Logger, SyncFunc, error) { + return AddSink(l, WithSentry(client, tags)) } // AddSink extends an existing logr.Logger with a new sink. It returns the new logr.Logger, a cleanup function, and an @@ -151,16 +113,12 @@ func AddSentry(l logr.Logger, opts sentry.ClientOptions, tags map[string]string) // // The new sink will not inherit any of the existing logger's key-value pairs. Key-value pairs can be added to the new // sink specifically by passing them to this function. -func AddSink(l logr.Logger, sink logConfig, keysAndValues ...any) (logr.Logger, func() error, error) { - if sink.err != nil { - return l, nil, sink.err - } - +func AddSink(l logr.Logger, core zapcore.Core, keysAndValues ...any) (logr.Logger, SyncFunc, error) { // New key-value pairs cannot be ergonomically added directly to cores. logr has code to do it, but that code is not // exported. Rather than replicating it ourselves, we indirectly use it by creating a temporary logger for the new // core, adding the key-value pairs to the temporary logger, and then extracting the temporary logger's modified // core. - newSinkLogger := zapr.NewLogger(zap.New(sink.core)) + newSinkLogger := zapr.NewLogger(zap.New(core)) newSinkLogger = newSinkLogger.WithValues(keysAndValues...) newCoreLogger, err := getZapLogger(newSinkLogger) if err != nil { @@ -186,7 +144,7 @@ func AddSink(l logr.Logger, sink logConfig, keysAndValues ...any) (logr.Logger, zapLogger = zapLogger.WithOptions(newLoggerOptions...) newLogger := zapr.NewLogger(zapLogger) - return newLogger, firstErrorFunc(zapLogger.Sync, sink.cleanup), nil + return newLogger, zapLogger.Sync, nil } // getZapLogger is a helper function that gets the underlying zap logger from a @@ -241,31 +199,14 @@ func ToSlogger(l logr.Logger) *slog.Logger { return slog.New(logr.ToSlogHandler(l)) } -// firstErrorFunc is a helper function that returns a function that executes -// all provided args and returns the first error, if any. -func firstErrorFunc(fs ...func() error) func() error { - return func() error { - var firstErr error = nil - for _, f := range fs { - if f == nil { - continue - } - if err := f(); err != nil && firstErr == nil { - firstErr = err - } - } - return firstErr - } -} - -// newCoreConfig is a helper function that creates a default sinkConfig, +// newCore is a helper function that creates a default sinkConfig, // applies the options, then creates a zapcore.Core. -func newCoreConfig( +func newCore( defaultEncoder zapcore.Encoder, defaultSink zapcore.WriteSyncer, defaultLevel levelSetter, opts ...SinkOption, -) logConfig { +) zapcore.Core { conf := sinkConfig{ encoder: defaultEncoder, sink: defaultSink, @@ -288,5 +229,5 @@ func newCoreConfig( core = &suppressCallerCore{Core: core} } - return logConfig{core: core} + return core } diff --git a/pkg/log/log_test.go b/pkg/log/log_test.go index b98356ca0..bab46fda5 100644 --- a/pkg/log/log_test.go +++ b/pkg/log/log_test.go @@ -11,8 +11,49 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" + "go.uber.org/zap/zapcore" ) +// testCore implement zapcore.Core with custom methods. +type testCore struct { + check func(zapcore.Entry, *zapcore.CheckedEntry) *zapcore.CheckedEntry + enabled func(zapcore.Level) bool + sync func() error + with func([]zapcore.Field) zapcore.Core + write func(zapcore.Entry, []zapcore.Field) error +} + +func (t *testCore) Check(e zapcore.Entry, ce *zapcore.CheckedEntry) *zapcore.CheckedEntry { + if t.check != nil { + return t.check(e, ce) + } + return ce +} +func (t *testCore) Enabled(l zapcore.Level) bool { + if t.enabled != nil { + return t.enabled(l) + } + return true +} +func (t *testCore) Sync() error { + if t.sync != nil { + return t.sync() + } + return nil +} +func (t *testCore) With(fields []zapcore.Field) zapcore.Core { + if t.with != nil { + return t.with(fields) + } + return t +} +func (t *testCore) Write(ent zapcore.Entry, fields []zapcore.Field) error { + if t.write != nil { + return t.write(ent, fields) + } + return nil +} + func TestNew(t *testing.T) { var jsonBuffer, consoleBuffer bytes.Buffer logger, flush := New("service-name", @@ -60,42 +101,18 @@ func TestSetLevel(t *testing.T) { assert.Equal(t, true, logger.GetSink().Enabled(2)) } -func TestWithSentryFailure(t *testing.T) { - var buffer bytes.Buffer - logger, flush := New("service-name", - WithSentry(sentry.ClientOptions{Dsn: "fail"}, nil), - WithConsoleSink(&buffer), - ) - logger.Info("yay") - assert.Nil(t, flush()) - - assert.Contains(t, buffer.String(), "error configuring logger") - assert.Contains(t, buffer.String(), "yay") -} - -func TestAddSentryFailure(t *testing.T) { - var buffer bytes.Buffer - logger, flush := New("service-name", WithConsoleSink(&buffer)) - logger, _, err := AddSentry(logger, sentry.ClientOptions{Dsn: "fail"}, nil) - assert.NotNil(t, err) - assert.NotContains(t, err.Error(), "unsupported") - - logger.Info("yay") - assert.Nil(t, flush()) - - assert.Contains(t, buffer.String(), "yay") -} - func TestAddSentry(t *testing.T) { var buffer bytes.Buffer var sentryMessage string - logger, _ := New("service-name", WithConsoleSink(&buffer)) - logger, flush, err := AddSentry(logger, sentry.ClientOptions{ + client, err := sentry.NewClient(sentry.ClientOptions{ BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { sentryMessage = event.Message return nil }, - }, nil) + }) + assert.Nil(t, err) + logger, _ := New("service-name", WithConsoleSink(&buffer)) + logger, flush, err := AddSentry(logger, client, nil) assert.Nil(t, err) logger.Error(nil, "oops") @@ -110,14 +127,16 @@ func TestAddSentry(t *testing.T) { func TestWithSentry(t *testing.T) { var buffer bytes.Buffer var sentryMessage string + client, err := sentry.NewClient(sentry.ClientOptions{ + BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { + sentryMessage = event.Message + return nil + }, + }) + assert.Nil(t, err) logger, flush := New("service-name", WithConsoleSink(&buffer), - WithSentry(sentry.ClientOptions{ - BeforeSend: func(event *sentry.Event, hint *sentry.EventHint) *sentry.Event { - sentryMessage = event.Message - return nil - }, - }, nil), + WithSentry(client, nil), ) logger.Info("yay") logger.Error(nil, "oops") @@ -291,3 +310,37 @@ func TestToSlogger(t *testing.T) { parsedJSON, ) } + +func TestSyncFunc(t *testing.T) { + var syncCalled bool + core := testCore{ + sync: func() error { + syncCalled = true + return nil + }, + } + _, flush := New("service-name", &core) + assert.NoError(t, flush()) + assert.True(t, syncCalled) +} + +func TestAddSinkStillSyncs(t *testing.T) { + var syncCalled [2]bool + core := testCore{ + sync: func() error { + syncCalled[0] = true + return nil + }, + } + l, _ := New("service-name", &core) + _, flush, err := AddSink(l, &testCore{ + sync: func() error { + syncCalled[1] = true + return nil + }, + }) + assert.NoError(t, err) + assert.NoError(t, flush()) + assert.True(t, syncCalled[0]) + assert.True(t, syncCalled[1]) +}