Emit slog attributes from every handler (closes #19)

The handlers took attributes and dropped them on the floor. Handle never
read record.Attrs, so the inline slog.Info("casting", "device", d) form
lost its fields; WithAttrs returned the receiver unchanged, so anything
attached to a derived logger vanished; and WithGroup did the same, so
grouping silently did nothing. The JSON and webhook handlers marshaled
the slog.Record value directly, which cannot work: a record keeps its
attributes in unexported fields, so encoding/json only ever saw Time,
Message, Level and PC.

The consequence was perverse. Converting log.Printf("[%s] Casting %s",
device, file) into structured attributes, as the Go styleguide asks,
left the output with strictly less information than before, and the
calling code reviewed as correct because it was correct.

A small shared attribute layer now holds the accumulated attributes and
open groups. It is copy-on-write, so two loggers derived from one parent
cannot leak attributes into each other, and it qualifies attributes by
the groups open at the time they were attached, per the slog.Handler
contract. Rendering follows each handler's format: the JSON and webhook
handlers emit attributes as object fields with groups as nested objects,
merging a group named twice rather than duplicating its key; the console
handler appends key=value pairs, groups flattened to dotted keys, quoted
only where a bare value would be ambiguous.

Values are resolved through slog.Value.Resolve, so LogValuer values are
reported as the value they stand for instead of as a struct, and errors
are reported as their message rather than as the empty object
encoding/json makes of them. Anything encoding/json cannot marshal falls
back to its slog string form, so an attribute is never silently emptied.

The record's own fields keep the names they have always had - Time,
Level, Message, PC - and win a collision with an attribute key, so
existing consumers of the json output see no change beyond the added
fields.

All three handlers are covered, including WebhookHandler, which had the
same defect and is reached through the same MultiplexHandler.

Two ignored error returns are also checked explicitly, because current
golangci-lint no longer excludes them by default and would otherwise
fail make check on a developer's toolchain.

README.md

@@ -23,8 +23,7 @@ Released v1.0.0 2024-06-14.  Works as intended.  No known bugs.
     `slog.Group` values nest, and `slog.LogValuer` values are resolved. In
     json output attributes are object fields (groups become nested objects);
     in console output they are appended as `key=value` pairs, with grouped
-    keys written as `group.key=value`. See
+    keys written as `group.key=value`
-    [Attribute output](#attribute-output) for the details worth knowing
 
 ## Planned Features
 
@@ -67,40 +66,6 @@ func main() {
 }
 ```
 
-## Attribute output
-
-Attributes reach every handler: the ones passed to the log call, the ones
-accumulated with `WithAttrs`, and the ones qualified by the groups open at
-the time they were attached. `slog.Group` values nest, and
-`slog.LogValuer` values are resolved to the value they stand for. A few
-behaviours are worth knowing before you rely on them.
-
-**Your values are never modified.** Whatever you log is read and rendered,
-never written to. A map or a slice you pass to `slog.Any` comes back from
-the logger exactly as you handed it over, even when a group later uses the
-same key.
-
-**Durations are nanoseconds in json, and readable on the console.** The
-json and webhook payloads emit a `slog.Duration` as a number of
-nanoseconds, matching `slog.NewJSONHandler`, so a consumer can compare and
-aggregate the field without parsing it. The console line emits the same
-duration as `3s`, matching `slog.NewTextHandler`, because a person reads
-it.
-
-**The json payload is an object, with the consequences an object has.**
-The record's own fields are named `Time`, `Level`, `Message` and `PC`, and
-they own those names: an attribute keyed after one of them is dropped from
-the json and webhook output. A key logged more than once keeps its last
-value there for the same reason. Neither applies to the console output,
-which is a line of text: both pairs appear, in order. If you need a field
-called `message`, pick a key that does not collide - the collision is
-silent.
-
-**Empty things follow the `slog.Handler` contract.** An empty `Attr` is
-ignored, an empty group is elided along with its key, a group with an
-empty key is inlined into its parent, and `WithGroup("")` returns the
-handler unchanged.
-
 ## License
 
 [WTFPL](./LICENSE)

attrs.go

@@ -73,24 +73,10 @@ func recordAttrs(record slog.Record) []slog.Attr {
 	return attrs
 }
 
-// groupMap is a JSON object this package built for itself: the payload of a
-// record, or one of the nested objects a slog.Group becomes.
-//
-// The distinct type is what keeps the handler from writing into data the
-// caller still owns. A value handed to slog.Any goes into the payload by
-// reference - copying every logged map and slice would be a real cost for no
-// gain, since rendering only reads. The one place the handler writes into a
-// value already in the payload is the group merge below, and a type assertion
-// to groupMap can only succeed on a map this package allocated: a caller's
-// map[string]any is a different type and never matches, however it was keyed.
-// So the invariant holds by construction - nothing reachable from the caller
-// is ever written to, only read.
-type groupMap map[string]any
-
 // recordToMap renders a record, with its handler's attributes, as the JSON
 // object the JSON and webhook handlers emit. The record's own fields keep the
 // names they have always had, and win a collision with an attribute key.
-func recordToMap(record slog.Record, attrs handlerAttrs) groupMap {
+func recordToMap(record slog.Record, attrs handlerAttrs) map[string]any {
 	fields := attrsToMap(attrs.forRecord(record))
 	fields["Time"] = record.Time
 	fields["Level"] = record.Level
@@ -100,17 +86,16 @@ func recordToMap(record slog.Record, attrs handlerAttrs) groupMap {
 }
 
 // attrsToMap renders attributes as a JSON object in which groups are nested
-// objects. A group named more than once is merged rather than duplicated;
+// objects. A group named more than once is merged rather than duplicated.
-// any other repeated key keeps the last value, as a JSON object must.
+func attrsToMap(attrs []slog.Attr) map[string]any {
-func attrsToMap(attrs []slog.Attr) groupMap {
+	fields := make(map[string]any, len(attrs))
-	fields := make(groupMap, len(attrs))
 	for _, attr := range attrs {
 		addAttrToMap(fields, attr)
 	}
 	return fields
 }
 
-func addAttrToMap(fields groupMap, attr slog.Attr) {
+func addAttrToMap(fields map[string]any, attr slog.Attr) {
 	value := attr.Value.Resolve()
 	if attr.Key == "" && value.Any() == nil {
 		// An empty Attr is ignored, per the slog.Handler contract.
@@ -126,13 +111,9 @@ func addAttrToMap(fields groupMap, attr slog.Attr) {
 		// A group with an empty key is inlined into its parent.
 		target := fields
 		if attr.Key != "" {
-			// Merge only into a group this package built. Anything else
+			nested, ok := fields[attr.Key].(map[string]any)
-			// at this key - including a map the caller logged - is
-			// replaced rather than written into, so the caller's own
-			// data structure is never touched.
-			nested, ok := fields[attr.Key].(groupMap)
 			if !ok {
-				nested = make(groupMap, len(group))
+				nested = make(map[string]any, len(group))
 				fields[attr.Key] = nested
 			}
 			target = nested
@@ -148,11 +129,8 @@ func addAttrToMap(fields groupMap, attr slog.Attr) {
 
 // jsonValue converts a resolved slog.Value into something encoding/json can
 // render usefully. Values it cannot marshal - and errors, which marshal to an
-// empty object - fall back to their slog string form, so a value is never
+// empty object - fall back to their slog string form, so an attribute is never
-// rendered as an empty object.
+// silently emptied.
-//
-// Values are returned as they were given, not copied: nothing here or in its
-// callers writes to a value the caller supplied.
 func jsonValue(value slog.Value) any {
 	switch value.Kind() {
 	case slog.KindString:
@@ -166,11 +144,7 @@ func jsonValue(value slog.Value) any {
 	case slog.KindBool:
 		return value.Bool()
 	case slog.KindDuration:
-		// Nanoseconds as a number, which is what slog.NewJSONHandler
+		return value.Duration().String()
-		// emits. A JSON consumer can then compare and aggregate the
-		// field; the "3s" form would have to be parsed first, and no
-		// common log pipeline knows how.
-		return value.Duration().Nanoseconds()
 	case slog.KindTime:
 		return value.Time()
 	default:
@@ -194,11 +168,6 @@ func jsonAnyValue(value slog.Value) any {
 
 // attrsToText renders attributes as the space separated key=value pairs the
 // console handler appends to a log line. Groups become dotted key prefixes.
-//
-// Values take their slog string form, so a duration reads as "3s" here where
-// the json output carries nanoseconds as a number. That is the same split the
-// stdlib makes between slog.NewTextHandler and slog.NewJSONHandler: the
-// console line is read by a person, the json line by a program.
 func attrsToText(attrs []slog.Attr) string {
 	var out strings.Builder
 	for _, attr := range attrs {

attrs_test.go

@@ -10,7 +10,6 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
-	"reflect"
 	"strings"
 	"testing"
 	"time"
@@ -107,25 +106,6 @@ func wantGroup(t *testing.T, decoded map[string]any, key string) map[string]any
 	return group
 }
 
-// wantNoField asserts that a decoded JSON object does not carry key.
-func wantNoField(t *testing.T, decoded map[string]any, key string) {
-	t.Helper()
-
-	if got, present := decoded[key]; present {
-		t.Fatalf("field %q unexpectedly present as %v: %v", key, got, decoded)
-	}
-}
-
-// wantUnchanged asserts that a value the caller still owns was not written to
-// by the handler. Logging must read what it is handed, never modify it.
-func wantUnchanged(t *testing.T, what string, got, want any) {
-	t.Helper()
-
-	if !reflect.DeepEqual(got, want) {
-		t.Fatalf("handler mutated the caller's %s: got %v, want %v", what, got, want)
-	}
-}
-
 // wantContains asserts that console output contains a fragment.
 func wantContains(t *testing.T, output, fragment string) {
 	t.Helper()
@@ -428,387 +408,3 @@ func TestMultiplexHandlerPassesAttrsThrough(t *testing.T) {
 	wantField(t, decoded, "service", "cattbox")
 	wantField(t, decoded, "device", "livingroom")
 }
-
-// A logging library must read the values it is handed and never write to them.
-// The dangerous shape is a group that shares a key with something the caller
-// logged by reference: merging the group's members into whatever already sits
-// at that key would reach straight back into the caller's own data structure.
-// The next four tests hold every handler to that, through both the record path
-// and the derived-logger path.
-
-func TestJSONHandlerDoesNotMutateCallerMap(t *testing.T) {
-	caller := map[string]any{"mine": "untouched"}
-
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler()
-		record := testRecord(
-			"casting",
-			slog.Any("g", caller),
-			slog.Group("g", slog.Int("injected", 1)),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantUnchanged(t, "map", caller, map[string]any{"mine": "untouched"})
-
-	// The later attribute wins the key outright, as any repeated key does.
-	group := wantGroup(t, decodeLine(t, output), "g")
-	wantField(t, group, "injected", float64(1))
-	wantNoField(t, group, "mine")
-}
-
-func TestJSONHandlerDoesNotMutateCallerMapThroughWithAttrs(t *testing.T) {
-	caller := map[string]any{"id": "req-1"}
-
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler().
-			WithAttrs([]slog.Attr{slog.Any("req", caller)}).
-			WithGroup("req")
-		record := testRecord("cast failed", slog.Int("status", 502))
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantUnchanged(t, "map", caller, map[string]any{"id": "req-1"})
-
-	group := wantGroup(t, decodeLine(t, output), "req")
-	wantField(t, group, "status", float64(502))
-	wantNoField(t, group, "id")
-}
-
-// TestJSONHandlerDoesNotMutateCallerSlice is the slice form of the same
-// hazard: a caller's slice is also handed over by reference, and appending
-// into its spare capacity would be just as visible to the caller as writing
-// into its map. The slice is built with room to spare so that an append within
-// capacity cannot hide behind an unchanged length.
-func TestJSONHandlerDoesNotMutateCallerSlice(t *testing.T) {
-	caller := make([]any, 2, 4)
-	caller[0] = "first"
-	caller[1] = "second"
-
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler()
-		record := testRecord(
-			"casting",
-			slog.Any("s", caller),
-			slog.Group("s", slog.Int("injected", 1)),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantUnchanged(t, "slice", caller, []any{"first", "second"})
-	wantUnchanged(
-		t,
-		"slice backing array",
-		caller[:cap(caller)],
-		[]any{"first", "second", nil, nil},
-	)
-
-	group := wantGroup(t, decodeLine(t, output), "s")
-	wantField(t, group, "injected", float64(1))
-}
-
-func TestConsoleHandlerDoesNotMutateCallerMap(t *testing.T) {
-	caller := map[string]any{"mine": "untouched"}
-
-	output := captureStdout(t, func() {
-		handler := NewConsoleHandler()
-		record := testRecord(
-			"casting",
-			slog.Any("g", caller),
-			slog.Group("g", slog.Int("injected", 1)),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantUnchanged(t, "map", caller, map[string]any{"mine": "untouched"})
-	wantContains(t, output, "g.injected=1")
-}
-
-func TestWebhookHandlerDoesNotMutateCallerMap(t *testing.T) {
-	caller := map[string]any{"id": "req-1"}
-
-	bodies := make(chan []byte, 1)
-	server := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			body, err := io.ReadAll(r.Body)
-			if err != nil {
-				t.Errorf("read webhook body: %v", err)
-			}
-			bodies <- body
-			w.WriteHeader(http.StatusOK)
-		},
-	))
-	defer server.Close()
-
-	handler, err := NewWebhookHandler(server.URL)
-	if err != nil {
-		t.Fatalf("NewWebhookHandler: %v", err)
-	}
-
-	derived := handler.
-		WithAttrs([]slog.Attr{slog.Any("req", caller)}).
-		WithGroup("req")
-	record := testRecord("cast failed", slog.Int("status", 502))
-	if err := derived.Handle(context.Background(), record); err != nil {
-		t.Fatalf("Handle: %v", err)
-	}
-
-	var body []byte
-	select {
-	case body = <-bodies:
-	case <-time.After(5 * time.Second):
-		t.Fatal("webhook handler posted nothing")
-	}
-
-	wantUnchanged(t, "map", caller, map[string]any{"id": "req-1"})
-
-	group := wantGroup(t, decodeLine(t, string(body)), "req")
-	wantField(t, group, "status", float64(502))
-}
-
-// TestJSONHandlerRendersDurationAsNanoseconds pins the json rendering of a
-// duration to a number of nanoseconds, which is what slog.NewJSONHandler
-// emits. A consumer that sums or compares the field needs a number; the "3s"
-// form would silently arrive as a string.
-func TestJSONHandlerRendersDurationAsNanoseconds(t *testing.T) {
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler()
-		record := testRecord("casting", slog.Duration("elapsed", 3*time.Second))
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	decoded := decodeLine(t, output)
-	if _, isNumber := decoded["elapsed"].(float64); !isNumber {
-		t.Fatalf("field \"elapsed\" = %#v, want a json number", decoded["elapsed"])
-	}
-	wantField(t, decoded, "elapsed", float64((3 * time.Second).Nanoseconds()))
-}
-
-// TestConsoleHandlerRendersDurationReadably is the other half of that choice:
-// the console line is read by a person, so it carries the same human-readable
-// form slog.NewTextHandler uses.
-func TestConsoleHandlerRendersDurationReadably(t *testing.T) {
-	output := captureStdout(t, func() {
-		handler := NewConsoleHandler()
-		record := testRecord("casting", slog.Duration("elapsed", 3*time.Second))
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantContains(t, output, "elapsed=3s")
-}
-
-// TestJSONHandlerContractEdgeCases pins the slog.Handler contract's four
-// edge cases. Every case carries a control attribute as well, so a handler
-// that emitted no attributes at all would fail rather than pass by omission.
-//
-// The empty group is attached through WithAttrs rather than to the record,
-// because slog.Record.AddAttrs elides empty groups itself: routed through the
-// record, the case would never reach the handler at all.
-func TestJSONHandlerContractEdgeCases(t *testing.T) {
-	tests := []struct {
-		name   string
-		build  func() slog.Handler
-		attrs  []slog.Attr
-		verify func(t *testing.T, decoded map[string]any)
-	}{
-		{
-			name:  "empty attr is ignored",
-			build: func() slog.Handler { return NewJSONHandler() },
-			attrs: []slog.Attr{{}, slog.String("kept", "yes")},
-			verify: func(t *testing.T, decoded map[string]any) {
-				wantField(t, decoded, "kept", "yes")
-				wantNoField(t, decoded, "")
-			},
-		},
-		{
-			name: "empty group is elided along with its key",
-			build: func() slog.Handler {
-				return NewJSONHandler().
-					WithAttrs([]slog.Attr{slog.Group("empty")})
-			},
-			attrs: []slog.Attr{slog.String("kept", "yes")},
-			verify: func(t *testing.T, decoded map[string]any) {
-				wantField(t, decoded, "kept", "yes")
-				wantNoField(t, decoded, "empty")
-			},
-		},
-		{
-			name:  "group with an empty key is inlined",
-			build: func() slog.Handler { return NewJSONHandler() },
-			attrs: []slog.Attr{
-				slog.Group("", slog.String("inner", "yes")),
-			},
-			verify: func(t *testing.T, decoded map[string]any) {
-				wantField(t, decoded, "inner", "yes")
-				wantNoField(t, decoded, "")
-			},
-		},
-		{
-			name: "WithGroup with an empty name is a no-op",
-			build: func() slog.Handler {
-				return NewJSONHandler().WithGroup("")
-			},
-			attrs: []slog.Attr{slog.String("kept", "yes")},
-			verify: func(t *testing.T, decoded map[string]any) {
-				wantField(t, decoded, "kept", "yes")
-				wantNoField(t, decoded, "")
-			},
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			output := captureStdout(t, func() {
-				handler := test.build()
-				record := testRecord("casting", test.attrs...)
-				if err := handler.Handle(context.Background(), record); err != nil {
-					t.Fatalf("Handle: %v", err)
-				}
-			})
-
-			test.verify(t, decodeLine(t, output))
-		})
-	}
-}
-
-// TestConsoleHandlerContractEdgeCases holds the console handler to the same
-// four rules, since it renders attributes through its own code path.
-func TestConsoleHandlerContractEdgeCases(t *testing.T) {
-	tests := []struct {
-		name    string
-		build   func() slog.Handler
-		attrs   []slog.Attr
-		want    string
-		notWant string
-	}{
-		{
-			name:    "empty attr is ignored",
-			build:   func() slog.Handler { return NewConsoleHandler() },
-			attrs:   []slog.Attr{{}, slog.String("kept", "yes")},
-			want:    "kept=yes",
-			notWant: " =",
-		},
-		{
-			name: "empty group is elided along with its key",
-			build: func() slog.Handler {
-				return NewConsoleHandler().
-					WithAttrs([]slog.Attr{slog.Group("empty")})
-			},
-			attrs:   []slog.Attr{slog.String("kept", "yes")},
-			want:    "kept=yes",
-			notWant: "empty=",
-		},
-		{
-			name:  "group with an empty key is inlined",
-			build: func() slog.Handler { return NewConsoleHandler() },
-			attrs: []slog.Attr{
-				slog.Group("", slog.String("inner", "yes")),
-			},
-			want:    " inner=yes",
-			notWant: ".inner=",
-		},
-		{
-			name: "WithGroup with an empty name is a no-op",
-			build: func() slog.Handler {
-				return NewConsoleHandler().WithGroup("")
-			},
-			attrs:   []slog.Attr{slog.String("kept", "yes")},
-			want:    " kept=yes",
-			notWant: ".kept=",
-		},
-	}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			output := captureStdout(t, func() {
-				handler := test.build()
-				record := testRecord("casting", test.attrs...)
-				if err := handler.Handle(context.Background(), record); err != nil {
-					t.Fatalf("Handle: %v", err)
-				}
-			})
-
-			wantContains(t, output, test.want)
-			wantNotContains(t, output, test.notWant)
-		})
-	}
-}
-
-// TestJSONHandlerRecordFieldsWinKeyCollision pins a caller-visible surprise:
-// the json payload is an object, so the record's own fields own their names
-// and an attribute keyed after one of them is dropped rather than emitted.
-func TestJSONHandlerRecordFieldsWinKeyCollision(t *testing.T) {
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler()
-		record := testRecord(
-			"the real message",
-			slog.String("Time", "hijacked"),
-			slog.String("Level", "hijacked"),
-			slog.String("Message", "hijacked"),
-			slog.String("PC", "hijacked"),
-			slog.String("kept", "yes"),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	decoded := decodeLine(t, output)
-	wantField(t, decoded, "kept", "yes")
-	wantField(t, decoded, "Message", "the real message")
-	wantField(t, decoded, "Level", "INFO")
-	for _, reserved := range []string{"Time", "PC"} {
-		if decoded[reserved] == "hijacked" {
-			t.Fatalf("attribute overwrote the record's own %q field: %v", reserved, decoded)
-		}
-	}
-}
-
-// TestJSONHandlerDuplicateKeysKeepLast pins the other consequence of the
-// payload being an object: a key logged twice collapses to its last value.
-func TestJSONHandlerDuplicateKeysKeepLast(t *testing.T) {
-	output := captureStdout(t, func() {
-		handler := NewJSONHandler()
-		record := testRecord(
-			"casting",
-			slog.String("device", "kitchen"),
-			slog.String("device", "livingroom"),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantField(t, decodeLine(t, output), "device", "livingroom")
-}
-
-// TestConsoleHandlerKeepsDuplicateKeys is the console counterpart: a line of
-// text is not an object, so both pairs survive there.
-func TestConsoleHandlerKeepsDuplicateKeys(t *testing.T) {
-	output := captureStdout(t, func() {
-		handler := NewConsoleHandler()
-		record := testRecord(
-			"casting",
-			slog.String("device", "kitchen"),
-			slog.String("device", "livingroom"),
-		)
-		if err := handler.Handle(context.Background(), record); err != nil {
-			t.Fatalf("Handle: %v", err)
-		}
-	})
-
-	wantContains(t, output, "device=kitchen")
-	wantContains(t, output, "device=livingroom")
-}

json_handler.go

@@ -21,7 +21,7 @@ func (j *JSONHandler) Handle(ctx context.Context, record slog.Record) error {
 	if err != nil {
 		return fmt.Errorf("error marshaling log record: %w", err)
 	}
-	fmt.Fprintln(os.Stdout, string(jsonData))
+	_, _ = fmt.Fprintln(os.Stdout, string(jsonData))
 	return nil
 }
 

webhook_handler.go

@@ -55,6 +55,6 @@ func (w *WebhookHandler) Handle(ctx context.Context, record slog.Record) error {
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer func() { _ = response.Body.Close() }()
 	return nil
 }