Created kitlog adapter wrapping slog - #38890
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a slog-based logging package with a kitlog adapter, test utilities, and replaces the main logger initialization to produce a slog-backed kitlog.Logger with optional JSON output and OpenTelemetry trace correlation. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as cmd/fleet (initLogger)
participant Slog as logging.NewSlogLogger
participant Otel as OtelHandler
participant Kit as KitlogAdapter
participant App as Application code (level.*.Log)
participant Sink as Output (stderr/file)
Main->>Slog: NewSlogLogger(opts JSON/Debug/Tracing)
Slog->>Otel: wrap handler (if TracingEnabled)
Slog->>Kit: return *slog.Logger -> NewKitlogAdapter(logger)
Main->>App: provide kitlog.Logger
App->>Kit: level.Info(logger).Log("msg", ...)
Kit->>Slog: translate keyvals -> slog.Record (level,msg,attrs)
Slog->>Otel: Handle(record) (OtelHandler extracts trace/span from context)
Otel->>Sink: format and write record (ts, level, msg, trace_id, span_id, attrs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/fleet/main.go`:
- Around line 132-139: The initLogger function currently builds logging.Options
with only JSON and Debug set; update it to also pass cfg.Logging.TracingEnabled
into logging.Options.TracingEnabled so OpenTelemetry trace correlation is
enabled; modify the call in initLogger (where
logging.NewSlogLogger(logging.Options{...}) is constructed) to include
TracingEnabled: cfg.Logging.TracingEnabled ensuring the kitlog.Logger returned
by initLogger supports trace_id/span_id injection.
🧹 Nitpick comments (3)
server/platform/logging/kitlog_adapter.go (3)
70-70: Trace context is lost when usingcontext.Background().Using
context.Background()means theOtelHandlerwill never see an active span context, sotrace_idandspan_idwon't be injected even when tracing is enabled. The kitlog interface doesn't support context, but consider adding a context-aware variant or documenting this limitation.This is acceptable for Phase 1 since the goal is backward compatibility, but trace correlation won't work through the kitlog adapter path until code migrates to slog's context-aware methods.
74-80: Potential slice mutation issue withappend.When
append(a.attrs, keyvals...)doesn't exceed capacity, it returns a slice backed by the same underlying array. Subsequent modifications to the parent adapter's attrs could affect the child.♻️ Suggested fix using slice copy
func (a *KitlogAdapter) With(keyvals ...any) kitlog.Logger { + newAttrs := make([]any, len(a.attrs), len(a.attrs)+len(keyvals)) + copy(newAttrs, a.attrs) return &KitlogAdapter{ logger: a.logger, - attrs: append(a.attrs, keyvals...), + attrs: append(newAttrs, keyvals...), } }
42-63: Consider handling odd number of keyvals.If
allKeyvalshas an odd length, the last element is silently ignored. This matches kitlog's behavior, but a debug log or comment documenting this would aid maintainability.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #38890 +/- ##
==========================================
- Coverage 66.18% 66.14% -0.04%
==========================================
Files 2422 2424 +2
Lines 193886 193935 +49
Branches 8513 8509 -4
==========================================
- Hits 128320 128277 -43
- Misses 53904 54007 +103
+ Partials 11662 11651 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/platform/logging/kitlog_adapter.go`:
- Line 66: KitlogAdapter currently calls a.logger.LogAttrs with
context.Background(), breaking OtelHandler trace extraction; update the adapter
by adding a new method LogContext(ctx context.Context, keyvals ...any) error
that forwards the provided ctx into a.logger.LogAttrs (so OtelHandler can
extract trace/span ids) and keep the existing Log(keyvals ...any) for
compatibility (it should call LogContext(context.Background(), keyvals...));
update KitlogAdapter.Log() to delegate to LogContext and add docs/comments near
KitlogAdapter, OtelHandler and the TracingEnabled configuration explaining that
only LogContext (or direct slog context-aware methods) will enable trace
correlation while plain Log() uses background context.
🧹 Nitpick comments (4)
server/platform/logging/logging.go (1)
112-123: Record modification in Handle may cause issues with concurrent handler chains.
slog.Record.AddAttrsmodifies the record in place. If the same record is passed to multiple handlers (e.g., in a fanout scenario), this could lead to unexpected attribute accumulation. Consider cloning the record first.♻️ Suggested fix to clone record before modification
func (h *OtelHandler) Handle(ctx context.Context, r slog.Record) error { // Extract span context from the context spanCtx := trace.SpanContextFromContext(ctx) if spanCtx.IsValid() { + // Clone the record to avoid modifying the original + r = r.Clone() // Add trace_id and span_id as attributes r.AddAttrs( slog.String("trace_id", spanCtx.TraceID().String()), slog.String("span_id", spanCtx.SpanID().String()), ) } return h.base.Handle(ctx, r) }server/platform/logging/kitlog_adapter_test.go (1)
62-107: Comprehensive level mapping tests using table-driven approach.Good coverage of all four log levels (info, debug, warn, error) with proper assertions on both message and level.
Consider adding edge case tests for robustness:
- Odd number of keyvals (dangling key)
- Non-string keys
- Empty
Log()callserver/platform/logging/kitlog_adapter.go (1)
43-64: Odd keyval count handling could be more explicit.The loop
i < len(allKeyvals)-1silently drops a dangling key if there's an odd number of keyvals. While this is defensive, consider logging a warning or adding a comment explaining this behavior for maintainability.♻️ Optional: Add explicit handling for odd keyval count
func (a *KitlogAdapter) Log(keyvals ...any) error { if len(keyvals) == 0 && len(a.attrs) == 0 { return nil } // Combine pre-set attrs with new keyvals allKeyvals := slices.Concat(a.attrs, keyvals) + // Note: If allKeyvals has odd length, the last key is silently dropped. + // This matches go-kit/log behavior which expects key-value pairs. + // Extract level and message from keyvalsserver/platform/logging/testutils/test_handler.go (1)
69-79: Thegroupfield is captured but never applied inHandle.
WithGroupstores the group name, butHandle(lines 36-55) doesn't use it. In slog, groups should nest subsequent attributes under the group name. This could lead to false confidence in tests that expect group behavior.Consider either implementing group handling or removing the field if it's intentionally unsupported:
Option 1: Minimal implementation that prefixes attribute keys
func (h *TestHandler) Handle(_ context.Context, r slog.Record) error { h.mu.Lock() defer h.mu.Unlock() clone := slog.NewRecord(r.Time, r.Level, r.Message, r.PC) // Add pre-set attrs first - clone.AddAttrs(h.attrs...) + for _, attr := range h.attrs { + if h.group != "" { + attr.Key = h.group + "." + attr.Key + } + clone.AddAttrs(attr) + } // Then add record attrs r.Attrs(func(a slog.Attr) bool { + if h.group != "" { + a.Key = h.group + "." + a.Key + } clone.AddAttrs(a) return true })Option 2: Remove group field if not needed
type TestHandler struct { mu *sync.Mutex records *[]slog.Record attrs []slog.Attr - group string }func (h *TestHandler) WithGroup(name string) slog.Handler { - h.mu.Lock() - defer h.mu.Unlock() - return &TestHandler{ - mu: h.mu, - records: h.records, - attrs: h.attrs, - group: name, - } + // Groups not supported in test handler - return self + return h }
| } | ||
| } | ||
|
|
||
| a.logger.LogAttrs(context.Background(), level, msg, attrs...) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure and locate relevant files
git ls-files | grep -E "(logging|kitlog)" | head -20Repository: fleetdm/fleet
Length of output: 806
🏁 Script executed:
#!/bin/bash
# Check if the file exists and examine its content
if [ -f "server/platform/logging/kitlog_adapter.go" ]; then
wc -l "server/platform/logging/kitlog_adapter.go"
fiRepository: fleetdm/fleet
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for OtelHandler to understand trace context extraction
rg -n "OtelHandler" --type go -A 5 -B 5Repository: fleetdm/fleet
Length of output: 5902
🏁 Script executed:
#!/bin/bash
# Look for kitlog.Logger interface and Log/LogAttrs method signature
rg -n "kitlog\.Logger" --type goRepository: fleetdm/fleet
Length of output: 25265
🏁 Script executed:
cat -n server/platform/logging/kitlog_adapter.goRepository: fleetdm/fleet
Length of output: 3412
🏁 Script executed:
# Check if kitlog is a local package or external dependency
rg "type Logger interface" --type go -B 2 -A 10 | head -50Repository: fleetdm/fleet
Length of output: 825
🏁 Script executed:
# Look for LogAttrs signature in go.mod or other references
rg "LogAttrs" --type go -B 2 -A 2 | head -40Repository: fleetdm/fleet
Length of output: 347
🏁 Script executed:
# Check if there are tests for kitlog adapter with OtelHandler/tracing
cat -n server/platform/logging/kitlog_adapter_test.goRepository: fleetdm/fleet
Length of output: 3411
🏁 Script executed:
# Check how the kitlog adapter is created/used with tracing enabled
rg -n "NewKitlogAdapter\|TracingEnabled" --type go -B 3 -A 3 | head -60Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look for direct slog usage in the codebase to see if tracing works there
rg "logger\.InfoContext\|logger\.DebugContext\|logger\.ErrorContext" --type go | head -20Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check how the slog logger is created and if OtelHandler is applied before wrapping with kitlog adapter
rg -n "NewOtelHandler\|NewKitlogAdapter" --type go -B 5 -A 5Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look for the New() function that creates loggers to see the order of wrapping
rg -n "func New\(.*Logger" --type go -A 20 | head -80Repository: fleetdm/fleet
Length of output: 5874
🏁 Script executed:
# Check if there's any documentation about the kitlog adapter limitation
rg -n "kitlog\|context" server/platform/logging/logging.go -B 2 -A 2 | head -100Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Look at the logging.go file to see the complete logger construction flow
cat -n server/platform/logging/logging.go | head -100Repository: fleetdm/fleet
Length of output: 3484
🏁 Script executed:
# Check if there are any tests that verify tracing with kitlog adapter
rg -n "kitlog\|OtelHandler" server/platform/logging/ --type goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Search for how loggers are actually instantiated in the main codebase
rg -n "NewKitlogAdapter\|initLogger" --type go cmd/Repository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Search for initLogger function in cmd/fleet/main.go
grep -n "initLogger\|NewKitlogAdapter\|NewSlogLogger" cmd/fleet/main.go -A 10 -B 2Repository: fleetdm/fleet
Length of output: 437
🏁 Script executed:
# Check how the logger is actually used in cmd/fleet/serve.go and where kitlog adapter is created
rg -n "NewKitlogAdapter" cmd/fleet/ -B 5 -A 5Repository: fleetdm/fleet
Length of output: 434
🏁 Script executed:
# Look for all places where kitlog adapter is instantiated
rg -n "NewKitlogAdapter" --type goRepository: fleetdm/fleet
Length of output: 440
🏁 Script executed:
# Verify that there are no tests checking trace correlation with kitlog adapter
rg -n "TestOtelHandler\|trace.*kitlog\|kitlog.*trace" server/platform/logging/ --type goRepository: fleetdm/fleet
Length of output: 39
🏁 Script executed:
# Check if there's any LogContext or context-aware methods in kitlog.Logger interface
rg -n "LogContext\|ErrorContext\|InfoContext" --type go | grep kitlogRepository: fleetdm/fleet
Length of output: 39
Trace correlation is broken when logging through the kitlog adapter with TracingEnabled: true.
The OtelHandler is designed to extract trace context from the context.Context parameter and inject trace_id and span_id into logs. However, KitlogAdapter.Log() cannot accept a context parameter because the kitlog.Logger interface signature is Log(keyvals ...any) error (defined in the external github.com/go-kit/log package). This forces the adapter to always pass context.Background(), which has no span context.
Since this is the primary logger instantiation path (see cmd/fleet/main.go:142), trace correlation silently fails even when TracingEnabled: true is configured. Direct slog usage with context-aware methods like logger.InfoContext(ctx, msg) would work, but all kitlog-based logging in the codebase bypasses trace correlation.
Consider:
- Adding a
LogContext(ctx, keyvals...)method alongsideLog()for code paths that have context available - Documenting this limitation so users understand trace correlation only works with direct slog usage, not through the kitlog adapter
🤖 Prompt for AI Agents
In `@server/platform/logging/kitlog_adapter.go` at line 66, KitlogAdapter
currently calls a.logger.LogAttrs with context.Background(), breaking
OtelHandler trace extraction; update the adapter by adding a new method
LogContext(ctx context.Context, keyvals ...any) error that forwards the provided
ctx into a.logger.LogAttrs (so OtelHandler can extract trace/span ids) and keep
the existing Log(keyvals ...any) for compatibility (it should call
LogContext(context.Background(), keyvals...)); update KitlogAdapter.Log() to
delegate to LogContext and add docs/comments near KitlogAdapter, OtelHandler and
the TracingEnabled configuration explaining that only LogContext (or direct slog
context-aware methods) will enable trace correlation while plain Log() uses
background context.
There was a problem hiding this comment.
This is expected. Trace correlation will become automatically available in phase 2, when the rest of the codebase switches to using slog methods which provide context to the logger.
| msg := "" | ||
| attrs := make([]slog.Attr, 0, len(allKeyvals)/2) | ||
|
|
||
| for i := 0; i < len(allKeyvals)-1; i += 2 { |
There was a problem hiding this comment.
They are pairs. key at allKeyvals[i] and value at allKeyvals[i+1]
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
Makes sense. I do wonder if this will have any performance implications. But I don't think we log fast enough to matter.
2 participants
Related issue: Resolves #38889
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.