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

Author: mcastorina

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,32 +102,23 @@ 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
 // error.
 //
 // 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
 }

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])
+}