slog: rewrite level synchronization with Logrus - #21
Open
thaJeztah wants to merge 4 commits into
Open
Conversation
thaJeztah
commented
Aug 13, 2026
Comment on lines
36
to
42
| slogOnce.Do(func() { | ||
| L.Logger.SetNoLock() | ||
| L.Logger.AddHook(slogHook{}) | ||
| L.Logger.SetFormatter(discardFormatter{}) | ||
| slogOut = L.Logger.Out | ||
| L.Logger.SetOutput(io.Discard) | ||
| slogLevel.Set(logrusToSlogLevel(L.Logger.GetLevel())) | ||
| }) |
Member
Author
There was a problem hiding this comment.
Potentially this could also do the reverse, and use the Handler that logrus has if we want to keep logrus for the formatting;
slog.SetDefault(slog.New(
logrusslog.NewHandler(L.Logger, nil),
))
thaJeztah
force-pushed
the
rewrite_slog
branch
3 times, most recently
from
August 14, 2026 17:45
ce8c72f to
7fd77f3
Compare
thaJeztah
marked this pull request as ready for review
August 14, 2026 17:46
thaJeztah
commented
Aug 14, 2026
| slogOnce.Do(func() { | ||
| L.Logger.SetNoLock() | ||
| L.Logger.AddHook(slogHook{}) | ||
| L.Logger.SetFormatter(discardFormatter{}) |
Member
Author
There was a problem hiding this comment.
Noticed that we logrus was still configured to do the formatting, even it it wasn't used.
Use the Logrus slog level adapter as a dynamic slog.Leveler instead of maintaining a separate slog.LevelVar and duplicating the Logrus-to-slog level mapping. This keeps the Logrus logger as the single source of truth for the configured level, so changes made through SetLevel or directly on L.Logger are reflected by slog handlers automatically. Also avoid formatting Logrus entries that are forwarded to slog and then discarded. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
rewrite_slog
branch
from
August 14, 2026 18:15
7fd77f3 to
5a292b7
Compare
When slog output is enabled, SetFormat reconfigures the slog handler but could leave logrus formatting or output enabled if either had been changed after UseSlog. Reset the logrus formatter and output when updating the slog format so logrus remains only the forwarding path and does not duplicate work or output. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Remove TestLogrusToSlogLevel now that containerd uses the level adapter from logrus/hooks/slog directly. The mapping itself is covered by the Logrus package, while containerd's integration tests verify that forwarded records use the expected slog levels and that filtering follows the configured Logrus level. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Verify that Logrus records are forwarded with the expected slog levels, and that slog filtering follows changes to the Logrus logger level. Cover both SetLevel and direct changes to L.Logger to ensure the dynamic level adapter remains in sync without separate level state. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
rewrite_slog
branch
from
August 14, 2026 18:18
5a292b7 to
683ef7d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
slog: rewrite level synchronization with Logrus
Use the Logrus slog level adapter as a dynamic slog.Leveler instead of
maintaining a separate slog.LevelVar and duplicating the Logrus-to-slog
level mapping.
This keeps the Logrus logger as the single source of truth for the configured
level, so changes made through SetLevel or directly on L.Logger are reflected
by slog handlers automatically.
Also avoid formatting Logrus entries that are forwarded to slog and then
discarded.
slog: remove duplicate level mapping test
Remove TestLogrusToSlogLevel now that containerd uses the level adapter from
logrus/hooks/slog directly.
The mapping itself is covered by the Logrus package, while containerd's
integration tests verify that forwarded records use the expected slog levels
and that filtering follows the configured Logrus level.
slog: improve bridge test coverage
Verify that Logrus records are forwarded with the expected slog levels, and
that slog filtering follows changes to the Logrus logger level.
Cover both SetLevel and direct changes to L.Logger to ensure the dynamic level
adapter remains in sync without separate level state.