Update logr and provide basic slog interop

[avorima]

Update logr dependency to 1.4.1.

It is recommended that log sink implementations are compatible with slog.
This PR makes the ToSlogHandler direction work by adding support for slog values.

There is one major caveat: errors passed to slog.Error must have the key error otherwise the two (2) error fields will be displayed. One is the WithError field that is always added and another is the actual error that will be printed.
I moved the WithError call before WithFields to ensure that the error at least can be overwritten.

I think this problem can only be solved by implementing a slog handler that knows in what context it's OK to log nil errors and when they can be omitted.

[bombsimon]

Thank you very much for this PR!

Sadly I haven't used slog at all so I don't know what's common around error handling. However, giving this a quick glance it looks like you're right - I don't see a way to add or customize ReplaceAttr after the logger has been created.

Do you know if this is the same for all logr implementations that supports ToSlogHandler? Is this something we can even solve or should this be tracked in go-logr/logr?

It looks like this is expected and similar for funcr here: https://github.com/go-logr/logr/blob/75599fab1431f2d8447fa7175b5b17fd49321b61/example_slogr_test.go#L76

However I'm still not sure I understand where the term error is coming from and why it always gets added. Also another thing worth pointing out is that it seems like we somehow ends up representing the go value for nil. In the linked example above the value for error is nil but in this PR the value gets set to "<nil>", maybe what's something we can fix at least?

[avorima]

The ToSlogHandler wrapper always calls Error(nil, ...) on the log sink if it doesn't implement logr.SlogSink. I don't think it's feasible to somehow plumb an error into the first argument, so this design probably won't change. I will see if I can circumvent this call by implementing logr.SlogSink on logrusr.

As to the differing output of nil errors:

In the case of funcr the error gets converted to an interface here so the subsequent switch case for errors is not taken and it reaches this line where the value is hard-coded to null.

In the case of logrusr the error is still an error when it gets added to the fields map and gets written using fmt.Sprint which results in <nil>.

So logrusr just leverages the way that logrus would print errors whereas funcr elides the type and falls back to using a default for all nil values.

[avorima]

Ok, I don't think I'll finish the slog handler implementation today. Are you okay with merging this as-is? I'm planning to replace a custom logrus+logr implementation with this repo, but I need 1.4.1.

[bombsimon]

Thanks for explaining! But I'll have to keep asking more basic questions here. 🙈

With the current state of this PR I see how we can get a slog from our logger where the handler will use us as a sink. However, I don't see why we would need to add the formatSlog here. logr seems to create the slog.Handler and will convert the slog.Attr to the key-value list that the sink (in this case we) expect: https://github.com/go-logr/logr/blob/75599fab1431f2d8447fa7175b5b17fd49321b61/sloghandler.go#L140-L160

In fact, since slog keeps both key and value in slog.Attr, I don't see how we could support that since something like log.Info("uh oh", slog.String("key", "value")) will discard the attr since it's not an even list and adding two slog values would discord the first one since we only switch over the value which is expected to be the second one. If we want to support this we'll have to rewrite how listToLogrusFields loops over keysAndValues. But, is this really reasonable to support? If you want to use slog.Attr, wouldn't you just do what you did and use ToSlogHandler first?

Maybe I'm missing something but I can't see when formatSlog is currently called? If I delete logrusr_slog.go and remove the build tags in logrusr_noslog.go the test in example_slog_test.go still works:

› go test ./... -v -run ExampleNew -count 1
?       github.com/bombsimon/logrusr/v4/example [no test files]
=== RUN   ExampleNew
--- PASS: ExampleNew (0.00s)
PASS
ok      github.com/bombsimon/logrusr/v4 0.001s

[avorima]

You're right, I mixed things up. slog.Attr handling is only required when logr.SlogSink is implemented.
I will reuse the some of the changes here to do that and draft the PR until I'm done. Until then it should be fine to just merge the 1.4.1 update from dependabot, since you showed that it already works.

[bombsimon]

Until then it should be fine to just merge the 1.4.1 update from dependabot

Awesome! I'll do that! Just let me know if there's anything else I can do!