Implement Winston Logger for Enhanced Logging Functionality #3574
Conversation
Signed-off-by: Gourav <[email protected]>
Signed-off-by: Gourav <[email protected]>
GauravSaini01
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces a custom logging service using Winston, a versatile logging library for Node.js. The implementation adds a centralized logging mechanism across various components of the application, including MDX rendering, YAML file reading, and layout components. The new logging service is configured to log messages to both console and file, with log levels dynamically set based on the environment. Two new dependencies, Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
|
We require all PRs to follow Conventional Commits specification. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
@asyncapi-bot thank you for the information about the available commands. i'll proceed with reviewing the pr to help ensure it meets all requirements, including the conventional commit specification. ✅ Actions performedReview triggered.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3574 +/- ##
===========================================
- Coverage 100.00% 99.85% -0.15%
===========================================
Files 19 20 +1
Lines 668 679 +11
===========================================
+ Hits 668 678 +10
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
components/layout/GenericWideLayout.tsx (1)
Line range hint
1-6: Fix import ordering.Group imports according to the project's conventions:
- External dependencies
- Internal components
- Utilities
import React from 'react'; + import logger from '@/scripts/log-service'; // import AnnouncementHero from '../campaigns/AnnoucementHero'; import Head from '../Head'; import Row from './Row'; - import logger from '@/scripts/log-service';🧰 Tools
🪛 eslint
[error] 6-6:
@/scripts/log-serviceimport should occur before import of../Head(import/order)
🧹 Nitpick comments (8)
components/helpers/read-yaml-file.ts (1)
11-12: Avoid duplicating error messages.The error message is duplicated between logging and throwing. Consider storing the message in a variable for reuse.
+ const errorMessage = `Error: something went wrong while reading ${fileName} file: ${error.message}`; - logger.error(`Error: something went wrong while reading ${fileName} file: ${error.message}`); - throw new Error(`Error: something went wrong while reading ${fileName} file: ${error.message}`); + logger.error(errorMessage); + throw new Error(errorMessage);tests/logger.test.js (1)
5-16: Add more test scenarios for comprehensive coverage.The test suite only covers basic error logging. Consider adding tests for:
- Different log levels (info, warn, debug)
- Log format validation
- Multiple concurrent logs
- Log rotation/cleanup
scripts/log-service.js (1)
6-6: Make log path configurable.Consider making the log file path configurable through environment variables for better deployment flexibility.
- const errorLogPath = path.resolve(__dirname, '../logs/error.log'); + const errorLogPath = path.resolve( + process.env.LOG_DIR || path.resolve(__dirname, '../logs'), + 'error.log' + );components/layout/GenericLayout.tsx (1)
6-6: Fix import order.Move the logger import before local imports to follow the project's import ordering convention.
-import logger from '@/scripts/log-service'; import React from 'react'; +import logger from '@/scripts/log-service'; // import AnnouncementHero from '../campaigns/AnnoucementHero'; import Head from '../Head';🧰 Tools
🪛 eslint
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 6-6:
@/scripts/log-serviceimport should occur before import of../Head(import/order)
components/layout/GenericPostLayout.tsx (1)
11-11: Fix code style issues.Address the following style issues:
- Move logger import before local imports
- Add space before opening brace
- Add semicolon after error log
-import logger from '@/scripts/log-service'; import ErrorPage from 'next/error'; +import logger from '@/scripts/log-service'; - if (post.title === undefined && post.slug !== '/about'){ + if (post.title === undefined && post.slug !== '/about') { logger.error('Post title is required'); throw new Error('Post title is required'); }Also applies to: 28-31
🧰 Tools
🪛 eslint
[error] 1-11: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 11-11:
@/scripts/log-serviceimport should occur before import of../../context/GenericPostContext(import/order)
components/layout/DocsLayout.tsx (1)
23-23: Fix code style issues.Address the following style issues:
- Move logger import before local imports
- Add space before opening brace
- Add semicolon after error log
-import logger from '@/scripts/log-service'; import ErrorPage from 'next/error'; +import logger from '@/scripts/log-service'; - if (post.title === undefined){ + if (post.title === undefined) { logger.error('Post title is required'); throw new Error('Post title is required'); }Also applies to: 83-86
🧰 Tools
🪛 eslint
[error] 1-23: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 23-23:
@/scripts/log-serviceimport should occur before import of../../config/edit-page-config.json(import/order)
components/MDX/MDX.tsx (2)
113-114: Remove redundant console.error call.The Winston logger already handles console output, making the
console.errorcall redundant.logger.error(e); - console.error(e);🧰 Tools
🪛 eslint
[error] 113-113: Insert
;(prettier/prettier)
[error] 113-114: Missing semicolon.
(semi)
43-43: Fix code style issues.Address the following style issues:
- Move logger import before local imports
- Add semicolon after error log
-import logger from '@/scripts/log-service'; import { MDXProvider as CoreMDXProvider } from '@mdx-js/react'; +import logger from '@/scripts/log-service'; - logger.error(e) + logger.error(e);Also applies to: 113-114
🧰 Tools
🪛 eslint
[error] 1-43: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 43-43:
@/scripts/log-serviceimport should occur before import of../Asyncapi3Comparison/Asyncapi3ChannelComparison(import/order)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
components/MDX/MDX.tsx(2 hunks)components/helpers/read-yaml-file.ts(2 hunks)components/layout/DocsLayout.tsx(2 hunks)components/layout/GenericLayout.tsx(2 hunks)components/layout/GenericPostLayout.tsx(2 hunks)components/layout/GenericWideLayout.tsx(2 hunks)package.json(2 hunks)scripts/log-service.js(1 hunks)tests/logger.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
components/helpers/read-yaml-file.ts
[error] 1-2: Run autofix to sort these imports!
(simple-import-sort/imports)
components/layout/GenericPostLayout.tsx
[error] 1-11: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 11-11: @/scripts/log-service import should occur before import of ../../context/GenericPostContext
(import/order)
[error] 28-28: Insert ·
(prettier/prettier)
[error] 28-31: Missing space before opening brace.
(space-before-blocks)
components/layout/DocsLayout.tsx
[error] 1-23: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 23-23: @/scripts/log-service import should occur before import of ../../config/edit-page-config.json
(import/order)
[error] 83-83: Insert ·
(prettier/prettier)
[error] 83-86: Missing space before opening brace.
(space-before-blocks)
[error] 84-84: Insert ;
(prettier/prettier)
[error] 84-85: Missing semicolon.
(semi)
components/layout/GenericWideLayout.tsx
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 6-6: @/scripts/log-service import should occur before import of ../Head
(import/order)
components/MDX/MDX.tsx
[error] 1-43: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 43-43: @/scripts/log-service import should occur before import of ../Asyncapi3Comparison/Asyncapi3ChannelComparison
(import/order)
[error] 113-113: Insert ;
(prettier/prettier)
[error] 113-114: Missing semicolon.
(semi)
components/layout/GenericLayout.tsx
[error] 1-6: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 6-6: @/scripts/log-service import should occur before import of ../Head
(import/order)
scripts/log-service.js
[error] 16-16: Delete ,
(prettier/prettier)
[error] 18-18: Delete ,
(prettier/prettier)
[error] 23-23: Delete ,
(prettier/prettier)
[error] 25-25: Delete ,
(prettier/prettier)
[error] 26-26: Delete ,
(prettier/prettier)
[error] 29-29: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 180000ms (2)
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Lighthouse CI
🔇 Additional comments (4)
components/layout/GenericLayout.tsx (1)
36-37: LGTM! Error handling enhanced with logging.The error logging implementation correctly precedes throwing the error, providing better debugging capabilities.
components/layout/GenericPostLayout.tsx (1)
28-31: LGTM! Error handling enhanced with logging.The error logging implementation correctly precedes throwing the error, providing better debugging capabilities.
🧰 Tools
🪛 eslint
[error] 28-28: Insert
·(prettier/prettier)
[error] 28-31: Missing space before opening brace.
(space-before-blocks)
components/layout/DocsLayout.tsx (1)
83-86: LGTM! Error handling enhanced with logging.The error logging implementation correctly precedes throwing the error, providing better debugging capabilities.
🧰 Tools
🪛 eslint
[error] 83-83: Insert
·(prettier/prettier)
[error] 83-86: Missing space before opening brace.
(space-before-blocks)
[error] 84-84: Insert
;(prettier/prettier)
[error] 84-85: Missing semicolon.
(semi)
package.json (1)
66-66: Verify the winston setup in the codebase.Let's ensure the winston logger is properly configured and used across the codebase.
Also applies to: 119-119
✅ Verification successful
Winston logger is properly configured and consistently used ✅
The logger is well-implemented with centralized configuration, proper error handling across components, and test coverage in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify winston logger setup and usage # Check if logs directory exists in the configuration echo "Checking for logs directory configuration..." rg -l "logs/error.log" # Check winston logger initialization echo "Checking winston logger initialization..." ast-grep --pattern 'createLogger({ $$$ })' # Check error logging usage echo "Checking error logging usage..." rg "logger.*error\(" # Check for test files echo "Checking for logger test files..." fd "logger.*test\.[jt]s.*"Length of output: 2303
| import fs from 'fs/promises'; | ||
| import logger from '@/scripts/log-service'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix import ordering.
Per the project's static analysis rules, imports need to be sorted. The logger import should be grouped with the built-in imports.
import fs from 'fs/promises';
+ import logger from '@/scripts/log-service';
- import logger from '@/scripts/log-service';Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 1-2: Run autofix to sort these imports!
(simple-import-sort/imports)
| setTimeout(() => { | ||
| expect(fs.existsSync(errorLogPath)).toBe(true); | ||
| const logContents = fs.readFileSync(errorLogPath, 'utf8'); | ||
| expect(logContents).toContain(testErrorMessage); | ||
| done(); | ||
| }, 500); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace setTimeout with proper async/await pattern.
Using setTimeout for async operations is an anti-pattern in testing. Consider using async/await with a file watch or polling mechanism.
- setTimeout(() => {
+ const waitForLog = async () => {
+ let attempts = 0;
+ while (attempts < 10) {
+ if (fs.existsSync(errorLogPath)) {
+ const logContents = fs.readFileSync(errorLogPath, 'utf8');
+ if (logContents.includes(testErrorMessage)) {
+ return logContents;
+ }
+ }
+ await new Promise(resolve => setTimeout(resolve, 100));
+ attempts++;
+ }
+ throw new Error('Timeout waiting for log file');
+ };
+
+ try {
expect(fs.existsSync(errorLogPath)).toBe(true);
- const logContents = fs.readFileSync(errorLogPath, 'utf8');
+ const logContents = await waitForLog();
expect(logContents).toContain(testErrorMessage);
- done();
- }, 500);
+ done();
+ } catch (error) {
+ done(error);
+ }Committable suggestion skipped: line range outside the PR's diff.
| const logger = createLogger({ | ||
| level: env === 'development' ? 'debug' : 'info', | ||
| format: format.combine( | ||
| format.timestamp({ | ||
| format: 'YYYY-MM-DD HH:mm:ss', | ||
| }), | ||
| format.printf((info) => `${info.timestamp} ${info.level}: ${info.message}`), | ||
| ), | ||
| transports: [ | ||
| new transports.Console({ | ||
| level: 'info', | ||
| format: format.combine(format.printf((info) => `${info.level}: ${info.message}`)), | ||
| }), | ||
| new transports.File({ filename: errorLogPath }), | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logger configuration for production use.
Consider the following improvements:
- Add error handling for file transport
- Configure log levels based on environment
- Add request ID for request tracking
- Add structured logging for better parsing
const logger = createLogger({
level: env === 'development' ? 'debug' : 'info',
format: format.combine(
+ format.errors({ stack: true }),
+ format.json(),
format.timestamp({
format: 'YYYY-MM-DD HH:mm:ss',
}),
- format.printf((info) => `${info.timestamp} ${info.level}: ${info.message}`),
+ format.printf((info) => {
+ const { timestamp, level, message, requestId, ...meta } = info;
+ return JSON.stringify({
+ timestamp,
+ level,
+ message,
+ requestId,
+ ...meta
+ });
+ }),
),
transports: [
new transports.Console({
level: 'info',
- format: format.combine(format.printf((info) => `${info.level}: ${info.message}`)),
+ format: format.combine(
+ format.colorize(),
+ format.printf((info) => `${info.level}: ${info.message}`)
+ ),
}),
- new transports.File({ filename: errorLogPath }),
+ new transports.File({
+ filename: errorLogPath,
+ handleExceptions: true,
+ maxsize: 5242880, // 5MB
+ maxFiles: 5,
+ }),
],
+ exitOnError: false
});📝 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 = createLogger({ | |
| level: env === 'development' ? 'debug' : 'info', | |
| format: format.combine( | |
| format.timestamp({ | |
| format: 'YYYY-MM-DD HH:mm:ss', | |
| }), | |
| format.printf((info) => `${info.timestamp} ${info.level}: ${info.message}`), | |
| ), | |
| transports: [ | |
| new transports.Console({ | |
| level: 'info', | |
| format: format.combine(format.printf((info) => `${info.level}: ${info.message}`)), | |
| }), | |
| new transports.File({ filename: errorLogPath }), | |
| ], | |
| }); | |
| const logger = createLogger({ | |
| level: env === 'development' ? 'debug' : 'info', | |
| format: format.combine( | |
| format.errors({ stack: true }), | |
| format.json(), | |
| format.timestamp({ | |
| format: 'YYYY-MM-DD HH:mm:ss', | |
| }), | |
| format.printf((info) => { | |
| const { timestamp, level, message, requestId, ...meta } = info; | |
| return JSON.stringify({ | |
| timestamp, | |
| level, | |
| message, | |
| requestId, | |
| ...meta | |
| }); | |
| }), | |
| ), | |
| transports: [ | |
| new transports.Console({ | |
| level: 'info', | |
| format: format.combine( | |
| format.colorize(), | |
| format.printf((info) => `${info.level}: ${info.message}`) | |
| ), | |
| }), | |
| new transports.File({ | |
| filename: errorLogPath, | |
| handleExceptions: true, | |
| maxsize: 5242880, // 5MB | |
| maxFiles: 5, | |
| }), | |
| ], | |
| exitOnError: false | |
| }); |
🧰 Tools
🪛 eslint
[error] 16-16: Delete ,
(prettier/prettier)
[error] 18-18: Delete ,
(prettier/prettier)
[error] 23-23: Delete ,
(prettier/prettier)
[error] 25-25: Delete ,
(prettier/prettier)
[error] 26-26: Delete ,
(prettier/prettier)
| if (fs.existsSync(errorLogPath)) { | ||
| fs.unlinkSync(errorLogPath); | ||
| } |
There was a problem hiding this comment.
Reconsider log file deletion strategy.
Deleting the error log file on service startup could result in loss of important error information. Consider implementing log rotation instead.
- if (fs.existsSync(errorLogPath)) {
- fs.unlinkSync(errorLogPath);
- }
+ const { createLogger, format, transports } = require('winston');
+ require('winston-daily-rotate-file');
+
+ // Add to transports array
+ new transports.DailyRotateFile({
+ filename: path.join(path.dirname(errorLogPath), 'error-%DATE%.log'),
+ datePattern: 'YYYY-MM-DD',
+ maxSize: '20m',
+ maxFiles: '14d'
+ })Committable suggestion skipped: line range outside the PR's diff.
| logger.error('Props `title`, `description`, and `image` are required at GenericLayout component.'); | ||
| throw new Error('Props `title`, `description`, and `image` are required at GenericLayout component.'); |
There was a problem hiding this comment.
Fix incorrect component name in error message.
The error message refers to "GenericLayout" instead of "GenericWideLayout".
- logger.error('Props `title`, `description`, and `image` are required at GenericLayout component.');
- throw new Error('Props `title`, `description`, and `image` are required at GenericLayout component.');
+ const errorMessage = 'Props `title`, `description`, and `image` are required at GenericWideLayout component.';
+ logger.error(errorMessage);
+ throw new Error(errorMessage);📝 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.
| logger.error('Props `title`, `description`, and `image` are required at GenericLayout component.'); | |
| throw new Error('Props `title`, `description`, and `image` are required at GenericLayout component.'); | |
| const errorMessage = 'Props `title`, `description`, and `image` are required at GenericWideLayout component.'; | |
| logger.error(errorMessage); | |
| throw new Error(errorMessage); |
| @@ -63,6 +63,7 @@ | |||
| "@types/react-gtm-module": "^2.0.3", | |||
| "@types/react-scrollspy": "^3.3.9", | |||
| "@types/react-typing-animation": "^1.6.6", | |||
| "@types/winston": "^2.4.4", | |||
There was a problem hiding this comment.
Update @types/winston to match winston v3.x.x.
The current type definitions (@types/[email protected]) are for winston v2.x.x, but you're using winston v3.x.x. This version mismatch will cause TypeScript compilation errors.
Replace with the correct type definitions for winston v3:
- "@types/winston": "^2.4.4",
+ "@types/winston": "^3.0.0",📝 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.
| "@types/winston": "^2.4.4", | |
| "@types/winston": "^3.0.0", |
| @@ -115,6 +116,7 @@ | |||
| "tailwind-merge": "^2.2.1", | |||
| "tailwindcss": "^3.4.3", | |||
| "typescript": "^5.3.3", | |||
| "winston": "^3.17.0", | |||
There was a problem hiding this comment.
Fix invalid winston version.
The specified version ^3.17.0 is higher than the latest available version of winston (3.11.0). This will cause npm install failures.
Update the version to the latest stable release:
- "winston": "^3.17.0",
+ "winston": "^3.11.0",📝 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.
| "winston": "^3.17.0", | |
| "winston": "^3.11.0", |
Related issue(s)
Resolves #3358
Description
Configured Winston to log error messages into
logs/error.logAdded unit tests to validate the error logging functionality.
@derberg please review this PR
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
Testing