From a61a546e7f87b30bf078a07285f5ce477222b774 Mon Sep 17 00:00:00 2001 From: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Date: Wed, 25 Mar 2026 14:44:39 +0100 Subject: [PATCH 1/7] feat: add OutputFile option for log destination in logger Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> --- logger/options.go | 9 +++++++++ logger/options_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/logger/options.go b/logger/options.go index 25a15ddb..f033ae9a 100644 --- a/logger/options.go +++ b/logger/options.go @@ -33,6 +33,9 @@ type Options struct { // OutputLevel is the level of logging OutputLevel string + + // OutputFile is the destination file path for logs. + OutputFile string } // SetOutputLevel sets the log output level. @@ -62,6 +65,11 @@ func (o *Options) AttachCmdFlags( "log-level", defaultOutputLevel, "Options are debug, info, warn, error, or fatal (default info)") + stringVar( + &o.OutputFile, + "log-file", + "", + "Path to a file where logs will be written") } if boolVar != nil { @@ -79,6 +87,7 @@ func DefaultOptions() Options { JSONFormatEnabled: defaultJSONOutput, appID: undefinedAppID, OutputLevel: defaultOutputLevel, + OutputFile: "", } } diff --git a/logger/options_test.go b/logger/options_test.go index e16ff89f..24c37b9e 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -14,6 +14,9 @@ limitations under the License. package logger import ( + "os" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -26,6 +29,7 @@ func TestOptions(t *testing.T) { assert.Equal(t, defaultJSONOutput, o.JSONFormatEnabled) assert.Equal(t, undefinedAppID, o.appID) assert.Equal(t, defaultOutputLevel, o.OutputLevel) + assert.Equal(t, "", o.OutputFile) }) t.Run("set dapr ID", func(t *testing.T) { @@ -40,10 +44,14 @@ func TestOptions(t *testing.T) { o := DefaultOptions() logLevelAsserted := false + logFileAsserted := false testStringVarFn := func(p *string, name string, value string, usage string) { if name == "log-level" && value == defaultOutputLevel { logLevelAsserted = true } + if name == "log-file" && value == "" { + logFileAsserted = true + } } logAsJSONAsserted := false @@ -57,6 +65,7 @@ func TestOptions(t *testing.T) { // assert assert.True(t, logLevelAsserted) + assert.True(t, logFileAsserted) assert.True(t, logAsJSONAsserted) }) } @@ -92,3 +101,28 @@ func TestApplyOptionsToLoggers(t *testing.T) { (l.(*daprLogger)).logger.Logger.GetLevel()) } } + +func TestApplyOptionsToLoggersFileOutput(t *testing.T) { + logPath := filepath.Join(t.TempDir(), "dapr.log") + + testOptions := Options{ + OutputLevel: "debug", + OutputFile: logPath, + } + + l := NewLogger("testLoggerFileOutput") + require.NoError(t, ApplyOptionsToLoggers(&testOptions)) + + dl, ok := l.(*daprLogger) + require.True(t, ok) + fileOut, ok := dl.logger.Logger.Out.(*os.File) + require.True(t, ok) + assert.Equal(t, logPath, fileOut.Name()) + + msg := "log-file-test-message" + l.Info(msg) + + b, err := os.ReadFile(logPath) + require.NoError(t, err) + assert.True(t, strings.Contains(string(b), msg)) +} From ef1d4d27e8eb90b7f5cb9aab8dc28dbf2a8a2d12 Mon Sep 17 00:00:00 2001 From: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Date: Tue, 31 Mar 2026 22:11:54 +0200 Subject: [PATCH 2/7] feat: add file output support for logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> --- logger/options.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/logger/options.go b/logger/options.go index f033ae9a..60c2a200 100644 --- a/logger/options.go +++ b/logger/options.go @@ -15,6 +15,7 @@ package logger import ( "fmt" + "os" ) const ( @@ -113,5 +114,14 @@ func ApplyOptionsToLoggers(options *Options) error { v.SetOutputLevel(daprLogLevel) } + if options.OutputFile != "" { + file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) + if err != nil { + return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) + } + for _, v := range internalLoggers { + v.SetOutput(file) + } + } return nil } From 74ae6dff7b3ce05d98170d85f069045a4f28ce3c Mon Sep 17 00:00:00 2001 From: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Date: Tue, 31 Mar 2026 22:18:21 +0200 Subject: [PATCH 3/7] lint Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> --- logger/options.go | 2 ++ logger/options_test.go | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/logger/options.go b/logger/options.go index 60c2a200..9ac3acb8 100644 --- a/logger/options.go +++ b/logger/options.go @@ -119,9 +119,11 @@ func ApplyOptionsToLoggers(options *Options) error { if err != nil { return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) } + for _, v := range internalLoggers { v.SetOutput(file) } } + return nil } diff --git a/logger/options_test.go b/logger/options_test.go index 24c37b9e..d9082365 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -16,7 +16,6 @@ package logger import ( "os" "path/filepath" - "strings" "testing" "github.com/stretchr/testify/assert" @@ -29,7 +28,7 @@ func TestOptions(t *testing.T) { assert.Equal(t, defaultJSONOutput, o.JSONFormatEnabled) assert.Equal(t, undefinedAppID, o.appID) assert.Equal(t, defaultOutputLevel, o.OutputLevel) - assert.Equal(t, "", o.OutputFile) + assert.Empty(t, o.OutputFile) }) t.Run("set dapr ID", func(t *testing.T) { @@ -49,6 +48,7 @@ func TestOptions(t *testing.T) { if name == "log-level" && value == defaultOutputLevel { logLevelAsserted = true } + if name == "log-file" && value == "" { logFileAsserted = true } @@ -111,6 +111,7 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { } l := NewLogger("testLoggerFileOutput") + require.NoError(t, ApplyOptionsToLoggers(&testOptions)) dl, ok := l.(*daprLogger) @@ -124,5 +125,5 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { b, err := os.ReadFile(logPath) require.NoError(t, err) - assert.True(t, strings.Contains(string(b), msg)) + assert.Contains(t, string(b), msg) } From efa95122f050ae082ae356734e3d6dbf353af88a Mon Sep 17 00:00:00 2001 From: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Date: Tue, 31 Mar 2026 22:23:20 +0200 Subject: [PATCH 4/7] add cleanup for file output logger in options tests Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> --- logger/options_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/logger/options_test.go b/logger/options_test.go index d9082365..1c329329 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -119,6 +119,13 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { fileOut, ok := dl.logger.Logger.Out.(*os.File) require.True(t, ok) assert.Equal(t, logPath, fileOut.Name()) + t.Cleanup(func() { + for _, logger := range getLoggers() { + logger.SetOutput(os.Stdout) + } + + require.NoError(t, fileOut.Close()) + }) msg := "log-file-test-message" l.Info(msg) From 1b068cf36227d164673a26d1e326dc56ca40fccb Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 2 Apr 2026 15:58:36 +0100 Subject: [PATCH 5/7] fix: address review feedback for log file output - Relax file permissions from 0600 to 0644 to allow reads (per Albert's review) - Track open log file handle in package-level state protected by mutex to prevent FD leaks when ApplyOptionsToLoggers is called multiple times - Close previous log file before opening a new one on re-apply - Revert output to stdout when OutputFile is empty (handles config reload) - Extract setLogOutput helper for clarity - Add TestApplyOptionsToLoggersFileOutputReapply test verifying re-apply closes the previous file and switches to the new one Signed-off-by: Nelson Parente --- logger/options.go | 43 +++++++++++++++++++++++++++++++++------ logger/options_test.go | 46 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/logger/options.go b/logger/options.go index 9ac3acb8..8a3ad5e1 100644 --- a/logger/options.go +++ b/logger/options.go @@ -15,7 +15,9 @@ package logger import ( "fmt" + "io" "os" + "sync" ) const ( @@ -24,6 +26,12 @@ const ( undefinedAppID = "" ) +var ( + // logOutputMu protects logOutputFile from concurrent access. + logOutputMu sync.Mutex + logOutputFile *os.File +) + // Options defines the sets of options for Dapr logging. type Options struct { // appID is the unique id of Dapr Application @@ -114,15 +122,38 @@ func ApplyOptionsToLoggers(options *Options) error { v.SetOutputLevel(daprLogLevel) } - if options.OutputFile != "" { - file, err := os.OpenFile(options.OutputFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o600) + if err := setLogOutput(options.OutputFile, internalLoggers); err != nil { + return err + } + + return nil +} + +// setLogOutput configures log output destination. If path is non-empty, logs +// are written to the file at that path. If empty, output reverts to stdout. +// Any previously opened log file is closed before opening a new one. +func setLogOutput(path string, loggers map[string]Logger) error { + logOutputMu.Lock() + defer logOutputMu.Unlock() + + // Close any previously opened log file. + if logOutputFile != nil { + logOutputFile.Close() + logOutputFile = nil + } + + var out io.Writer = os.Stdout + if path != "" { + file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { - return fmt.Errorf("failed to open log file %q: %w", options.OutputFile, err) + return fmt.Errorf("failed to open log file %q: %w", path, err) } + logOutputFile = file + out = file + } - for _, v := range internalLoggers { - v.SetOutput(file) - } + for _, v := range loggers { + v.SetOutput(out) } return nil diff --git a/logger/options_test.go b/logger/options_test.go index 1c329329..ef3823dd 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -120,11 +120,10 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { require.True(t, ok) assert.Equal(t, logPath, fileOut.Name()) t.Cleanup(func() { - for _, logger := range getLoggers() { - logger.SetOutput(os.Stdout) - } - - require.NoError(t, fileOut.Close()) + // Revert to stdout, which also closes the log file. + require.NoError(t, ApplyOptionsToLoggers(&Options{ + OutputLevel: "info", + })) }) msg := "log-file-test-message" @@ -134,3 +133,40 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { require.NoError(t, err) assert.Contains(t, string(b), msg) } + +func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { + dir := t.TempDir() + logPath1 := filepath.Join(dir, "dapr1.log") + logPath2 := filepath.Join(dir, "dapr2.log") + + l := NewLogger("testLoggerReapply") + + // Apply first file output. + require.NoError(t, ApplyOptionsToLoggers(&Options{ + OutputLevel: "debug", + OutputFile: logPath1, + })) + l.Info("message-one") + + // Re-apply with a different file — should close the first. + require.NoError(t, ApplyOptionsToLoggers(&Options{ + OutputLevel: "debug", + OutputFile: logPath2, + })) + l.Info("message-two") + + t.Cleanup(func() { + require.NoError(t, ApplyOptionsToLoggers(&Options{ + OutputLevel: "info", + })) + }) + + b1, err := os.ReadFile(logPath1) + require.NoError(t, err) + assert.Contains(t, string(b1), "message-one") + assert.NotContains(t, string(b1), "message-two") + + b2, err := os.ReadFile(logPath2) + require.NoError(t, err) + assert.Contains(t, string(b2), "message-two") +} From aa1dc880ba32458c5c28d1407ce5bc415c8ea503 Mon Sep 17 00:00:00 2001 From: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> Date: Wed, 8 Apr 2026 11:00:36 +0200 Subject: [PATCH 6/7] fix: improve error handling and output file management in logging Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com> --- logger/options.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/logger/options.go b/logger/options.go index 8a3ad5e1..7859e801 100644 --- a/logger/options.go +++ b/logger/options.go @@ -122,7 +122,8 @@ func ApplyOptionsToLoggers(options *Options) error { v.SetOutputLevel(daprLogLevel) } - if err := setLogOutput(options.OutputFile, internalLoggers); err != nil { + err := setLogOutput(options.OutputFile, internalLoggers) + if err != nil { return err } @@ -143,13 +144,18 @@ func setLogOutput(path string, loggers map[string]Logger) error { } var out io.Writer = os.Stdout + if path != "" { file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { return fmt.Errorf("failed to open log file %q: %w", path, err) } + logOutputFile = file - out = file + } + + if logOutputFile != nil { + out = logOutputFile } for _, v := range loggers { From 98aedc3aa6ff12aeecd2052d027971660bf56814 Mon Sep 17 00:00:00 2001 From: Nelson Parente Date: Thu, 9 Apr 2026 11:30:54 +0100 Subject: [PATCH 7/7] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20atomic=20file=20swap=20and=20early=20test=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Open new log file before closing the old one so loggers are never left pointing at a closed FD if OpenFile fails - Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent FD leaks on assertion failures Signed-off-by: Nelson Parente --- logger/options.go | 32 ++++++++++++++++++-------------- logger/options_test.go | 24 ++++++++++++------------ 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/logger/options.go b/logger/options.go index 7859e801..a0149c96 100644 --- a/logger/options.go +++ b/logger/options.go @@ -132,35 +132,39 @@ func ApplyOptionsToLoggers(options *Options) error { // setLogOutput configures log output destination. If path is non-empty, logs // are written to the file at that path. If empty, output reverts to stdout. -// Any previously opened log file is closed before opening a new one. +// The new file is opened before closing the previous one so that loggers are +// never left pointing at a closed file descriptor. func setLogOutput(path string, loggers map[string]Logger) error { logOutputMu.Lock() defer logOutputMu.Unlock() - // Close any previously opened log file. - if logOutputFile != nil { - logOutputFile.Close() - logOutputFile = nil - } - - var out io.Writer = os.Stdout + var ( + out io.Writer = os.Stdout + newFile *os.File + ) if path != "" { - file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) + var err error + + newFile, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644) if err != nil { return fmt.Errorf("failed to open log file %q: %w", path, err) } - logOutputFile = file - } - - if logOutputFile != nil { - out = logOutputFile + out = newFile } + // Switch all loggers to the new output before closing the old file. for _, v := range loggers { v.SetOutput(out) } + // Close the previous log file after loggers have been redirected. + if logOutputFile != nil { + logOutputFile.Close() + } + + logOutputFile = newFile + return nil } diff --git a/logger/options_test.go b/logger/options_test.go index ef3823dd..1d4810ff 100644 --- a/logger/options_test.go +++ b/logger/options_test.go @@ -113,12 +113,6 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { l := NewLogger("testLoggerFileOutput") require.NoError(t, ApplyOptionsToLoggers(&testOptions)) - - dl, ok := l.(*daprLogger) - require.True(t, ok) - fileOut, ok := dl.logger.Logger.Out.(*os.File) - require.True(t, ok) - assert.Equal(t, logPath, fileOut.Name()) t.Cleanup(func() { // Revert to stdout, which also closes the log file. require.NoError(t, ApplyOptionsToLoggers(&Options{ @@ -126,6 +120,12 @@ func TestApplyOptionsToLoggersFileOutput(t *testing.T) { })) }) + dl, ok := l.(*daprLogger) + require.True(t, ok) + fileOut, ok := dl.logger.Logger.Out.(*os.File) + require.True(t, ok) + assert.Equal(t, logPath, fileOut.Name()) + msg := "log-file-test-message" l.Info(msg) @@ -141,6 +141,12 @@ func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { l := NewLogger("testLoggerReapply") + t.Cleanup(func() { + require.NoError(t, ApplyOptionsToLoggers(&Options{ + OutputLevel: "info", + })) + }) + // Apply first file output. require.NoError(t, ApplyOptionsToLoggers(&Options{ OutputLevel: "debug", @@ -155,12 +161,6 @@ func TestApplyOptionsToLoggersFileOutputReapply(t *testing.T) { })) l.Info("message-two") - t.Cleanup(func() { - require.NoError(t, ApplyOptionsToLoggers(&Options{ - OutputLevel: "info", - })) - }) - b1, err := os.ReadFile(logPath1) require.NoError(t, err) assert.Contains(t, string(b1), "message-one")