fix: define local Logger interface to remove lib-observability type leak#25
Closed
gandalf-at-lerian wants to merge 1 commit into
Closed
Conversation
Introduce a minimal, version-agnostic libsd.Logger interface (slog-style context methods) and switch Config.Logger and WithLogger to it. Internally adapt to lib-observability's log.Logger via a thin adapter so registry and resolver call sites are unchanged. Consumers can now pass any structured logger (e.g. *slog.Logger) without coupling to a lib-observability version. Closes LerianStudio#24
📝 WalkthroughWalkthroughChangesLogger abstraction and migration
Sequence Diagram(s)sequenceDiagram
participant Service
participant Manager
participant observabilityAdapter
participant slogLogger
Service->>Manager: WithLogger(slogLogger)
Manager->>observabilityAdapter: toInternalLogger
Manager->>observabilityAdapter: Emit log event
observabilityAdapter->>slogLogger: InfoContext/ErrorContext with fields
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: 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 `@logger.go`:
- Around line 51-56: Use one shared typed-nil-safe logger check in logger.go at
toInternalLogger and config.go at withDefaults: detect nil interfaces and
interfaces containing nil pointers, and apply the check in both locations so
nil-like loggers consistently fall back to the no-op logger.
In `@manager_test.go`:
- Around line 18-33: Update captureLogger and its InfoContext, WarnContext,
ErrorContext, and DebugContext methods to retain structured arguments and the
originating log level instead of recording only msg. Ensure adapter tests can
assert both field preservation and correct level routing, either by extending
the captured representation or adding dedicated assertions.
In `@README.md`:
- Around line 70-79: Add a concise migration note near the README logger
interface and functional-options documentation stating that existing
lib-observability/log.Logger values no longer implement libsd.Logger. Direct
callers to provide a *slog.Logger or use an adapter, while preserving the
existing injection and NewNopLogger guidance.
🪄 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: b54fff22-5b94-4254-af66-9c9acb04a149
📒 Files selected for processing (10)
README.mdclose_test.goconfig.goconsul_dual_integration_test.goconsul_integration_test.gologger.gomanaged_resolver_test.gomanager.gomanager_test.goservices/main.go
Comment on lines
+51
to
+56
| func toInternalLogger(l Logger) log.Logger { | ||
| if l == nil { | ||
| return log.NewNop() | ||
| } | ||
|
|
||
| return &observabilityAdapter{inner: l} |
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 '(^|/)(logger\.go|config\.go)$'
printf '\n== logger.go outline ==\n'
ast-grep outline logger.go --view expanded || true
printf '\n== config.go outline ==\n'
ast-grep outline config.go --view expanded || true
printf '\n== Relevant lines: logger.go ==\n'
nl -ba logger.go | sed -n '1,140p'
printf '\n== Relevant lines: config.go ==\n'
nl -ba config.go | sed -n '220,290p'
printf '\n== Search for Logger type and related helpers ==\n'
rg -n "type Logger|NewNopLogger|toInternalLogger|withDefaults|observabilityAdapter|isNil" .Repository: LerianStudio/lib-service-discovery
Length of output: 3092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fallback if the previous request didn't return results yet: inspect the files directly.
printf '\n== logger.go ==\n'
nl -ba logger.go | sed -n '1,140p'
printf '\n== config.go ==\n'
nl -ba config.go | sed -n '220,290p'
printf '\n== Logger-related definitions ==\n'
rg -n "type Logger|func .*NewNopLogger|func .*toInternalLogger|func .*withDefaults|observabilityAdapter|isNil" logger.go config.goRepository: LerianStudio/lib-service-discovery
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pwd
git rev-parse --show-toplevel
git ls-files | head -n 50Repository: LerianStudio/lib-service-discovery
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== logger.go ==\n'
sed -n '1,140p' logger.go
printf '\n== config.go (selected) ==\n'
sed -n '230,280p' config.go
printf '\n== Logger usages and constructors ==\n'
grep -RIn --exclude-dir=.git -E 'Logger|NewNopLogger|toInternalLogger|withDefaults|observabilityAdapter' . | head -n 200Repository: LerianStudio/lib-service-discovery
Length of output: 16857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== logger.go ==\n'
sed -n '1,140p' logger.go
printf '\n== config.go (selected) ==\n'
sed -n '230,280p' config.go
printf '\n== Logger-related call sites ==\n'
grep -RIn --exclude-dir=.git -E 'NewNopLogger\(|toInternalLogger\(|withDefaults\(|Logger\s*[:=]' . | head -n 200Repository: LerianStudio/lib-service-discovery
Length of output: 8852
Use a typed-nil-safe logger fallback.
l == nil and c.Logger == nil miss interface values that wrap nil pointers, so a logger can slip through withDefaults() and later panic when observabilityAdapter.Log calls through it. Add one shared nil-like check and use it in both logger.go#L51-L56 and config.go#L251-L254.
📍 Affects 2 files
logger.go#L51-L56(this comment)config.go#L251-L254
🤖 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 51 - 56, Use one shared typed-nil-safe logger check
in logger.go at toInternalLogger and config.go at withDefaults: detect nil
interfaces and interfaces containing nil pointers, and apply the check in both
locations so nil-like loggers consistently fall back to the no-op logger.
Comment on lines
18
to
+33
| type captureLogger struct { | ||
| mu sync.Mutex | ||
| msgs []string | ||
| } | ||
|
|
||
| func (c *captureLogger) Log(_ context.Context, _ log.Level, msg string, _ ...log.Field) { | ||
| func (c *captureLogger) record(msg string) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| c.msgs = append(c.msgs, msg) | ||
| } | ||
|
|
||
| func (c *captureLogger) With(_ ...log.Field) log.Logger { return c } | ||
| func (c *captureLogger) WithGroup(_ string) log.Logger { return c } | ||
| func (c *captureLogger) Enabled(_ log.Level) bool { return true } | ||
| func (c *captureLogger) Sync(_ context.Context) error { return nil } | ||
| func (c *captureLogger) InfoContext(_ context.Context, msg string, _ ...any) { c.record(msg) } | ||
| func (c *captureLogger) WarnContext(_ context.Context, msg string, _ ...any) { c.record(msg) } | ||
| func (c *captureLogger) ErrorContext(_ context.Context, msg string, _ ...any) { c.record(msg) } | ||
| func (c *captureLogger) DebugContext(_ context.Context, msg string, _ ...any) { c.record(msg) } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Preserve structured fields and log levels in the capture logger.
All four methods collapse calls to record(msg), discarding arguments and method/level information. Adapter tests can therefore pass even if structured fields are dropped or levels are routed incorrectly; capture these values or add dedicated adapter assertions.
🤖 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 18 - 33, Update captureLogger and its
InfoContext, WarnContext, ErrorContext, and DebugContext methods to retain
structured arguments and the originating log level instead of recording only
msg. Ensure adapter tests can assert both field preservation and correct level
routing, either by extending the captured representation or adding dedicated
assertions.
Comment on lines
+70
to
+79
| - `Logger` — minimal structured-logging interface the library needs. It mirrors | ||
| the context-aware methods of the stdlib `*slog.Logger` | ||
| (`InfoContext`/`WarnContext`/`ErrorContext`/`DebugContext`), so any structured | ||
| logger can be injected without coupling the consumer to a specific | ||
| lib-observability version. Use `libsd.NewNopLogger()` to disable logging. | ||
|
|
||
| **Functional options:** | ||
|
|
||
| ```go | ||
| libsd.WithLogger(logger) // inject a lib-commons log.Logger | ||
| libsd.WithLogger(logger) // inject any logger implementing libsd.Logger (e.g. *slog.Logger) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the breaking logger migration.
The README explains the new interface but does not warn that existing lib-observability/log.Logger values no longer satisfy libsd.Logger. Add a short migration note directing callers to *slog.Logger or an adapter.
🤖 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 `@README.md` around lines 70 - 79, Add a concise migration note near the README
logger interface and functional-options documentation stating that existing
lib-observability/log.Logger values no longer implement libsd.Logger. Direct
callers to provide a *slog.Logger or use an adapter, while preserving the
existing injection and NewNopLogger guidance.
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
2 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.
Summary
Requested by: Rodrigo Schieck
Rodrigo reported that
lib-service-discoveryleaks thelib-observabilitylogging type through its public API (Config.LoggerandWithLoggerare typed aslog.Loggerfromgithub.com/LerianStudio/lib-observability/log). This forces every consumer to pin the exact samelib-observabilityversion as this library; using a different version causes a type incompatibility. On plugin-access-manager he had to write an adapter wrapper as a workaround.This PR removes that coupling from the public API.
What changed
libsd.Loggerinterface (logger.go): a minimal, version-agnostic structured-logging interface mirroring the context-aware methods of the stdlib*slog.Logger:Config.LoggerandWithLogger()now uselibsd.Loggerinstead oflog.Logger. Thelib-observabilityimport is gone from the public-facing types (config.go,manager.go).logger.go):observabilityAdapterwraps alibsd.Loggerand satisfieslib-observability'slog.Logger, so the registry/resolver internal call sites (consul.go,resolver.go,managed_resolver.go,manager.go) are unchanged.NewNopLogger()helper for an explicit no-op logger (also the default when none is supplied).services/main.go) switched to the stdlib*slog.Loggerto show the decoupled usage with nolib-observabilitydependency.Key types/ options updated. Unit + integration tests updated to the new interface (captureLoggernow implementslibsd.Logger).Files changed
logger.go(new),config.go,manager.go,services/main.go,README.md, and tests (manager_test.go,managed_resolver_test.go,close_test.go,consul_dual_integration_test.go,consul_integration_test.go).Do existing callers need changes?
Yes, this is a breaking API change.
lib-observability'slog.LoggerusesLog(ctx, level, msg, ...Field)and does not structurally satisfy the newslog-style interface, so callers that passed alib-observabilitylogger directly must now pass anslog-compatible logger (e.g.*slog.Logger) or a tiny 4-method adapter. This is the intended trade-off: it is exactly what eliminates the version coupling.Verification
go build ./...— passgo vet ./...andgo vet -tags integration ./...— passgo test -tags unit -race ./...— passCloses #24