Add SlogSink support to funcr #241

thockin · 2023-12-01T23:49:28Z

See commits for comments. This unearthed at least 3 bugs.

funcr now passes all but one of the slog tests (that being the timestamp one).

pohly

Looks good to me, but I've not tried to really understand all implementation details in funcr.

sloghandler.go

thockin · 2023-12-04T17:02:29Z

Yeah, I don't expect you to dig into funcr too deeply. Mostly I was interested in your take on the changes to TestFuncrHandler() - specifically that the exceptions list is now reduced to just 1.

It feels like that SHOULD be in the funcr subdir, but we also use testSlog to test slogSink ?

thockin · 2023-12-04T17:09:23Z

As I stare at slogr_test, I am a little confused - testSlogSink seems like a copy of slogSink - why?

thockin · 2023-12-04T17:18:41Z

I pushed another commit - please look at the "Get rid of testSlogSink" and tell me if it is wrong?

thockin · 2023-12-04T17:34:36Z

I pushed on more commit, moving slogtests to a helper and funcr tests to funcr.

pohly · 2023-12-05T15:52:08Z

One observation: because we are now testing only with a logr.Sink which is also a logr.SlogSink, we lose coverage of those code paths that bridge slog with "traditional" logr backends. This primarily shows up in sloghandler.go (97.9% percent before, now 53.2%).

To restore full coverage, we would have to define a logr.LogSink which uses the funcr formatter without providing the logr.SlogSink interface.

thockin · 2023-12-05T16:07:46Z

What would you say to having an internal trivial implementation which is basically discard but not nil? We could have one which supports SlogSink and one which does not.

I guess that is testLogSink...

thockin · 2023-12-05T16:09:40Z

Are we generating a coverage report for PRs?

pohly · 2023-12-05T17:02:22Z

What would you say to having an internal trivial implementation which is basically discard but not nil?

For slogtest.TestHandler, that test sink would have to produce valid JSON. I can suggest a solution tomorrow.

Are we generating a coverage report for PRs?

No. I was testing locally because I wanted to understand whether moving the testing around had an effect. There's an effect of the PR, it's just because for another reason.

pohly · 2023-12-05T17:03:19Z

benchmark/benchmark_slog_test.go

+
+//
+// slogSink wrapper of discard
+//


Did you learn anything from these benchmarks? I'm just curious.

I learned that funcr with JSON is more than 2x faster than slog's JSON handler, but has more allocs. And that the slogSink (wrapper) path is (as expected) low overhead.

I did a similar comparison between slog's JSON handler and zap (kubernetes/kubernetes@9100c3c). Slog was slower, so there was no justification for switching from zap to slog as backend in Kubernetes.

