Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
WIP: Fix coverage and make sloghandler slog compliant
Browse files Browse the repository at this point in the history
This is not yet concurrent-safe nor optimized.
thockin committed Dec 6, 2023
1 parent 80c9c67 commit 45bb084
Showing 8 changed files with 412 additions and 348 deletions.
1 change: 1 addition & 0 deletions funcr/slogsink.go
Original file line number Diff line number Diff line change
@@ -56,6 +56,7 @@ func (l fnlogger) WithAttrs(attrs []slog.Attr) logr.SlogSink {
}

func (l fnlogger) WithGroup(name string) logr.SlogSink {
//FIXME: if name == "" it's just inline
l.startGroup(name)
return &l
}
23 changes: 0 additions & 23 deletions logr_noslog_test.go

This file was deleted.

218 changes: 0 additions & 218 deletions logr_slog_test.go

This file was deleted.

79 changes: 0 additions & 79 deletions logr_test.go
Original file line number Diff line number Diff line change
@@ -24,85 +24,6 @@ import (
"testing"
)

// testLogSink is a Logger just for testing that calls optional hooks on each method.
type testLogSink struct {
fnInit func(ri RuntimeInfo)
fnEnabled func(lvl int) bool
fnInfo func(lvl int, msg string, kv ...any)
fnError func(err error, msg string, kv ...any)
fnWithValues func(kv ...any)
fnWithName func(name string)

withValues []any

// testSlogSink contains some additional fields if (and only if) slog is supported by Go.
// See logr_slog_test.go.
//nolint:unused // Only unused with Go < 1.21.
testSlogSink
}

var _ LogSink = &testLogSink{}

func (l *testLogSink) Init(ri RuntimeInfo) {
if l.fnInit != nil {
l.fnInit(ri)
}
}

func (l *testLogSink) Enabled(lvl int) bool {
if l.fnEnabled != nil {
return l.fnEnabled(lvl)
}
return false
}

func (l *testLogSink) Info(lvl int, msg string, kv ...any) {
if l.fnInfo != nil {
l.fnInfo(lvl, msg, kv...)
}
}

func (l *testLogSink) Error(err error, msg string, kv ...any) {
if l.fnError != nil {
l.fnError(err, msg, kv...)
}
}

func (l *testLogSink) WithValues(kv ...any) LogSink {
if l.fnWithValues != nil {
l.fnWithValues(kv...)
}
out := *l
n := len(out.withValues)
out.withValues = append(out.withValues[:n:n], kv...)
return &out
}

func (l *testLogSink) WithName(name string) LogSink {
if l.fnWithName != nil {
l.fnWithName(name)
}
out := *l
return &out
}

type testCallDepthLogSink struct {
testLogSink
callDepth int
fnWithCallDepth func(depth int)
}

var _ CallDepthLogSink = &testCallDepthLogSink{}

func (l *testCallDepthLogSink) WithCallDepth(depth int) LogSink {
if l.fnWithCallDepth != nil {
l.fnWithCallDepth(depth)
}
out := *l
out.callDepth += depth
return &out
}

