pr feedback

internal/alloy/logging/deferred_handler.go

@@ -12,7 +12,6 @@ type deferredSlogHandler struct {
 	group    string
 	attrs    []slog.Attr
 	children []*deferredSlogHandler
-	parent   *deferredSlogHandler
 	handle   slog.Handler
 	l        *Logger
 }
@@ -24,25 +23,25 @@ func newDeferredHandler(l *Logger) *deferredSlogHandler {
 	}
 }
 
-func (d *deferredSlogHandler) Handle(_ context.Context, r slog.Record) error {
+func (d *deferredSlogHandler) Handle(ctx context.Context, r slog.Record) error {
 	d.mut.RLock()
 	defer d.mut.RUnlock()
 
 	if d.handle != nil {
-		return d.handle.Handle(context.Background(), r)
+		return d.handle.Handle(ctx, r)
 	}
 	d.l.addRecord(r, d)
 	return nil
 }
 
 // Enabled reports whether the handler handles records at the given level.
 // The handler ignores records whose level is lower.
-func (d *deferredSlogHandler) Enabled(_ context.Context, level slog.Level) bool {
+func (d *deferredSlogHandler) Enabled(ctx context.Context, level slog.Level) bool {
 	d.mut.RLock()
 	defer d.mut.RUnlock()
 
 	if d.handle != nil {
-		return d.handle.Enabled(context.Background(), level)
+		return d.handle.Enabled(ctx, level)
 	}
 	return true
 }
@@ -61,7 +60,6 @@ func (d *deferredSlogHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
 		attrs:    attrs,
 		children: make([]*deferredSlogHandler, 0),
 		l:        d.l,
-		parent:   d,
 	}
 	d.children = append(d.children, child)
 	return child
@@ -79,7 +77,6 @@ func (d *deferredSlogHandler) WithGroup(name string) slog.Handler {
 		children: make([]*deferredSlogHandler, 0),
 		group:    name,
 		l:        d.l,
-		parent:   d,
 	}
 	d.children = append(d.children, child)
 	return child
@@ -104,4 +101,5 @@ func (d *deferredSlogHandler) buildHandlers(parent slog.Handler) {
 	for _, child := range d.children {
 		child.buildHandlers(d.handle)
 	}
+	d.children = nil
 }

internal/alloy/logging/deferred_handler_test.go

@@ -9,29 +9,48 @@ import (
 	"log/slog"
 	"strings"
 	"testing"
+	"testing/slogtest"
 )
 
 func TestDefferredSlogTester(t *testing.T) {
 	buf := &bytes.Buffer{}
+	var l *Logger
 	results := func(t *testing.T) map[string]any {
+		// Nothing has been written to the byte stream, it only exists in the internal logger buffer
+		// We need to call l.Update to flush it to the byte stream.
+		// This does something a bit ugly where it DEPENDS on the var in slogtest.Run, if the behavior of slogtest.Run
+		// changes this may break the tests.
+		updateErr := l.Update(Options{
+			Level:   "debug",
+			Format:  "json",
+			WriteTo: nil,
+		})
+		require.NoError(t, updateErr)
 		line := buf.Bytes()
 		if len(line) == 0 {
 			return nil
 		}
 		var m map[string]any
 		unmarshalErr := json.Unmarshal(line, &m)
 		require.NoError(t, unmarshalErr)
-		// Need to reset the buffer between each test
+		// The tests expect time field and not ts.
+		if _, found := m["ts"]; found {
+			m[slog.TimeKey] = m["ts"]
+			delete(m, "ts")
+		}
+		// Need to reset the buffer and logger between each test.
+		l = nil
 		buf.Reset()
 		return m
 	}
 
 	// Had to add some custom logic to handle updated for the deferred tests.
 	// Also ignore anything that modifies the log line, which are two tests.
-	RunDeferredTests(t, func(t *testing.T) *Logger {
-		l, err := NewDeferred(buf)
+	slogtest.Run(t, func(t *testing.T) slog.Handler {
+		var err error
+		l, err = NewDeferred(buf)
 		require.NoError(t, err)
-		return l
+		return l.Handler()
 	}, results)
 }
 

internal/alloy/logging/handler.go

@@ -107,7 +107,7 @@ func (h *handler) buildHandler() slog.Handler {
 }
 
 func (h *handler) WithAttrs(attrs []slog.Attr) slog.Handler {
-	newNest := make([]nesting, len(h.nested)+1)
+	newNest := make([]nesting, 0, len(h.nested)+1)
 	newNest = append(newNest, h.nested...)
 	newNest = append(newNest, nesting{
 		attrs: attrs,
@@ -124,7 +124,7 @@ func (h *handler) WithAttrs(attrs []slog.Attr) slog.Handler {
 }
 
 func (h *handler) WithGroup(name string) slog.Handler {
-	newNest := make([]nesting, len(h.nested)+1)
+	newNest := make([]nesting, 0, len(h.nested)+1)
 	newNest = append(newNest, h.nested...)
 	newNest = append(newNest, nesting{
 		group: name,
@@ -194,24 +194,3 @@ func replace(groups []string, a slog.Attr) slog.Attr {
 
 	return a
 }
-
-// testReplace is used for unit tests so we can ensure the time and source fields are consistent.
-func testReplace(groups []string, a slog.Attr) slog.Attr {
-	ra := replace(groups, a)
-	switch a.Key {
-	case "ts":
-		fallthrough
-	case "time":
-		return slog.Attr{
-			Key:   "ts",
-			Value: slog.StringValue("2024-04-29T18:26:21.37723798Z"),
-		}
-	case "source":
-		return slog.Attr{
-			Key:   "source",
-			Value: slog.StringValue("test_source"),
-		}
-	default:
-		return ra
-	}
-}

internal/alloy/logging/handler_test.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"log/slog"
 	"testing"
+	"testing/slogtest"
 	"time"
 
 	"github.com/stretchr/testify/require"
@@ -60,11 +61,16 @@ func TestSlogTester(t *testing.T) {
 			var m map[string]any
 			unmarshalErr := json.Unmarshal(line, &m)
 			require.NoError(t, unmarshalErr)
+			// The tests expect time field and not ts.
+			if _, found := m["ts"]; found {
+				m[slog.TimeKey] = m["ts"]
+				delete(m, "ts")
+			}
 			ms = append(ms, m)
 		}
 		return ms
 	}
-	err = TestHandler(l.handler, results)
+	err = slogtest.TestHandler(l.handler, results)
 	require.NoError(t, err)
 }
 
@@ -83,3 +89,36 @@ func getTestHandler(t *testing.T, w io.Writer) slog.Handler {
 
 	return l.handler
 }
+
+// testReplace is used for unit tests so we can ensure the time and source fields are consistent.
+func testReplace(groups []string, a slog.Attr) slog.Attr {
+	ra := replace(groups, a)
+	switch a.Key {
+	case "ts":
+		fallthrough
+	case "time":
+		return slog.Attr{
+			Key:   "ts",
+			Value: slog.StringValue("2024-04-29T18:26:21.37723798Z"),
+		}
+	case "source":
+		return slog.Attr{
+			Key:   "source",
+			Value: slog.StringValue("test_source"),
+		}
+	default:
+		return ra
+	}
+}
+
+// newDeferredTest creates a new logger with the default log level and format. Used for tests.
+// The logger is not updated during initialization.
+func newDeferredTest(w io.Writer) (*Logger, error) {
+	l, err := NewDeferred(w)
+	if err != nil {
+		return nil, err
+	}
+	l.handler.replacer = testReplace
+
+	return l, nil
+}

internal/alloy/logging/logger.go

@@ -43,33 +43,11 @@ func (l *Logger) Enabled(ctx context.Context, level slog.Level) bool {
 
 // New creates a New logger with the default log level and format.
 func New(w io.Writer, o Options) (*Logger, error) {
-	var (
-		leveler slog.LevelVar
-		format  formatVar
-		writer  writerVar
-	)
-
-	l := &Logger{
-		inner: w,
-
-		buffer:       make([]*bufferedItem, 0),
-		hasLogFormat: false,
-
-		level:  &leveler,
-		format: &format,
-		writer: &writer,
-		handler: &handler{
-			w:         &writer,
-			leveler:   &leveler,
-			formatter: &format,
-			replacer:  replace,
-		},
+	l, err := NewDeferred(w)
+	if err != nil {
+		return nil, err
 	}
-	// We always need to make a deferred handler since that is what is always exposed,
-	// that being said the Update command will immediately set its interior handler.
-	l.deferredSlog = newDeferredHandler(l)
-
-	if err := l.Update(o); err != nil {
+	if err = l.Update(o); err != nil {
 		return nil, err
 	}
 
@@ -106,18 +84,6 @@ func NewDeferred(w io.Writer) (*Logger, error) {
 	return l, nil
 }
 
-// newDeferredTest creates a new logger with the default log level and format. Used for tests.
-// The logger is not updated during initialization.
-func newDeferredTest(w io.Writer) (*Logger, error) {
-	l, err := NewDeferred(w)
-	if err != nil {
-		return nil, err
-	}
-	l.handler.replacer = testReplace
-
-	return l, nil
-}
-
 // Handler returns a [slog.Handler]. The returned Handler remains valid if l is
 // updated.
 func (l *Logger) Handler() slog.Handler { return l.deferredSlog }
@@ -155,7 +121,7 @@ func (l *Logger) Update(o Options) error {
 			slogadapter.GoKit(l.handler).Log(bufferedLogChunk.kvps...)
 		} else {
 			// We now can check to see if if our buffered log is at the right level.
-			if l.level.Level() <= bufferedLogChunk.record.Level {
+			if bufferedLogChunk.handler.handle.Enabled(context.Background(), bufferedLogChunk.record.Level) {
 				// These will always be valid due to the build handlers call above.
 				bufferedLogChunk.handler.handle.Handle(context.Background(), bufferedLogChunk.record)
 			}