feat!: decouple public logger from lib-observability (fixes #24)#27
Merged
Conversation
Requested by: Guilherme Rodrigues (fixes #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.
📝 WalkthroughWalkthroughThe library replaces its public observability logger dependency with a stdlib-compatible ChangesLogger decoupling
Sequence Diagram(s)sequenceDiagram
participant DemoService
participant libsd.New
participant Manager
participant slog.Logger
DemoService->>slog.Logger: create slog logger
DemoService->>libsd.New: pass logger through WithLogger
libsd.New->>Manager: initialize adapted logger
Manager->>slog.Logger: emit context-aware structured logs
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
lerian-studio
added
size/M
core
Contributor
🔒 Security Scan Results —
|
| Stage | Status | Blocking? |
|---|---|---|
| Filesystem Scan | ✅ Clean | — |
| Docker Image Scan | ➖ Skipped | — |
| Docker Hub Health Score | ➖ Skipped | — |
| Pre-release Version Check | ✅ Clean | — |
Trivy
Filesystem Scan
✅ No vulnerabilities or secrets found.
Pre-release Version Check
✅ No unstable version pins found.
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 91.4% ✅ PASS |
| Threshold | 80% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/lib-service-discovery |
93.2% |
Generated by Go PR Analysis workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@config.go`:
- Around line 172-174: Update toObsLogger to detect both a nil Logger interface
and an interface containing a typed-nil logger, returning the silent logger
behavior in either case before wrapping or invoking methods. Add a regression
test using a nil *slog.Logger assigned to Logger and verify logging does not
panic or emit output.
In `@logger.go`:
- Around line 38-43: The toObsLogger nil check does not handle typed-nil Logger
values, and manager construction paths must consistently use that guard. Update
toObsLogger to perform a nil-aware check before creating obsLoggerAdapter, then
route both WithLogger and New in manager.go through toObsLogger; apply the
changes at logger.go lines 38-43, manager.go lines 75-83, and manager.go lines
127-130.
In `@manager_test.go`:
- Around line 204-212: Update the logger assertion in the test around New and
obsLoggerAdapter to use assert.Same instead of assert.Equal, verifying that
adapter.l is the exact nop logger instance passed to WithLogger.
🪄 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: 398057fa-3c76-45bd-9310-9fe405801a5b
📒 Files selected for processing (13)
CHANGELOG.mdREADME.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
Comment on lines
+172
to
+174
| // It is a version-agnostic, slog-compatible interface (see libsd.Logger): | ||
| // pass any *slog.Logger or equivalent directly. A nil Logger silences output. | ||
| Logger Logger |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg '(^|/)(config\.go|logger\.go|.*test.*\.go)$'
printf '\n== AST outline: config.go ==\n'
ast-grep outline config.go --view expanded || true
printf '\n== AST outline: logger.go ==\n'
ast-grep outline logger.go --view expanded || true
printf '\n== Relevant symbols/text ==\n'
rg -n "toObsLogger|withDefaults|type Logger|Logger interface|InfoContext|slog.Logger|nil Logger|silences output|New\\(" config.go logger.go . -g '!**/vendor/**'
printf '\n== config.go excerpt ==\n'
sed -n '1,260p' config.go
printf '\n== logger.go excerpt ==\n'
sed -n '1,260p' logger.goRepository: LerianStudio/lib-service-discovery
Length of output: 32276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg '(^|/)(config\.go|logger\.go|.*test.*\.go)$'
printf '\n== config.go / logger.go symbols ==\n'
rg -n "toObsLogger|withDefaults|type Logger|InfoContext|slog.Logger|nil Logger|silences output|New\\(" config.go logger.go . -g '!**/vendor/**'
printf '\n== config.go excerpt ==\n'
sed -n '1,260p' config.go
printf '\n== logger.go excerpt ==\n'
sed -n '1,260p' logger.goRepository: LerianStudio/lib-service-discovery
Length of output: 29783
Handle typed-nil loggers as nil. toObsLogger only treats a nil interface as silent; a var l *slog.Logger = nil still wraps and can panic on the first log call because slog.Logger methods dereference the receiver. Handle the underlying nil case here and add a regression test.
🤖 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 `@config.go` around lines 172 - 174, Update toObsLogger to detect both a nil
Logger interface and an interface containing a typed-nil logger, returning the
silent logger behavior in either case before wrapping or invoking methods. Add a
regression test using a nil *slog.Logger assigned to Logger and verify logging
does not panic or emit output.
Comment on lines
+38
to
+43
| 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 | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant files and inspect the surrounding code.
git ls-files | rg '(^|/)(logger\.go|manager\.go|.*logger.*\.go)$'
echo '--- logger.go ---'
wc -l logger.go
cat -n logger.go | sed -n '1,220p'
echo '--- manager.go ---'
wc -l manager.go
cat -n manager.go | sed -n '1,220p'Repository: LerianStudio/lib-service-discovery
Length of output: 14102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the Logger interface and any typed-nil-related helper usages.
rg -n --hidden --glob '!**/vendor/**' 'type Logger interface|toObsLogger|WithLogger|NewNop|obsLoggerAdapter|slog\.Logger' .Repository: LerianStudio/lib-service-discovery
Length of output: 3467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the interface is defined elsewhere, inspect just that file and any adapter implementation.
files=$(rg -l --hidden --glob '!**/vendor/**' 'type Logger interface|obsLoggerAdapter|toObsLogger' . | sort -u)
printf '%s\n' "$files"
for f in $files; do
echo "--- $f ---"
wc -l "$f"
# Print only the relevant slices with line numbers.
cat -n "$f" | sed -n '1,220p'
doneRepository: LerianStudio/lib-service-discovery
Length of output: 25136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe: determine whether a typed-nil implementation stored in the Logger interface
# compares equal to nil and whether the adapter receives a non-nil interface.
python3 - <<'PY'
print("pending repository inspection")
PYRepository: LerianStudio/lib-service-discovery
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat >/tmp/typed_nil_probe.go <<'EOF'
package main
import "fmt"
type Logger interface {
InfoContext()
}
type impl struct{}
func (*impl) InfoContext() {
fmt.Println("called")
}
func main() {
var p *impl = nil
var l Logger = p
fmt.Printf("interface == nil: %v\n", l == nil)
fmt.Printf("dynamic type: %T\n", l)
defer func() {
if r := recover(); r != nil {
fmt.Printf("panic: %v\n", r)
}
}()
l.InfoContext()
}
EOF
go run /tmp/typed_nil_probe.goRepository: LerianStudio/lib-service-discovery
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Config.Logger contract and any nil-related documentation.
rg -n --hidden --glob '!**/vendor/**' 'Logger\s+Logger|Config\.Logger|nil Logger|WithLogger\(' config.go README.md CHANGELOG.md logger.go manager.goRepository: LerianStudio/lib-service-discovery
Length of output: 1002
Handle typed-nil Logger values before wrapping them.
l == nil only catches an untyped nil; a typed-nil Logger can still be wrapped and will panic on the first log call.
logger.go#L38-L43: add a nil-aware guard intoObsLogger.manager.go#L77-L83andmanager.go#L127-L130: route bothWithLoggerandNewthrough the same guard.
📍 Affects 2 files
logger.go#L38-L43(this comment)manager.go#L75-L83
🤖 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 nil check does not handle
typed-nil Logger values, and manager construction paths must consistently use
that guard. Update toObsLogger to perform a nil-aware check before creating
obsLoggerAdapter, then route both WithLogger and New in manager.go through
toObsLogger; apply the changes at logger.go lines 38-43, manager.go lines 75-83,
and manager.go lines 127-130.
Comment on lines
+204
to
+212
| nop := nopLogger() | ||
|
|
||
| m, err := New(Config{Enabled: false}, WithLogger(nop)) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, nop, m.logger) | ||
| // m.logger is the internal lib-observability adapter that bridges the | ||
| // public libsd.Logger; assert it wraps exactly the logger we passed. | ||
| adapter, ok := m.logger.(*obsLoggerAdapter) | ||
| require.True(t, ok) | ||
| assert.Equal(t, nop, adapter.l) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the test and the logger implementations.
git ls-files | rg 'manager_test\.go$|logger|observability|obsLoggerAdapter|nopLogger'
echo '--- manager_test.go ---'
sed -n '1,260p' manager_test.go
echo '--- search for obsLoggerAdapter and nopLogger ---'
rg -n 'type obsLoggerAdapter|func nopLogger|obsLoggerAdapter|nopLogger\(' .
echo '--- possible logger interface definitions ---'
rg -n 'type .*Logger interface|interface .*Logger' .Repository: LerianStudio/lib-service-discovery
Length of output: 9451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- logger.go ---'
sed -n '1,180p' logger.go
echo '--- logger_testhelpers_test.go ---'
sed -n '1,120p' logger_testhelpers_test.go
echo '--- testify assert Equal/Same usage nearby ---'
rg -n 'assert\.(Equal|Same)\(' manager_test.go logger_test.go logger_testhelpers_test.goRepository: LerianStudio/lib-service-discovery
Length of output: 8286
Assert logger identity, not deep equality.
assert.Equal(t, nop, adapter.l) can pass for a different *slog.Logger with equivalent fields, so the test doesn’t prove the exact instance was preserved. Use assert.Same(t, nop, adapter.l) instead.
🤖 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 `@manager_test.go` around lines 204 - 212, Update the logger assertion in the
test around New and obsLoggerAdapter to use assert.Same instead of assert.Equal,
verifying that adapter.l is the exact nop logger instance passed to WithLogger.
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
3 participants
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.
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)Closes #24