-
Notifications
You must be signed in to change notification settings - Fork 2
feat!: decouple public logger from lib-observability (fixes #24) #26
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
Closed
gandalf-at-lerian
wants to merge
2
commits into
LerianStudio:develop
from
gandalf-at-lerian:fix/logger-abstraction-leak-24
Closed
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| package libsd | ||
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
|
|
||
| "github.com/LerianStudio/lib-observability/log" | ||
| ) | ||
|
|
||
| // Logger is the minimal, version-agnostic structured-logging contract that | ||
| // lib-service-discovery accepts from consumers. It uses only stdlib types and | ||
| // mirrors the context-aware methods of *slog.Logger, so any slog-compatible | ||
| // logger (including the stdlib *slog.Logger) satisfies it directly β with no | ||
| // dependency on lib-observability and no adapter wrapper on the caller side. | ||
| // | ||
| // The variadic args follow slog semantics: alternating key/value pairs | ||
| // (e.g. "consul", addr) or slog.Attr values. | ||
| type Logger interface { | ||
| InfoContext(ctx context.Context, msg string, args ...any) | ||
| WarnContext(ctx context.Context, msg string, args ...any) | ||
| ErrorContext(ctx context.Context, msg string, args ...any) | ||
| DebugContext(ctx context.Context, msg string, args ...any) | ||
| } | ||
|
|
||
| // obsLoggerAdapter bridges a consumer-supplied libsd.Logger to the | ||
| // lib-observability log.Logger contract used internally by the registry and | ||
| // resolvers, so no internal call site changes. lib-observability remains a | ||
| // private implementation detail and never appears in the public API. | ||
| type obsLoggerAdapter struct { | ||
| l Logger | ||
| attrs []any | ||
| groups []string | ||
| } | ||
|
|
||
| // toObsLogger wraps a public libsd.Logger as an internal log.Logger. A nil | ||
| // logger yields the lib-observability no-op logger, preserving the prior | ||
| // "nil logger is silent" behaviour. | ||
| func toObsLogger(l Logger) log.Logger { | ||
| if l == nil { | ||
| return log.NewNop() | ||
| } | ||
|
|
||
| return &obsLoggerAdapter{l: l} | ||
| } | ||
|
|
||
| // fieldsToArgs converts lib-observability Fields into slog-style key/value | ||
| // args, qualifying keys with the active group prefix. | ||
| func fieldsToArgs(prefix string, fields []log.Field) []any { | ||
| out := make([]any, 0, len(fields)*2) | ||
|
|
||
| for _, f := range fields { | ||
| key := f.Key | ||
| if prefix != "" { | ||
| key = prefix + "." + f.Key | ||
| } | ||
|
|
||
| out = append(out, key, f.Value) | ||
| } | ||
|
|
||
| return out | ||
| } | ||
|
|
||
| func (a *obsLoggerAdapter) Log(ctx context.Context, level log.Level, msg string, fields ...log.Field) { | ||
| prefix := strings.Join(a.groups, ".") | ||
| args := append(append([]any{}, a.attrs...), fieldsToArgs(prefix, fields)...) | ||
|
|
||
| switch level { | ||
| case log.LevelError: | ||
| a.l.ErrorContext(ctx, msg, args...) | ||
| case log.LevelWarn: | ||
| a.l.WarnContext(ctx, msg, args...) | ||
| case log.LevelDebug: | ||
| a.l.DebugContext(ctx, msg, args...) | ||
| default: // LevelInfo and any unknown level default to info. | ||
| a.l.InfoContext(ctx, msg, args...) | ||
| } | ||
| } | ||
|
|
||
| func (a *obsLoggerAdapter) With(fields ...log.Field) log.Logger { | ||
| prefix := strings.Join(a.groups, ".") | ||
|
|
||
| return &obsLoggerAdapter{ | ||
| l: a.l, | ||
| attrs: append(append([]any{}, a.attrs...), fieldsToArgs(prefix, fields)...), | ||
| groups: a.groups, | ||
| } | ||
| } | ||
|
|
||
| func (a *obsLoggerAdapter) WithGroup(name string) log.Logger { | ||
| if name == "" { | ||
| return a | ||
| } | ||
|
|
||
| return &obsLoggerAdapter{ | ||
| l: a.l, | ||
| attrs: append([]any{}, a.attrs...), | ||
| groups: append(append([]string{}, a.groups...), name), | ||
| } | ||
| } | ||
|
|
||
| // Enabled always reports true: level filtering is delegated to the underlying | ||
| // slog-compatible logger supplied by the consumer. | ||
| func (a *obsLoggerAdapter) Enabled(_ log.Level) bool { return true } | ||
|
|
||
| // Sync is a no-op: the underlying slog-compatible logger manages its own | ||
| // flushing, and the libsd.Logger contract does not expose a sync hook. | ||
| func (a *obsLoggerAdapter) Sync(_ context.Context) error { return nil } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| //go:build unit | ||
|
|
||
| package libsd | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/LerianStudio/lib-observability/log" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // recordingLogger is a libsd.Logger that captures the level, message and args | ||
| // of each call, so the adapter's level mapping and field translation can be | ||
| // asserted. | ||
| type recordingLogger struct { | ||
| level string | ||
| msg string | ||
| args []any | ||
| } | ||
|
|
||
| func (r *recordingLogger) InfoContext(_ context.Context, msg string, args ...any) { | ||
| r.level, r.msg, r.args = "info", msg, args | ||
| } | ||
|
|
||
| func (r *recordingLogger) WarnContext(_ context.Context, msg string, args ...any) { | ||
| r.level, r.msg, r.args = "warn", msg, args | ||
| } | ||
|
|
||
| func (r *recordingLogger) ErrorContext(_ context.Context, msg string, args ...any) { | ||
| r.level, r.msg, r.args = "error", msg, args | ||
| } | ||
|
|
||
| func (r *recordingLogger) DebugContext(_ context.Context, msg string, args ...any) { | ||
| r.level, r.msg, r.args = "debug", msg, args | ||
| } | ||
|
|
||
| func TestToObsLogger_NilYieldsNop(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // A nil public logger must produce a usable, silent internal logger. | ||
| got := toObsLogger(nil) | ||
| require.NotNil(t, got) | ||
| assert.NotPanics(t, func() { | ||
| got.Log(context.Background(), log.LevelInfo, "silent") | ||
| }) | ||
| } | ||
|
|
||
| func TestObsLoggerAdapter_LevelMapping(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cases := []struct { | ||
| level log.Level | ||
| want string | ||
| }{ | ||
| {log.LevelError, "error"}, | ||
| {log.LevelWarn, "warn"}, | ||
| {log.LevelInfo, "info"}, | ||
| {log.LevelDebug, "debug"}, | ||
| {log.LevelUnknown, "info"}, // unknown levels default to info | ||
| } | ||
|
|
||
| for _, tc := range cases { | ||
| rec := &recordingLogger{} | ||
| toObsLogger(rec).Log(context.Background(), tc.level, "msg") | ||
| assert.Equal(t, tc.want, rec.level, "level %v", tc.level) | ||
| } | ||
| } | ||
|
|
||
| func TestObsLoggerAdapter_FieldsBecomeSlogArgs(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| rec := &recordingLogger{} | ||
| toObsLogger(rec).Log(context.Background(), log.LevelInfo, "resolved", | ||
| log.String("service", "svc-a"), log.Int("count", 3)) | ||
|
|
||
| assert.Equal(t, "resolved", rec.msg) | ||
| assert.Equal(t, []any{"service", "svc-a", "count", 3}, rec.args) | ||
| } | ||
|
|
||
| func TestObsLoggerAdapter_WithAndWithGroup(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| rec := &recordingLogger{} | ||
|
|
||
| l := toObsLogger(rec). | ||
| With(log.String("base", "b")). | ||
| WithGroup("grp") | ||
| l.Log(context.Background(), log.LevelWarn, "m", log.String("k", "v")) | ||
|
|
||
| // Accumulated With attrs come first; grouped keys are dotted. | ||
| assert.Equal(t, []any{"base", "b", "grp.k", "v"}, rec.args) | ||
| assert.True(t, l.Enabled(log.LevelDebug)) | ||
| assert.NoError(t, l.Sync(context.Background())) | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| //go:build unit || integration | ||
|
|
||
| package libsd | ||
|
|
||
| import "log/slog" | ||
|
|
||
| // nopLogger returns a silent, slog-compatible libsd.Logger for tests that do | ||
| // not assert on emitted messages. Shared by the unit and integration suites. | ||
| func nopLogger() Logger { return slog.New(slog.DiscardHandler) } |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
π©Ί Stability & Availability | π‘ Minor | β‘ Quick win
π§© Analysis chain
π Script executed:
Repository: LerianStudio/lib-service-discovery
Length of output: 8461
π Script executed:
Repository: LerianStudio/lib-service-discovery
Length of output: 8451
π Script executed:
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:
slog.DiscardHandlerΒ golang/go#62005Handle typed-nil public loggers before wrapping them. A nil
*slog.Loggercan still satisfyLoggeras a non-nil interface value, sotoObsLoggerwill wrap it and the firstLogcall 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