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

slogr: add glue code for logging to slog.Handler and with slog.Logger #205

Merged
merged 2 commits into from Aug 22, 2023

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Aug 7, 2023

Interoperability with log/slog from Go 1.21 includes the ability to use a slog.Handler as backend with logr.Logger as frontend and vice versa.

This is only the initial step. In particular writing with slog.Logger to a logr.LogSink is not working well (time stamp and call site from record get ignored). Further work is needed to improve this.

slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogr.go Outdated Show resolved Hide resolved
@pohly
Copy link
Contributor Author

pohly commented Aug 7, 2023

Tests cover 100% of the new code lines. I also tried to consider edge cases.

@pohly pohly force-pushed the slog-glue branch 3 times, most recently from ee2b73c to 0211c75 Compare August 7, 2023 12:27
Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

I am AFK for a few days, will look more when I get back

logr.go Show resolved Hide resolved
Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

First commit

logr_test.go Show resolved Hide resolved
Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

I like having runnable examples for quick-testing. Worthing including?

diff --git a/slogr/slog_example/main.go b/slogr/slog_example/main.go
new file mode 100644
index 0000000..2e67539
--- /dev/null
+++ b/slogr/handler_example/main.go
@@ -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")
+}

In particular, this lets us verify caller information visually. I like your tests, too, and maybe we don't need both?

slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogsink.go Show resolved Hide resolved
slogr/slogsink.go Show resolved Hide resolved
slogr/sloghandler.go Show resolved Hide resolved
slogr/slogsink.go Show resolved Hide resolved
slogr/discardhandler.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
slogr/slogr.go Show resolved Hide resolved
@pohly
Copy link
Contributor Author

pohly commented Aug 17, 2023

I like having runnable examples for quick-testing. Worthing including?

Yes. I merged it with the example for the other direction #205 (comment) because some parts are common (error) and this way it's easier to see both approaches without having to open two files side-by-side.

Initially, the slog.Logger calls recorded the wrong logger.go from the slog package as caller. This is inevitable, but at least for the current Go 1.21.0 and simple usage (i.e. slog.Logger directly calls the handler) it can be compensated through WithCallDepth calls - I've added that.

I like your tests, too, and maybe we don't need both?

Both = example and tests? I think we want (example) and need (tests) both.

@pohly pohly force-pushed the slog-glue branch 2 times, most recently from 12dd30b to 77f7a0f Compare August 17, 2023 21:28
@thockin
Copy link
Contributor

thockin commented Aug 17, 2023

Did you forget to add the example?

Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

First commit only

logr.go Outdated Show resolved Hide resolved
slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogsink.go Show resolved Hide resolved
slogr/sloghandler.go Show resolved Hide resolved
slogr/sloghandler.go Show resolved Hide resolved
slogr/sloghandler.go Outdated Show resolved Hide resolved
@pohly
Copy link
Contributor Author

pohly commented Aug 18, 2023

Did you forget to add the example?

Yes 😢

Fixed.

The call is needed for interoperability with other logging packages when
replacing the logr.Logger high-level API with some other API, like slog.Logger.
@pohly pohly force-pushed the slog-glue branch 2 times, most recently from 19c78e0 to 9b69cdd Compare August 18, 2023 07:45
slogr/slogr.go Outdated

// NewLogr returns a logr.Logger which writes to the slog.Handler.
//
// In the output the logr verbosity level gets negated, so V(4) becomes
Copy link
Contributor

Choose a reason for hiding this comment

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

Sort of a nit, but the "gets negated" is very precise. Can we write this a bit more loosely, like:

"""
NewLogr returns a logr.Logger which writes to the slog.Handler. The logr verbosity level is mapped to slog levels such that V(0) becomes slog.LevelInfo and V(4) becomes slog.LevelDebug.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

slogr/slogr.go Outdated Show resolved Hide resolved
slogr/slogsink.go Show resolved Hide resolved
Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

closer!

type slogHandler struct {
sink logr.LogSink
groupPrefix string
level slog.Level
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to levelBias ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and added documentation, also for groupPrefix.

slogr/sloghandler.go Outdated Show resolved Hide resolved
logrLogger := slogr.NewLogr(handler)
logrExample(logrLogger)

logrLogger = funcr.New(
Copy link
Contributor

Choose a reason for hiding this comment

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

NewJSON for equivalence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@pohly pohly force-pushed the slog-glue branch 2 times, most recently from 57acb9d to 29c6382 Compare August 21, 2023 17:46
slogr/slogr.go Outdated
// error log entries with LogSink.Error, regardless of the verbosity level of
// the logr.Logger:
//
// slogLogger.Warning(...) -> logSink.Error(...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this line - Warning is 4, but the comment above says slog.LevelError which is 8, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was simply wrong: I mis-remembered and thought that Warning >= Error.

Because Info < Warning < Error, slog.Logger.Warning can be used as example for a level that gets mapped to the same logr.Level 0 - changed.

Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Just that one comment I don't understand

Interoperability with log/slog from Go 1.21 includes the ability to use a
slog.Handler as backend with logr.Logger as frontend and vice versa.

This is only the initial step. In particular writing with slog.Logger to
a logr.LogSink is not working well (time stamp and call site from
record get ignored). Further work is needed to improve this.
@thockin thockin merged commit b785b9f into go-logr:master Aug 22, 2023
14 checks passed
@thockin
Copy link
Contributor

thockin commented Aug 22, 2023

merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants