chore: slog update, pre-release v6#90
Merged
Merged
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReplaces Uber Zap with Go’s stdlib log/slog across the logger plugin: adds FormatHandler and RawHandler, changes BuildLogger to return a BuildResult (Logger + Closers), implements output writer resolution (stdout/stderr/files), removes Zap encoders/deps, updates schema, tests, and CI; module bumped to v6. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin
participant Config
participant Resolver
participant FileSystem
participant HandlerFactory
Plugin->>Config: BuildLogger()
Config->>Resolver: resolveOutputWriter(cfg.Output)
Resolver->>FileSystem: open file(s) or return stdout/stderr writer
FileSystem-->>Resolver: io.Writer + io.Closer(s)
Resolver-->>Config: io.MultiWriter + Closers
Config->>HandlerFactory: choose handler (FormatHandler/Raw/JSON) based on Mode/Format
HandlerFactory-->>Config: slog.Handler (wrapped writer)
Config-->>Plugin: BuildResult{Logger:*slog.Logger, Closers:[]io.Closer}
Plugin->>Plugin: store Logger and Closers
Note right of Plugin: On Stop -> iterate Closers and Close()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the RoadRunner logger plugin to a v6 module and migrates the implementation and tests from zap to Go’s log/slog, including adding a custom raw-mode handler and adjusting CI coverage collection.
Changes:
- Switch core logger/plugin interfaces and implementations to
log/slog(including a newRawHandlerand JSON attribute remapping). - Update tests and test module dependencies to target
github.com/roadrunner-server/logger/v6. - Adjust CI workflow steps and coverage aggregation to reflect the new module version and artifact layout.
Reviewed changes
Copilot reviewed 14 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Moves plugin base logger to *slog.Logger and introduces closer tracking for shutdown. |
config.go |
Replaces zap configuration with slog handlers, output writer resolution, and level parsing. |
logger.go |
Implements named loggers with per-channel overrides and caching for slog loggers. |
handler.go |
Adds slog raw-mode handler and production JSON handler configuration. |
doc.go |
Adds package documentation for the slog-backed logger plugin. |
schema.json |
Updates schema log levels (drops panic). |
tests/logger_test.go |
Updates tests to import/logger v6 and refactors request creation & assertions. |
tests/plugin.go |
Updates test plugin to use slog logger API. |
tests/go.mod / tests/go.sum |
Bumps logger dependency to v6 and updates transitive deps. |
tests/pkgs.txt |
Removes package list previously used for -coverpkg. |
.github/workflows/linux.yml |
Updates CI test/coverage steps and artifact handling for codecov. |
.gitignore |
Adds .claude/settings.local.json to ignored files. |
go.mod / go.sum |
Renames module path to /v6 and removes zap-related dependencies. |
encoder.go |
Removes zap-specific encoders (no longer needed with slog). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
.gitignore (1)
20-20: Consider removing PHP-related ignore pattern.The
**/composer.lockentry is PHP-specific (Composer package manager) and appears unrelated to this Go project. While this doesn't impact functionality, cleaning up unrelated ignore patterns improves.gitignoreclarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 20, Remove the PHP-specific gitignore entry '**/composer.lock' from .gitignore; locate the pattern string '**/composer.lock' and delete that line so the repo's ignore rules only contain Go-relevant patterns and unrelated PHP/Composer entries are not present.go.mod (1)
1-11: LGTM - clean module definition for v6.The module correctly bumps to v6 with minimal dependencies. The removal of
go.uber.org/zapandgithub.com/fatih/coloraligns with the slog migration. Go 1.26 is valid; consider updating the toolchain to go1.26.1, the latest stable patch version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 1 - 11, Update the Go toolchain version in the module definition: in the go.mod content for module github.com/roadrunner-server/logger/v6, change the toolchain directive from "go1.26.0" to "go1.26.1" so the project uses the latest stable patch release while keeping the rest of the module declarations (require block and go 1.26) unchanged.config.go (1)
213-227: Silent fallback toLevelDebugfor invalid levels may mask configuration errors.The
parseLevelfunction silently defaults toLevelDebugfor unrecognized values. While the JSON schema restricts valid values, runtime configuration errors (typos like"debg"or"warn ing") will silently produce debug-level logging instead of surfacing the misconfiguration.Consider logging a warning or returning an error for invalid level strings to help users identify configuration mistakes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 213 - 227, The parseLevel function currently returns slog.LevelDebug on any unrecognized string which hides config typos; change parseLevel(s string) to return (slog.Level, error) (keep the same cases for "debug","info","warn","warning","error") and when the default branch is hit return an explicit error that includes the original input (e.g., fmt.Errorf("invalid log level: %q", s)); update any callers of parseLevel to handle the error (log a clear warning or fail fast) so misconfigured values are surfaced instead of silently becoming LevelDebug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.go`:
- Around line 118-141: The call to cfg.resolveOutputWriter() opens writers and
returns w and closers but when cfg.FileLogger != nil the code ignores w (and any
opened closers) causing a resource leak; update the logic in the function
containing resolveOutputWriter and the FileLogger branch so either (A) move the
resolveOutputWriter() call after the cfg.FileLogger nil-check to avoid opening
outputs when using FileLogger, or (B) if you must call resolveOutputWriter()
early, ensure you add w (and any returned closers) to the returned Closers slice
or explicitly close w before returning; specifically modify the branch that
builds the lumberjack.Logger and returns the BuildResult so that w and the
original closers from resolveOutputWriter are not discarded (referencing
cfg.resolveOutputWriter, variable w, variable closers, cfg.FileLogger,
lumberjack.Logger, and BuildResult).
In `@logger.go`:
- Around line 44-46: The code currently panics on configuration error in
NamedLogger (panic(err)); instead handle the error gracefully by either changing
NamedLogger to return (Logger, error) or catch err inside NamedLogger and
fallback to the base logger while logging the configuration failure (e.g.,
processLogger.Error("NamedLogger config invalid", "err", err)). Locate the panic
in NamedLogger, remove panic(err), and implement one of the two fixes: 1)
propagate the error from NamedLogger to callers (update its signature and call
sites) or 2) swallow the error, log it via the base logger and return the base
logger instance as the fallback so the application continues running without
crashing.
- Around line 41-53: The channel logger closers returned by cfg.BuildLogger()
(res.Closers) are currently dropped, causing resource leaks; modify the Log
struct to store channel-specific closers (e.g., a map[string][]io.Closer or
similar) and when creating a channel logger in the function that calls
cfg.BuildLogger() save res.Closers into that map keyed by name before caching
the logger in l.cache; add a Close() method on Log that iterates and closes all
stored closers (and clears the map), and ensure consumers (e.g., Plugin.Stop()
or callers of ServiceLogger()) call Log.Close() on shutdown so file handles and
buffers are flushed and released.
In `@plugin.go`:
- Around line 85-92: Stop only closing p.closers misses closers created by
per-channel loggers from NamedLogger (as discussed in logger.go), causing file
handle leaks for channel-specific file_logger_options; update the logger
creation flow (where NamedLogger and BuildLogger create file writers) to
register their closer(s) into the Plugin's closer collection (p.closers) so Stop
will close them all, and then ensure Stop(p *Plugin).Stop iterates and closes
every registered closer (including those added by NamedLogger), taking care to
use the same synchronization/lock semantics as p.closers manipulation to avoid
races.
In `@tests/logger_test.go`:
- Around line 135-142: The test is flaky because time.Sleep(time.Second) doesn't
guarantee the server is listening; replace that sleep with a retry/poll loop
that attempts http.NewRequestWithContext(t.Context(), http.MethodGet,
"http://127.0.0.1:34999", nil) and http.DefaultClient.Do(req) repeatedly with
short delays and exponential backoff (or a fixed small interval) until a
successful response or a deadline (use t.Context() for cancellation); locate the
startup/wait logic around time.Sleep and swap it for the polling loop so the
test only proceeds when the server actually accepts connections.
---
Nitpick comments:
In @.gitignore:
- Line 20: Remove the PHP-specific gitignore entry '**/composer.lock' from
.gitignore; locate the pattern string '**/composer.lock' and delete that line so
the repo's ignore rules only contain Go-relevant patterns and unrelated
PHP/Composer entries are not present.
In `@config.go`:
- Around line 213-227: The parseLevel function currently returns slog.LevelDebug
on any unrecognized string which hides config typos; change parseLevel(s string)
to return (slog.Level, error) (keep the same cases for
"debug","info","warn","warning","error") and when the default branch is hit
return an explicit error that includes the original input (e.g.,
fmt.Errorf("invalid log level: %q", s)); update any callers of parseLevel to
handle the error (log a clear warning or fail fast) so misconfigured values are
surfaced instead of silently becoming LevelDebug.
In `@go.mod`:
- Around line 1-11: Update the Go toolchain version in the module definition: in
the go.mod content for module github.com/roadrunner-server/logger/v6, change the
toolchain directive from "go1.26.0" to "go1.26.1" so the project uses the latest
stable patch release while keeping the rest of the module declarations (require
block and go 1.26) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48005200-6861-4be9-bd67-c554196ffdb9
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/linux.yml.gitignoreconfig.godoc.goencoder.gogo.modhandler.gologger.goplugin.goschema.jsontests/go.modtests/logger_test.gotests/pkgs.txttests/plugin.go
💤 Files with no reviewable changes (2)
- tests/pkgs.txt
- encoder.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/logger_test.go (1)
131-138:⚠️ Potential issue | 🟡 MinorTest flakiness: HTTP server may not be ready after 1-second sleep.
The pipeline failure confirms that
time.Sleep(time.Second)is insufficient to guarantee the HTTP server is listening. The test fails with "connection refused" because the server hasn't started accepting connections yet.Replace the fixed sleep with a retry loop that polls for server readiness.
🔧 Proposed fix: poll for server readiness
time.Sleep(time.Second) - - req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:34999", nil) - assert.NoError(t, err) - - resp, err := http.DefaultClient.Do(req) - assert.NoError(t, err) + + var resp *http.Response + var lastErr error + for i := 0; i < 20; i++ { + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "http://127.0.0.1:34999", nil) + if err != nil { + t.Fatal(err) + } + resp, lastErr = http.DefaultClient.Do(req) + if lastErr == nil { + break + } + time.Sleep(100 * time.Millisecond) + } + assert.NoError(t, lastErr) require.NotNil(t, resp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/logger_test.go` around lines 131 - 138, Replace the fragile fixed sleep in tests/logger_test.go (the time.Sleep(time.Second) before calling http.NewRequestWithContext and http.DefaultClient.Do) with a retry/poll loop that waits for the test HTTP server to accept connections: repeatedly attempt a lightweight probe (e.g., net.Dial or an HTTP GET) against the server address with short intervals until success or a total timeout elapses, then proceed to create the request with http.NewRequestWithContext and call http.DefaultClient.Do; this ensures the server is ready and prevents intermittent "connection refused" failures.logger.go (2)
25-31:⚠️ Potential issue | 🟠 MajorResource leak: channel logger closers are still discarded.
The
res.Closersreturned bycfg.BuildLogger()on line 26 are not captured or stored. These contain file handles and lumberjack loggers that will never be closed during shutdown, causing file descriptor leaks and unflushed log buffers.The
Logstruct needs a mechanism to track channel-specific closers (e.g., aclosers []io.Closerfield) and expose aClose()method, which should be called during plugin shutdown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logger.go` around lines 25 - 31, The channel-specific io.Closer values returned from cfg.BuildLogger() are discarded in the if-block that resolves channels (when reading l.channels.Channels[name]); capture and store them on the Log instance so they can be closed at shutdown. Add a closers []io.Closer field to the Log struct, append res.Closers (from BuildLogger) when constructing the channel logger (the branch that returns res.Logger.With("logger", name)), and implement a Close() method on Log that iterates and closes all collected closers (and any global closers), then ensure plugin shutdown calls Log.Close(). This preserves res.Logger.With(...) behavior while preventing file descriptor/log-buffer leaks.
27-29:⚠️ Potential issue | 🟡 MinorPanic on logger build error is still present.
Using
panic(err)for configuration errors will crash the entire application if a channel has invalid configuration. Consider returning an error or falling back to the base logger with an error log message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logger.go` around lines 27 - 29, The panic(err) call in the logger build path should be replaced with graceful error handling: change the builder function that currently calls panic(err) (the function that constructs the logger and checks err) to either return the error to the caller (i.e., add an error return and propagate it) or, if you prefer a runtime fallback, log the error via the base logger and continue using a minimal/default logger instance; specifically remove panic(err), handle the failing error from the logger construction (err variable) by calling baseLogger.Error(...) with the error details and returning/using a default logger instead (or update the function signature to return (Logger, error) and return err). Ensure you update all call sites of that builder to handle the returned error or accept the fallback.
🧹 Nitpick comments (1)
config.go (1)
118-141: Unnecessary file opens when FileLogger is configured.When
cfg.FileLogger != nil, theresolveOutputWriter()on line 118 still opens any files specified incfg.Output, but the returned writerwis never used—only the lumberjack logger writes. While the closers are properly tracked (line 135), opening files that won't be written to is wasteful.Consider moving
resolveOutputWriter()after the FileLogger check or skipping it when FileLogger is configured.♻️ Proposed fix: skip resolveOutputWriter when FileLogger is set
switch mode { case off, none: return &BuildResult{Logger: slog.New(slog.DiscardHandler)}, nil case production, development, raw: // handled below default: // Unknown mode — fall through to development-like behavior. } + // File Logger: use lumberjack writer with JSON handler. + if cfg.FileLogger != nil { + cfg.FileLogger.InitDefaults() + + lj := &lumberjack.Logger{ + Filename: cfg.FileLogger.LogOutput, + MaxSize: cfg.FileLogger.MaxSize, + MaxAge: cfg.FileLogger.MaxAge, + MaxBackups: cfg.FileLogger.MaxBackups, + Compress: cfg.FileLogger.Compress, + } + + return &BuildResult{ + Logger: slog.New(slog.NewJSONHandler(lj, &slog.HandlerOptions{Level: level})), + Closers: []io.Closer{lj}, + }, nil + } + w, closers, err := cfg.resolveOutputWriter() if err != nil { return nil, errors.E(op, err) } - // File Logger: use lumberjack writer with JSON handler. - if cfg.FileLogger != nil { - cfg.FileLogger.InitDefaults() - - lj := &lumberjack.Logger{ - Filename: cfg.FileLogger.LogOutput, - MaxSize: cfg.FileLogger.MaxSize, - MaxAge: cfg.FileLogger.MaxAge, - MaxBackups: cfg.FileLogger.MaxBackups, - Compress: cfg.FileLogger.Compress, - } - - closers = append(closers, lj) - - return &BuildResult{ - Logger: slog.New(slog.NewJSONHandler(lj, &slog.HandlerOptions{Level: level})), - Closers: closers, - }, nil - } - var handler slog.Handler🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 118 - 141, The code currently calls cfg.resolveOutputWriter() (producing w and closers) before checking cfg.FileLogger, which causes unnecessary file opens when FileLogger is set; change the flow in the function containing resolveOutputWriter, so that the cfg.FileLogger != nil branch is evaluated first and returns the BuildResult with the lumberjack.Logger and closers without calling resolveOutputWriter; if other branches still need resolveOutputWriter, call it only when cfg.FileLogger is nil, ensuring you still append any returned closers to the BuildResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.go`:
- Around line 47-51: The InitDefaults method on FileLoggerConfig incorrectly
sets LogOutput to a directory (os.TempDir()); update InitDefaults (function
InitDefaults on type FileLoggerConfig) to either leave LogOutput empty so
lumberjack uses its own default, or build a proper file path (e.g.,
filepath.Join(os.TempDir(), fmt.Sprintf("%s-lumberjack.log",
filepath.Base(os.Args[0])))) and assign that to LogOutput before returning;
ensure references to LogOutput are consistent with lumberjack.Logger's Filename
expectation and import/path utilities (filepath, os, fmt) as needed.
---
Duplicate comments:
In `@logger.go`:
- Around line 25-31: The channel-specific io.Closer values returned from
cfg.BuildLogger() are discarded in the if-block that resolves channels (when
reading l.channels.Channels[name]); capture and store them on the Log instance
so they can be closed at shutdown. Add a closers []io.Closer field to the Log
struct, append res.Closers (from BuildLogger) when constructing the channel
logger (the branch that returns res.Logger.With("logger", name)), and implement
a Close() method on Log that iterates and closes all collected closers (and any
global closers), then ensure plugin shutdown calls Log.Close(). This preserves
res.Logger.With(...) behavior while preventing file descriptor/log-buffer leaks.
- Around line 27-29: The panic(err) call in the logger build path should be
replaced with graceful error handling: change the builder function that
currently calls panic(err) (the function that constructs the logger and checks
err) to either return the error to the caller (i.e., add an error return and
propagate it) or, if you prefer a runtime fallback, log the error via the base
logger and continue using a minimal/default logger instance; specifically remove
panic(err), handle the failing error from the logger construction (err variable)
by calling baseLogger.Error(...) with the error details and returning/using a
default logger instead (or update the function signature to return (Logger,
error) and return err). Ensure you update all call sites of that builder to
handle the returned error or accept the fallback.
In `@tests/logger_test.go`:
- Around line 131-138: Replace the fragile fixed sleep in tests/logger_test.go
(the time.Sleep(time.Second) before calling http.NewRequestWithContext and
http.DefaultClient.Do) with a retry/poll loop that waits for the test HTTP
server to accept connections: repeatedly attempt a lightweight probe (e.g.,
net.Dial or an HTTP GET) against the server address with short intervals until
success or a total timeout elapses, then proceed to create the request with
http.NewRequestWithContext and call http.DefaultClient.Do; this ensures the
server is ready and prevents intermittent "connection refused" failures.
---
Nitpick comments:
In `@config.go`:
- Around line 118-141: The code currently calls cfg.resolveOutputWriter()
(producing w and closers) before checking cfg.FileLogger, which causes
unnecessary file opens when FileLogger is set; change the flow in the function
containing resolveOutputWriter, so that the cfg.FileLogger != nil branch is
evaluated first and returns the BuildResult with the lumberjack.Logger and
closers without calling resolveOutputWriter; if other branches still need
resolveOutputWriter, call it only when cfg.FileLogger is nil, ensuring you still
append any returned closers to the BuildResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 779c72e3-4f89-4f3f-bd0f-68d60ed1a7e1
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
config.gohandler.gologger.gotests/logger_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- handler.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/php_test_files/composer.json (1)
6-23:⚠️ Potential issue | 🔴 CriticalRemove
php-http/guzzle7-adapter:^1.1or provide an alternative solution for PHP 8.5 compatibility.The fixture now requires
php-http/guzzle7-adapter:^1.1, which constrains PHP to^7.3 | ^8.0and does not support PHP 8.5. This dependency will fail to install during CI runs with PHP 8.5, blocking test execution. Either replace the adapter with a version that supports PHP 8.5 or conditionally exclude this dependency for PHP 8.5+ environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php_test_files/composer.json` around lines 6 - 23, The composer fixture pins "php-http/guzzle7-adapter": "^1.1" which constrains PHP to ^7.3 | ^8.0 and fails on PHP 8.5; remove that dependency from composer.json (delete the "php-http/guzzle7-adapter": "^1.1" entry) or replace it with a version that explicitly supports PHP 8.5 (update the package constraint to a known PHP 8.5-compatible release), or alternatively make the adapter conditional (move it to a platform-conditional require or require-dev that is skipped for PHP >=8.5) so CI with PHP 8.5 can install dependencies successfully.
🧹 Nitpick comments (3)
tests/logger_test.go (1)
321-370: Test lacks output verification for custom format.The test verifies the system initializes and shuts down without errors, but doesn't validate that the custom format
%time% [%level%] %message% %attrs%produces correctly formatted output. Consider capturing logs and asserting the format is applied correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/logger_test.go` around lines 321 - 370, TestLoggerCustomFormat currently only exercises startup/shutdown; modify it to capture and assert formatted log output by redirecting the logger output to a buffer or test hook, emit a known log (e.g., call the logger from TestPlugin or directly via the logger plugin's exported logger), then read the buffer and assert it matches the configured format "%time% [%level%] %message% %attrs%" (check presence/order of time-like token, "[LEVEL]" token, the exact message text, and attributes), finally proceed with the existing shutdown flow; refer to TestLoggerCustomFormat, logger.Plugin, and TestPlugin to locate where to inject the buffer/hook and the log call.format_handler.go (2)
221-237: Consider quoting attribute values that contain spaces.If an attribute value contains spaces (e.g.,
slog.String("msg", "hello world")), the outputmsg=hello worldcould be ambiguous when parsing. Consider quoting values that contain spaces or special characters if log parsing is a requirement.💡 Example fix for value quoting
func formatAttrValue(v slog.Value) string { if v.Kind() == slog.KindGroup { var sb strings.Builder for i, a := range v.Group() { if i > 0 { sb.WriteByte(' ') } sb.WriteString(a.Key) sb.WriteByte('=') sb.WriteString(formatAttrValue(a.Value)) } return sb.String() } - return v.String() + s := v.String() + if strings.ContainsAny(s, " \t\n\"") { + return strconv.Quote(s) + } + return s }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format_handler.go` around lines 221 - 237, The formatAttrValue function currently returns unquoted strings (in formatAttrValue and when recursing through v.Group()), which makes outputs like msg=hello world ambiguous; update formatAttrValue to detect when a value string (v.String()) contains spaces or special characters (e.g., whitespace, '=', '"' or '\' ) and return a quoted and escaped representation (wrap in double quotes and escape internal quotes/backslashes) so group rendering in the v.Kind()==slog.KindGroup branch uses the safe, quoted form; ensure recursion (formatAttrValue(a.Value)) preserves quoting/escaping for nested group values.
155-170: Consider reusingstrings.Replacerfor high-throughput scenarios.Creating a new
strings.NewReplaceron everyHandlecall has some overhead. For most use cases this is fine, but if this handler will be used in high-throughput logging scenarios, you could potentially cache the replacer and only substitute the dynamic values. This is a minor optimization consideration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format_handler.go` around lines 155 - 170, The current code recreates strings.NewReplacer on every call (using h.format) which is unnecessary in high-throughput paths; instead parse/cache a template once on handler creation (add a field like formatTmpl *template.Template to the handler struct and initialize it from h.format), then in the Handle method execute formatTmpl.Execute with a small data struct containing timeStr, r.Level, r.Message, attrsStr, loggerName, sourceFile, sourceLine, sourceFunc and append h.lineEnding before writing; keep the existing h.mu lock and io.WriteString(h.w, ...) but replace per-call NewReplacer creation with the cached template execution to avoid repeated allocations from strings.NewReplacer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@format_handler_test.go`:
- Around line 546-548: Update the misleading comment above the test assertion:
clarify that pid is attached before the group and therefore does NOT get the
group prefix (so expected "pid=1" is correct), while http.method is inside the
group and will be prefixed; modify the comment near the variables `got :=
strings.TrimSpace(buf.String())` and `want := "req pid=1 http.method=GET"` to
state this accurate behavior.
In `@format_handler.go`:
- Around line 56-79: NewFormatHandler currently assigns opts.Level directly to
the FormatHandler.level which can be nil and lead to a panic in Enabled when
calling h.level.Level(); update NewFormatHandler to defensively default
opts.Level to slog.LevelInfo when opts.Level is nil (check opts.Level and set
f.Level = slog.LevelInfo-like default before storing into FormatHandler.level),
so FormatHandler.level is never nil and Enabled can safely call h.level.Level();
reference NewFormatHandler, FormatHandlerOptions, FormatHandler.level, and the
Enabled method/h.level.Level() when making the change.
---
Outside diff comments:
In `@tests/php_test_files/composer.json`:
- Around line 6-23: The composer fixture pins "php-http/guzzle7-adapter": "^1.1"
which constrains PHP to ^7.3 | ^8.0 and fails on PHP 8.5; remove that dependency
from composer.json (delete the "php-http/guzzle7-adapter": "^1.1" entry) or
replace it with a version that explicitly supports PHP 8.5 (update the package
constraint to a known PHP 8.5-compatible release), or alternatively make the
adapter conditional (move it to a platform-conditional require or require-dev
that is skipped for PHP >=8.5) so CI with PHP 8.5 can install dependencies
successfully.
---
Nitpick comments:
In `@format_handler.go`:
- Around line 221-237: The formatAttrValue function currently returns unquoted
strings (in formatAttrValue and when recursing through v.Group()), which makes
outputs like msg=hello world ambiguous; update formatAttrValue to detect when a
value string (v.String()) contains spaces or special characters (e.g.,
whitespace, '=', '"' or '\' ) and return a quoted and escaped representation
(wrap in double quotes and escape internal quotes/backslashes) so group
rendering in the v.Kind()==slog.KindGroup branch uses the safe, quoted form;
ensure recursion (formatAttrValue(a.Value)) preserves quoting/escaping for
nested group values.
- Around line 155-170: The current code recreates strings.NewReplacer on every
call (using h.format) which is unnecessary in high-throughput paths; instead
parse/cache a template once on handler creation (add a field like formatTmpl
*template.Template to the handler struct and initialize it from h.format), then
in the Handle method execute formatTmpl.Execute with a small data struct
containing timeStr, r.Level, r.Message, attrsStr, loggerName, sourceFile,
sourceLine, sourceFunc and append h.lineEnding before writing; keep the existing
h.mu lock and io.WriteString(h.w, ...) but replace per-call NewReplacer creation
with the cached template execution to avoid repeated allocations from
strings.NewReplacer.
In `@tests/logger_test.go`:
- Around line 321-370: TestLoggerCustomFormat currently only exercises
startup/shutdown; modify it to capture and assert formatted log output by
redirecting the logger output to a buffer or test hook, emit a known log (e.g.,
call the logger from TestPlugin or directly via the logger plugin's exported
logger), then read the buffer and assert it matches the configured format
"%time% [%level%] %message% %attrs%" (check presence/order of time-like token,
"[LEVEL]" token, the exact message text, and attributes), finally proceed with
the existing shutdown flow; refer to TestLoggerCustomFormat, logger.Plugin, and
TestPlugin to locate where to inject the buffer/hook and the log call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6eebe0b5-6002-4ef6-a28a-34797f36e277
📒 Files selected for processing (7)
config.goformat_handler.goformat_handler_test.goschema.jsontests/configs/.rr-custom-format.yamltests/logger_test.gotests/php_test_files/composer.json
✅ Files skipped from review due to trivial changes (1)
- tests/configs/.rr-custom-format.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- schema.json
- config.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
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
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.
Reason for This PR
Closes: roadrunner-server/roadrunner#1750
Closes: roadrunner-server/roadrunner#1708
Description of Changes
Migrate the logger plugin from
go.uber.org/zapto Go's standardlog/slog. This is a breaking change for v6.What changed
Core migration:
BuildLogger()now returns*slog.LoggerviaBuildResult(which also tracks[]io.Closerfor proper resource cleanup)NamedLogger()returns*slog.Loggerinstead of*zap.Logger, with channel logger cachingPlugin.Stop()closes file handles opened for log output (fixes fd leak)RawHandler(message-only slog handler) forrawmodenewJSONHandlerforproductionmode with zap-compatible key mapping (ts/level/msg)slog.DiscardHandler(Go 1.24+) foroff/nonemodesslog.NewTextHandlerfordevelopmentmodeBug fix (#1708):
file_logger_optionserrors are now surfaced properly instead of being silently swallowedDependencies removed:
go.uber.org/zap,go.uber.org/multierrgithub.com/fatih/color,github.com/mattn/go-colorable,github.com/mattn/go-isattyFiles deleted:
encoder.go(zap-specific colored encoders — colors removed per design decision)tests/pkgs.txt(replaced bycoverpkg=./...)Other:
logger/v5→logger/v6schema.json: removed"panic"from LogLevel enum (only debug/info/warn/error supported)coverpkg=./..., codecov sync with amqp plugin approachwg.Go(),t.Context(), lint fixesapi-plugins (prerequisite, separate repo)
logger/interface.go:Named.NamedLoggerreturns*slog.Logger,Loginterface removedgo.mod: zap dependency removedLicense Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
git commit -s).CHANGELOG.md.Note:
TestLoggerRawErrandTestFileLoggercurrently fail because they depend on HTTP/server/RPC v5 plugins that still use the old zap-based Logger interface. They will pass once those plugins are migrated to v6.Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores