Commit d50b07f9 authored by Elliot Forbes's avatar Elliot Forbes 2️⃣
Browse files

feat: Adds a NewWithFile constructor function for specifying log file output 

When working through the gitlab-shell migration, I identified the
need to write log messages to a log file. The code to do this can
be a little clunky, and it's easy to mess up the combination of flags
required to be successful in all cases.

An alternative option explored was to offer a ToFile method, but this
solution didn't provide an elegant way to close the file on application
shutdown. As a result, we've changed direction to go for NewWithFile
that returns the closer. It's a little clunkier, but I'd prefer this
over the potential for incidents.

Whilst digging in further, by default o.sFile will be finalized by the GC
when the application exits according to the SetFinalizer documentation.

https://pkg.go.dev/runtime#SetFinalizer

However, this feels like a recipe for some weird edge cases that will
be forgotten about at some point in the future. As a result, I've
opted to change the API for a slightly more verbose option that
returns the io.Closer which the application can then decide to close
when it's terminating.
parent 3618676e

v2/log/README.md

+1 −0
Original line number Diff line number Diff line
@@ -125,3 +125,4 @@ ctx = log.WithFields(ctx, slog.String("step", "1")) 
// context up until this point.

logger.InfoContext(ctx, "canonical_log")

```

v2/log/log.go

+35 −3
Original line number Diff line number Diff line 
package log



import (

	"fmt"

	"io"

	"log/slog"

	"os"
@@ -13,9 +14,24 @@ type TimeFunc func() time.Time 


// Config holds the configuration for creating a new logger.

type Config struct {

	// Writer - a lower level construct that allows

	// engineers to have finer-grained control over

	// how and where logs are written to file.

	Writer io.Writer



	// UseTextFormat - set this to true if you require

	// text formatted logs.

	UseTextFormat bool



	// Clock - allows the consumer to provide their own

	// TimeFunc that is used to provide the timestamp when

	// emitting logs.

	// The logger defaults to UTC to try and ensure

	// consistency across our services.

	Clock TimeFunc



	// LogLevel - represents the minimum log level that

	// should be output by the logger.

	LogLevel slog.Level

}
@@ -73,6 +89,22 @@ func NewWithConfig(cfg *Config) *slog.Logger { 
	return slog.New(NewContextHandler(baseHandler))

}



// NewWithFile creates a logger writing to a file.

// Caller is responsible for closing the returned file.

func NewWithFile(filePath string, cfg *Config) (*slog.Logger, io.Closer, error) {

	logFile, err := os.OpenFile(filePath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600)

	if err != nil {

		return NewWithConfig(cfg), nil, fmt.Errorf("failed to open log file: %w", err)

	}



	if cfg == nil {

		cfg = &Config{}

	}

	cfg.Writer = logFile



	return NewWithConfig(cfg), logFile, nil

}



func defaultHandlerOpts(cfg *Config) *slog.HandlerOptions {

	opts := &slog.HandlerOptions{

		Level: cfg.LogLevel,

v2/log/log_test.go

+98 −0
Original line number Diff line number Diff line
@@ -186,3 +186,101 @@ func readTextFile(t *testing.T, filename string) string { 
	assert.Nil(t, err)

	return string(data)

}



func TestNewWithFile(t *testing.T) {

	t.Run("ToFile returns a writer that writes to the specified file", func(t *testing.T) {

		filePath := t.TempDir() + "/test.log"

		logger, closer, err := NewWithFile(filePath, nil)

		assert.Nil(t, err)

		//nolint

		defer closer.Close()



		testMsg := "hello from ToFile"

		logger.Info(testMsg)



		content := readTextFile(t, filePath)

		assert.Contains(t, content, testMsg)

	})



	t.Run("ToFile creates a new file if it doesn't exist", func(t *testing.T) {

		filePath := t.TempDir() + "/new_file.log"

		logger, closer, err := NewWithFile(filePath, nil)

		assert.Nil(t, err)

		//nolint

		defer closer.Close()



		testMsg := "creating new file"

		logger.Info(testMsg)



		_, err = os.Stat(filePath)

		assert.NoError(t, err, "file should exist")



		content := readTextFile(t, filePath)

		assert.Contains(t, content, testMsg)

	})



	t.Run("falls back to Stderr successfully", func(t *testing.T) {

		// Capture stderr output

		r, w, err := os.Pipe()

		require.NoError(t, err)

		oldStderr := os.Stderr

		os.Stderr = w



		// Call ToFile with an invalid path to trigger fallback

		invalidPath := "/nonexistent/path/to/log.log"

		logger, _, err := NewWithFile(invalidPath, nil)

		assert.Error(t, err)



		testMsg := "error logging to file, falling back to stderr"

		logger.Error(testMsg)



		// Close the pipe writer and restore stderr

		w.Close()

		os.Stderr = oldStderr



		// Read the captured stderr output

		out, err := io.ReadAll(r)

		require.NoError(t, err)



		assert.Contains(t, string(out), testMsg)

		assert.Contains(t, string(out), "falling back to stderr")

	})



	t.Run("ToFile appends to an existing file", func(t *testing.T) {

		filePath := t.TempDir() + "/existing.log"



		// Write initial content

		initialContent := "initial log line\n"

		err := os.WriteFile(filePath, []byte(initialContent), 0600)

		require.NoError(t, err)



		logger, closer, err := NewWithFile(filePath, nil)

		assert.Nil(t, err)

		//nolint

		defer closer.Close()



		appendedMsg := "appended log line"

		logger.Info(appendedMsg)



		content := readTextFile(t, filePath)

		assert.Contains(t, content, initialContent[:len(initialContent)-1]) // Remove newline for comparison

		assert.Contains(t, content, appendedMsg)

		assert.True(t, len(content) > len(initialContent), "content should be longer after appending")

	})



	t.Run("test LogFile config", func(t *testing.T) {

		filePath := t.TempDir() + "/config_file.log"

		logger, closer, err := NewWithFile(filePath, nil)

		assert.Nil(t, err)

		//nolint

		defer closer.Close()

		testMsg := "logging via config file path"

		logger.Info(testMsg)



		t.Log(filePath)

		content := readTextFile(t, filePath)

		assert.Contains(t, content, testMsg)

		// Clean up the created file

		os.Remove(filePath)

	})

}