From 59d4b5d9fb5237b381c444f3990bb11a70b0b32b Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Wed, 23 Jul 2025 18:57:28 +0100 Subject: [PATCH 1/7] :sparkles: [logs] Added a rolling file logger --- changes/20250723181707.bugfix | 1 + changes/20250723182149.bugfix | 1 + changes/20250723185203.feature | 1 + utils/go.mod | 2 +- utils/go.sum | 4 +- utils/logs/fifo_logger_test.go | 3 + utils/logs/file_logger.go | 38 ++++++++++++ utils/logs/file_logger_test.go | 6 +- utils/logs/hclog_logger_test.go | 3 + utils/logs/json_logger.go | 40 ++++++++++-- utils/logs/json_logger_test.go | 40 +++++++++--- utils/logs/log.go | 11 +++- utils/logs/log_test.go | 3 + utils/logs/logr_logger_test.go | 8 +++ utils/logs/logrimp/logr_test.go | 5 +- utils/logs/logrimp/quiet_logger_test.go | 2 +- utils/logs/logrimp/slog.go | 10 +-- utils/logs/logrus_logger.go | 4 +- utils/logs/logrus_logger_test.go | 2 + utils/logs/multiple_logger_test.go | 4 ++ utils/logs/noop_logger_test.go | 2 + utils/logs/pipe_logger_test.go | 2 + utils/logs/quiet_logger_test.go | 2 + utils/logs/slog_logger.go | 4 +- utils/logs/slog_logger_test.go | 4 +- utils/logs/std_logger.go | 14 +++-- utils/logs/std_logger_test.go | 3 + utils/logs/string_logger_test.go | 3 + utils/logs/writer.go | 81 ++++++++++++++++++------- utils/logs/writer_test.go | 17 ++++-- utils/logs/zap_logger_test.go | 3 + 31 files changed, 262 insertions(+), 61 deletions(-) create mode 100644 changes/20250723181707.bugfix create mode 100644 changes/20250723182149.bugfix create mode 100644 changes/20250723185203.feature diff --git a/changes/20250723181707.bugfix b/changes/20250723181707.bugfix new file mode 100644 index 0000000000..a797dc5e0f --- /dev/null +++ b/changes/20250723181707.bugfix @@ -0,0 +1 @@ +:recycle: [logs] update slog utilities to use standard library slog rather than golang extension's implementation diff --git a/changes/20250723182149.bugfix b/changes/20250723182149.bugfix new file mode 100644 index 0000000000..361c4ff3a5 --- /dev/null +++ b/changes/20250723182149.bugfix @@ -0,0 +1 @@ +:recycle: [logs] Update ways to not close writers when loggers close diff --git a/changes/20250723185203.feature b/changes/20250723185203.feature new file mode 100644 index 0000000000..6517307817 --- /dev/null +++ b/changes/20250723185203.feature @@ -0,0 +1 @@ +:sparkles: [logs] Added a rolling file logger diff --git a/utils/go.mod b/utils/go.mod index 5bb7215861..33c3794bd1 100644 --- a/utils/go.mod +++ b/utils/go.mod @@ -39,7 +39,6 @@ require ( github.com/spf13/pflag v1.0.7 github.com/spf13/viper v1.20.1 github.com/stretchr/testify v1.10.0 - github.com/zailic/slogr v0.0.2-alpha go.uber.org/atomic v1.11.0 go.uber.org/goleak v1.3.0 go.uber.org/mock v0.5.2 @@ -52,6 +51,7 @@ require ( golang.org/x/sync v0.16.0 golang.org/x/sys v0.34.0 golang.org/x/text v0.27.0 + gopkg.in/natefinch/lumberjack.v2 v2.2.1 ) require ( diff --git a/utils/go.sum b/utils/go.sum index 568966eef5..97705e938a 100644 --- a/utils/go.sum +++ b/utils/go.sum @@ -235,8 +235,6 @@ github.com/xanzy/ssh-agent v0.3.3 h1:+/15pJfg/RsTxqYcX6fHqOXZwwMP+2VyYWJeWM2qQFM github.com/xanzy/ssh-agent v0.3.3/go.mod h1:6dzNDKs0J9rVPHPhaGCukekBHKqfl+L3KghI1Bc68Uw= github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0= github.com/yusufpapurcu/wmi v1.2.4/go.mod h1:SBZ9tNy3G9/m5Oi98Zks0QjeHVDvuK0qfxQmPyzfmi0= -github.com/zailic/slogr v0.0.2-alpha h1:ZZ+96+AOnk4L9JoPkZ6aGbGXnn90/53A9zm9JcjYSYc= -github.com/zailic/slogr v0.0.2-alpha/go.mod h1:cwplHb/RBT+83E4QzPcVtCs0Z/sAjmgMtC09XGm9SCU= go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE= go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0= go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= @@ -326,6 +324,8 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= +gopkg.in/natefinch/lumberjack.v2 v2.2.1 h1:bBRl1b0OH9s/DuPhuXpNl+VtCaJXFZ5/uEFST95x9zc= +gopkg.in/natefinch/lumberjack.v2 v2.2.1/go.mod h1:YD8tP3GAjkrDg1eZH7EGmyESg/lsYskCTPBJVb9jqSc= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= diff --git a/utils/logs/fifo_logger_test.go b/utils/logs/fifo_logger_test.go index ece2dba43b..18ef384143 100644 --- a/utils/logs/fifo_logger_test.go +++ b/utils/logs/fifo_logger_test.go @@ -16,9 +16,11 @@ import ( "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/commonerrors/errortest" "github.com/ARM-software/golang-utils/utils/parallelisation" + "go.uber.org/goleak" ) func TestFIFOLoggerLineIterator(t *testing.T) { + defer goleak.VerifyNone(t) t.Run("logger tests", func(t *testing.T) { loggers, err := NewFIFOLogger() require.NoError(t, err) @@ -48,6 +50,7 @@ func TestFIFOLoggerLineIterator(t *testing.T) { } func TestPlainFIFOLoggerLineIterator(t *testing.T) { + defer goleak.VerifyNone(t) t.Run("logger tests", func(t *testing.T) { loggers, err := NewPlainFIFOLogger() require.NoError(t, err) diff --git a/utils/logs/file_logger.go b/utils/logs/file_logger.go index fda594276c..fa150f95b3 100644 --- a/utils/logs/file_logger.go +++ b/utils/logs/file_logger.go @@ -6,9 +6,19 @@ package logs import ( + "fmt" "io" + "log" + "time" "github.com/sirupsen/logrus" + "gopkg.in/natefinch/lumberjack.v2" + + "github.com/ARM-software/golang-utils/utils/commonerrors" + "github.com/ARM-software/golang-utils/utils/parallelisation" + "github.com/ARM-software/golang-utils/utils/reflection" + "github.com/ARM-software/golang-utils/utils/safecast" + sizeUnits "github.com/ARM-software/golang-utils/utils/units/size" ) // NewFileLogger creates a logger to a file. @@ -29,3 +39,31 @@ func NewFileOnlyLogger(logFile string, loggerSource string) (loggers Loggers, er func CreateFileLogger(logFile string, loggerSource string) (loggers Loggers, err error) { return NewFileLogger(logFile, loggerSource) } + +// NewRollingFilesLogger creates a rolling file logger using [lumberjack](https://github.com/natefinch/lumberjack) under the bonnet. +// maxSize is the maximum size in bytes of a log file before it gets rotated. +// maxBackups is the maximum number of old log files to retain. +// maxAge is the maximum duration old log files are retained. +func NewRollingFilesLogger(logFile string, loggerSource string, maxFileSize float64, maxBackups int, maxAge time.Duration) (loggers Loggers, err error) { + if reflection.IsEmpty(logFile) { + err = commonerrors.New(commonerrors.ErrInvalidDestination, "missing file destination") + return + } + l := &lumberjack.Logger{ + Filename: logFile, + MaxSize: safecast.ToInt(maxFileSize / sizeUnits.MiB), + MaxAge: safecast.ToInt(maxAge.Hours() / 24), + MaxBackups: maxBackups, + LocalTime: false, + Compress: false, + } + closerStore := parallelisation.NewCloserStore(false) + closerStore.RegisterCloser(l) + + loggers = &GenericLoggers{ + Output: log.New(l, fmt.Sprintf("[%v] Output: ", loggerSource), log.LstdFlags), + Error: log.New(l, fmt.Sprintf("[%v] Error: ", loggerSource), log.LstdFlags), + closeStore: closerStore, + } + return +} diff --git a/utils/logs/file_logger_test.go b/utils/logs/file_logger_test.go index a286c21188..74c06b2493 100644 --- a/utils/logs/file_logger_test.go +++ b/utils/logs/file_logger_test.go @@ -6,15 +6,16 @@ package logs import ( "fmt" - "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" + "testing" "github.com/ARM-software/golang-utils/utils/filesystem" ) func TestFileLogger(t *testing.T) { + defer goleak.VerifyNone(t) var tests = []struct { loggerCreationFunc func(path string) (Loggers, error) }{ @@ -28,6 +29,7 @@ func TestFileLogger(t *testing.T) { for i := range tests { test := tests[i] t.Run(fmt.Sprintf("logger %v", i), func(t *testing.T) { + defer goleak.VerifyNone(t) file, err := filesystem.TouchTempFileInTempDir("test-filelog-*.log") require.NoError(t, err) diff --git a/utils/logs/hclog_logger_test.go b/utils/logs/hclog_logger_test.go index 5367634af6..fd18056f85 100644 --- a/utils/logs/hclog_logger_test.go +++ b/utils/logs/hclog_logger_test.go @@ -9,11 +9,13 @@ import ( "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/logs/logstest" ) func TestHclogLogger(t *testing.T) { + defer goleak.VerifyNone(t) logger := hclog.New(nil) loggers, err := NewHclogLogger(logger, "Test") require.NoError(t, err) @@ -21,6 +23,7 @@ func TestHclogLogger(t *testing.T) { } func TestHclogWrapper(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewLogrLogger(logstest.NewTestLogger(t), "test") require.NoError(t, err) hcLogger, err := NewHclogWrapper(loggers) diff --git a/utils/logs/json_logger.go b/utils/logs/json_logger.go index 580100f878..90d1dbe542 100644 --- a/utils/logs/json_logger.go +++ b/utils/logs/json_logger.go @@ -12,6 +12,7 @@ import ( "github.com/rs/zerolog" "github.com/ARM-software/golang-utils/utils/commonerrors" + "github.com/ARM-software/golang-utils/utils/parallelisation" ) // JSONLoggers defines a JSON logger @@ -22,6 +23,7 @@ type JSONLoggers struct { loggerSource string writer WriterWithSource zerologger zerolog.Logger + closerStore *parallelisation.CloserStore } func (l *JSONLoggers) SetLogSource(source string) error { @@ -88,16 +90,31 @@ func (l *JSONLoggers) LogError(err ...interface{}) { func (l *JSONLoggers) Close() error { l.mu.Lock() defer l.mu.Unlock() - return l.writer.Close() + return l.closerStore.Close() } // NewJSONLogger creates a Json logger. -func NewJSONLogger(writer WriterWithSource, loggerSource string, source string) (loggers Loggers, err error) { +func NewJSONLogger(writer WriterWithSource, loggerSource string, source string) (Loggers, error) { + return newJSONLogger(true, writer, loggerSource, source) +} + +// NewJSONLogger creates a Json logger. It is similar to NewJSONLogger but does not close the writer on Close(). +func NewJSONLoggerWithWriter(writer WriterWithSource, loggerSource string, source string) (Loggers, error) { + return newJSONLogger(false, writer, loggerSource, source) +} + +func newJSONLogger(closeWriterOnClose bool, writer WriterWithSource, loggerSource string, source string) (loggers Loggers, err error) { + closeStore := parallelisation.NewCloserStore(false) + if closeWriterOnClose { + closeStore.RegisterCloser(writer) + } + zerroLogger := JSONLoggers{ source: source, loggerSource: loggerSource, writer: writer, zerologger: zerolog.New(writer), + closerStore: closeStore, } err = zerroLogger.Check() if err != nil { @@ -112,8 +129,8 @@ func NewJSONLogger(writer WriterWithSource, loggerSource string, source string) return } -// NewJSONLoggerForSlowWriter creates a lock free, non blocking & thread safe logger -// wrapped around slowWriter +// NewJSONLoggerForSlowWriter creates a lock free, non-blocking & thread safe logger +// wrapped around slowWriter. The slowWriter is closed on Close. // // params: // slowWriter : writer used to write data streams @@ -126,3 +143,18 @@ func NewJSONLogger(writer WriterWithSource, loggerSource string, source string) func NewJSONLoggerForSlowWriter(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, loggerSource string, source string, droppedMessagesLogger Loggers) (loggers Loggers, err error) { return NewJSONLogger(NewDiodeWriterForSlowWriter(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) } + +// NewJSONLoggerForSlowWriters creates a lock free, non-blocking & thread safe logger +// wrapped around slowWriter. It is similar to NewJSONLoggerForSlowWriter but does not close the writer on Close(). +// +// params: +// slowWriter : writer used to write data streams +// ringBufferSize : size of ring buffer used to receive messages +// pollInterval : polling duration to check buffer content +// loggerSource : logger application name +// source : source string +// droppedMessagesLogger : logger for dropped messages +// If pollInterval is greater than 0, a poller is used otherwise a waiter is used. +func NewJSONLoggerForSlowWriters(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, loggerSource string, source string, droppedMessagesLogger Loggers) (loggers Loggers, err error) { + return NewJSONLogger(NewDiodeWriter(slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger), loggerSource, source) +} diff --git a/utils/logs/json_logger_test.go b/utils/logs/json_logger_test.go index 2a754b0865..efd0d50711 100644 --- a/utils/logs/json_logger_test.go +++ b/utils/logs/json_logger_test.go @@ -9,19 +9,45 @@ import ( "time" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestLogMessage(t *testing.T) { - loggers, err := NewJSONLogger(&StdWriter{}, "Test", "TestLogMessage") - require.NoError(t, err) - testLog(t, loggers) + defer goleak.VerifyNone(t) + t.Run("with writer closing", func(t *testing.T) { + loggers, err := NewJSONLogger(NewStdWriterWithSource(), "Test", "TestLogMessage") + require.NoError(t, err) + testLog(t, loggers) + }) + t.Run("without writer closing", func(t *testing.T) { + writer := NewStdWriterWithSource() + defer func() { _ = writer.Close() }() + loggers, err := NewJSONLoggerWithWriter(writer, "Test", "TestLogMessage") + require.NoError(t, err) + testLog(t, loggers) + require.NoError(t, writer.Close()) + }) } func TestLogMessageToSlowLogger(t *testing.T) { + defer goleak.VerifyNone(t) stdloggers, err := NewStdLogger("ERR:") require.NoError(t, err) - loggers, err := NewJSONLoggerForSlowWriter(&SlowWriter{}, 1024, 2*time.Millisecond, "Test", "TestLogMessageToSlowLogger", stdloggers) - require.NoError(t, err) - testLog(t, loggers) - time.Sleep(100 * time.Millisecond) + + defer goleak.VerifyNone(t) + t.Run("with writer closing", func(t *testing.T) { + loggers, err := NewJSONLoggerForSlowWriter(NewTestSlowWriter(t), 1024, 2*time.Millisecond, "Test", "TestLogMessageToSlowLogger", stdloggers) + require.NoError(t, err) + testLog(t, loggers) + time.Sleep(100 * time.Millisecond) + }) + t.Run("without writer closing", func(t *testing.T) { + writer := NewTestSlowWriter(t) + defer func() { _ = writer.Close() }() + loggers, err := NewJSONLoggerForSlowWriters(writer, 1024, 2*time.Millisecond, "Test", "TestLogMessageToSlowLogger", stdloggers) + require.NoError(t, err) + testLog(t, loggers) + time.Sleep(100 * time.Millisecond) + require.NoError(t, writer.Close()) + }) } diff --git a/utils/logs/log.go b/utils/logs/log.go index d37013099a..7f6127fb53 100644 --- a/utils/logs/log.go +++ b/utils/logs/log.go @@ -12,11 +12,13 @@ import ( "time" "github.com/ARM-software/golang-utils/utils/commonerrors" + "github.com/ARM-software/golang-utils/utils/parallelisation" ) type GenericLoggers struct { - Output *log.Logger - Error *log.Logger + Output *log.Logger + Error *log.Logger + closeStore *parallelisation.CloserStore } func (l *GenericLoggers) Check() error { @@ -44,7 +46,10 @@ func (l *GenericLoggers) LogError(err ...interface{}) { // Close closes the logger func (l *GenericLoggers) Close() error { - return nil + if l.closeStore == nil { + return nil + } + return l.closeStore.Close() } type AsynchronousLoggers struct { diff --git a/utils/logs/log_test.go b/utils/logs/log_test.go index cb8272ca2d..171f9ddf5a 100644 --- a/utils/logs/log_test.go +++ b/utils/logs/log_test.go @@ -9,11 +9,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/commonerrors" ) func TestLog(t *testing.T) { + defer goleak.VerifyNone(t) var loggers Loggers = &GenericLoggers{} err := loggers.Check() assert.Error(t, err) @@ -22,6 +24,7 @@ func TestLog(t *testing.T) { } func testLog(t *testing.T, loggers Loggers) { + t.Helper() err := loggers.Check() require.NoError(t, err) defer func() { _ = loggers.Close() }() diff --git a/utils/logs/logr_logger_test.go b/utils/logs/logr_logger_test.go index 47be7d539e..5b2380089c 100644 --- a/utils/logs/logr_logger_test.go +++ b/utils/logs/logr_logger_test.go @@ -14,6 +14,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/commonerrors/errortest" @@ -21,6 +22,7 @@ import ( ) func TestLogrLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewLogrLogger(logstest.NewTestLogger(t), "Test") require.NoError(t, err) testLog(t, loggers) @@ -31,6 +33,7 @@ func TestLogrLogger(t *testing.T) { } func TestLogrLoggerConversion(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewLogrLogger(logstest.NewTestLogger(t), "Test") require.NoError(t, err) converted := NewLogrLoggerFromLoggers(loggers) @@ -39,6 +42,7 @@ func TestLogrLoggerConversion(t *testing.T) { } func TestLogrFromLoggersWithMultipleName(t *testing.T) { + defer goleak.VerifyNone(t) loggerSource := "src-" + faker.Name() strLogger, err := NewStringLogger(loggerSource) require.NoError(t, err) @@ -52,6 +56,7 @@ func TestLogrFromLoggersWithMultipleName(t *testing.T) { } func TestLoggersFromLoggerWithMultipleSource(t *testing.T) { + defer goleak.VerifyNone(t) loggerSource := "src-" + faker.Name() strLogger, err := NewStringLogger(loggerSource) require.NoError(t, err) @@ -74,6 +79,7 @@ func TestLoggersFromLoggerWithMultipleSource(t *testing.T) { } func TestLogrLoggerConversionPlain(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewPipeLogger() require.NoError(t, err) converted := NewPlainLogrLoggerFromLoggers(loggers) @@ -82,6 +88,7 @@ func TestLogrLoggerConversionPlain(t *testing.T) { } func TestGetLogrFromEmptyContext(t *testing.T) { + defer goleak.VerifyNone(t) ctx := context.Background() logger, err := GetLogrLoggerFromContext(ctx) @@ -90,6 +97,7 @@ func TestGetLogrFromEmptyContext(t *testing.T) { } func TestGetLogrFromContext(t *testing.T) { + defer goleak.VerifyNone(t) ctx := context.Background() logger := logstest.NewTestLogger(t) ctx = logr.NewContext(ctx, logger) diff --git a/utils/logs/logrimp/logr_test.go b/utils/logs/logrimp/logr_test.go index 023c163d0d..c4c106e25b 100644 --- a/utils/logs/logrimp/logr_test.go +++ b/utils/logs/logrimp/logr_test.go @@ -1,6 +1,7 @@ package logrimp import ( + "log/slog" "os" "testing" @@ -9,13 +10,14 @@ import ( "github.com/hashicorp/go-hclog" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "go.uber.org/zap" - "golang.org/x/exp/slog" "github.com/ARM-software/golang-utils/utils/commonerrors" ) func TestLoggerImplementations(t *testing.T) { + defer goleak.VerifyNone(t) zl, err := zap.NewDevelopment() require.NoError(t, err) tests := []struct { @@ -50,6 +52,7 @@ func TestLoggerImplementations(t *testing.T) { for i := range tests { test := tests[i] t.Run(test.name, func(t *testing.T) { + defer goleak.VerifyNone(t) logger := test.Logger logger.WithName(faker.Name()).WithValues("foo", "bar").Info(faker.Sentence()) logger.Error(commonerrors.ErrUnexpected, faker.Sentence(), faker.Word(), faker.Name()) diff --git a/utils/logs/logrimp/quiet_logger_test.go b/utils/logs/logrimp/quiet_logger_test.go index ca26df1136..ad38ee2efe 100644 --- a/utils/logs/logrimp/quiet_logger_test.go +++ b/utils/logs/logrimp/quiet_logger_test.go @@ -1,6 +1,7 @@ package logrimp import ( + "log/slog" "os" "testing" @@ -10,7 +11,6 @@ import ( "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "go.uber.org/zap" - "golang.org/x/exp/slog" "github.com/ARM-software/golang-utils/utils/commonerrors" ) diff --git a/utils/logs/logrimp/slog.go b/utils/logs/logrimp/slog.go index c6d94764dc..7354df0fc8 100644 --- a/utils/logs/logrimp/slog.go +++ b/utils/logs/logrimp/slog.go @@ -1,13 +1,15 @@ package logrimp import ( + "log/slog" + "github.com/go-logr/logr" - "github.com/zailic/slogr" - "golang.org/x/exp/slog" ) // NewSlogLogger returns a new [slog logger](see https://pkg.go.dev/golang.org/x/exp/slog) which will be part of the standard library. func NewSlogLogger(logger *slog.Logger) logr.Logger { - // FIXME change dependency when needed https://github.com/go-logr/logr/issues/171 - return slogr.New(logger) + if logger == nil { + return logr.Discard() + } + return logr.FromSlogHandler(logger.Handler()) } diff --git a/utils/logs/logrus_logger.go b/utils/logs/logrus_logger.go index bec30a7d85..75d4a15eb5 100644 --- a/utils/logs/logrus_logger.go +++ b/utils/logs/logrus_logger.go @@ -7,8 +7,6 @@ package logs import ( - "fmt" - "github.com/rifflock/lfshook" "github.com/sirupsen/logrus" @@ -33,7 +31,7 @@ func NewLogrusLoggerWithFileHook(logrusL *logrus.Logger, loggerSource string, lo return } if reflection.IsEmpty(logFilePath) { - err = fmt.Errorf("%w: missing file destination", commonerrors.ErrInvalidDestination) + err = commonerrors.New(commonerrors.ErrInvalidDestination, "missing file destination") return } pathMap := lfshook.PathMap{ diff --git a/utils/logs/logrus_logger_test.go b/utils/logs/logrus_logger_test.go index 969e04cdd0..43e6a80b19 100644 --- a/utils/logs/logrus_logger_test.go +++ b/utils/logs/logrus_logger_test.go @@ -9,9 +9,11 @@ import ( "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestLogrusLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewLogrusLogger(logrus.StandardLogger(), "Test") require.NoError(t, err) testLog(t, loggers) diff --git a/utils/logs/multiple_logger_test.go b/utils/logs/multiple_logger_test.go index 8efa71da9a..a8af6e6519 100644 --- a/utils/logs/multiple_logger_test.go +++ b/utils/logs/multiple_logger_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/commonerrors/errortest" @@ -19,12 +20,14 @@ import ( ) func TestMultipleLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewMultipleLoggers("Test") require.NoError(t, err) testLog(t, loggers) } func TestCombinedLogger(t *testing.T) { + defer goleak.VerifyNone(t) _, err := NewCombinedLoggers() errortest.RequireError(t, err, commonerrors.ErrNoLogger) testLogger, err := NewLogrLogger(logstest.NewTestLogger(t), "Test") @@ -37,6 +40,7 @@ func TestCombinedLogger(t *testing.T) { } func TestMultipleLoggers(t *testing.T) { + defer goleak.VerifyNone(t) t.Run("Manually add loggers", func(t *testing.T) { // With default logger loggers, err := NewMultipleLoggers("Test Multiple") diff --git a/utils/logs/noop_logger_test.go b/utils/logs/noop_logger_test.go index 508cab0291..cf9af4add9 100644 --- a/utils/logs/noop_logger_test.go +++ b/utils/logs/noop_logger_test.go @@ -8,9 +8,11 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestNoopLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewNoopLogger("Test") require.NoError(t, err) testLog(t, loggers) diff --git a/utils/logs/pipe_logger_test.go b/utils/logs/pipe_logger_test.go index b533b0ca8b..8ef0ac6672 100644 --- a/utils/logs/pipe_logger_test.go +++ b/utils/logs/pipe_logger_test.go @@ -8,9 +8,11 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestPipeLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewPipeLogger() require.NoError(t, err) testLog(t, loggers) diff --git a/utils/logs/quiet_logger_test.go b/utils/logs/quiet_logger_test.go index dac2b30e91..a992fce72e 100644 --- a/utils/logs/quiet_logger_test.go +++ b/utils/logs/quiet_logger_test.go @@ -8,9 +8,11 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestQuietLogger(t *testing.T) { + defer goleak.VerifyNone(t) logger, err := NewStdLogger("Test") require.NoError(t, err) loggers, err := NewQuietLogger(logger) diff --git a/utils/logs/slog_logger.go b/utils/logs/slog_logger.go index 81c5ff55be..49754cab11 100644 --- a/utils/logs/slog_logger.go +++ b/utils/logs/slog_logger.go @@ -7,13 +7,13 @@ package logs import ( - "golang.org/x/exp/slog" + "log/slog" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/logs/logrimp" ) -// NewSlogLogger returns a logger which uses slog logger (andard library package ) +// NewSlogLogger returns a logger which uses slog logger (standard library package) func NewSlogLogger(slogL *slog.Logger, loggerSource string) (loggers Loggers, err error) { if slogL == nil { err = commonerrors.ErrNoLogger diff --git a/utils/logs/slog_logger_test.go b/utils/logs/slog_logger_test.go index 39a89a216d..120fff812f 100644 --- a/utils/logs/slog_logger_test.go +++ b/utils/logs/slog_logger_test.go @@ -5,14 +5,16 @@ package logs import ( + "log/slog" "os" "testing" "github.com/stretchr/testify/require" - "golang.org/x/exp/slog" + "go.uber.org/goleak" ) func TestSlogLogger(t *testing.T) { + defer goleak.VerifyNone(t) logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) loggers, err := NewSlogLogger(logger, "Test") require.NoError(t, err) diff --git a/utils/logs/std_logger.go b/utils/logs/std_logger.go index 2479326349..2a31cbd4d0 100644 --- a/utils/logs/std_logger.go +++ b/utils/logs/std_logger.go @@ -16,7 +16,6 @@ import ( ) type StdWriter struct { - WriterWithSource } func (w *StdWriter) Write(p []byte) (n int, err error) { @@ -32,8 +31,11 @@ func (w *StdWriter) SetSource(source string) error { return err } +func NewStdWriterWithSource() WriterWithSource { + return &StdWriter{} +} + type StdErrWriter struct { - WriterWithSource } func (w *StdErrWriter) Write(p []byte) (n int, err error) { @@ -44,10 +46,14 @@ func (w *StdErrWriter) Close() error { return nil } -func (w *StdErrWriter) SetSource(source string) error { +func (w *StdErrWriter) SetSource(_ string) error { return nil } +func NewStdErrWriterWithSource() WriterWithSource { + return &StdErrWriter{} +} + // NewStdLogger creates a logger to standard output/error func NewStdLogger(loggerSource string) (loggers Loggers, err error) { loggers = &GenericLoggers{ @@ -65,7 +71,7 @@ func CreateStdLogger(loggerSource string) (loggers Loggers, err error) { } func NewAsynchronousStdLogger(loggerSource string, ringBufferSize int, pollInterval time.Duration, source string) (loggers Loggers, err error) { - return NewAsynchronousLoggers(&StdWriter{}, &StdErrWriter{}, ringBufferSize, pollInterval, loggerSource, source, nil) + return NewAsynchronousLoggers(NewStdWriterWithSource(), NewStdErrWriterWithSource(), ringBufferSize, pollInterval, loggerSource, source, nil) } func NewGolangStdLoggerFromLoggers(loggers Loggers, toStdErr bool) StdLogger { diff --git a/utils/logs/std_logger_test.go b/utils/logs/std_logger_test.go index e930502096..05684bff94 100644 --- a/utils/logs/std_logger_test.go +++ b/utils/logs/std_logger_test.go @@ -9,15 +9,18 @@ import ( "time" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestStdLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewStdLogger("Test") require.NoError(t, err) testLog(t, loggers) } func TestAsynchronousStdLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewAsynchronousStdLogger("Test", 1024, 2*time.Millisecond, "test source") require.NoError(t, err) testLog(t, loggers) diff --git a/utils/logs/string_logger_test.go b/utils/logs/string_logger_test.go index 3135a118ed..d18f66d748 100644 --- a/utils/logs/string_logger_test.go +++ b/utils/logs/string_logger_test.go @@ -9,9 +9,11 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) func TestStringLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewStringLogger("Test") require.NoError(t, err) testLog(t, loggers) @@ -24,6 +26,7 @@ func TestStringLogger(t *testing.T) { } func TestPlainStringLogger(t *testing.T) { + defer goleak.VerifyNone(t) loggers, err := NewPlainStringLogger() require.NoError(t, err) testLog(t, loggers) diff --git a/utils/logs/writer.go b/utils/logs/writer.go index d30b5dadbe..20b12831d5 100644 --- a/utils/logs/writer.go +++ b/utils/logs/writer.go @@ -14,11 +14,14 @@ import ( "github.com/rs/zerolog/diode" "github.com/ARM-software/golang-utils/utils/commonerrors" + "github.com/ARM-software/golang-utils/utils/parallelisation" ) type MultipleWritersWithSource struct { - mu sync.RWMutex - writers []WriterWithSource + mu sync.RWMutex + writers []WriterWithSource + closeWriters bool + closerStore *parallelisation.CloserStore } func (w *MultipleWritersWithSource) GetWriters() ([]WriterWithSource, error) { @@ -31,6 +34,11 @@ func (w *MultipleWritersWithSource) AddWriters(writers ...WriterWithSource) erro w.mu.Lock() defer w.mu.Unlock() w.writers = append(w.writers, writers...) + if w.closeWriters { + for i := range writers { + w.closerStore.RegisterCloser(writers[i]) + } + } return nil } func (w *MultipleWritersWithSource) Write(p []byte) (n int, err error) { @@ -38,8 +46,8 @@ func (w *MultipleWritersWithSource) Write(p []byte) (n int, err error) { if err != nil { return } - for _, writer := range writers { - n, _ = writer.Write(p) + for i := range writers { + n, _ = writers[i].Write(p) } return } @@ -49,28 +57,35 @@ func (w *MultipleWritersWithSource) SetSource(source string) (err error) { if err != nil { return } - for _, writer := range writers { - err = writer.SetSource(source) + for i := range writers { + err = writers[i].SetSource(source) } return } func (w *MultipleWritersWithSource) Close() (err error) { - writers, err := w.GetWriters() - if err != nil { - return - } - for _, writer := range writers { - err1 := writer.Close() - if err1 != nil { - err = err1 - } - } + err = w.closerStore.Close() return } -func NewMultipleWritersWithSource(writers ...WriterWithSource) (writer *MultipleWritersWithSource, err error) { - writer = &MultipleWritersWithSource{} +// NewMultipleWritersWithSource returns a writer which writes to multiple writers. +// On close, all sub writers are also closed. +func NewMultipleWritersWithSource(writers ...WriterWithSource) (*MultipleWritersWithSource, error) { + return newWritersWithSource(true, writers...) +} + +// NewWritersWithSource returns a writer which writes to multiple writers. +// It is similar to NewWritersWithSource but differs when closing as the sub writers are not closed and, it is the responsibility of their creator to do so. +func NewWritersWithSource(writers ...WriterWithSource) (*MultipleWritersWithSource, error) { + return newWritersWithSource(false, writers...) +} + +func newWritersWithSource(closeWriterOnClose bool, writers ...WriterWithSource) (writer *MultipleWritersWithSource, err error) { + writer = &MultipleWritersWithSource{ + writers: nil, + closeWriters: closeWriterOnClose, + closerStore: parallelisation.NewCloserStore(false), + } err = writer.AddWriters(writers...) return } @@ -86,6 +101,7 @@ type DiodeWriter struct { WriterWithSource diodeWriter io.Writer slowWriter WriterWithSource + closeStore *parallelisation.CloserStore } func (w *DiodeWriter) Write(p []byte) (n int, err error) { @@ -107,13 +123,36 @@ func (w *DiodeWriter) SetSource(source string) error { return w.slowWriter.SetSource(source) } +// NewDiodeWriterForSlowWriter returns a thread-safe, lock-free, non-blocking WriterWithSource using a diode. On close, the writer is also closed. func NewDiodeWriterForSlowWriter(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, droppedMessagesLogger Loggers) WriterWithSource { - return &DiodeWriter{diodeWriter: diode.NewWriter(slowWriter, ringBufferSize, pollInterval, func(missed int) { + return newDiodeWriterForSlowWriter(true, slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger) +} + +// NewDiodeWriter returns a thread-safe, lock-free, non-blocking WriterWithSource using a diode. It is similar to NewDiodeWriterForSlowWriter but differs in that the writer is not closed when closing, only the internal diode. +func NewDiodeWriter(slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, droppedMessagesLogger Loggers) WriterWithSource { + return newDiodeWriterForSlowWriter(false, slowWriter, ringBufferSize, pollInterval, droppedMessagesLogger) +} + +func newDiodeWriterForSlowWriter(closeWriterOnClose bool, slowWriter WriterWithSource, ringBufferSize int, pollInterval time.Duration, droppedMessagesLogger Loggers) WriterWithSource { + closerStore := parallelisation.NewCloserStore(false) + if closeWriterOnClose { + closerStore.RegisterCloser(slowWriter) + } + d := diode.NewWriter(slowWriter, ringBufferSize, pollInterval, func(missed int) { if droppedMessagesLogger != nil { droppedMessagesLogger.LogError(fmt.Sprintf("Logger dropped %d messages", missed)) } - }), - slowWriter: slowWriter, + }) + var diodeWriter io.Writer + diodeWriter = d + if diodeCloser, ok := diodeWriter.(io.Closer); ok { + closerStore.RegisterCloser(diodeCloser) + } + + return &DiodeWriter{ + diodeWriter: diodeWriter, + slowWriter: slowWriter, + closeStore: closerStore, } } diff --git a/utils/logs/writer_test.go b/utils/logs/writer_test.go index a595380d23..4df4b64e46 100644 --- a/utils/logs/writer_test.go +++ b/utils/logs/writer_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/stretchr/testify/require" + "go.uber.org/goleak" ) type SlowWriter struct { @@ -22,9 +23,16 @@ func (w *SlowWriter) Write(p []byte) (n int, err error) { return os.Stdout.Write(p) } +func NewTestSlowWriter(t *testing.T) *SlowWriter { + t.Helper() + return &SlowWriter{} +} + // Creates a logger to standard output/error -func CreateMultipleWriterLogger(prefix string) (loggers Loggers, err error) { - writer, err := NewMultipleWritersWithSource(&StdWriter{}, &SlowWriter{}) +func NewTestMultipleWriterLogger(t *testing.T, prefix string) (loggers Loggers) { + t.Helper() + writer, err := NewMultipleWritersWithSource(NewTestSlowWriter(t), NewTestSlowWriter(t)) + require.NoError(t, err) loggers = &GenericLoggers{ Output: log.New(writer, "["+prefix+"] Output: ", log.LstdFlags), Error: log.New(writer, "["+prefix+"] Error: ", log.LstdFlags), @@ -33,8 +41,7 @@ func CreateMultipleWriterLogger(prefix string) (loggers Loggers, err error) { } func TestMultipleWriters(t *testing.T) { - stdloggers, err := CreateMultipleWriterLogger("Test") - require.NoError(t, err) - testLog(t, stdloggers) + defer goleak.VerifyNone(t) + testLog(t, NewTestMultipleWriterLogger(t, "Test")) time.Sleep(100 * time.Millisecond) } diff --git a/utils/logs/zap_logger_test.go b/utils/logs/zap_logger_test.go index 77f82d1a96..a93d03bf0d 100644 --- a/utils/logs/zap_logger_test.go +++ b/utils/logs/zap_logger_test.go @@ -8,10 +8,12 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "go.uber.org/zap" ) func TestZapLoggerDev(t *testing.T) { + defer goleak.VerifyNone(t) logger, err := zap.NewDevelopment() require.NoError(t, err) loggers, err := NewZapLogger(logger, "Test") @@ -20,6 +22,7 @@ func TestZapLoggerDev(t *testing.T) { } func TestZapLoggerProd(t *testing.T) { + defer goleak.VerifyNone(t) logger, err := zap.NewProduction() require.NoError(t, err) loggers, err := NewZapLogger(logger, "Test") From 9402001a482c3fbc5574a27751ac0494bb739a18 Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 05:38:54 +0100 Subject: [PATCH 2/7] :green_heart: Linting --- utils/go.mod | 2 +- utils/go.sum | 6 ++++-- utils/logs/fifo_logger_test.go | 2 +- utils/logs/file_logger.go | 33 ++++++++++++++++++++++++++++++--- utils/logs/file_logger_test.go | 10 +++++++++- utils/logs/writer.go | 2 +- 6 files changed, 46 insertions(+), 9 deletions(-) diff --git a/utils/go.mod b/utils/go.mod index 33c3794bd1..dc13e47045 100644 --- a/utils/go.mod +++ b/utils/go.mod @@ -5,6 +5,7 @@ go 1.23.0 toolchain go1.24.1 require ( + github.com/DeRuina/timberjack v1.3.2 github.com/OneOfOne/xxhash v1.2.8 github.com/avast/retry-go/v4 v4.6.1 github.com/bmatcuk/doublestar/v3 v3.0.0 @@ -51,7 +52,6 @@ require ( golang.org/x/sync v0.16.0 golang.org/x/sys v0.34.0 golang.org/x/text v0.27.0 - gopkg.in/natefinch/lumberjack.v2 v2.2.1 ) require ( diff --git a/utils/go.sum b/utils/go.sum index 97705e938a..b3081af935 100644 --- a/utils/go.sum +++ b/utils/go.sum @@ -2,6 +2,8 @@ bitbucket.org/creachadair/stringset v0.0.9/go.mod h1:t+4WcQ4+PXTa8aQdNKe40ZP6iwe dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/DeRuina/timberjack v1.3.2 h1:ei548+RsKNhWNCoah4X0TKnR8VWRnbjsEMFZRBbSNdU= +github.com/DeRuina/timberjack v1.3.2/go.mod h1:oIz9NBX34JRQd9U3ZuSEih49LuYlFnATRWiCfwbeiJ8= github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= @@ -52,6 +54,8 @@ github.com/evanphx/hclogr v0.2.0/go.mod h1:1jTbx9bMs6/CBoiPwzNobWb7BFkK/KF5XljvP github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk= github.com/fatih/color v1.16.0 h1:zmkK9Ngbjj+K0yRhTVONQh1p/HknKYSlNT+vZCzyokM= github.com/fatih/color v1.16.0/go.mod h1:fL2Sau1YI5c0pdGEVCbKQbLXB6edEj1ZgiY4NijnWvE= +github.com/fortytw2/leaktest v1.3.0 h1:u8491cBMTQ8ft8aeV+adlcytMZylmA5nnwwkRZjI8vw= +github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= @@ -324,8 +328,6 @@ gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= -gopkg.in/natefinch/lumberjack.v2 v2.2.1 h1:bBRl1b0OH9s/DuPhuXpNl+VtCaJXFZ5/uEFST95x9zc= -gopkg.in/natefinch/lumberjack.v2 v2.2.1/go.mod h1:YD8tP3GAjkrDg1eZH7EGmyESg/lsYskCTPBJVb9jqSc= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= diff --git a/utils/logs/fifo_logger_test.go b/utils/logs/fifo_logger_test.go index 18ef384143..8b4ab921bd 100644 --- a/utils/logs/fifo_logger_test.go +++ b/utils/logs/fifo_logger_test.go @@ -12,11 +12,11 @@ import ( "github.com/go-faker/faker/v4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/commonerrors/errortest" "github.com/ARM-software/golang-utils/utils/parallelisation" - "go.uber.org/goleak" ) func TestFIFOLoggerLineIterator(t *testing.T) { diff --git a/utils/logs/file_logger.go b/utils/logs/file_logger.go index fa150f95b3..530f5e00e8 100644 --- a/utils/logs/file_logger.go +++ b/utils/logs/file_logger.go @@ -11,8 +11,9 @@ import ( "log" "time" + "github.com/DeRuina/timberjack" "github.com/sirupsen/logrus" - "gopkg.in/natefinch/lumberjack.v2" + "go.uber.org/atomic" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/parallelisation" @@ -49,14 +50,14 @@ func NewRollingFilesLogger(logFile string, loggerSource string, maxFileSize floa err = commonerrors.New(commonerrors.ErrInvalidDestination, "missing file destination") return } - l := &lumberjack.Logger{ + l := newTimberJackLogger(&timberjack.Logger{ Filename: logFile, MaxSize: safecast.ToInt(maxFileSize / sizeUnits.MiB), MaxAge: safecast.ToInt(maxAge.Hours() / 24), MaxBackups: maxBackups, LocalTime: false, Compress: false, - } + }) closerStore := parallelisation.NewCloserStore(false) closerStore.RegisterCloser(l) @@ -67,3 +68,29 @@ func NewRollingFilesLogger(logFile string, loggerSource string, maxFileSize floa } return } + +// FIXME the following has only been put in place to avoid a panic when closing more than once: https://github.com/DeRuina/timberjack/issues/26 +type timberjackLogger struct { + l *timberjack.Logger + closed *atomic.Bool +} + +func (l *timberjackLogger) Write(p []byte) (n int, err error) { + return l.l.Write(p) +} + +func newTimberJackLogger(l *timberjack.Logger) io.WriteCloser { + return &timberjackLogger{ + l: l, + closed: atomic.NewBool(false), + } +} + +func (l *timberjackLogger) Close() error { + if l.closed.Load() { + return nil + } + err := l.l.Close() + l.closed.Store(true) + return err +} diff --git a/utils/logs/file_logger_test.go b/utils/logs/file_logger_test.go index 74c06b2493..580264946c 100644 --- a/utils/logs/file_logger_test.go +++ b/utils/logs/file_logger_test.go @@ -6,12 +6,15 @@ package logs import ( "fmt" + "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" - "testing" "github.com/ARM-software/golang-utils/utils/filesystem" + sizeUnits "github.com/ARM-software/golang-utils/utils/units/size" ) func TestFileLogger(t *testing.T) { @@ -25,6 +28,11 @@ func TestFileLogger(t *testing.T) { { loggerCreationFunc: func(path string) (Loggers, error) { return NewFileOnlyLogger(path, "Test") }, }, + { + loggerCreationFunc: func(path string) (Loggers, error) { + return NewRollingFilesLogger(path, "Test", sizeUnits.MiB, 2, time.Second) + }, + }, } for i := range tests { test := tests[i] diff --git a/utils/logs/writer.go b/utils/logs/writer.go index 20b12831d5..484efcca6f 100644 --- a/utils/logs/writer.go +++ b/utils/logs/writer.go @@ -143,7 +143,7 @@ func newDiodeWriterForSlowWriter(closeWriterOnClose bool, slowWriter WriterWithS droppedMessagesLogger.LogError(fmt.Sprintf("Logger dropped %d messages", missed)) } }) - var diodeWriter io.Writer + var diodeWriter io.Writer //nolint:gosimple // S1021: should merge variable declaration with assignment on next line (gosimple) diodeWriter = d if diodeCloser, ok := diodeWriter.(io.Closer); ok { closerStore.RegisterCloser(diodeCloser) From 6edaa8d5fbee8aa869a059709a6bca63732e370a Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 05:48:36 +0100 Subject: [PATCH 3/7] fix flaky test --- utils/serialization/maps/map_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/utils/serialization/maps/map_test.go b/utils/serialization/maps/map_test.go index bc49cb98ec..4f95714ca8 100644 --- a/utils/serialization/maps/map_test.go +++ b/utils/serialization/maps/map_test.go @@ -107,6 +107,10 @@ func TestToMap(t *testing.T) { t.Run("generic", func(t *testing.T) { testStruct := TestStruct1{} require.NoError(t, faker.FakeData(&testStruct)) + if len(testStruct.Array) == 0 { + // This is to avoid the case where the slice is empty and so the comparison may differ because the slice could be set to nil instead of an empty slice + testStruct.Array = []int{1212, 544} + } structMap, err := ToMap[TestStruct1](&testStruct) require.NoError(t, err) From 5de5f66e69679fe5baf2817dbee74afb7944cb31 Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 14:50:20 +0100 Subject: [PATCH 4/7] Update utils/logs/writer.go Co-authored-by: Abdelrahman Abdelraouf --- utils/logs/writer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/logs/writer.go b/utils/logs/writer.go index 484efcca6f..821fd9876a 100644 --- a/utils/logs/writer.go +++ b/utils/logs/writer.go @@ -68,7 +68,7 @@ func (w *MultipleWritersWithSource) Close() (err error) { return } -// NewMultipleWritersWithSource returns a writer which writes to multiple writers. +// NewMultipleWritersWithSource returns a writer which writes using multiple writers. // On close, all sub writers are also closed. func NewMultipleWritersWithSource(writers ...WriterWithSource) (*MultipleWritersWithSource, error) { return newWritersWithSource(true, writers...) From 2a1792d3d641c06b6038aa4c5d581f697cde2f7e Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 14:50:28 +0100 Subject: [PATCH 5/7] Update utils/logs/writer.go Co-authored-by: Abdelrahman Abdelraouf --- utils/logs/writer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/logs/writer.go b/utils/logs/writer.go index 821fd9876a..2f7000ade4 100644 --- a/utils/logs/writer.go +++ b/utils/logs/writer.go @@ -74,7 +74,7 @@ func NewMultipleWritersWithSource(writers ...WriterWithSource) (*MultipleWriters return newWritersWithSource(true, writers...) } -// NewWritersWithSource returns a writer which writes to multiple writers. +// NewWritersWithSource returns a writer which writes using multiple writers. // It is similar to NewWritersWithSource but differs when closing as the sub writers are not closed and, it is the responsibility of their creator to do so. func NewWritersWithSource(writers ...WriterWithSource) (*MultipleWritersWithSource, error) { return newWritersWithSource(false, writers...) From 93351443f0692bb981ba5ca3689b756f9b2fd803 Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 15:15:56 +0100 Subject: [PATCH 6/7] address review comments --- utils/go.mod | 8 +-- utils/go.sum | 7 +++ utils/logs/file_logger.go | 91 ++++++++++++++++++++-------------- utils/logs/file_logger_test.go | 7 ++- 4 files changed, 72 insertions(+), 41 deletions(-) diff --git a/utils/go.mod b/utils/go.mod index dc13e47045..7ec12d09dc 100644 --- a/utils/go.mod +++ b/utils/go.mod @@ -5,7 +5,7 @@ go 1.23.0 toolchain go1.24.1 require ( - github.com/DeRuina/timberjack v1.3.2 + github.com/DeRuina/timberjack v1.3.3 github.com/OneOfOne/xxhash v1.2.8 github.com/avast/retry-go/v4 v4.6.1 github.com/bmatcuk/doublestar/v3 v3.0.0 @@ -27,7 +27,7 @@ require ( github.com/hashicorp/go-cleanhttp v0.5.2 github.com/hashicorp/go-hclog v1.6.3 github.com/hashicorp/go-retryablehttp v0.7.8 - github.com/iamacarpet/go-win64api v0.0.0-20230324134531-ef6dbdd6db97 + github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847 github.com/joho/godotenv v1.5.1 github.com/mitchellh/go-homedir v1.1.0 github.com/rifflock/lfshook v0.0.0-20180920164130-b9218ef580f5 @@ -45,7 +45,7 @@ require ( go.uber.org/mock v0.5.2 go.uber.org/zap v1.27.0 golang.org/x/crypto v0.40.0 - golang.org/x/exp v0.0.0-20250506013437-ce4c2cf36ca6 + golang.org/x/exp v0.0.0-20250718183923-645b1fa84792 golang.org/x/mod v0.26.0 golang.org/x/net v0.42.0 golang.org/x/oauth2 v0.30.0 @@ -96,7 +96,7 @@ require ( github.com/xanzy/ssh-agent v0.3.3 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect go.uber.org/multierr v1.10.0 // indirect - golang.org/x/tools v0.34.0 // indirect + golang.org/x/tools v0.35.0 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/utils/go.sum b/utils/go.sum index b3081af935..751c461dd3 100644 --- a/utils/go.sum +++ b/utils/go.sum @@ -4,6 +4,8 @@ dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/DeRuina/timberjack v1.3.2 h1:ei548+RsKNhWNCoah4X0TKnR8VWRnbjsEMFZRBbSNdU= github.com/DeRuina/timberjack v1.3.2/go.mod h1:oIz9NBX34JRQd9U3ZuSEih49LuYlFnATRWiCfwbeiJ8= +github.com/DeRuina/timberjack v1.3.3 h1:R+3oEvJP1wd8iYkGGsnq9e6pHGAXFqizPKReEMj0jvQ= +github.com/DeRuina/timberjack v1.3.3/go.mod h1:pbNOvT1AIUxBqiH/ytL9nNsKrHUrDGJdz7T23y79RRU= github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= @@ -136,6 +138,8 @@ github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpO github.com/iamacarpet/go-win64api v0.0.0-20210311141720-fe38760bed28/go.mod h1:oGJx9dz0Ny7HC7U55RZ0Smd6N9p3hXP/+hOFtuYrAxM= github.com/iamacarpet/go-win64api v0.0.0-20230324134531-ef6dbdd6db97 h1:VjwKCN2PMLlMKM2k9AW8QQsfmEH43ldlX+JGeWW9cEE= github.com/iamacarpet/go-win64api v0.0.0-20230324134531-ef6dbdd6db97/go.mod h1:B7zFQPAznj+ujXel5X+LUoK3LgY6VboCdVYHZNn7gpg= +github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847 h1:cRHZFGwIDgQlr9abL/P93JXR7pYxzvf0xAIt0xzwrh0= +github.com/iamacarpet/go-win64api v0.0.0-20240507095429-873e84e85847/go.mod h1:B7zFQPAznj+ujXel5X+LUoK3LgY6VboCdVYHZNn7gpg= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= github.com/joho/godotenv v1.5.1 h1:7eLL/+HRGLY0ldzfGMeQkb7vMd0as4CfYvUVzLqw0N0= @@ -256,6 +260,8 @@ golang.org/x/crypto v0.40.0 h1:r4x+VvoG5Fm+eJcxMaY8CQM7Lb0l1lsmjGBQ6s8BfKM= golang.org/x/crypto v0.40.0/go.mod h1:Qr1vMER5WyS2dfPHAlsOj01wgLbsyWtFn/aY+5+ZdxY= golang.org/x/exp v0.0.0-20250506013437-ce4c2cf36ca6 h1:y5zboxd6LQAqYIhHnB48p0ByQ/GnQx2BE33L8BOHQkI= golang.org/x/exp v0.0.0-20250506013437-ce4c2cf36ca6/go.mod h1:U6Lno4MTRCDY+Ba7aCcauB9T60gsv5s4ralQzP72ZoQ= +golang.org/x/exp v0.0.0-20250718183923-645b1fa84792 h1:R9PFI6EUdfVKgwKjZef7QIwGcBKu86OEFpJ9nUEP2l4= +golang.org/x/exp v0.0.0-20250718183923-645b1fa84792/go.mod h1:A+z0yzpGtvnG90cToK5n2tu8UJVP2XUATh+r+sfOOOc= golang.org/x/mod v0.26.0 h1:EGMPT//Ezu+ylkCijjPc+f4Aih7sZvaAr+O3EHBxvZg= golang.org/x/mod v0.26.0/go.mod h1:/j6NAhSk8iQ723BGAUyoAcn7SlD7s15Dp9Nd/SfeaFQ= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -315,6 +321,7 @@ golang.org/x/text v0.27.0/go.mod h1:1D28KMCvyooCX9hBiosv5Tz/+YLxj0j7XhWjpSUF7CU= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.34.0 h1:qIpSLOxeCYGg9TrcJokLBG4KFA6d795g0xkBkiESGlo= golang.org/x/tools v0.34.0/go.mod h1:pAP9OwEaY1CAW3HOmg3hLZC5Z0CCmzjAF2UQMSqNARg= +golang.org/x/tools v0.35.0/go.mod h1:NKdj5HkL/73byiZSJjqJgKn3ep7KjFkBOkR/Hps3VPw= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= diff --git a/utils/logs/file_logger.go b/utils/logs/file_logger.go index 530f5e00e8..8439c2b763 100644 --- a/utils/logs/file_logger.go +++ b/utils/logs/file_logger.go @@ -13,7 +13,6 @@ import ( "github.com/DeRuina/timberjack" "github.com/sirupsen/logrus" - "go.uber.org/atomic" "github.com/ARM-software/golang-utils/utils/commonerrors" "github.com/ARM-software/golang-utils/utils/parallelisation" @@ -41,23 +40,69 @@ func CreateFileLogger(logFile string, loggerSource string) (loggers Loggers, err return NewFileLogger(logFile, loggerSource) } +type FileLoggerOptions struct { + maxFileSize float64 + maxAge time.Duration + maxBackups int +} + +type FileLoggerOption func(*FileLoggerOptions) *FileLoggerOptions + +// WithMaxFileSize sets the maximum size in bytes of a log file before it gets rotated. +func WithMaxFileSize(maxFileSize float64) FileLoggerOption { + return func(o *FileLoggerOptions) *FileLoggerOptions { + if o == nil { + return o + } + o.maxFileSize = maxFileSize + return o + } +} + +// WithMaxAge sets the maximum duration old log files are retained. +func WithMaxAge(maxAge time.Duration) FileLoggerOption { + return func(o *FileLoggerOptions) *FileLoggerOptions { + if o == nil { + return o + } + o.maxAge = maxAge + return o + } +} + +// WithMaxBackups sets the maximum number of old log files to retain. +func WithMaxBackups(maxBackups int) FileLoggerOption { + return func(o *FileLoggerOptions) *FileLoggerOptions { + if o == nil { + return o + } + o.maxBackups = maxBackups + return o + } +} + // NewRollingFilesLogger creates a rolling file logger using [lumberjack](https://github.com/natefinch/lumberjack) under the bonnet. -// maxSize is the maximum size in bytes of a log file before it gets rotated. -// maxBackups is the maximum number of old log files to retain. -// maxAge is the maximum duration old log files are retained. -func NewRollingFilesLogger(logFile string, loggerSource string, maxFileSize float64, maxBackups int, maxAge time.Duration) (loggers Loggers, err error) { +func NewRollingFilesLogger(logFile string, loggerSource string, options ...FileLoggerOption) (loggers Loggers, err error) { + opts := &FileLoggerOptions{ + maxFileSize: 100 * sizeUnits.MiB, + maxAge: 24 * time.Hour, + maxBackups: 3, + } + for i := range options { + opts = options[i](opts) + } if reflection.IsEmpty(logFile) { err = commonerrors.New(commonerrors.ErrInvalidDestination, "missing file destination") return } - l := newTimberJackLogger(&timberjack.Logger{ + l := &timberjack.Logger{ Filename: logFile, - MaxSize: safecast.ToInt(maxFileSize / sizeUnits.MiB), - MaxAge: safecast.ToInt(maxAge.Hours() / 24), - MaxBackups: maxBackups, + MaxSize: safecast.ToInt(opts.maxFileSize / sizeUnits.MiB), + MaxAge: safecast.ToInt(opts.maxAge.Hours() / 24), + MaxBackups: opts.maxBackups, LocalTime: false, Compress: false, - }) + } closerStore := parallelisation.NewCloserStore(false) closerStore.RegisterCloser(l) @@ -68,29 +113,3 @@ func NewRollingFilesLogger(logFile string, loggerSource string, maxFileSize floa } return } - -// FIXME the following has only been put in place to avoid a panic when closing more than once: https://github.com/DeRuina/timberjack/issues/26 -type timberjackLogger struct { - l *timberjack.Logger - closed *atomic.Bool -} - -func (l *timberjackLogger) Write(p []byte) (n int, err error) { - return l.l.Write(p) -} - -func newTimberJackLogger(l *timberjack.Logger) io.WriteCloser { - return &timberjackLogger{ - l: l, - closed: atomic.NewBool(false), - } -} - -func (l *timberjackLogger) Close() error { - if l.closed.Load() { - return nil - } - err := l.l.Close() - l.closed.Store(true) - return err -} diff --git a/utils/logs/file_logger_test.go b/utils/logs/file_logger_test.go index 580264946c..b5aac4438d 100644 --- a/utils/logs/file_logger_test.go +++ b/utils/logs/file_logger_test.go @@ -30,7 +30,12 @@ func TestFileLogger(t *testing.T) { }, { loggerCreationFunc: func(path string) (Loggers, error) { - return NewRollingFilesLogger(path, "Test", sizeUnits.MiB, 2, time.Second) + return NewRollingFilesLogger(path, "Test", WithMaxFileSize(sizeUnits.MiB), WithMaxBackups(2), WithMaxAge(time.Second)) + }, + }, + { + loggerCreationFunc: func(path string) (Loggers, error) { + return NewRollingFilesLogger(path, "Test") }, }, } From f8349225946630466b4197682a2a91c5c2addc74 Mon Sep 17 00:00:00 2001 From: Adrien CABARBAYE Date: Thu, 24 Jul 2025 15:42:03 +0100 Subject: [PATCH 7/7] race condition --- utils/logs/file_logger.go | 5 ++++- utils/logs/file_logger_test.go | 28 ++++++++++++++++------------ 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/utils/logs/file_logger.go b/utils/logs/file_logger.go index 8439c2b763..c4e4fcf875 100644 --- a/utils/logs/file_logger.go +++ b/utils/logs/file_logger.go @@ -65,7 +65,10 @@ func WithMaxAge(maxAge time.Duration) FileLoggerOption { if o == nil { return o } - o.maxAge = maxAge + // This is necessary to avoid a Race Condition + if maxAge >= time.Minute { + o.maxAge = maxAge + } return o } } diff --git a/utils/logs/file_logger_test.go b/utils/logs/file_logger_test.go index b5aac4438d..7936afab7f 100644 --- a/utils/logs/file_logger_test.go +++ b/utils/logs/file_logger_test.go @@ -7,14 +7,12 @@ package logs import ( "fmt" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" "github.com/ARM-software/golang-utils/utils/filesystem" - sizeUnits "github.com/ARM-software/golang-utils/utils/units/size" ) func TestFileLogger(t *testing.T) { @@ -28,16 +26,22 @@ func TestFileLogger(t *testing.T) { { loggerCreationFunc: func(path string) (Loggers, error) { return NewFileOnlyLogger(path, "Test") }, }, - { - loggerCreationFunc: func(path string) (Loggers, error) { - return NewRollingFilesLogger(path, "Test", WithMaxFileSize(sizeUnits.MiB), WithMaxBackups(2), WithMaxAge(time.Second)) - }, - }, - { - loggerCreationFunc: func(path string) (Loggers, error) { - return NewRollingFilesLogger(path, "Test") - }, - }, + // FIXME uncomment when data race is fixed + // { + // loggerCreationFunc: func(path string) (Loggers, error) { + // return NewRollingFilesLogger(path, "Test", WithMaxFileSize(sizeUnits.MiB), WithMaxBackups(2), WithMaxAge(10*time.Minute)) + // }, + // }, + // { + // loggerCreationFunc: func(path string) (Loggers, error) { + // return NewRollingFilesLogger(path, "Test", WithMaxAge(time.Second)) + // }, + // }, + // { + // loggerCreationFunc: func(path string) (Loggers, error) { + // return NewRollingFilesLogger(path, "Test") + // }, + // }, } for i := range tests { test := tests[i]