-
Notifications
You must be signed in to change notification settings - Fork 54
fix: go-libp2p and slog interop #176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
go-libp2p migrated from go-log to slog, breaking dynamic level control via SetLogLevel(). This adds automatic slog integration that routes slog logs through go-log's zap core when SetupLogging() is called. This restores runtime level adjustments for go-libp2p subsystems without requiring daemon restarts, making it possible to debug production issues by changing log levels on the fly. The integration detects subsystem names via logger attributes and applies per-subsystem level control. Libraries can opt-in via duck typing to avoid adding go-log as a dependency (see go-libp2p's gologshim implementation). Users can disable this behavior with GOLOG_CAPTURE_DEFAULT_SLOG=false. Prerequisite for addressing ipfs/kubo#11035
The migration to slog in #3364 broke go-log's ability to adjust subsystem log levels at runtime via SetLogLevel() and control output formatting (colors, json). This was a key debugging feature that allowed changing log verbosity without restarting daemons. This fix detects go-log's slog bridge via duck typing and uses it when available, restoring dynamic level control and unified formatting. When go-log isn't present, gologshim falls back to standalone slog handlers as before. The lazy handler pattern solves initialization order issues where package-level loggers are created before go-log's bridge is installed. Users just need to update to this version of go-libp2p and go-log with ipfs/go-log#176 - no code changes or init() hacks required. Prerequisite for addressing ipfs/kubo#11035
tests were modifying global state without restoring it, causing failures when run with -shuffle on some platforms
tests were not restoring the global defaultLevel variable, causing race conditions when getOrCreateAtomicLevel() created new subsystem levels using the stale defaultLevel from previous tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is brilliant!
|
Triage notes
|
|
I've not reviewed this PR. Linking this in case you hadn't seen it: https://pkg.go.dev/go.uber.org/zap/exp/zapslog. |
add SlogHandler() function that returns go-log's slog.Handler directly. this allows applications to wire slog-based libraries to go-log even when GOLOG_CAPTURE_DEFAULT_SLOG=false. the bridge is now always created during SetupLogging(), but only installed as slog.Default() when automatic capture is enabled. use atomic.Pointer[slog.Handler] for thread-safe access without locks, following idiomatic Go patterns for single-writer, multiple-reader scenarios. benefits: - works when GOLOG_CAPTURE_DEFAULT_SLOG=false - more explicit API (no indirection through slog.Default()) - simpler application code (no duck-typing verification needed) - thread-safe without mutex overhead addresses feedback from libp2p/go-libp2p#3419
|
@MarcoPolo thx, we've seen it, iirc ended up being not enough, thats why go-log has own opinionated slog bridge. Update: found my notes from when i was researching our options here:
|
|
Since go-libp2p decided to not use automatic bridge described in libp2p/go-libp2p#3419 (review), and go-libp2p 0.45 (libp2p/go-libp2p#3424) will require every end application that uses both import (
logging "github.com/ipfs/go-log/v2"
"github.com/libp2p/go-libp2p/gologshim"
)
func init() {
// use the explicit go-log bridge
gologshim.SetDefaultHandler(golog.SlogHandler())
}.. I do not think it makes sense to set Now that every + slog.SetDefault(slog.New(golog.SlogHandler()))
gologshim.SetDefaultHandler(golog.SlogHandler())I will keep Update: done in b453280 |
change GOLOG_CAPTURE_DEFAULT_SLOG to default to false, requiring explicit opt-in for automatic slog.Default() capture. applications wanting slog integration must now explicitly call slog.SetDefault(slog.New(golog.SlogHandler())). rationale: - go-libp2p decided to use explicit wiring (gologshim.SetDefaultHandler) rather than relying on automatic slog.Default() capture - automatic capture was the original motivation for default=true, but is no longer needed - makes go-log a better Go library by not modifying global state by default - follows Go best practice: libraries should not call slog.SetDefault() - opt-in is better than opt-out for invasive features changes: - setup.go: change condition from != "false" to == "true" - README.md: updated documentation to show explicit setup pattern, reposition GOLOG_CAPTURE_DEFAULT_SLOG as development/opt-in feature - tests: updated expectations to match new default behavior references: - #176 (comment) - libp2p/go-libp2p#3419 (review)
update to go-log v2.8.3-0.20251105220843-b453280b0ce2 which changes GOLOG_CAPTURE_DEFAULT_SLOG to opt-in (default=false). kubo now explicitly calls slog.SetDefault(slog.New(logging.SlogHandler())) to integrate slog with go-log's formatting and level control. changes: - cmd/ipfs/kubo/start.go: add slog.SetDefault() call for application-wide slog integration, maintaining existing gologshim.SetDefaultHandler() for go-libp2p subsystem attribution - docs/environment-variables.md: remove GOLOG_LOG_LABELS and GOLOG_CAPTURE_DEFAULT_SLOG sections (no longer relevant to kubo users) - go.mod: update go-log to b453280b0ce2 while automatic slog.Default() capture was convenient, go-libp2p chose to require explicit gologshim registration for clarity. for consistency, we now do the same for slog.Default() - making both integration points explicit rather than mixing automatic and manual approaches. references: - ipfs/go-log#176 (comment) - ipfs/go-log@b453280
|
All green in
I'm going to merge this and then make go-log release to ensure it is available before go-libp2p 0.45 (libp2p/go-libp2p#3424) ships |
update SlogHandler() godoc to match kubo's usage pattern, showing both: 1. slog.SetDefault() for application-wide integration 2. gologshim.SetDefaultHandler() for go-libp2p subsystem attribution this provides a complete example of explicit slog integration rather than just showing the library-specific wiring.
Problem
go-libp2p migrated from go-log to slog in
Unfortunately, the gologshim from that PR was not enough. go-libp2p. 0.44 breaks dynamic level control via
SetLogLevel()which in turn broke things likeipfs log leveland the Diagnostic screen from ipfs-webui (ipfs/ipfs-webui#2392) which allows users to dynamically adjust log level per system to investigate bugs.Solution
This PR adds automatic slog integration that routes slog logs through go-log's zap core when
SetupLogging()is called.This restores runtime level adjustments for go-libp2p subsystems without requiring daemon restarts, making it possible to debug production issues by changing log levels on the fly.
The integration detects subsystem names via logger attributes and applies per-subsystem level control. Libraries can opt-in via duck typing to avoid adding go-log as a dependency
(see go-libp2p'sgologshimimplementation).Important
Original version of this PR based on turn-key runtime detection was rejected by go-libp2p maintainers, and instead go-libp2p 0.45 requires go-log users to manually set up wiring. See https://github.com/ipfs/go-log/releases/tag/v2.9.0 and libp2p/go-libp2p#3419 (comment)
Context
ipfs log tailkubo#11035