feat(logging): split application/output logs using stderr/stdout#607
feat(logging): split application/output logs using stderr/stdout#607
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a dual-format logging system for event output. The changes redirect JSON logs from stdout to stderr across the logging layer, add text and JSON format options to the event output handler, and implement asynchronous event processing with graceful shutdown synchronization. A new command-line flag Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="output/log/log_test.go">
<violation number="1" location="output/log/log_test.go:50">
P2: Restore `os.Stdout` with a `defer` right after reassignment so a failed assertion doesn’t leave stdout redirected for subsequent tests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| oldStdout := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| os.Stdout = w |
There was a problem hiding this comment.
P2: Restore os.Stdout with a defer right after reassignment so a failed assertion doesn’t leave stdout redirected for subsequent tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At output/log/log_test.go, line 50:
<comment>Restore `os.Stdout` with a `defer` right after reassignment so a failed assertion doesn’t leave stdout redirected for subsequent tests.</comment>
<file context>
@@ -0,0 +1,318 @@
+ oldStdout := os.Stdout
+ r, w, err := os.Pipe()
+ require.NoError(t, err)
+ os.Stdout = w
+
+ l := New(WithFormat(FormatJSON))
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/logging/kugoCustomLogger.go`:
- Around line 66-67: The inline comment above the slog handler is stale—update
the comment to correctly state that the logger writes JSON to stderr (or change
the handler to os.Stdout if stdout was intended); locate the slog.NewJSONHandler
call (variable handler) in kugoCustomLogger.go and either modify the comment to
say "logs to stderr using JSON format" or change os.Stderr to os.Stdout so the
comment and code match.
In `@output/log/log_test.go`:
- Around line 45-89: In TestFormatJSONOutput, after saving oldStdout and
assigning os.Stdout = w, restore os.Stdout in a defer to avoid leaking the pipe
on test failures: capture oldStdout, set os.Stdout = w, then immediately defer
restoring os.Stdout = oldStdout and deferring w.Close() so the pipe is always
closed even if require calls fail; apply the same pattern to every test that
captures stdout (e.g., other tests using os.Pipe(), r/w, New, WithFormat,
l.Start, l.Stop) so stdout and the writer are reliably restored/closed on test
exit.
In `@output/log/log.go`:
- Around line 37-38: The struct's level field is unused: update
output/log/log.go by either removing the level field and the CLI registration in
WithLevel (and any flag parsing that sets it) to avoid misleading users, or
implement level-based filtering inside Start(), writeText(), and writeJSON() so
events below the configured l.level are dropped/ignored; locate the level field
and WithLevel method to remove/adjust the CLI wiring, or add a helper (e.g.,
parseLevel and a shouldLog(level) check) and call it from Start(), writeText(),
and writeJSON() to filter events accordingly.
In `@README.md`:
- Around line 106-111: Add a language identifier to the fenced code block in the
README to satisfy MD040; replace the opening ``` with ```text (or ```plaintext)
so the example output is explicitly marked as plain text—look for the
triple-backtick fenced block containing the log lines (the
BLOCK/TX/ROLLBACK/GOVERNANCE sample) and update its opening fence to include the
language specifier.
🧹 Nitpick comments (4)
output/log/options.go (1)
34-40: No format validation — invalid values silently fall back to text.
WithFormataccepts any string; an invalid value (e.g.,"xml") will silently default to text output via thedefaultbranch inStart(). This is consistent with the webhook output'sWithFormat, but consider validating eagerly to surface misconfiguration early.💡 Optional: validate format in the option func
func WithFormat(format string) LogOptionFunc { return func(o *LogOutput) { + switch format { + case FormatText, FormatJSON: + default: + format = FormatText + } o.format = format } }output/log/log.go (2)
78-133:writeText— silent zero-value fallback on context type assertions.The bare type assertions (
ctx, _ := evt.Context.(event.BlockContext)) silently produce zero-valued contexts if the actual type doesn't match. This is fine as a defensive fallback, but for debugging purposes you may want to log a warning to stderr when the assertion fails, so operators know their event contexts are misconfigured.
137-148: Minor: avoidstring(data)allocation inwriteJSON.
fmt.Fprintln(os.Stdout, string(data))converts the[]byteto astring, allocating a copy. You can write directly and append the newline yourself.♻️ Proposed change
- fmt.Fprintln(os.Stdout, string(data)) + data = append(data, '\n') + os.Stdout.Write(data) //nolint:errcheckoutput/log/log_test.go (1)
64-65:time.Sleepis redundant —Stop()already drains all events.
Stop()on line 67 closeseventChanand waits ondoneChan, guaranteeing the goroutine has finished processing. The 100ms sleep before it adds unnecessary latency to every test. This applies to all the test functions in this file.♻️ Remove the redundant sleep
l.InputChan() <- testEvent - // Give the goroutine time to process - time.Sleep(100 * time.Millisecond) - require.NoError(t, l.Stop())
| func TestFormatJSONOutput(t *testing.T) { | ||
| // Capture stdout | ||
| oldStdout := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
| os.Stdout = w | ||
|
|
||
| l := New(WithFormat(FormatJSON)) | ||
| require.NoError(t, l.Start()) | ||
|
|
||
| testEvent := event.New( | ||
| "chainsync.block", | ||
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | ||
| map[string]any{"blockNumber": 12345}, | ||
| map[string]any{"hash": "abc123"}, | ||
| ) | ||
|
|
||
| l.InputChan() <- testEvent | ||
|
|
||
| // Give the goroutine time to process | ||
| time.Sleep(100 * time.Millisecond) | ||
|
|
||
| require.NoError(t, l.Stop()) | ||
|
|
||
| // Read captured output | ||
| w.Close() | ||
| output, err := io.ReadAll(r) | ||
| require.NoError(t, err) | ||
| os.Stdout = oldStdout | ||
|
|
||
| // Verify it's valid JSON | ||
| line := strings.TrimSpace(string(output)) | ||
| assert.NotEmpty(t, line) | ||
|
|
||
| var parsed event.Event | ||
| err = json.Unmarshal([]byte(line), &parsed) | ||
| require.NoError(t, err, "output should be valid JSON: %s", line) | ||
|
|
||
| assert.Equal(t, "chainsync.block", parsed.Type) | ||
| assert.Equal( | ||
| t, | ||
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | ||
| parsed.Timestamp, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Restore os.Stdout with defer to avoid leaking the pipe on test failure.
If any require call between setting os.Stdout = w and the restore on line 73 fails, os.Stdout remains pointed at the closed pipe, breaking all subsequent tests. This pattern repeats in every stdout-capturing test.
🛡️ Proposed fix (apply same pattern to all stdout-capturing tests)
oldStdout := os.Stdout
r, w, err := os.Pipe()
require.NoError(t, err)
os.Stdout = w
+ defer func() { os.Stdout = oldStdout }()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestFormatJSONOutput(t *testing.T) { | |
| // Capture stdout | |
| oldStdout := os.Stdout | |
| r, w, err := os.Pipe() | |
| require.NoError(t, err) | |
| os.Stdout = w | |
| l := New(WithFormat(FormatJSON)) | |
| require.NoError(t, l.Start()) | |
| testEvent := event.New( | |
| "chainsync.block", | |
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | |
| map[string]any{"blockNumber": 12345}, | |
| map[string]any{"hash": "abc123"}, | |
| ) | |
| l.InputChan() <- testEvent | |
| // Give the goroutine time to process | |
| time.Sleep(100 * time.Millisecond) | |
| require.NoError(t, l.Stop()) | |
| // Read captured output | |
| w.Close() | |
| output, err := io.ReadAll(r) | |
| require.NoError(t, err) | |
| os.Stdout = oldStdout | |
| // Verify it's valid JSON | |
| line := strings.TrimSpace(string(output)) | |
| assert.NotEmpty(t, line) | |
| var parsed event.Event | |
| err = json.Unmarshal([]byte(line), &parsed) | |
| require.NoError(t, err, "output should be valid JSON: %s", line) | |
| assert.Equal(t, "chainsync.block", parsed.Type) | |
| assert.Equal( | |
| t, | |
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | |
| parsed.Timestamp, | |
| ) | |
| } | |
| func TestFormatJSONOutput(t *testing.T) { | |
| // Capture stdout | |
| oldStdout := os.Stdout | |
| r, w, err := os.Pipe() | |
| require.NoError(t, err) | |
| os.Stdout = w | |
| defer func() { os.Stdout = oldStdout }() | |
| l := New(WithFormat(FormatJSON)) | |
| require.NoError(t, l.Start()) | |
| testEvent := event.New( | |
| "chainsync.block", | |
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | |
| map[string]any{"blockNumber": 12345}, | |
| map[string]any{"hash": "abc123"}, | |
| ) | |
| l.InputChan() <- testEvent | |
| // Give the goroutine time to process | |
| time.Sleep(100 * time.Millisecond) | |
| require.NoError(t, l.Stop()) | |
| // Read captured output | |
| w.Close() | |
| output, err := io.ReadAll(r) | |
| require.NoError(t, err) | |
| // Verify it's valid JSON | |
| line := strings.TrimSpace(string(output)) | |
| assert.NotEmpty(t, line) | |
| var parsed event.Event | |
| err = json.Unmarshal([]byte(line), &parsed) | |
| require.NoError(t, err, "output should be valid JSON: %s", line) | |
| assert.Equal(t, "chainsync.block", parsed.Type) | |
| assert.Equal( | |
| t, | |
| time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), | |
| parsed.Timestamp, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In `@output/log/log_test.go` around lines 45 - 89, In TestFormatJSONOutput, after
saving oldStdout and assigning os.Stdout = w, restore os.Stdout in a defer to
avoid leaking the pipe on test failures: capture oldStdout, set os.Stdout = w,
then immediately defer restoring os.Stdout = oldStdout and deferring w.Close()
so the pipe is always closed even if require calls fail; apply the same pattern
to every test that captures stdout (e.g., other tests using os.Pipe(), r/w, New,
WithFormat, l.Start, l.Stop) so stdout and the writer are reliably
restored/closed on test exit.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
feature/jsonl-output
branch
from
49476e2 to
aa1f682
Compare
|
@cubic-dev-ai review |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
2 participants
Summary by cubic
Split logs by stream: application logs now go to stderr and event output goes to stdout. Added selectable text or JSON (NDJSON) formats for clean, pipe-friendly event logs.
New Features
Migration
Written for commit aa1f682. Summary will update on new commits.
Summary by CodeRabbit
New Features
--output-log-formatflag, supporting text and JSON formats (defaults to text).Bug Fixes
Documentation