-
Notifications
You must be signed in to change notification settings - Fork 2
feat!: decouple public logger from lib-observability (fixes #24) #27
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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} | ||
|
Comment on lines
+38
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π©Ί 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
π Affects 2 files
π€ Prompt for AI Agents |
||
| } | ||
|
|
||
| // 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 } | ||
| 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())) | ||
| } |
| 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) } |
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 | π Major | β‘ Quick win
π§© Analysis chain
π Script executed:
Repository: LerianStudio/lib-service-discovery
Length of output: 32276
π Script executed:
Repository: LerianStudio/lib-service-discovery
Length of output: 29783
Handle typed-nil loggers as nil.
toObsLoggeronly treats a nil interface as silent; avar l *slog.Logger = nilstill wraps and can panic on the first log call becauseslog.Loggermethods dereference the receiver. Handle the underlying nil case here and add a regression test.π€ Prompt for AI Agents