slog context support #237

pohly · 2023-11-26T17:00:38Z

Conversion is done on demand. A program which uses either slog or logr for
logging remains as efficient as before. What is less efficient is storing a
logger of one type and then retrieving it as the other.

Ideally, reusable packages should avoid using a slog.Logger and prefer
logr.Logger instead when retrieving a logger from a context. There is a
significant amount of packages which already work that way (Kubernetes and
related packages), while storing and retrieving a slog.Logger is new and not
done anywhere yet. Emulating existing behavior ensures better performance.

To enable on demand conversion, the slogr implementation must be moved
into the main package.

Related-to: #234

context_slog.go

thockin

This isn't horrible, to me, but it seems like one day slog will decide to add context support and then we'll have to do it all again.

context_noslog.go

slogr.go

thockin · 2023-11-27T18:59:09Z

slogr.go

+//
+// The logr verbosity level is mapped to slog levels such that V(0) becomes
+// slog.LevelInfo and V(4) becomes slog.LevelDebug.
+func NewLogr(handler slog.Handler) Logger {


So we have logr.New(logr.LogSink) and logr.NewLogr(slog.Handler) ? Eww.

NewFromSlogHandler() ?

I hadn't thought much about the names. I agree that it makes sense to reconsider, now that they are being used with logr as package selector instead of slogr.

I'm fine with NewFromSlogHandler.

Renamed in a separate commit, to have an explanation for the reason in the commit history.

I also restored slogr/slogr_test.go because that has an example and it's worth testing the code there. It doesn't do much, but better safe than sorry...

context_slog.go

pohly · 2023-11-27T19:38:34Z

This isn't horrible, to me, but it seems like one day slog will decide to add context support

Very unlikely. @jba again confirmed it, saying "They [context support methods] were very controversial.I don't see that changing." -- golang/go#56345 (comment)

thockin · 2023-11-27T22:00:36Z

I suspect that Context methods will be reconsidered as people adopt slog and find that to be egregiously absent. It was contentious among vocal early-adopters. It won't be contentious among the larger population (I think).

pohly · 2023-11-28T20:16:00Z

I've pushed an update:

first commit updated to restore slogr/slogr_test.go
new commits for renaming and documentation

thockin · 2023-11-29T00:38:47Z

README.md

@@ -145,11 +146,12 @@ There are implementations for the following logging libraries:
 ## slog interoperability

 Interoperability goes both ways, using the `logr.Logger` API with a `slog.Handler`
-and using the `slog.Logger` API with a `logr.LogSink`. [slogr](./slogr) provides `NewLogr` and
-`NewSlogHandler` API calls to convert between a `logr.Logger` and a `slog.Handler`.
+and using the `slog.Logger` API with a `logr.LogSink`. [slogr](./slogr) provides `NewLogr` (= `logr.NewFromSlogHandler`) and


If this effectively replaces slogr, then we should just document the logr APIs here, not slogr.

I wasn't sure whether we'd want to deprecate slogr - it just got added 🥵

But we can also do that, no strong opinion from me about it.

I don't love having two equivaent ways of saying the same thng which are both "suported". Deprecated doesn't mean gone, just "look here instead". IMO

Makes sense.

slogr/slogr.go

thockin · 2023-11-29T00:41:31Z

slogr/slogr.go

@@ -36,14 +36,11 @@ import (
 //
 // The logr verbosity level is mapped to slog levels such that V(0) becomes
 // slog.LevelInfo and V(4) becomes slog.LevelDebug.
+//
+// NewLogr is identical to [logr.NewFromSlogHandler]. Either of these can
+// be used.


Should these be Deprecated: Use logr.NewFromSlogHandler instead ?

We could also kill off all the duplicate docs that way - users just go read the logr version.

The duplicated docs are indeed annoying. I'm leaning towards simply deprecating slogr now.

thockin · 2023-11-29T00:42:05Z

slogr/slogr.go

@@ -64,16 +61,11 @@ func NewLogr(handler slog.Handler) logr.Logger {
 //	slog.New(NewSlogHandler(logger)).Warning(...) -> logger.GetSink().Info(level=0, ...)
 //	slog.New(NewSlogHandler(logger)).Info(...) -> logger.GetSink().Info(level=0, ...)
 //	slog.New(NewSlogHandler(logger.V(4))).Info(...) -> logger.GetSink().Info(level=4, ...)
+//
+// NewSlogHandler is identical to [logr.NewSlogHandler]. Either of these can
+// be used.


same above re: Deprecated and var

slogr_test.go

thockin · 2023-11-29T00:55:23Z

slogr_test.go

+	// level=DEBUG msg="with values, verbosity and name" x=1 y=2 str=abc logger=foo/bar
+}
+
+func ExampleNewSlogLogger() {


Should this be ExampleNewSlogHandler ?

Yes, fixed.

thockin · 2023-11-29T00:56:20Z

slogr_test.go

+}
+
+func ExampleNew() {
+	logger := logr.NewFromSlogHandler(slog.NewTextHandler(os.Stdout, debugWithoutTime))


can we name the var logrLogger ?

thockin · 2023-11-29T00:56:38Z

slogr_test.go

+		Verbosity: 10,
+	})
+
+	logger := slog.New(logr.NewSlogHandler(funcrLogger))


can we name the var slogLogger ?

thockin · 2023-11-29T00:58:53Z

context_slog.go

+	}
+}
+
+// FromContext returns a slog.Logger from ctx or nil if no such Logger is found.


