Introduce a centralized, file-based logging infrastructure using Winston to enhance server monitoring#114
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a Winston-based logging system (file + console) and integrates it into the WebSocket server to capture and route console output and runtime events to Changes
Sequence DiagramsequenceDiagram
participant App as Application (WebSocket Server)
participant Logger as Logger Utility (src/utils/logger.ts)
participant Winston as Winston Logger
participant File as File System (~/.rein/log.txt)
participant Console as Console Output
App->>Logger: emit log (info/warn/error)
Logger->>Winston: forward log entry
Winston->>File: write JSON log with timestamp and stack
Winston->>Console: output formatted/colorized text (non-production)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/server/websocket.ts (2)
8-8: Importingloggereagerly monkey-patches the globalconsoleas a side effect.As soon as
websocket.tsimports'../utils/logger',console.logandconsole.errorare globally replaced for the entire process. Any other module imported after this point — including Vite internals, third-party libraries, or test helpers — will have theirconsole.log/console.errorsilently rerouted through Winston without being aware of it. This makes the behaviour non-local and difficult to reason about.Consider exporting an explicit
interceptConsole()function fromlogger.tsand calling it at a deliberate entry point (e.g., server startup), keeping the side effect opt-in rather than automatic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` at line 8, The module currently imports logger eagerly in websocket.ts which triggers a global console monkey-patch as a side effect; change logger.ts to export an explicit interceptConsole() function and stop performing the patch during module initialization, then in your application startup (not in websocket.ts) call interceptConsole() where you intentionally want Winston to replace console methods; update websocket.ts to remove the direct import of logger and instead import only non-side-effect utilities it needs (or nothing) so that console interception is opt-in and only invoked from the designated startup entry that calls interceptConsole().
99-102: Silently discarded duplicate messages are invisible in logs.When a rapid-duplicate message is suppressed, execution returns immediately with no trace. While the suppression itself is intentional, having zero observability means it's impossible to distinguish "no messages arrived" from "messages were silently dropped" when troubleshooting. A
debug-level log here would preserve the intent while aiding diagnostics.♻️ Proposed change
if (raw === lastRaw && (now - lastTime) < DUPLICATE_WINDOW_MS) { + logger.debug('Duplicate message suppressed within window'); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/websocket.ts` around lines 99 - 102, The duplicate-suppression branch that returns when raw === lastRaw and (now - lastTime) < DUPLICATE_WINDOW_MS should emit a debug-level log before returning so suppressed messages are observable; add a call to the module/logger's debug (or console.debug) that logs the fact of suppression along with raw (or a truncated preview), the time delta (now - lastTime), and DUPLICATE_WINDOW_MS, and include any available connection identifier (e.g., clientId or socket identifier) to make the log actionable, then return as before.src/utils/logger.ts (1)
12-33: Consider log rotation to prevent unbounded file growth.
~/.rein/log.txtwill grow indefinitely over the life of the process; there's no size cap or rotation policy. For a tool targeting end users (the PR's stated goal is to provide a log file for issue reporting), an ever-growing file can become a pain point.Consider
winston-daily-rotate-fileor, at minimum,maxsize+maxFileson theFiletransport:♻️ Lightweight rotation option using built-in File transport options
new winston.transports.File({ filename: LOG_FILE, + maxsize: 5 * 1024 * 1024, // 5 MB per file + maxFiles: 3, // keep last 3 rotated files }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.ts` around lines 12 - 33, The File transports (transports, exceptionHandlers, rejectionHandlers) currently write to LOG_FILE without rotation; update the winston transport configuration used by logger to enable log rotation by replacing or augmenting winston.transports.File with a rotating transport (e.g., winston-daily-rotate-file) or by adding maxsize and maxFiles options to each File transport; update the instantiation of logger and the three places where new winston.transports.File({ filename: LOG_FILE }) appears so they include the rotation options (or use new DailyRotateFile with appropriate filename/datePattern, maxFiles, and maxSize) to prevent unbounded growth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/websocket.ts`:
- Line 45: The code currently logs raw client IPs via
request.socket.remoteAddress (seen in the logger.info calls that log the
"Upgrade request received" and the similar log at the other occurrence); replace
those direct usages with a reusable anonymization helper (e.g., anonymizeIp) and
call that helper in both places instead of logging the raw address — the helper
should deterministically redact or hash (with a salt or truncation) the IP to a
non-reversible token to preserve uniqueness for debugging while removing PII,
and ensure both occurrences (the logger.info that mentions Upgrade request and
the later logger.info using request.socket.remoteAddress) use this helper for
consistent behavior.
- Around line 176-179: The WebSocket error handler is double-logging because
console.error is monkey-patched; remove the redundant console.error call inside
the ws.on('error', (error: Error) => { ... }) callback and keep the existing
logger.error usage (update logger.error to include full error details such as
error.message or error.stack if desired) so only logger.error is invoked for
errors reported from the WebSocket.
In `@src/utils/logger.ts`:
- Around line 51-59: The console intercept overrides (console.log and
console.error) cause double terminal output because Winston's Console transport
already writes to stdout/stderr, so remove the calls to
originalConsoleLog.apply(console, args) and originalConsoleError.apply(console,
args) from those overrides (and delete the now-unused
originalConsoleLog/originalConsoleError variables). Also fix serialization:
instead of JSON.stringify for each arg (which quotes plain strings), serialize
with a small helper inside the overrides that returns the string unchanged when
typeof arg === 'string' and otherwise returns JSON.stringify(arg) (or a safe
stringify), then pass the joined result to logger.info/logger.error to preserve
readable plain strings while still serializing objects.
- Around line 7-9: The logger code currently assumes LOG_DIR (derived from
HOMEDIR) exists which causes ENOENT on first run; before calling
winston.createLogger (and before configuring
exceptionHandlers/rejectionHandlers), synchronously ensure LOG_DIR exists by
calling fs.mkdirSync(LOG_DIR, { recursive: true }) (wrap in try/catch if you
want to log/ignore errors) so the File transport can create LOG_FILE without
crashing; update the initialization around LOG_DIR/LOG_FILE and createLogger to
perform this guard first.
---
Nitpick comments:
In `@src/server/websocket.ts`:
- Line 8: The module currently imports logger eagerly in websocket.ts which
triggers a global console monkey-patch as a side effect; change logger.ts to
export an explicit interceptConsole() function and stop performing the patch
during module initialization, then in your application startup (not in
websocket.ts) call interceptConsole() where you intentionally want Winston to
replace console methods; update websocket.ts to remove the direct import of
logger and instead import only non-side-effect utilities it needs (or nothing)
so that console interception is opt-in and only invoked from the designated
startup entry that calls interceptConsole().
- Around line 99-102: The duplicate-suppression branch that returns when raw ===
lastRaw and (now - lastTime) < DUPLICATE_WINDOW_MS should emit a debug-level log
before returning so suppressed messages are observable; add a call to the
module/logger's debug (or console.debug) that logs the fact of suppression along
with raw (or a truncated preview), the time delta (now - lastTime), and
DUPLICATE_WINDOW_MS, and include any available connection identifier (e.g.,
clientId or socket identifier) to make the log actionable, then return as
before.
In `@src/utils/logger.ts`:
- Around line 12-33: The File transports (transports, exceptionHandlers,
rejectionHandlers) currently write to LOG_FILE without rotation; update the
winston transport configuration used by logger to enable log rotation by
replacing or augmenting winston.transports.File with a rotating transport (e.g.,
winston-daily-rotate-file) or by adding maxsize and maxFiles options to each
File transport; update the instantiation of logger and the three places where
new winston.transports.File({ filename: LOG_FILE }) appears so they include the
rotation options (or use new DailyRotateFile with appropriate
filename/datePattern, maxFiles, and maxSize) to prevent unbounded growth.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/logger.ts (2)
28-35: Reuse a singleFiletransport instance acrosstransports,exceptionHandlers, andrejectionHandlers.Three independent
winston.transports.Fileinstances open three separatefs.WriteStreamhandles to the same path. Under concurrent writes, buffered data from independent streams can interleave in the file. Sharing one instance avoids this and cuts file descriptor usage by two thirds.♻️ Proposed refactor
+const fileTransport = new winston.transports.File({ filename: LOG_FILE }); + const logger = winston.createLogger({ level: 'info', format: winston.format.combine( winston.format.timestamp({ format: 'YYYY-MM-DD HH:mm:ss' }), winston.format.errors({ stack: true }), winston.format.splat(), winston.format.json() ), defaultMeta: { service: 'rein-server' }, transports: [ - new winston.transports.File({ filename: LOG_FILE }), + fileTransport, ], exceptionHandlers: [ - new winston.transports.File({ filename: LOG_FILE }) + fileTransport, ], rejectionHandlers: [ - new winston.transports.File({ filename: LOG_FILE }) + fileTransport, ] });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.ts` around lines 28 - 35, The three separate winston.transports.File instances (created with new winston.transports.File({ filename: LOG_FILE })) open independent write streams and should be replaced by a single shared File transport instance; create one File transport (e.g., const fileTransport = new winston.transports.File({ filename: LOG_FILE })) and reuse fileTransport in the logger configuration arrays transports, exceptionHandlers, and rejectionHandlers so that the same instance is referenced everywhere (keep LOG_FILE and the winston imports as-is).
26-29: Consider configuring log rotation to prevent unbounded disk growth.The
Filetransport has nomaxsizeormaxFileslimit. On a long-running server~/.rein/log.txtwill grow indefinitely. Winston's built-in options (maxsize+maxFiles: N, tailable: true) or thewinston-daily-rotate-filetransport can cap disk usage with minimal changes.💡 Example using built-in options
-new winston.transports.File({ filename: LOG_FILE }) +new winston.transports.File({ + filename: LOG_FILE, + maxsize: 10 * 1024 * 1024, // 10 MB per file + maxFiles: 5, // keep 5 rotated files + tailable: true, // newest data always in log.txt +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/logger.ts` around lines 26 - 29, The File transport in the logger configuration (the transports array where new winston.transports.File({ filename: LOG_FILE }) is created) lacks rotation settings and can grow unbounded; update that transport to enable log rotation by adding built-in options such as maxsize and maxFiles with tailable: true (or swap to winston-daily-rotate-file if preferred) so the File transport limits file size and keeps a bounded number of rotated files; ensure the change is made in the logger initialization where LOG_FILE is passed to winston.transports.File.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/logger.ts`:
- Line 12: The module currently calls fs.mkdirSync(LOG_DIR, { recursive: true })
at import time which can throw and crash the process on read-only filesystems;
wrap that call in a try/catch around the fs.mkdirSync invocation (referencing
LOG_DIR and fs.mkdirSync) so failures are caught, log a clear error with
console.error or processLogger if available, and continue without throwing (skip
creating the directory) so the module load doesn't crash; ensure you only
swallow the error after logging it so other code can continue to initialize.
---
Duplicate comments:
In `@src/utils/logger.ts`:
- Around line 54-62: The console overrides are causing double terminal output
and mis-serialized strings: update the console.log/console.error wrappers (the
overrides that call originalConsoleLog/originalConsoleError and use
JSON.stringify on args) to stop calling
originalConsoleLog.apply/originalConsoleError.apply (remove those calls so
Winston's Console transport is the only terminal writer) and replace
JSON.stringify(args) with Node's util.format(...args) (import util) or otherwise
serialize each arg without adding quotes for plain strings (e.g., typeof arg ===
'string' ? arg : JSON.stringify(arg)); keep using logger.info and logger.error
for file logging and ensure the overrides call logger.info(util.format(...args))
/ logger.error(util.format(...args)).
---
Nitpick comments:
In `@src/utils/logger.ts`:
- Around line 28-35: The three separate winston.transports.File instances
(created with new winston.transports.File({ filename: LOG_FILE })) open
independent write streams and should be replaced by a single shared File
transport instance; create one File transport (e.g., const fileTransport = new
winston.transports.File({ filename: LOG_FILE })) and reuse fileTransport in the
logger configuration arrays transports, exceptionHandlers, and rejectionHandlers
so that the same instance is referenced everywhere (keep LOG_FILE and the
winston imports as-is).
- Around line 26-29: The File transport in the logger configuration (the
transports array where new winston.transports.File({ filename: LOG_FILE }) is
created) lacks rotation settings and can grow unbounded; update that transport
to enable log rotation by adding built-in options such as maxsize and maxFiles
with tailable: true (or swap to winston-daily-rotate-file if preferred) so the
File transport limits file size and keeps a bounded number of rotated files;
ensure the change is made in the logger initialization where LOG_FILE is passed
to winston.transports.File.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.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
2 participants
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.
Addressed Issues:
Closes #99
Description
The logging system is designed to provide structured, persistent, and production-ready logging without modifying existing core logic.
What This PR Includes
Added
src/utils/logger.tsIntegrated structured logging across the WebSocket server
Configured file-based logging at:
~/.rein/log.txtC:\Users\Hp\.rein\log.txt`
Enabled:
Timestamped logs
JSON structured format
Exception handling
Promise rejection handling
Console logging in non-production environments
Screenshots/Recordings:
screen-capture.36.webm
Checklist
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit
New Features
Chores