From: Jonathan Amsterdam Date: Fri, 14 Apr 2023 10:31:02 +0000 (-0400) Subject: log/slog: remove calls to Value.Resolve from core X-Git-Tag: go1.21rc1~839 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=547e8e22fe565d65d1fd4d6e71436a5a855447b0;p=gostls13.git log/slog: remove calls to Value.Resolve from core Remove calls to Value.Resolve from Record.AddAttrs, Record.Add and Logger.With. Handlers must resolve values themselves; document that in Handler. Call Value.Resolve in the built-in handlers. Updates #59292. Change-Id: I00ba2131be0b16e3b1a22741249fd6f81c3efde1 Reviewed-on: https://go-review.googlesource.com/c/go/+/486375 TryBot-Result: Gopher Robot Run-TryBot: Jonathan Amsterdam Reviewed-by: Alan Donovan --- diff --git a/src/log/slog/handler.go b/src/log/slog/handler.go index d2f919800a..aa76fab514 100644 --- a/src/log/slog/handler.go +++ b/src/log/slog/handler.go @@ -50,6 +50,7 @@ type Handler interface { // Handle methods that produce output should observe the following rules: // - If r.Time is the zero time, ignore the time. // - If r.PC is zero, ignore it. + // - Attr's values should be resolved. // - If an Attr's key and value are both the zero value, ignore the Attr. // This can be tested with attr.Equal(Attr{}). // - If a group's key is empty, inline the group's Attrs. @@ -60,7 +61,6 @@ type Handler interface { // WithAttrs returns a new Handler whose attributes consist of // both the receiver's attributes and the arguments. // The Handler owns the slice: it may retain, modify or discard it. - // [Logger.With] will resolve the Attrs. WithAttrs(attrs []Attr) Handler // WithGroup returns a new Handler with the given group appended to @@ -443,11 +443,11 @@ func (s *handleState) appendAttr(a Attr) { if s.groups != nil { gs = *s.groups } - a = rep(gs, a) - // Although all attributes in the Record are already resolved, - // This one came from the user, so it may not have been. + // Resolve before calling ReplaceAttr, so the user doesn't have to. a.Value = a.Value.Resolve() + a = rep(gs, a) } + a.Value = a.Value.Resolve() // Elide empty Attrs. if a.isEmpty() { return diff --git a/src/log/slog/handler_test.go b/src/log/slog/handler_test.go index 0ddd312645..6be78e0ac1 100644 --- a/src/log/slog/handler_test.go +++ b/src/log/slog/handler_test.go @@ -234,6 +234,16 @@ func TestJSONAndTextHandlers(t *testing.T) { wantText: "msg=message a=1 name.first=Ren name.last=Hoek b=2", wantJSON: `{"msg":"message","a":1,"name":{"first":"Ren","last":"Hoek"},"b":2}`, }, + { + // Test resolution when there is no ReplaceAttr function. + name: "resolve", + attrs: []Attr{ + Any("", &replace{Value{}}), // should be elided + Any("name", logValueName{"Ren", "Hoek"}), + }, + wantText: "time=2000-01-02T03:04:05.000Z level=INFO msg=message name.first=Ren name.last=Hoek", + wantJSON: `{"time":"2000-01-02T03:04:05Z","level":"INFO","msg":"message","name":{"first":"Ren","last":"Hoek"}}`, + }, { name: "with-group", replace: removeKeys(TimeKey, LevelKey), diff --git a/src/log/slog/json_handler.go b/src/log/slog/json_handler.go index ce249acfd3..c965a99152 100644 --- a/src/log/slog/json_handler.go +++ b/src/log/slog/json_handler.go @@ -142,7 +142,7 @@ func appendJSONValue(s *handleState, v Value) error { return appendJSONMarshal(s.buf, a) } default: - panic(fmt.Sprintf("bad kind: %d", v.Kind())) + panic(fmt.Sprintf("bad kind: %s", v.Kind())) } return nil } diff --git a/src/log/slog/logger.go b/src/log/slog/logger.go index 7c31cfc97b..c997dd31dc 100644 --- a/src/log/slog/logger.go +++ b/src/log/slog/logger.go @@ -89,7 +89,7 @@ func (l *Logger) clone() *Logger { func (l *Logger) Handler() Handler { return l.handler } // With returns a new Logger that includes the given arguments, converted to -// Attrs as in [Logger.Log] and resolved. +// Attrs as in [Logger.Log]. // The Attrs will be added to each output from the Logger. // The new Logger shares the old Logger's context. // The new Logger's handler is the result of calling WithAttrs on the receiver's diff --git a/src/log/slog/record.go b/src/log/slog/record.go index 4a5d916119..3cbcccf7c3 100644 --- a/src/log/slog/record.go +++ b/src/log/slog/record.go @@ -87,7 +87,6 @@ func (r Record) NumAttrs() int { // Attrs calls f on each Attr in the Record. // Iteration stops if f returns false. -// The Attrs are already resolved. func (r Record) Attrs(f func(Attr) bool) { for i := 0; i < r.nFront; i++ { if !f(r.front[i]) { @@ -102,9 +101,7 @@ func (r Record) Attrs(f func(Attr) bool) { } // AddAttrs appends the given Attrs to the Record's list of Attrs. -// It resolves the Attrs before doing so. func (r *Record) AddAttrs(attrs ...Attr) { - resolveAttrs(attrs) n := copy(r.front[r.nFront:], attrs) r.nFront += n // Check if a copy was modified by slicing past the end @@ -120,7 +117,6 @@ func (r *Record) AddAttrs(attrs ...Attr) { // Add converts the args to Attrs as described in [Logger.Log], // then appends the Attrs to the Record's list of Attrs. -// It resolves the Attrs before doing so. func (r *Record) Add(args ...any) { var a Attr for len(args) > 0 { @@ -154,7 +150,7 @@ const badKey = "!BADKEY" // argsToAttr turns a prefix of the nonempty args slice into an Attr // and returns the unconsumed portion of the slice. -// If args[0] is an Attr, it returns it, resolved. +// If args[0] is an Attr, it returns it. // If args[0] is a string, it treats the first two elements as // a key-value pair. // Otherwise, it treats args[0] as a value with a missing key. @@ -164,12 +160,9 @@ func argsToAttr(args []any) (Attr, []any) { if len(args) == 1 { return String(badKey, x), nil } - a := Any(x, args[1]) - a.Value = a.Value.Resolve() - return a, args[2:] + return Any(x, args[1]), args[2:] case Attr: - x.Value = x.Value.Resolve() return x, args[1:] default: diff --git a/src/log/slog/value.go b/src/log/slog/value.go index d07d9e33a4..71a59d2639 100644 --- a/src/log/slog/value.go +++ b/src/log/slog/value.go @@ -438,19 +438,12 @@ const maxLogValues = 100 // Resolve repeatedly calls LogValue on v while it implements LogValuer, // and returns the result. -// If v resolves to a group, the group's attributes' values are also resolved. +// If v resolves to a group, the group's attributes' values are not recursively +// resolved. // If the number of LogValue calls exceeds a threshold, a Value containing an // error is returned. // Resolve's return value is guaranteed not to be of Kind KindLogValuer. -func (v Value) Resolve() Value { - v = v.resolve() - if v.Kind() == KindGroup { - resolveAttrs(v.Group()) - } - return v -} - -func (v Value) resolve() (rv Value) { +func (v Value) Resolve() (rv Value) { orig := v defer func() { if r := recover(); r != nil { @@ -491,11 +484,3 @@ func stack(skip, nFrames int) string { } return b.String() } - -// resolveAttrs replaces the values of the given Attrs with their -// resolutions. -func resolveAttrs(as []Attr) { - for i, a := range as { - as[i].Value = a.Value.Resolve() - } -} diff --git a/src/log/slog/value_test.go b/src/log/slog/value_test.go index e0c60c3652..1196e7595b 100644 --- a/src/log/slog/value_test.go +++ b/src/log/slog/value_test.go @@ -175,14 +175,11 @@ func TestLogValue(t *testing.T) { t.Errorf("expected error, got %T", got) } - // Test Resolve group. - r = &replace{GroupValue( - Int("a", 1), - Group("b", Any("c", &replace{StringValue("d")})), - )} - v = AnyValue(r) + // Groups are not recursively resolved. + c := Any("c", &replace{StringValue("d")}) + v = AnyValue(&replace{GroupValue(Int("a", 1), Group("b", c))}) got2 := v.Resolve().Any().([]Attr) - want2 := []Attr{Int("a", 1), Group("b", String("c", "d"))} + want2 := []Attr{Int("a", 1), Group("b", c)} if !attrsEqual(got2, want2) { t.Errorf("got %v, want %v", got2, want2) } @@ -196,7 +193,6 @@ func TestLogValue(t *testing.T) { } // The error should provide some context information. // We'll just check that this function name appears in it. - fmt.Println(got) if got, want := gotErr.Error(), "TestLogValue"; !strings.Contains(got, want) { t.Errorf("got %q, want substring %q", got, want) }