feat: Implement production-ready logging with Winston#46
Conversation
- Installed Winston and configured a logger module (`server/src/logger/index.ts`). - Logger includes JSON formatting, timestamps, error stack traces, and a 'service: markdownmate' meta field. - Configured file transports for combined logs (`logs/combined.log`) and error logs (`logs/error.log`). - Added conditional console transport with colorization for non-production environments. - Integrated the new logger into `server/index.ts` and `server/routes/auth.ts`, replacing previous `console.log` calls. - Created `logs` directory (user to ensure correct permissions in deployment). - Configured PM2 for log rotation (`pm2-logrotate`) with settings for retention, max size, and compression.
WalkthroughLogging across the server codebase was refactored to use a new Winston-based logger module, replacing all direct console logging. The logger is configured for structured, contextual logging with output to files and the console. Logging statements now include additional metadata for improved traceability. Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
server/routes/auth.ts (1)
96-96: Consider removing error details from client response.While logging the full error details is good for debugging, exposing
error.messagein the client response might leak sensitive information in production.Consider this approach for better security:
- error: error.message // Keep original error message for client if needed, but logged with full details + error: process.env.NODE_ENV === 'development' ? error.message : 'Authentication failed'server/index.ts (3)
68-68: Consider using 'info' level instead of 'debug' for API route tracking.Debug level logs are typically disabled in production. If you want to track API routes in production, consider using 'info' level.
- logger.debug(`API Route Hit: ${req.method} ${req.path}`, { path: req.path, method: req.method, ip: req.ip }); + logger.info(`API Route Hit: ${req.method} ${req.path}`, { path: req.path, method: req.method, ip: req.ip });
152-153: Consider standardizing on Winston logger throughout.There's mixed usage of the vite
logfunction and winston logger. For consistency, consider whether all logging should go through winston.If you want to standardize on winston for all logging:
- // The 'log' function from vite.ts might still be useful for vite specific logs - // For general server logs, use the winston logger. logger.info(`Server re-attempting to listen on port ${port}`);- // The 'log' function from vite.ts seems specific to vite's output, let's keep it for that. - log(`serving on port ${port}`); logger.info(`Server started on port ${port}`);However, keeping vite-specific logs separate might be intentional for development tooling.
Also applies to: 167-168
46-63: Consider migrating request logging to Winston.The current middleware uses the vite
logfunction for API request logging. For consistency with the new logging system, consider using winston here too.res.on("finish", () => { const duration = Date.now() - start; if (path.startsWith("/api") || path.startsWith("/user")) { let logLine = `${req.method} ${path} ${res.statusCode} in ${duration}ms`; if (capturedJsonResponse) { logLine += ` :: ${JSON.stringify(capturedJsonResponse)}`; } - if (logLine.length > 80) { - logLine = logLine.slice(0, 79) + "…"; - } - - log(logLine); + logger.info(`API Request`, { + method: req.method, + path: path, + statusCode: res.statusCode, + duration: `${duration}ms`, + response: capturedJsonResponse, + ip: req.ip + }); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)server/index.ts(8 hunks)server/routes/auth.ts(7 hunks)server/src/logger/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (7)
package.json (1)
99-99: Verify Winston version is current and secure.The Winston version 3.17.0 looks reasonable, but let's confirm it's the latest stable version and check for any known security vulnerabilities.
What is the latest stable version of Winston npm package and are there any known security vulnerabilities in version 3.17.0?server/routes/auth.ts (2)
3-3: Logger import looks correct.The import path is appropriate for the new logger module structure.
9-9: Excellent structured logging implementation.The logging statements provide excellent traceability with contextual metadata including request details and user IDs. The log levels are appropriate (info for successful operations, error for failures).
Also applies to: 26-26, 38-38, 60-60, 73-73, 93-93, 114-114
server/src/logger/index.ts (1)
32-48: Console transport configuration is well implemented.The conditional console transport with colorization and custom formatting for non-production environments is excellent for development experience.
server/index.ts (3)
7-7: Logger import looks correct.The import path matches the new logger module structure.
78-78: Excellent structured logging implementation.The winston logger usage provides good structured logging with appropriate metadata and context throughout the server lifecycle.
Also applies to: 95-95, 104-104, 119-119, 128-128, 130-130, 136-136, 138-138, 145-145, 154-154, 158-158, 169-171
106-111: Good decision to comment out error re-throwing.The explanation is clear and the decision to prevent potential unhandled promise rejections is sound. This prevents the server from crashing on errors while still logging them properly.
| // Ensure the logs directory exists (though typically this should be handled outside the logger) | ||
| // For this exercise, we'll assume it's handled by the setup script or deployment process. | ||
| const logsDir = path.join(process.cwd(), 'logs'); // Adjusted to be relative to project root, assuming server is run from project root |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure logs directory exists and handle path assumptions.
The comment acknowledges that directory creation should be handled elsewhere, but this creates a potential runtime failure if the directory doesn't exist.
Consider adding directory creation and more robust path handling:
+import fs from 'fs';
+
// Ensure the logs directory exists (though typically this should be handled outside the logger)
// For this exercise, we'll assume it's handled by the setup script or deployment process.
-const logsDir = path.join(process.cwd(), 'logs'); // Adjusted to be relative to project root, assuming server is run from project root
+const logsDir = path.join(process.cwd(), 'logs');
+
+// Create logs directory if it doesn't exist
+if (!fs.existsSync(logsDir)) {
+ fs.mkdirSync(logsDir, { recursive: true });
+}📝 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.
| // Ensure the logs directory exists (though typically this should be handled outside the logger) | |
| // For this exercise, we'll assume it's handled by the setup script or deployment process. | |
| const logsDir = path.join(process.cwd(), 'logs'); // Adjusted to be relative to project root, assuming server is run from project root | |
| import fs from 'fs'; | |
| // Ensure the logs directory exists (though typically this should be handled outside the logger) | |
| // For this exercise, we'll assume it's handled by the setup script or deployment process. | |
| const logsDir = path.join(process.cwd(), 'logs'); | |
| // Create logs directory if it doesn't exist | |
| if (!fs.existsSync(logsDir)) { | |
| fs.mkdirSync(logsDir, { recursive: true }); | |
| } |
🤖 Prompt for AI Agents
In server/src/logger/index.ts around lines 6 to 8, the code assumes the 'logs'
directory exists but does not ensure its creation, risking runtime errors.
Modify the code to check if the 'logs' directory exists and create it if it does
not before using it. Use appropriate Node.js filesystem methods to synchronously
or asynchronously verify and create the directory, ensuring robust path handling
relative to the project root.
| const logger = winston.createLogger({ | ||
| level: 'info', | ||
| format: combine( | ||
| timestamp(), | ||
| errors({ stack: true }), // Log stack traces for errors | ||
| json() // Output logs in JSON format | ||
| ), | ||
| defaultMeta: { service: 'markdownmate' }, // Add a meta field for service name | ||
| transports: [ | ||
| // - Write all logs with level `error` and below to `error.log` | ||
| new winston.transports.File({ | ||
| filename: path.join(logsDir, 'error.log'), | ||
| level: 'error', | ||
| }), | ||
| // - Write all logs with level `info` and below to `combined.log` | ||
| new winston.transports.File({ | ||
| filename: path.join(logsDir, 'combined.log'), | ||
| }), | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding log rotation configuration.
The current setup will create log files that grow indefinitely. For production environments, log rotation is essential to prevent disk space issues.
Consider adding the winston-daily-rotate-file transport:
+import DailyRotateFile from 'winston-daily-rotate-file';
+
const logger = winston.createLogger({
level: 'info',
format: combine(
timestamp(),
errors({ stack: true }),
json()
),
defaultMeta: { service: 'markdownmate' },
transports: [
- new winston.transports.File({
- filename: path.join(logsDir, 'error.log'),
- level: 'error',
- }),
- new winston.transports.File({
- filename: path.join(logsDir, 'combined.log'),
- }),
+ new DailyRotateFile({
+ filename: path.join(logsDir, 'error-%DATE%.log'),
+ datePattern: 'YYYY-MM-DD',
+ level: 'error',
+ maxFiles: '14d',
+ maxSize: '20m'
+ }),
+ new DailyRotateFile({
+ filename: path.join(logsDir, 'combined-%DATE%.log'),
+ datePattern: 'YYYY-MM-DD',
+ maxFiles: '30d',
+ maxSize: '20m'
+ }),
],
});This would require adding winston-daily-rotate-file to dependencies.
📝 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.
| const logger = winston.createLogger({ | |
| level: 'info', | |
| format: combine( | |
| timestamp(), | |
| errors({ stack: true }), // Log stack traces for errors | |
| json() // Output logs in JSON format | |
| ), | |
| defaultMeta: { service: 'markdownmate' }, // Add a meta field for service name | |
| transports: [ | |
| // - Write all logs with level `error` and below to `error.log` | |
| new winston.transports.File({ | |
| filename: path.join(logsDir, 'error.log'), | |
| level: 'error', | |
| }), | |
| // - Write all logs with level `info` and below to `combined.log` | |
| new winston.transports.File({ | |
| filename: path.join(logsDir, 'combined.log'), | |
| }), | |
| ], | |
| }); | |
| +import DailyRotateFile from 'winston-daily-rotate-file'; | |
| + | |
| const logger = winston.createLogger({ | |
| level: 'info', | |
| format: combine( | |
| timestamp(), | |
| errors({ stack: true }), | |
| json() | |
| ), | |
| defaultMeta: { service: 'markdownmate' }, | |
| transports: [ | |
| - new winston.transports.File({ | |
| - filename: path.join(logsDir, 'error.log'), | |
| - level: 'error', | |
| - }), | |
| - new winston.transports.File({ | |
| - filename: path.join(logsDir, 'combined.log'), | |
| - }), | |
| + new DailyRotateFile({ | |
| + filename: path.join(logsDir, 'error-%DATE%.log'), | |
| + datePattern: 'YYYY-MM-DD', | |
| + level: 'error', | |
| + maxFiles: '14d', | |
| + maxSize: '20m' | |
| + }), | |
| + new DailyRotateFile({ | |
| + filename: path.join(logsDir, 'combined-%DATE%.log'), | |
| + datePattern: 'YYYY-MM-DD', | |
| + maxFiles: '30d', | |
| + maxSize: '20m' | |
| + }), | |
| ], | |
| }); |
🤖 Prompt for AI Agents
In server/src/logger/index.ts around lines 10 to 29, the current Winston logger
configuration writes logs to files without rotation, which can cause log files
to grow indefinitely. To fix this, add the winston-daily-rotate-file transport
to enable log rotation by installing the winston-daily-rotate-file package and
configuring it to rotate logs daily or based on size limits. Replace or
supplement the existing file transports with daily rotate file transports,
specifying options like filename pattern, date pattern, max size, and max files
to manage disk usage effectively.
Merge pull request #46 from hexawulf/feat/production-logging-winston
1 participant
server/src/logger/index.ts).logs/combined.log) and error logs (logs/error.log).server/index.tsandserver/routes/auth.ts, replacing previousconsole.logcalls.logsdirectory (user to ensure correct permissions in deployment).pm2-logrotate) with settings for retention, max size, and compression.Summary by CodeRabbit
New Features
Chores