feat!: decouple public logger from lib-observability (fixes #24)#26
feat!: decouple public logger from lib-observability (fixes #24)#26gandalf-at-lerian wants to merge 2 commits into
Conversation
Requested by: Guilherme Rodrigues (fixes LerianStudio#24 - logger abstraction leak) The public API leaked lib-observability's log.Logger through Config.Logger and WithLogger(), forcing consumers to pin the exact lib-observability version or hand-roll an adapter. - Add a minimal, stdlib-only, slog-compatible libsd.Logger interface (InfoContext/WarnContext/ErrorContext/DebugContext). - Switch Config.Logger and WithLogger() to libsd.Logger. - Add an internal obsLoggerAdapter bridging libsd.Logger -> lib-observability log.Logger, so no internal call site changes; lib-observability stays an implementation detail. - Remove lib-observability from the public API surface. - Update the demo, docs and tests to use *slog.Logger. BREAKING CHANGE: Config.Logger and WithLogger() now take libsd.Logger instead of lib-observability's log.Logger. A log.Logger no longer satisfies the API (it uses Log(ctx, level, msg, ...Field)); pass any slog-compatible logger (e.g. *slog.Logger) directly, with no lib-observability dependency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces a local ChangesLogger API migration
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
managed_resolver_test.go (1)
146-210: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMake these modified unit tests parallel-safe.
These tests do not call
t.Parallel(). Because they inspect process-wide goroutine counts, first replace that global measurement with scoped synchronization/cleanup that remains isolated under parallel execution, then callt.Parallel().As per coding guidelines, “Every test and subtest must call
t.Parallel().”Also applies to: 222-260, 540-585
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@managed_resolver_test.go` around lines 146 - 210, Update TestManagedResolver_CloseDuringInFlightSeedDoesNotLeak and the other modified tests to call t.Parallel(), replacing process-wide runtime.NumGoroutine baseline/assertions with test-scoped synchronization and cleanup that verifies watcher shutdown without relying on global goroutine counts. Preserve the existing bounded waits and leak-detection behavior while ensuring parallel tests cannot observe or affect one another.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@logger.go`:
- Around line 38-43: The toObsLogger function must detect typed-nil Logger
implementations before creating obsLoggerAdapter; add the appropriate typed-nil
check while preserving the existing plain-nil behavior. In logger_test.go,
extend the existing nil logger coverage with a regression test using a typed-nil
*slog.Logger and verify it produces the no-op logger without panicking.
---
Outside diff comments:
In `@managed_resolver_test.go`:
- Around line 146-210: Update
TestManagedResolver_CloseDuringInFlightSeedDoesNotLeak and the other modified
tests to call t.Parallel(), replacing process-wide runtime.NumGoroutine
baseline/assertions with test-scoped synchronization and cleanup that verifies
watcher shutdown without relying on global goroutine counts. Preserve the
existing bounded waits and leak-detection behavior while ensuring parallel tests
cannot observe or affect one another.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 092ef718-8b21-4962-9667-9e4875e271a0
📒 Files selected for processing (12)
README.mdclose_test.goconfig.goconsul_dual_integration_test.goconsul_integration_test.gologger.gologger_test.gologger_testhelpers_test.gomanaged_resolver_test.gomanager.gomanager_test.goservices/main.go
| func toObsLogger(l Logger) log.Logger { | ||
| if l == nil { | ||
| return log.NewNop() | ||
| } | ||
|
|
||
| return &obsLoggerAdapter{l: l} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== logger.go ==\n'
sed -n '1,180p' logger.go
printf '\n== logger_test.go ==\n'
sed -n '1,140p' logger_test.go
printf '\n== search for Logger interface/type defs ==\n'
rg -n "type Logger|toObsLogger|obsLoggerAdapter|slog.Logger|NewNop" -S .Repository: LerianStudio/lib-service-discovery
Length of output: 8461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== logger.go ==\n'
sed -n '1,220p' logger.go
printf '\n== logger_test.go ==\n'
sed -n '1,220p' logger_test.go
printf '\n== logger-related declarations ==\n'
rg -n "type Logger|obsLoggerAdapter|toObsLogger|NewNop|slog.Logger" -S --glob '!**/vendor/**' .Repository: LerianStudio/lib-service-discovery
Length of output: 8451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "type Logger|toObsLogger|obsLoggerAdapter|NewNop|slog.Logger" -S .Repository: LerianStudio/lib-service-discovery
Length of output: 2473
🌐 Web query:
Go standard library slog.Logger nil receiver panic InfoContext nil pointer method behavior
💡 Result:
Calling methods on a nil *slog.Logger will cause a runtime panic [1]. In Go, calling a method on a nil pointer is technically permitted by the language specification [2][3], but it only succeeds if the method does not dereference the receiver [4][3]. The methods of *slog.Logger (such as Info, InfoContext, Log, etc.) dereference the receiver to access its underlying handler [5][6][7]. Because these methods internally invoke l.Handler or otherwise access the Logger's fields, calling them on a nil *slog.Logger results in a nil pointer dereference panic [1]. Regarding nil contexts passed to methods like InfoContext: - Passing a nil context.Context to slog methods is discouraged [8][9], even though it is technically permitted by the function signatures [10]. - Internally, slog handles nil contexts by converting them into context.Background before performing any operations [6][7][9]. This behavior is designed for resilience [8], but developers are still advised to provide a valid context whenever one is available to ensure proper tracing and cancellation propagation [10][11]. In summary, while you should not pass a nil context, the slog package handles that gracefully [6][7][9]; however, a nil *slog.Logger itself is not usable and will cause a crash if any logging method is called on it [1].
Citations:
- 1: log/slog: add
slog.DiscardHandlergolang/go#62005 - 2: https://groups.google.com/g/golang-nuts/c/wcrZ3P1zeAk/m/WI88iQgFMvwJ
- 3: https://stackoverflow.com/questions/42238624/calling-a-method-on-a-nil-struct-pointer-doesnt-panic-why-not
- 4: https://go101.org/article/nil.html
- 5: https://go.dev/src/log/slog/logger.go
- 6: https://github.com/golang/go/blob/master/src/log/slog/logger.go
- 7: https://go.dev/src/log/slog/logger.go?m=text
- 8: log/slog: documentation passes nil contexts, which is discouraged golang/go#61219
- 9: https://git.jordan.im/go/commit/?h=go1.21.2&id=651869716a449a1868f5a5333796ab47482d7c65
- 10: https://pkg.go.dev/log/slog
- 11: https://go.dev/src/log/slog/doc.go
Handle typed-nil public loggers before wrapping them. A nil *slog.Logger can still satisfy Logger as a non-nil interface value, so toObsLogger will wrap it and the first Log call will panic. Add a typed-nil regression test alongside the existing nil case.
📍 Affects 2 files
logger.go#L38-L43(this comment)logger_test.go#L39-L48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@logger.go` around lines 38 - 43, The toObsLogger function must detect
typed-nil Logger implementations before creating obsLoggerAdapter; add the
appropriate typed-nil check while preserving the existing plain-nil behavior. In
logger_test.go, extend the existing nil logger coverage with a regression test
using a typed-nil *slog.Logger and verify it produces the no-op logger without
panicking.
2 participants
Requested by: Guilherme Rodrigues (fixes #24 - logger abstraction leak)
🎯 Release target: v1.2.0 (first tag:
v1.2.0-beta.1)v1.2.0(pre-v1.0compat is notguaranteed and the module is still
v1, so no module path change and no/v2suffix: the import path staysgithub.com/LerianStudio/lib-service-discovery).log.Loggeryou pass toWithLogger()or set onConfig.Loggerwith a*slog.Logger(e.g.slog.Default()or anyslog-compatible logger). No adapter wrapper is needed on the caller side.of this lib — you pass a stdlib
*slog.Loggerdirectly. (lib-observabilityremains an internal implementation detail of this lib, bridged by a private
adapter, and never appears in the public API.)
CHANGELOG.mdupdated with aBREAKINGentry under the unreleased/1.2.0section.Problem
The public API leaked
lib-observability'slog.LoggerthroughConfig.LoggerandWithLogger(). Becauselog.Logger's method set references concretelib-observabilitytypes (log.Level,log.Field), consumers had to pin the exactlib-observabilityversion or hand-roll an adapter wrapper.Fix
libsd.Logger— minimal, stdlib-only,slog-compatible:InfoContext/WarnContext/ErrorContext/DebugContext(ctx, msg, ...any).Config.LoggerandWithLogger()now takelibsd.Logger.obsLoggerAdapterbridgeslibsd.Logger→lib-observabilitylog.Logger, so no internal call site changes;lib-observabilitystays a private implementation detail.lib-observabilityremoved from the public API surface.services/main.go), README and tests updated to use*slog.Logger.Config.Logger/WithLogger()no longer acceptlib-observability'slog.Logger(it usesLog(ctx, level, msg, ...Field)and does not structurally satisfy the new interface). Consumers now pass anyslog-compatible logger (e.g. the stdlib*slog.Logger) directly, with nolib-observabilitydependency or adapter. This is precisely what removes the coupling.Changed files
logger.go(new) —Loggerinterface + internal adapterlogger_test.go,logger_testhelpers_test.go(new) — adapter/level/field tests + shared nop loggerconfig.go,manager.go— public types switch tolibsd.Loggerservices/main.go— demo uses*slog.LoggerREADME.md— documents the interface + breaking changeCHANGELOG.md— BREAKING entry for the logger decoupling (v1.2.0)captureLogger/nop logger to the new interfaceVerification
go build ./...— OKgo vet -tags unit ./...andgo vet -tags integration ./...— cleango test -tags unit ./...— all passgolangci-lint run --build-tags unit ./...— 0 issuesgithub.com/LerianStudio/lib-service-discovery, no/v2)