WIP: add slogr #195
WIP: add slogr #195
Conversation
thockin
force-pushed
the
slogr
branch
4 times, most recently
from
c66ecab to
9ee2c75
Compare
Not sure if this should be in logr or in a new repo, but I wanted to get feedback. No tests yet, and I am not sure how much testing this warrants.
I am not sure this belongs in the same package as slogr. There are a few aspects of slog that logr has no equivalent for: - time passed in to the handler - passing in a PC rather than a call-depth - WithGroup() We need to think about whether to extend logr to accommodate or just ignore these.
| if record.Level >= slog.LevelError { | ||
| h.logger.Error(nil, record.Message, args...) | ||
| } else { | ||
| h.logger.V(int(-record.Level)).Info(record.Message, args...) |
There was a problem hiding this comment.
Consider dividing by 4, so:
Info -> V(0)
Debug -> V(1)
Warn -> V(-1)
I don't know if that's better.
There was a problem hiding this comment.
A direct mapping is better because Kubernetes uses V(4) for debug messages, which corresponds nicely with slog.LevelDebug = -4.
There was a problem hiding this comment.
Agree - I don't see why we need to modify it - am I missing some trap?
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| // NewSlogHandler returns a slog.Handler which logs through an arbitrary | ||
| // logr.Logger. | ||
| func NewSlogHandler(logger logr.Logger) slog.Handler { |
There was a problem hiding this comment.
You should test this with the testing/slogtest package to make sure you get all the subtle rules right.
There was a problem hiding this comment.
...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.
There was a problem hiding this comment.
See golang/go#61758 (comment) for a comment about allowing to skip some of the testing/slogtest checks.
|
@pohly I did this to see how I would do it, without looking at yours. We are about 80% identitcal, which is good. Let's focus on yours, where I sent a bunch of comments to discuss. |
Not sure if this should be in logr or in a new repo, but I wanted to get feedback. No tests yet, and I am not sure how much testing this warrants.
xref #171