While I have your attention (sorry, couldn't resist 😁 ), could you perhaps review the PR in Kubernetes that this commit came from? The dependency update is in another pending PR, that's the only reason it's WIP.

thockin · 2023-12-05T17:59:28Z

I am cooking something wrt test impls and coverage.

thockin · 2023-12-05T22:30:40Z

I have an extra commit in flight, but opinion wanted.

I can make slogHandler do groups properly, but it would store them itself and bypass calling sink.WithValues() . It would come close to passing slog tests, though. Worthwhile? Or just leave it with things like a.b=<value> ?

thockin · 2023-12-06T07:24:06Z

OK, I added a big commit. It makes sloghandler pass almost all of the slog tests wrt WithGroup and WithAttrs, at the cost of bypassing the LogSink.WithValues. It's not yet safe - maps get copied and then mutated nor have I really looked at performance. It needs maps in order to get groups right, and that makes me sad (expensive, loses ordering).

I could abandon the maps part and just go back to dot-prefixes (simpler, won't pass as many slog tests but maybe OK?). Opinion wanted.

pohly · 2023-12-06T08:55:56Z

I can make slogHandler do groups properly, but it would store them itself and bypass calling sink.WithValues() . It would come close to passing slog tests, though. Worthwhile? Or just leave it with things like a.b= ?

I prefer to keep slogHandler as simple as possible, i.e. use a.b=<value> for those sinks which don't implement SlogSink. There are other things that sinks should support if they care about being used with slog, like special slog values, so eventually either a sink supports slog and SlogSink or it should be replaced with something else. Once we are at that point, the additional complexity in slogHandler won't be needed.

thockin · 2023-12-06T16:39:17Z

After having done it, I think I agree. Though the testing is satisfying :)

thockin · 2023-12-06T16:47:19Z

One last thought: as it is, with prefixes, someone who calls:

slogger.Info("msg", slog.Group("g", "k", "v"))

gets a different result than:

slogger.WithGroup("g", Info("msg", "k", "v"))

It's {"g": { "k": "v"}} in the first and {"g.k": "v"} in the second.

pohly · 2023-12-06T17:29:23Z

One last thought: as it is, with prefixes, someone who calls:

slogger.Info("msg", slog.Group("g", "k", "v"))
[...]
It's {"g": { "k": "v"}} in the first

Is that perhaps because slogHandler is passing the parameter down into a sink which partially supports slog, as in "I can handle slog.Group, but I don't implement the full logr.SlogSink"?

I can see how this happens with a simple test sink based on funcr. I think "real" sinks will fully support slog, then the WithGroup case will also work.

thockin · 2023-12-06T17:37:04Z

I pushed now with a much simpler change to sloghandler, coverage is restored, tests are cleaner (IMO). PTAL.

context_slog_test.go

context_test.go

pohly · 2023-12-07T13:35:33Z

.github/workflows/lint.yaml

+        uses: actions/setup-go@v4
+        with:
+          go-version: '>=1.21.0'
+          cache: false


I agree that we should cover the new code with linting. I don't think it's worth to lint also with older Go because the non-slog code isn't that complex and going away at some point.

i cant tell if you are agreeing with me or want a change?

I agree with you.

pohly · 2023-12-07T13:39:45Z

sloghandler.go

-		if attr.Key != "" {
-			kvList = append(kvList, l.addGroupPrefix(attr.Key), attr.Value.Resolve().Any())
-		}
+		kvList = attrToKVs(attr, l.groupPrefix, kvList)


I think this code change deserves to be a separate commit with a description of why it is made. Right now it is in "Clean up slog testing and restore coverage" which doesn't tell me why sloghandler.go needs to be changed.

I can either a) break this last commit to a PR which applies AFTER this PR (rebasing it is tedious and it has a lot of deps on other commits); or b) beef up the comments.

I'll do B now and you can decide if you prefer A.

sloghandler.go

thockin · 2023-12-07T19:46:06Z

pushed with small changes, will look at splitting sloghandler to a new PR or beefing up comments.

pohly · 2023-12-08T10:38:25Z

sloghandler.go