func TestNew(t *testing.T) {
calledInit := 0

127 changes: 102 additions & 25 deletions sloghandler.go
Original file line number Diff line number Diff line change
@@ -30,9 +30,14 @@ type slogHandler struct {
// Non-nil if sink is non-nil and implements SlogSink.
slogSink SlogSink

// groupPrefix collects values from WithGroup calls. It gets added as
// prefix to value keys when handling a log record.
groupPrefix string
// groupName is the name of the current group.
groupName string

// these keep track of the current set of values, the direct parent of this
// set, and the root of the tree.
currentValues []any
parentValues map[string]any
rootValues map[string]any

// levelBias can be set when constructing the handler to influence the
// slog.Level of log records. A positive levelBias reduces the
@@ -68,13 +73,26 @@ func (l *slogHandler) Handle(ctx context.Context, record slog.Record) error {
// No need to check for nil sink here because Handle will only be called
// when Enabled returned true.

kvList := make([]any, 0, 2*record.NumAttrs())
record.Attrs(func(attr slog.Attr) bool {
if attr.Key != "" {
kvList = append(kvList, l.addGroupPrefix(attr.Key), attr.Value.Resolve().Any())
var kvList []any

// Collect all the values for this group.
if n := len(l.currentValues) + record.NumAttrs(); n > 0 {
kvList = make([]any, 0, 2*n)
kvList = append(kvList, l.currentValues...)
record.Attrs(func(attr slog.Attr) bool {
kvList = attrToKVs(attr, kvList)
return true
})
if l.parentValues != nil {
l.parentValues[l.groupName] = listToMap(kvList)
}
return true
})
}

// If this is under group, start at the root of the group structure.
if l.parentValues != nil {
kvList = mapToList(l.rootValues)
}

if record.Level >= slog.LevelError {
l.sinkWithCallDepth().Error(nil, record.Message, kvList...)
} else {
@@ -84,6 +102,48 @@ func (l *slogHandler) Handle(ctx context.Context, record slog.Record) error {
return nil
}

// attrToKVs appends a slog.Attr to a logr-style kvList. It handle slog Groups
// and other details of slog.
func attrToKVs(attr slog.Attr, kvList []any) []any {
attrVal := attr.Value.Resolve()
if attrVal.Kind() == slog.KindGroup {
groupAttr := attrVal.Group()
grpKVs := make([]any, 0, 2*len(groupAttr))
for _, attr := range groupAttr {
grpKVs = attrToKVs(attr, grpKVs)
}
if attr.Key == "" {
// slog says we have to inline these.
kvList = append(kvList, grpKVs...)
} else {
// Convert the list into a map for rendering.
kvList = append(kvList, attr.Key, listToMap(grpKVs))
}
} else if attr.Key != "" {
kvList = append(kvList, attr.Key, attrVal.Any())
}

return kvList
}

func listToMap(kvList []any) map[string]any {
kvMap := map[string]any{}
for i := 0; i < len(kvList); i += 2 {
k := kvList[i].(string)
v := kvList[i+1]
kvMap[k] = v
}
return kvMap
}

func mapToList(kvMap map[string]any) []any {
kvList := make([]any, 0, 2*len(kvMap))
for k, v := range kvMap {
kvList = append(kvList, k, v)
}
return kvList
}

// sinkWithCallDepth adjusts the stack unwinding so that when Error or Info
// are called by Handle, code in slog gets skipped.
//
@@ -111,37 +171,54 @@ func (l *slogHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
if l.slogSink != nil {
clone.slogSink = l.slogSink.WithAttrs(attrs)
clone.sink = clone.slogSink
} else {
kvList := make([]any, 0, 2*len(attrs))
for _, attr := range attrs {
if attr.Key != "" {
kvList = append(kvList, l.addGroupPrefix(attr.Key), attr.Value.Resolve().Any())
}
}
clone.sink = l.sink.WithValues(kvList...)
return &clone
}

kvList := make([]any, 0, 2*len(attrs))
for _, attr := range attrs {
kvList = attrToKVs(attr, kvList)
}
n := len(clone.currentValues)
clone.currentValues = append(clone.currentValues[:n:n], kvList...) // copy on write

return &clone
}

func (l *slogHandler) WithGroup(name string) slog.Handler {
if l.sink == nil {
return l
}

clone := *l
if l.slogSink != nil {
clone.slogSink = l.slogSink.WithGroup(name)
clone.sink = clone.slogSink
} else {
clone.groupPrefix = clone.addGroupPrefix(name)
return &clone
}

// If the group name is empty, values are inlined.
if name == "" {
return &clone
}
return &clone
}

func (l *slogHandler) addGroupPrefix(name string) string {
if l.groupPrefix == "" {
return name
// The first time we get a group with a non-empty name, we initialize
// the tree of maps. This means that all of the group fields are set or
// unset together.

currentMap := listToMap(clone.currentValues)
if clone.groupName == "" {
// We don't have a root yet, so the current values are it.
clone.rootValues = currentMap
} else {
// The current values become a member of the parent.
//FIXME: shared value, need a deep copy?
clone.parentValues[clone.groupName] = currentMap
}
return l.groupPrefix + groupSeparator + name
clone.parentValues = currentMap
clone.currentValues = nil
clone.groupName = name

return &clone
}

// levelFromSlog adjusts the level by the logger's verbosity and negates it.
80 changes: 77 additions & 3 deletions slogr_test.go
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@ import (
"fmt"
"io"
"log/slog"
"os"
"path"
"runtime"
"strings"
@@ -32,6 +33,58 @@ import (
"github.com/go-logr/logr/internal/testhelp"
)

func TestToSlogHandler(t *testing.T) {
t.Run("from simple Logger", func(t *testing.T) {
logger := New(&testLogSink{})
handler := ToSlogHandler(logger)
if _, ok := handler.(*slogHandler); !ok {
t.Errorf("expected type *slogHandler, got %T", handler)
}
})

t.Run("from slog-enabled Logger", func(t *testing.T) {
logger := New(&testSlogSink{})
handler := ToSlogHandler(logger)
if _, ok := handler.(*slogHandler); !ok {
t.Errorf("expected type *slogHandler, got %T", handler)
}
})

t.Run("from slogSink Logger", func(t *testing.T) {
logger := New(&slogSink{handler: slog.NewJSONHandler(os.Stderr, nil)})
handler := ToSlogHandler(logger)
if _, ok := handler.(*slog.JSONHandler); !ok {
t.Errorf("expected type *slog.JSONHandler, got %T", handler)
}
})
}

func TestFromSlogHandler(t *testing.T) {
t.Run("from slog Handler", func(t *testing.T) {
handler := slog.NewJSONHandler(os.Stderr, nil)
logger := FromSlogHandler(handler)
if _, ok := logger.sink.(*slogSink); !ok {
t.Errorf("expected type *slogSink, got %T", logger.sink)
}
})

t.Run("from simple slogHandler Handler", func(t *testing.T) {
handler := &slogHandler{sink: &testLogSink{}}
logger := FromSlogHandler(handler)
if _, ok := logger.sink.(*testLogSink); !ok {
t.Errorf("expected type *testSlogSink, got %T", logger.sink)
}
})

t.Run("from discard slogHandler Handler", func(t *testing.T) {
handler := &slogHandler{}
logger := FromSlogHandler(handler)
if logger != Discard() {
t.Errorf("expected type *testSlogSink, got %T", logger.sink)
}
})
}

var debugWithoutTime = &slog.HandlerOptions{
ReplaceAttr: func(groups []string, a slog.Attr) slog.Attr {
if a.Key == "time" {
@@ -61,11 +114,32 @@ func TestWithCallDepth(t *testing.T) {
}
}

func TestRunSlogTestsOnSlogSink(t *testing.T) {
// This proves that slogSink passes slog's own tests.
func TestRunSlogTestsOnSlogHandlerLogSink(t *testing.T) {
// This proves that slogHandler passes slog's own tests when given a
// non-SlogSink LogSink.
exceptions := []string{
"a Handler should ignore a zero Record.Time",
}
testhelp.RunSlogTests(t, func(buffer *bytes.Buffer) slog.Handler {
// We want a known-good Logger that emits JSON but is not a slogHandler
// or SlogSink (since those get speical treatment). We can trust that
// the slog JSONHandler works.
handler := slog.NewJSONHandler(buffer, nil)
logger := FromSlogHandler(handler)
sink := &passthruLogSink{handler: handler}
logger := New(sink)
return ToSlogHandler(logger)
}, exceptions...)
}

func TestRunSlogTestsOnSlogHandlerSlogSink(t *testing.T) {
// This proves that slogHandler passes slog's own tests when given a
// SlogSink.
testhelp.RunSlogTests(t, func(buffer *bytes.Buffer) slog.Handler {
// We want a known-good Logger that emits JSON and implements SlogSink,
// to cover those paths. We can trust that the slog JSONHandler works.
handler := slog.NewJSONHandler(buffer, nil)
sink := &passthruSlogSink{handler: handler}
logger := New(sink)
return ToSlogHandler(logger)
})
}
140 changes: 140 additions & 0 deletions testimpls_slog_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
//go:build go1.21
// +build go1.21

/*
Copyright 2023 The logr Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logr

import (
"context"
"log/slog"
"time"
)

var _ SlogSink = &testSlogSink{}

// testSlogSink is a trivial SlogSink implementation, just for testing, which
// calls (optional) hooks on each method.
type testSlogSink struct {
// embed a plain LogSink
testLogSink

attrs []slog.Attr
groups []string

fnHandle func(ss *testSlogSink, ctx context.Context, record slog.Record)
fnWithAttrs func(ss *testSlogSink, attrs []slog.Attr)
fnWithGroup func(ss *testSlogSink, name string)
}

func (ss *testSlogSink) Handle(ctx context.Context, record slog.Record) error {
if ss.fnHandle != nil {
ss.fnHandle(ss, ctx, record)
}
return nil
}

func (ss *testSlogSink) WithAttrs(attrs []slog.Attr) SlogSink {
if ss.fnWithAttrs != nil {
ss.fnWithAttrs(ss, attrs)
}
out := *ss
n := len(out.attrs)
out.attrs = append(out.attrs[:n:n], attrs...)
return &out
}

func (ss *testSlogSink) WithGroup(name string) SlogSink {
if ss.fnWithGroup != nil {
ss.fnWithGroup(ss, name)
}
out := *ss
n := len(out.groups)
out.groups = append(out.groups[:n:n], name)
return &out
}

// passthruLogSink is a trivial LogSink implementation, which implements the
// logr.LogSink methods in terms of a slog.Handler.
type passthruLogSink struct {
handler slog.Handler
}

func (pl passthruLogSink) Init(RuntimeInfo) {}

func (pl passthruLogSink) Enabled(int) bool { return true }

func (pl passthruLogSink) Error(_ error, msg string, kvList ...interface{}) {
var record slog.Record
record.Message = msg
record.Level = slog.LevelError
record.Time = time.Now()
record.Add(kvList...)
pl.handler.Handle(context.Background(), record)
}

func (pl passthruLogSink) Info(_ int, msg string, kvList ...interface{}) {
var record slog.Record
record.Message = msg
record.Level = slog.LevelInfo
record.Time = time.Now()
record.Add(kvList...)
pl.handler.Handle(context.Background(), record)
}

func (pl passthruLogSink) WithName(string) LogSink { return &pl }

func (pl passthruLogSink) WithValues(kvList ...interface{}) LogSink {
var values slog.Record
values.Add(kvList...)
var attrs []slog.Attr
add := func(attr slog.Attr) bool {
attrs = append(attrs, attr)
return true
}
values.Attrs(add)

pl.handler = pl.handler.WithAttrs(attrs)
return &pl
}

// passthruSlogSink is a trivial SlogSink implementation, which stubs out the
// logr.LogSink methods and passes Logr.SlogSink thru to a slog.Handler.
type passthruSlogSink struct {
handler slog.Handler
}

func (ps passthruSlogSink) Init(RuntimeInfo) {}
func (ps passthruSlogSink) Enabled(int) bool { return true }
func (ps passthruSlogSink) Error(error, string, ...interface{}) {}
func (ps passthruSlogSink) Info(int, string, ...interface{}) {}
func (ps passthruSlogSink) WithName(string) LogSink { return &ps }
func (ps passthruSlogSink) WithValues(kvList ...interface{}) LogSink { return &ps }

func (ps *passthruSlogSink) Handle(ctx context.Context, record slog.Record) error {
return ps.handler.Handle(ctx, record)
}

func (ps passthruSlogSink) WithAttrs(attrs []slog.Attr) SlogSink {
ps.handler = ps.handler.WithAttrs(attrs)
return &ps
}

func (ps passthruSlogSink) WithGroup(name string) SlogSink {
ps.handler = ps.handler.WithGroup(name)
return &ps
}
92 changes: 92 additions & 0 deletions testimpls_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
Copyright 2021 The logr Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logr

// testLogSink is a trivial LogSink implementation, just for testing, which
// calls (optional) hooks on each method.
type testLogSink struct {
fnInit func(ri RuntimeInfo)
fnEnabled func(lvl int) bool
fnInfo func(lvl int, msg string, kv ...any)
fnError func(err error, msg string, kv ...any)
fnWithValues func(kv ...any)
fnWithName func(name string)

withValues []any
}

var _ LogSink = &testLogSink{}

func (ls *testLogSink) Init(ri RuntimeInfo) {
if ls.fnInit != nil {
ls.fnInit(ri)
}
}

func (ls *testLogSink) Enabled(lvl int) bool {
if ls.fnEnabled != nil {
return ls.fnEnabled(lvl)
}
return false
}

func (ls *testLogSink) Info(lvl int, msg string, kv ...any) {
if ls.fnInfo != nil {
ls.fnInfo(lvl, msg, kv...)
}
}

func (ls *testLogSink) Error(err error, msg string, kv ...any) {
if ls.fnError != nil {
ls.fnError(err, msg, kv...)
}
}

func (ls *testLogSink) WithValues(kv ...any) LogSink {
if ls.fnWithValues != nil {
ls.fnWithValues(kv...)
}
out := *ls
n := len(out.withValues)
out.withValues = append(out.withValues[:n:n], kv...)
return &out
}

func (ls *testLogSink) WithName(name string) LogSink {
if ls.fnWithName != nil {
ls.fnWithName(name)
}
out := *ls
return &out
}

type testCallDepthLogSink struct {
testLogSink
callDepth int
fnWithCallDepth func(depth int)
}

var _ CallDepthLogSink = &testCallDepthLogSink{}

func (ls *testCallDepthLogSink) WithCallDepth(depth int) LogSink {
if ls.fnWithCallDepth != nil {
ls.fnWithCallDepth(depth)
}
out := *ls
out.callDepth += depth
return &out
}

0 comments on commit 45bb084

Please sign in to comment.