feat(observe/log): add a slog.Handler adapter - #665
Conversation
pkg/observe/log shipped adapters for logrus and zap but none for the
stdlib's log/slog, so any service whose own code calls slog.Default() was
silently disconnected from the platform logger: pkg/service builds a
logging.Logger from --debug and --json-formatting-logger and installs the
OTel hook that stamps trace_id/span_id, but slog.Default() stayed Go's
stock stderr text handler. Measured in connectivity-plugins: 58 non-test
files logging via slog.Default(), every line ignoring both flags and
carrying no trace_id, so requests were traced but their logs could not be
joined to the trace.
NewSlogHandler closes that gap with no call-site changes:
slog.SetDefault(slog.New(logging.NewSlogHandler(logger)))
Handle binds the logger to the call's context via WithContext, which is
what lets the existing logrus OTel hook read the active span; Enabled
consults the underlying logger so --debug actually gates slog debug calls;
groups are flattened into dotted keys, since Logger carries flat fields and
flat scalars index better downstream; a nil Logger discards rather than
panicking.
Tested with the stdlib's own conformance suite, testing/slogtest. It passes
with exactly one deviation, allow-listed by exact message so every other
case stays enforced and a second deviation fails the test: "a Handler
should ignore a zero Record.Time" cannot pass, because emission goes
through Logger's Info(args ...any) signature, which has no way to carry or
suppress a timestamp, and the backing logger always stamps its own.
Logger gains Warn and Warnf. A slog Warn otherwise had nowhere correct to
go: Error turns every warning into an alert, Info loses the severity. Both
bundled adapters already implemented these methods, so the omission from
the interface looks accidental. No WarnLevel is added to Level -- nothing
gates on warn, and it would renumber ErrorLevel.
BREAKING CHANGE: logging.Logger now declares Warn(args ...any) and
Warnf(fmt string, args ...any). In-tree adapters already had them;
out-of-tree implementations of the interface must add both.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #665 +/- ##
==========================================
+ Coverage 40.99% 42.20% +1.20%
==========================================
Files 191 192 +1
Lines 8481 8582 +101
==========================================
+ Hits 3477 3622 +145
+ Misses 4803 4753 -50
- Partials 201 207 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
The need for a slog.Handler adapter is well demonstrated: current slog.Default() call sites bypass the platform logger’s formatting, debug gating, and trace correlation. The adapter itself is carefully implemented: context binding, handler cloning, group flattening, level mapping, and the slogtest coverage all look sound. The targeted package tests and current CI pass.
The blocker is that this opt-in adapter unnecessarily makes the existing v5 Logger interface source-incompatible (inline comment). A service that never uses slog can stop compiling solely because one of its mocks/adapters lacks two new methods.
Please keep the feature non-breaking, for example by using a private optional warning capability in the slog adapter and falling back for older implementations, or reserve the interface expansion for a new major version.
One need/rollout question remains: this PR deliberately does not wire the handler into pkg/service, so merging it alone changes nothing for the 58 cited call sites. Which concrete consumer will install it first, and should that wiring be part of the acceptance criterion or an explicitly linked follow-up? Once the v5 compatibility issue is removed, I see no other blocker.
| Infof(fmt string, args ...any) | ||
| // Warnf, like Warn, exists so adapters that receive a warning from an | ||
| // upstream API have somewhere correct to put it. | ||
| Warnf(fmt string, args ...any) |
There was a problem hiding this comment.
Blocking — the slog adapter does not require a v5-wide interface break. Adding Warn/Warnf makes every out-of-tree Logger implementation, test double, and generated mock fail to compile, including consumers that never opt into slog. The bundled loggers already expose these methods, so the adapter can detect a private optional interface such as interface { Warn(...any) } and use it when available, with a documented fallback for legacy implementations. Otherwise this change belongs in a new major version. Please preserve source compatibility for /v5.
2 participants
What
Adds
logging.NewSlogHandler(logging.Logger) slog.Handlertopkg/observe/log, alongside the existing logrus and zap adapters.Existing
slogcall sites then gain the platform logger's formatting, level control and trace correlation with no changes.Why
pkg/servicebuilds alogging.Loggerfrom--debugand--json-formatting-logger, installs the OTel hook that stampstrace_id/span_id, and injects it into fx — but a service whose own code callsslog.Default()never touches any of it.slog.Default()stays Go's stock stderr text handler.Measured in one consumer (
formancehq/connectivity-plugins): 58 non-test files logging viaslog.Default(). Every one of those lines ignored--json-formatting-logger, ignored--debug, and carried notrace_id— requests were traced, but their logs could not be joined to the trace. That consumer wrote an adapter locally;slogis the stdlib default and new services will keep hitting this, so it belongs here.Design notes
Handlecallslogger.WithContext(ctx)before emitting. That is what lets the existing logrustraceHookread the active span — without it there is no trace correlation, which is most of the point. A context with no recording span adds no trace fields rather than empty ones.Enabledconsults the underlying logger, so--debugactually gates slog debug calls instead of every record being built and then dropped.slog.LevelWarnmaps toInfoLevelfor that check — deliberately permissive, since the backing logger filters again at emit, so an over-generous answer costs a wasted field build, never a suppressed line getting through.G.H.key) rather than nested maps.Loggercarries flat fields, and flat scalar keys are what log search backends index well. Worth a second opinion if you'd rather have nested maps.clone()copies the attrs slice.slogbranches handlers, and two branches appending into a shared backing array overwrite each other's attributes.Loggerdiscards rather than panicking — a logging call is the worst place to take a process down.LoggergainsWarn/WarnfA slog
Warnhad nowhere correct to go. The interface declaredTrace/Debug/Info/Error(+fvariants) but notWarn, whileLogrusLoggerandZapLoggerboth already implementedWarn/Warnf— the omission from the interface looks accidental. Routing warnings toErrorwould turn every warning into an alert; routing them toInfoloses the severity. So the interface now declares both, with the rationale in the source.Out-of-tree implementations of
logging.Loggermust addWarn(args ...any)andWarnf(fmt string, args ...any). In-tree adapters already had them; only the generated mock and three test doubles needed updating.No
WarnLevelwas added toLevel: nothing gates on warn, and it would renumberErrorLevel.This repo has no automated semantic-release, so the
BREAKING CHANGE:footer on the commit is documentary. Flagging for a versioning call per the usual process for interface breaks.Testing
testing/slogtest, the stdlib's own conformance suite — the strongest available check, covering group nesting, empty-attr elision, inline groups andLogValuerresolution.It passes with exactly one deviation, allow-listed by exact message rather than skipping the suite, so every other case stays enforced and a second deviation fails the test:
This one cannot pass. Emission goes through
logging.Logger, whose signature isInfo(args ...any)— there is no way to pass or suppress a timestamp, and the backing logger always stamps its own. That is correct behaviour in production and unreachable in practice:slog.Loggeralways setsRecord.Timefrom the clock, so only a hand-builtRecordhas a zero one. The test also fails if the deviation ever stops occurring, so the allow-list cannot go stale.Also covered: trace correlation end to end against a real recorded span (
sdktracewithAlwaysSample, assertingtrace_id/span_idmatchspan.SpanContext()); the no-span case; level gating with and without--debug; warn landing at warn and not error; the full level mapping; andWithAttrs/WithGroupbranch independence.I mutation-checked the suite rather than trusting a green run — sharing the attrs slice instead of cloning makes the branch test report
rightwhereleftwas expected, and dropping either the inline-group or the empty-Attr handling produces a slogtest failure. The tests bite.just pre-commitis clean (0 issues);go vet ./...and the tests forpkg/observe/...,pkg/transport/httpclient,pkg/workflow/temporalandpkg/authn/licencepass under-race.Not included
pkg/observe/logstays Layer 1 — stdlib plus existing dependencies, nofx.This only takes effect once a service calls
slog.SetDefault(...)itself. Wiring it intopkg/servicestartup would be a Layer 3 change and is out of scope here; happy to follow up if you want it on by default.🤖 Generated with Claude Code