Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: add slogr #195

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions slogr/handler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
//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 slogr

import (
"context"
"log/slog"

"github.com/go-logr/logr"
)

// NewSlogHandler returns a slog.Handler which logs through an arbitrary
// logr.Logger.
func NewSlogHandler(logger logr.Logger) slog.Handler {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should test this with the testing/slogtest package to make sure you get all the subtle rules right.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...but that may not work if you punt on features like groups.

FTR I'm thrilled that there will be some level of interoperability and I'm fine if it's not perfect. If giving up on groups means that a PR will actually land, then that works for me. In that case just use testing/slogtest informally to find the bugs that are worthwhile fixing. Maybe have a permanent t.Skip in the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See golang/go#61758 (comment) for a comment about allowing to skip some of the testing/slogtest checks.

return &logrHandler{
// Account for extra wrapper functions.
logger: logger.WithCallDepth(3),
}
}

// logrHandler implements slog.Handler in terms of a logr.Logger.
type logrHandler struct {
logger logr.Logger
}

func (h logrHandler) Enabled(_ context.Context, level slog.Level) bool {
return h.logger.V(int(-level)).Enabled()
}

func (h logrHandler) Handle(_ context.Context, record slog.Record) error {
//FIXME: I don't know what to do with record.Time or record.PC. Neither of
// them map to logr.
args := recordToKV(record)
if record.Level >= slog.LevelError {
h.logger.Error(nil, record.Message, args...)
} else {
h.logger.V(int(-record.Level)).Info(record.Message, args...)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider dividing by 4, so:

Info -> V(0)
Debug -> V(1)
Warn -> V(-1)

I don't know if that's better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A direct mapping is better because Kubernetes uses V(4) for debug messages, which corresponds nicely with slog.LevelDebug = -4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - I don't see why we need to modify it - am I missing some trap?

}
return nil
}

func recordToKV(record slog.Record) []any {
kv := make([]any, 0, record.NumAttrs()*2)
fn := func(attr slog.Attr) bool {
kv = append(kv, attr.Key, attr.Value.Any())
return true
}
record.Attrs(fn)
return kv
}

func attrsToKV(attrs []slog.Attr) []any {
kv := make([]any, 0, len(attrs)*2)
for _, attr := range attrs {
kv = append(kv, attr.Key, attr.Value.Any())
}
return kv
}

func (h logrHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
args := attrsToKV(attrs)
h.logger = h.logger.WithValues(args...)
return h
}

func (h logrHandler) WithGroup(name string) slog.Handler {
//FIXME: I don't know how to implement this in logr, but it's an
//interesting idea
return h
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "flat" formats like the one implemented by slog.TextHandler, I recommend prefixing the key with GROUP. You can store a string in the handler that is the dot-separated concatenation of all the groups you see here, then slap that on to each attribute you encounter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@thockin thockin Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunate. It seems like it would be nicer to aggregate and produse one struct, but then I have to accumulate Attrs in calls to WithAttrs, and then send those plus the per-logline Attrs at the same time, which probably defeats pre-formatiing optimizations in handlers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that prefixing keys is sub-optimal. It's acceptable when the underlying logr.LogSink would do something like that itself (klog text format), but not when it writes JSON (zapr). I did not put further effort into this because I prefer to avoid glue code like this entirely by using backends which support both slog and logr and only falling back to glue code when such support is missing.

The alternative is a major update of logr which brings it to feature parity with slog - that doesn't seem worthwhile.

}

var _ slog.Handler = logrHandler{}
65 changes: 65 additions & 0 deletions slogr/handler_example/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
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 main is an example of using funcr.
package main

import (
"context"
"fmt"
"log/slog"

"github.com/go-logr/logr/funcr"
"github.com/go-logr/logr/slogr"
)

type e struct {
str string
}

func (e e) Error() string {
return e.str
}

func helper(log *slog.Logger, msg string) {
helper2(log, msg)
}

func helper2(log *slog.Logger, msg string) {
log.Info(msg)
}

func main() {
logrLogger := funcr.New(
func(pfx, args string) { fmt.Println(pfx, args) },
funcr.Options{
LogCaller: funcr.All,
LogTimestamp: true,
Verbosity: 1,
})
log := slog.New(slogr.NewSlogHandler(logrLogger))
example(log)
}

func example(log *slog.Logger) {
log = log.With("saved", "value")
log.Info("1) hello", "val1", 1, "val2", map[string]int{"k": 1})
log.Log(context.TODO(), slog.Level(-1), "2) you should see this")
log.Log(context.TODO(), slog.Level(-2), "you should NOT see this")
log.Error("3) uh oh", "trouble", true, "reasons", []float64{0.1, 0.11, 3.14})
log.Error("4) goodbye", "code", -1, "err", e{"an error occurred"})
helper(log, "5) thru a helper")
}
65 changes: 65 additions & 0 deletions slogr/logr_example/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
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 main is an example of using funcr.
package main