+// and other details of slog.
+func attrToKVs(attr slog.Attr, groupPrefix string, kvList []any) []any {
+	attrVal := attr.Value.Resolve()
+	if attrVal.Kind() == slog.KindGroup {


I suppose this is the "retools sloghandler to handle slog Attrs a bit better" part.

Looks okay to me now. Let's rebase and then merge it without further refactoring (your plan B).

Some of the testing done in slogr_test.go should probably be in funcr, but the test jig is just complex enough that I don't want to duplicate it in this commit. Later.

This uncovered a bug where it would sometimes write to the input list, corrupting tests.

thockin · 2023-12-08T16:39:31Z

rebased and pushed

thockin assigned pohly Dec 1, 2023

github-actions bot assigned thockin Dec 1, 2023

thockin force-pushed the master branch 7 times, most recently from 8d309b3 to 7651624 Compare December 2, 2023 05:24

pohly reviewed Dec 3, 2023

View reviewed changes

sloghandler.go Outdated Show resolved Hide resolved

thockin force-pushed the master branch from 7651624 to e21e309 Compare December 4, 2023 16:58

thockin force-pushed the master branch 2 times, most recently from d7be8c5 to 76cc254 Compare December 4, 2023 22:29

pohly reviewed Dec 5, 2023

View reviewed changes

thockin force-pushed the master branch from 76cc254 to 45bb084 Compare December 6, 2023 07:21

thockin force-pushed the master branch from 45bb084 to e4f3211 Compare December 6, 2023 07:27

thockin force-pushed the master branch 2 times, most recently from c6e67c9 to 0dd6551 Compare December 6, 2023 17:36

thockin force-pushed the master branch from 0dd6551 to 0141228 Compare December 6, 2023 17:47

pohly reviewed Dec 7, 2023

View reviewed changes

thockin force-pushed the master branch from 0141228 to cd8a878 Compare December 7, 2023 19:45

thockin force-pushed the master branch from cd8a878 to 5243e37 Compare December 7, 2023 22:05

pohly reviewed Dec 8, 2023

View reviewed changes

thockin added 11 commits December 8, 2023 08:35

Minor cleanups in slogr_test (readability)

cdb930d

Get rid of testSlogSink

d95f9b6

Add SlogSink support to funcr

8221825

Some of the testing done in slogr_test.go should probably be in funcr, but the test jig is just complex enough that I don't want to duplicate it in this commit. Later.

funcr: Be consistent about commas

955d2aa

This uncovered a bug where it would sometimes write to the input list, corrupting tests.

funcr: Be consistent about colons

44c6ac8

funcr: Be consistent about quoted

b5e7d9f

Fix some lint

83dbe72

Put slog tests in a helper, move funcr test

f558531

Add benchmarks for slogSink

6432877

Break examples to new file

b228ba8

Clean up slog testing and restore coverage

98ee9d9

thockin force-pushed the master branch from 5243e37 to 98ee9d9 Compare December 8, 2023 16:39

pohly merged commit d55b4e2 into go-logr:master Dec 8, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SlogSink support to funcr #241

Add SlogSink support to funcr #241

thockin commented Dec 1, 2023 •

edited

Loading

pohly left a comment •

edited

Loading

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

pohly commented Dec 5, 2023

thockin commented Dec 5, 2023 •

edited

Loading

thockin commented Dec 5, 2023

pohly commented Dec 5, 2023

pohly Dec 5, 2023

thockin Dec 5, 2023

pohly Dec 6, 2023

pohly Dec 6, 2023

thockin commented Dec 5, 2023

thockin commented Dec 5, 2023

thockin commented Dec 6, 2023

pohly commented Dec 6, 2023

thockin commented Dec 6, 2023

thockin commented Dec 6, 2023

pohly commented Dec 6, 2023

thockin commented Dec 6, 2023

pohly Dec 7, 2023

thockin Dec 7, 2023

pohly Dec 8, 2023

pohly Dec 7, 2023

thockin Dec 7, 2023

thockin Dec 7, 2023

thockin commented Dec 7, 2023

pohly Dec 8, 2023

thockin commented Dec 8, 2023

Add SlogSink support to funcr #241

Add SlogSink support to funcr #241

Conversation

thockin commented Dec 1, 2023 • edited Loading

pohly left a comment • edited Loading

Choose a reason for hiding this comment

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

thockin commented Dec 4, 2023

pohly commented Dec 5, 2023

thockin commented Dec 5, 2023 • edited Loading

thockin commented Dec 5, 2023

pohly commented Dec 5, 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

thockin commented Dec 5, 2023

thockin commented Dec 5, 2023

thockin commented Dec 6, 2023

pohly commented Dec 6, 2023

thockin commented Dec 6, 2023

thockin commented Dec 6, 2023

pohly commented Dec 6, 2023

thockin commented Dec 6, 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

thockin commented Dec 7, 2023

Choose a reason for hiding this comment

thockin commented Dec 8, 2023

thockin commented Dec 1, 2023 •

edited

Loading

pohly left a comment •

edited

Loading

thockin commented Dec 5, 2023 •

edited

Loading