Fix JSON-RPC output by redirecting console logs to stderr#9
Fix JSON-RPC output by redirecting console logs to stderr#9parmar-abhinav wants to merge 2 commits into
Conversation
WalkthroughA logging destination is redirected in the FalkorDB service file, moving connection success messages from stdout to stderr. A trailing newline is added for proper file formatting. No public entity signatures are modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/services/falkordb.service.ts (1)
25-25: Consider adding a comment explaining the stderr usage.While this change correctly solves the JSON-RPC stdout pollution issue, using
console.errorfor success messages is semantically confusing. Log monitoring and aggregation tools typically treat stderr as errors, which may cause false alerts or misclassification.Consider adding a brief comment explaining the JSON-RPC constraint, or introducing a logging abstraction that makes the intent clearer.
Example with comment:
- console.error('Successfully connected to FalkorDB'); + // Log to stderr to avoid polluting stdout (used for JSON-RPC protocol messages) + console.error('Successfully connected to FalkorDB');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/falkordb.service.ts(2 hunks)
🔇 Additional comments (1)
src/services/falkordb.service.ts (1)
76-76: LGTM!Adding the trailing newline is good practice and aligns with standard formatting conventions.
|
Hi @gkorland, could you please review this PR. |
|
Hi @gkorland could you please check this PR. |
There was a problem hiding this comment.
Pull request overview
Fixes JSON-RPC protocol corruption by ensuring non-protocol logs don’t write to stdout (reserved for JSON-RPC messages).
Changes:
- Redirected a successful DB connection log from
console.log(stdout) toconsole.error(stderr)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const connection = await this.client.connection; | ||
| await connection.ping(); | ||
| console.log('Successfully connected to FalkorDB'); | ||
| console.error('Successfully connected to FalkorDB'); |
There was a problem hiding this comment.
Using console.error for a success/informational message can cause it to be treated as an error by log collectors/CI and may trigger alerts. Consider routing to a logger with configurable streams/levels (e.g., logger.info(...) mapped to stderr for JSON-RPC safety), or use console.warn/an info-level logger explicitly configured to write to stderr.
| console.error('Successfully connected to FalkorDB'); | |
| console.log('Successfully connected to FalkorDB'); |
Summary
This PR fixes an issue where
console.logstatements were writing tostdout, which interfered with the JSON-RPC protocol output. Since JSON-RPC messages are transmitted overstdout, any additional log text caused parsing errors on the client side.Changes
Replaced
console.logwithconsole.errorfor non-protocol logging and debugging messages.Summary by CodeRabbit