import (
"log/slog"
"os"

"github.com/go-logr/logr"
"github.com/go-logr/logr/slogr"
)

type e struct {
str string
}

func (e e) Error() string {
return e.str
}

func helper(log logr.Logger, msg string) {
helper2(log, msg)
}

func helper2(log logr.Logger, msg string) {
log.WithCallDepth(2).Info(msg)
}

func main() {
opts := slog.HandlerOptions{
AddSource: true,
Level: slog.Level(-1),
}
handler := slog.NewJSONHandler(os.Stderr, &opts)
log := slogr.New(handler)
example(log)
}

func example(log logr.Logger) {
log = log.WithName("my")
log = log.WithName("logger")
log = log.WithName("name")
log = log.WithValues("saved", "value")
log.Info("1) hello", "val1", 1, "val2", map[string]int{"k": 1})
log.V(1).Info("2) you should see this")
log.V(1).V(1).Info("you should NOT see this")
log.Error(nil, "3) uh oh", "trouble", true, "reasons", []float64{0.1, 0.11, 3.14})
log.Error(e{"an error occurred"}, "4) goodbye", "code", -1)
helper(log, "5) thru a helper")
}
139 changes: 139 additions & 0 deletions slogr/slogr.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
//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 slogr provides bridges between github.com/go-logr/logr and Go's
// slog package.
package slogr

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

"github.com/go-logr/logr"
)

// New returns a logr.Logger which logs through an arbitrary slog.Handler.
func New(handler slog.Handler) logr.Logger {
return logr.New(newSink(handler))
}

// Underlier exposes access to the underlying logging function. Since
// callers only have a logr.Logger, they have to know which
// implementation is in use, so this interface is less of an
// abstraction and more of a way to test type conversion.
type Underlier interface {
GetUnderlying() slog.Handler
}

func newSink(handler slog.Handler) logr.LogSink {
sink := &slogSink{
handler: handler,
}
// For skipping logr.Logger.Info and .Error.
return sink.WithCallDepth(1)
}

// slogSink inherits some of its LogSink implementation from Formatter
// and just needs to add some glue code.
type slogSink struct {
handler slog.Handler
name string
depth int
}

// Init configures this Formatter from runtime info, such as the call depth
// imposed by logr itself.
// Note that this receiver is a pointer, so depth can be saved.
func (sink *slogSink) Init(info logr.RuntimeInfo) {
sink.depth += info.CallDepth
}

// Enabled checks whether an info message at the given level should be logged.
func (sink slogSink) Enabled(level int) bool {
return sink.handler.Enabled(context.Background(), slog.Level(-level))
}

func (sink slogSink) WithName(name string) logr.LogSink {
if len(sink.name) > 0 {
sink.name += "/"
}
sink.name += name
return &sink
}

func (sink slogSink) WithValues(kvList ...interface{}) logr.LogSink {
r := slog.NewRecord(time.Time{}, 0, "", 0)
r.Add(kvList...)
attrs := make([]slog.Attr, 0, r.NumAttrs())
r.Attrs(func(attr slog.Attr) bool {
attrs = append(attrs, attr)
return true
})
sink.handler = sink.handler.WithAttrs(attrs)
return &sink
}

func (sink slogSink) WithCallDepth(depth int) logr.LogSink {
sink.depth += depth
return &sink
}

func (sink slogSink) Info(level int, msg string, kvList ...interface{}) {
args := make([]interface{}, 0, len(kvList)+4)
if len(sink.name) != 0 {
args = append(args, "logger", sink.name)
}
args = append(args, "vlevel", level)
args = append(args, kvList...)

sink.log(slog.Level(-level), msg, args...)
}

func (sink slogSink) Error(err error, msg string, kvList ...interface{}) {
args := make([]interface{}, 0, len(kvList)+4)
if len(sink.name) != 0 {
args = append(args, "logger", sink.name)
}
args = append(args, "err", err)
args = append(args, kvList...)

sink.log(slog.LevelError, msg, args...)
}

func (sink slogSink) log(level slog.Level, msg string, kvList ...interface{}) {
// TODO: Should this be optional via HandlerOptions? Literally
// HandlerOptions or something like it?
var pcs [1]uintptr
runtime.Callers(sink.depth+2, pcs[:]) // 2 = this frame and Info/Error
pc := pcs[0]
r := slog.NewRecord(time.Now(), level, msg, pc)
r.Add(kvList...)
sink.handler.Handle(context.Background(), r)
}

func (sink slogSink) GetUnderlying() slog.Handler {
return sink.handler
}

// Assert conformance to the interfaces.
var _ logr.LogSink = &slogSink{}
var _ logr.CallDepthLogSink = &slogSink{}
var _ Underlier = &slogSink{}