comment doesn't match code wrt the name

thockin · 2023-11-29T00:59:49Z

context_slog.go

+}
+
+// FromContext returns a slog.Logger from ctx or nil if no such Logger is found.
+func SlogLoggerFromContext(ctx context.Context) *slog.Logger {


can I argue for FromContextAsSlogLogger so the FromContext and NewContext prefixes survive?

Yes, make sense.

thockin · 2023-11-29T16:54:08Z

ICYMI - there was no new push :)

pohly · 2023-11-29T17:37:28Z

ICYMI - there was no new push :)

I wanted to revise the documentation part first regarding deprecation, but I am not going to get to that this evening anymore, so I pushed what I have just now. The next update will be tomorrow.

pohly · 2023-11-30T09:39:01Z

I've updated the documentation and removed the slogr/slogr_test.go: https://github.com/go-logr/logr/compare/9ac998326cdc223355affa6baeab839316117be4..ca4a15b75ca78b8f2db2de3e41b660c810ef5b9e

pohly · 2023-11-30T09:39:43Z

README.md

+provides additional support code for this approach. It also contains wrappers
+for the context functions in logr, so developers who prefer to not use the logr
+APIs directly can use those instead and the resulting code will still be
+interoperable with logr.


@veqryn: does this look okay to you?

Looks good to me. Thank you.

thockin

LGTM overall

README.md

This is necessary to give the context handling functions access to the slogr functionality. Functions get renamed to match names in the main package where needed. The slogr package is now deprecated, but continues to be supported as part of the logr v1 API by keeping all exported items as wrappers or aliases for the main package.

Conversion is done on demand. A program which uses either slog or logr for logging remains as efficient as before. What is less efficient is storing a logger of one type and then retrieving it as the other. Ideally, reusable packages should avoid using a slog.Logger and prefer logr.Logger instead when retrieving a logger from a context. There is a significant amount of packages which already work that way (Kubernetes and related packages), while storing and retrieving a logr.Logger is new and not done anywhere yet. Emulating existing behavior ensures better performance.

pohly · 2023-11-30T20:42:30Z

New update pushed. I also found a left-over NewLogr and comments about "is identical" in the main package and removed them. I moved the slogr/example into examples/slog and updated it to use the main package's API.

=> https://github.com/go-logr/logr/compare/ca4a15b75ca78b8f2db2de3e41b660c810ef5b9e..f1edf4f6aed3da9f4599674ea66e013fd37a5446

thockin · 2023-11-30T21:08:28Z

LGTM - hold for @veqryn to take a look?

veqryn

👍 LGTM
After the next release, I will update slog-context to use this, and pull you all into the PR to take a look. :)

veqryn · 2023-12-02T09:15:47Z

@pohly @thockin Could I get your eyes on this pr please: veqryn/slog-context#1

pohly · 2023-12-21T15:20:49Z

slogr/slogr.go

-	}
-	return handler
+// Deprecated: use [logr.ToSlogHandler] instead.
+func ToSlogHandler(logger logr.Logger) slog.Handler {


Argh!

I messed up some global search/replace and renamed NewSlogHandler to ToSlogHandler although here we have to keep the old name.

Found when updating logr in klog. Will prepare a fix.

github-actions bot assigned pohly and thockin Nov 26, 2023

pohly mentioned this pull request Nov 26, 2023

contextual logging for slog #234

Closed

pohly commented Nov 26, 2023

View reviewed changes

context_slog.go Outdated Show resolved Hide resolved

thockin reviewed Nov 27, 2023

View reviewed changes

pohly force-pushed the slog-context-in-logr branch from 4482fc7 to 277269c Compare November 28, 2023 20:15

thockin reviewed Nov 29, 2023

View reviewed changes

pohly force-pushed the slog-context-in-logr branch 2 times, most recently from 9ac9983 to ca4a15b Compare November 30, 2023 09:04

pohly commented Nov 30, 2023

View reviewed changes

thockin reviewed Nov 30, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

pohly added 2 commits November 30, 2023 21:41

pohly force-pushed the slog-context-in-logr branch from ca4a15b to f1edf4f Compare November 30, 2023 20:42

veqryn approved these changes Dec 1, 2023

View reviewed changes

thockin merged commit 6151b2f into go-logr:master Dec 1, 2023
14 checks passed

pohly commented Dec 21, 2023

View reviewed changes

renovate bot mentioned this pull request Feb 8, 2024

chore(deps): update all non-major dependencies amuraru/koperator#69

Open

1 task

slog context support #237

slog context support #237

Conversation

pohly commented Nov 26, 2023 • edited by thockin Loading

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 27, 2023

thockin commented Nov 27, 2023

pohly commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Nov 29, 2023

pohly commented Nov 29, 2023

pohly commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

pohly commented Nov 30, 2023

thockin commented Nov 30, 2023

veqryn left a comment • edited Loading

Choose a reason for hiding this comment

veqryn commented Dec 2, 2023

Choose a reason for hiding this comment

pohly commented Nov 26, 2023 •

edited by thockin

Loading

veqryn left a comment •

edited

Loading