Add slog.Handler writing to logrus.Logger - #1553
Conversation
thaJeztah
left a comment
There was a problem hiding this comment.
thanks! overall this looks good; I left some comments, and was already drafting some of those changes, so will do a quick rebase of your PR and push commits with those changes (to be squashed)
| // DefaultSlogLevelMapper is the default mapping from slog levels to logrus levels. | ||
| // | ||
| // slog level >= Error → ErrorLevel | ||
| // slog level >= Warn → WarnLevel | ||
| // slog level >= Info → InfoLevel | ||
| // slog level >= Debug → DebugLevel | ||
| // otherwise → TraceLevel | ||
| func DefaultSlogLevelMapper(level slog.Level) logrus.Level { | ||
| switch { | ||
| case level >= slog.LevelError: | ||
| return logrus.ErrorLevel | ||
| case level >= slog.LevelWarn: | ||
| return logrus.WarnLevel | ||
| case level >= slog.LevelInfo: | ||
| return logrus.InfoLevel | ||
| case level >= slog.LevelDebug: | ||
| return logrus.DebugLevel | ||
| default: | ||
| return logrus.TraceLevel | ||
| } | ||
| } | ||
|
|
||
| func (h *Handler) toLogrusLevel(level slog.Level) logrus.Level { | ||
| if h.LevelMapper != nil { | ||
| return h.LevelMapper(level) | ||
| } | ||
| return DefaultSlogLevelMapper(level) | ||
| } |
There was a problem hiding this comment.
I want to reduce the public API; DefaultSlogLevelMapper is only used to set the defaults; external callers wouldn't need it (they can leave the LevelMapper nil, which means that the defaults are used).
I was also having another look at the Hook implementation and realised we don't have to drop the extended levels provided by Logrus (similar to containerd/log#12) so I updated the Hook's mapping, and added consts that we can use for clarity;
We can take the same approach here as is used for the Hook;
Lines 67 to 91 in ed957d5
Then document the mapping on the Handler struct;
Lines 21 to 36 in ed957d5
And put relevant docs that are currently on DefaultSlogLevelMapper on the map's field;
Lines 39 to 42 in ed957d5
HOWEVER; when looking at that; this would also add Panic and Fatal levels; logrus may make those run os.Exit(1) or panic - that's probably unexpected for the slog integration; there was a bug in the code that didn't work as advertised; opened a PR for that so that we can integrate those levels;
| // SlogLevelMapper maps a [slog.Level] to a [logrus.Level]. | ||
| // | ||
| // To change the default level mapping, for instance to map custom slog levels | ||
| // or to send [slog.LevelError] to [logrus.FatalLevel], set | ||
| // [Handler.LevelMapper] to your own implementation of this function. | ||
| type SlogLevelMapper func(slog.Level) logrus.Level |
There was a problem hiding this comment.
We can drop this type; I noticed we did the same for the Hook, but then realised it doesn't bring us much, and it's better to just put the function signature in the Handler struct directly, and document it there; see
| level := h.toLogrusLevel(record.Level) | ||
|
|
There was a problem hiding this comment.
Given the extra processing needed to do the conversion, perhaps it's worth taking advantage of the level here and return early if the log-level is not enabled;
level := h.toLogrusLevel(record.Level)
if !h.logger.IsLevelEnabled(level) {
return nil
}| if len(fields) > 0 { | ||
| entry = entry.WithFields(fields) | ||
| } | ||
| entry.Log(level, record.Message) |
There was a problem hiding this comment.
We'll probably pay the price of duplicating the entry 2 - 3 times here (each WithXXX creates a copy of the entry, and clones the Field.Data map); as all the fields we need to set on the entry are exported, we can use a struct-literal to create the entry;
entry := &logrus.Entry{
Logger: h.logger,
Data: fields,
Time: record.Time,
Context: ctx,
}
entry.Log(level, record.Message)783de71 to
bfaf1ef
Compare
There was a problem hiding this comment.
Pull request overview
Adds the missing half of the log/slog compatibility layer by introducing a slog.Handler implementation that forwards slog records into an existing logrus.Logger, complementing the existing Logrus→slog hook in hooks/slog.
Changes:
- Add
hooks/slog.Handler+NewHandler(*logrus.Logger)to map levels and convert attrs/groups into Logrus fields while preserving context/time. - Add comprehensive tests covering level mapping, custom mapping, level filtering, attrs/groups flattening, context/time propagation, and nil logger panic.
- Document the new handler in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hooks/slog/handler.go | New slog.Handler implementation that logs into a logrus.Logger, including level mapping and field/group flattening. |
| hooks/slog/handler_test.go | New tests validating handler behavior and integration with Logrus output. |
| CHANGELOG.md | Adds a feature entry describing the new hooks/slog.NewHandler. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bfaf1ef to
ddb265f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
hooks/slog/handler.go:203
- Valid non-group slog attributes may have an empty key, but this branch drops them even though the zero Attr was already filtered above. Built-in slog handlers retain such attributes, and
logrus.Fieldscan represent the empty key, so records containingslog.Attr{Key: "", Value: ...}silently lose data. Only an empty key on a group should trigger inline-group behavior.
if attr.Key == "" {
return
}
hooks/slog/handler.go:117
- When
Logger.ReportCalleris enabled, leavingEntry.Callerunset makesEntry.Loginspect the current stack and reportHandler.Handleas the caller rather than the original slog call site.slog.Record.PCcarries that original source location and should be converted toruntime.Framewhen caller reporting is enabled. A race-safe fix likely needs a logger accessor for the protectedReportCallersetting, plus a caller-preservation test analogous to the reverse slog hook.
entry := &logrus.Entry{
Logger: h.logger,
Data: h.fields,
hooks/slog/handler.go:118
- The claimed record-time preservation is not tested with a known record timestamp.
TestHandler_contextAndTimesendstsonly as an attribute and merely checks that the logger-generated entry time is nonzero, so replacingrecord.Timewith the current time would still pass. Add a directHandletest usingslog.NewRecord(ts, ...)and assertentry.Time.Equal(ts).
Time: record.Time,
Hm; yeah possibly we should do that.
Yup, already noticed that one, and made preparations to make that work; will do it in a follow-up;
Good one; added a test |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
hooks/slog/handler.go:119
- When the underlying logger has
ReportCallerenabled, leavingCallerunset makesEntry.Logdiscover(*Handler).Handleas the first frame outside the root logrus package, so every bridged record reportshooks/slog/handler.goinstead of the original slog call site.slog.Record.PCcarries that call site; convert it to aruntime.Frameand use it as the entry caller when caller reporting is enabled (while preserving the existing behavior for records withPC == 0).
entry := &logrus.Entry{
Logger: h.logger,
Data: h.fields,
Time: record.Time,
Context: ctx,
hooks/slog/handler.go:208
- Assigning
attr.Value.Any()directly intoEntry.Databypasses Logrus's unsupported-field handling inEntry.WithField(s). A slog attribute containing a function or pointer-to-function therefore reachesJSONFormatter, whose marshal fails and causes the entire log record (including otherwise valid fields) to be dropped; normal Logrus field APIs instead omit that value and emitlogrus_error. Route the flattened fields throughEntry.WithFields(or provide equivalent validation/error propagation) before logging.
fields[key] = attr.Value.Any()
Yup, considered that one; for now I think it's too much of a corner case (and reason for complexity in logrus itself); for now I don't think we need to bother with that. |
|
I'm gonna squash the commits; changes look good now 👍 (will do a follow-up for the |
Complete the second half of #1401 by providing a slog.Handler bridge into Logrus, complementary to the existing Logrus→slog hook. - Map slog levels to logrus levels (customizable via Handler.LevelMapper) - Pass attributes as logrus fields; flatten groups with "." prefixes - Preserve context and record time - Enabled defers to the underlying logger's level - Preserve Logrus's extended Trace, Fatal, and Panic levels using the same custom slog level boundaries as Hook, and document the mapping directly on Handler. Co-authored-by: sonnemusk <sonnemusk@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
826addf to
ea7cc5b
Compare
Other half of #1401: slog.Handler that writes into a logrus.Logger. Reverse of the hook from #1407.
Handler.LevelMapper).(same idea as slog.TextHandler)tests for mapping, custom mapper, level filter, WithAttrs/WithGroup, inline groups, ctx/time, nil logger panic.
Closes #1401
(same patch as #1551 — closed that one by mistake, reopening here)