feat: add custom LogService with Winston for error logging #3406
Conversation
YashGupt29
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a new logging utility in the project by adding a Changes
Possibly related issues
Poem
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
|
![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!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3406--asyncapi-website.netlify.app/ |
![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
🧹 Outside diff range and nitpick comments (3)
logger.js (1)
26-36: Consider adding log rotation and configurable pathsThe logger configuration looks good overall, but there are some important considerations:
- Add log rotation to prevent logs from growing indefinitely:
const { createLogger, format, transports } = require('winston'); require('winston-daily-rotate-file'); // Add this transport instead of current File transport new transports.DailyRotateFile({ filename: 'logs/error-%DATE%.log', datePattern: 'YYYY-MM-DD', level: 'error', maxSize: '20m', maxFiles: '14d' })
- Consider making log paths configurable through environment variables or configuration file
- Add directory creation/verification before initializing file transports
Would you like me to provide a complete implementation addressing these points?
🧰 Tools
🪛 eslint
[error] 31-31: Delete
··(prettier/prettier)
[error] 34-34: Delete
,(prettier/prettier)
[error] 35-35: Delete
,(prettier/prettier)
scripts/tools/tools-object.js (2)
11-11: LGTM! Minor style suggestion.The logger import is correctly placed with other dependencies.
Consider using single quotes for consistency with other imports:
-const logger = require("../../logger"); +const logger = require('../../logger');🧰 Tools
🪛 eslint
[error] 11-12: Replace
"../../logger");⏎with'../../logger');(prettier/prettier)
115-121: Consider error handling strategy.The current implementation logs the error and then throws it, which could lead to duplicate logging if caught by upper layers.
Consider either:
- Just logging without throwing:
- // console.error(err) logger.error('Error processing tool', { message: err.message, stack: err.stack, tool_name: tool?.name, url: tool?.url - }); - throw err; + });
- Or just throwing without logging here, and handling all logging in a top-level error handler:
- // console.error(err) - logger.error('Error processing tool', { - message: err.message, - stack: err.stack, - tool_name: tool?.name, - url: tool?.url - }); throw err;🧰 Tools
🪛 eslint
[error] 116-116: Delete
··(prettier/prettier)
[error] 117-117: Delete
··(prettier/prettier)
[error] 118-118: Delete
··(prettier/prettier)
[error] 119-119: Delete
··(prettier/prettier)
[error] 120-120: Delete
··(prettier/prettier)
[error] 121-121: Replace
··});·····with});(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
logger.js(1 hunks)package.json(2 hunks)scripts/tools/tools-object.js(2 hunks)
🧰 Additional context used
🪛 eslint
logger.js
[error] 5-5: Delete ·
(prettier/prettier)
[error] 6-6: Delete ·
(prettier/prettier)
[error] 7-7: Replace all:true}),· with ·all:·true·}),
(prettier/prettier)
[error] 8-8: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 8-9: Delete ⏎··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 12-12: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 13-13: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Replace ········ with ······
(prettier/prettier)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ··
(prettier/prettier)
[error] 22-23: Delete ,⏎
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 34-34: Delete ,
(prettier/prettier)
[error] 35-35: Delete ,
(prettier/prettier)
scripts/tools/tools-object.js
[error] 11-12: Replace "../../logger");⏎ with '../../logger');
(prettier/prettier)
[error] 104-104: Delete ··········
(prettier/prettier)
[error] 105-105: Expected exception block, space or tab after '//' in comment.
(spaced-comment)
[error] 105-105: Delete ·
(prettier/prettier)
[error] 106-106: Replace ············ with ··········
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 107-107: 'err' is not defined.
(no-undef)
[error] 108-108: Replace ·············· with ············
(prettier/prettier)
[error] 108-108: 'err' is not defined.
(no-undef)
[error] 109-109: Delete ··
(prettier/prettier)
[error] 110-110: Delete ··
(prettier/prettier)
[error] 111-111: Replace ············});·········· with ··········});
(prettier/prettier)
[error] 116-116: Delete ··
(prettier/prettier)
[error] 117-117: Delete ··
(prettier/prettier)
[error] 118-118: Delete ··
(prettier/prettier)
[error] 119-119: Delete ··
(prettier/prettier)
[error] 120-120: Delete ··
(prettier/prettier)
[error] 121-121: Replace ··});····· with });
(prettier/prettier)
🔇 Additional comments (7)
logger.js (3)
38-38: LGTM!
The module export is clean and follows standard Node.js practices.
1-38: Verify logging implementation completeness
Let's ensure this implementation aligns with the codebase:
#!/bin/bash
# Description: Check for existing logging implementations and verify usage
# Check for any existing logging utilities
echo "Checking for existing logging implementations..."
rg -l "createLogger|winston" --type js
# Verify proper usage in tools-object.js
echo "Verifying logger usage in tools-object.js..."
rg "console\.(log|error|warn)" scripts/tools/tools-object.js🧰 Tools
🪛 eslint
[error] 5-5: Delete ·
(prettier/prettier)
[error] 6-6: Delete ·
(prettier/prettier)
[error] 7-7: Replace all:true}),· with ·all:·true·}),
(prettier/prettier)
[error] 8-8: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 8-9: Delete ⏎··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 12-12: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 13-13: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Replace ········ with ······
(prettier/prettier)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ··
(prettier/prettier)
[error] 22-23: Delete ,⏎
(prettier/prettier)
[error] 31-31: Delete ··
(prettier/prettier)
[error] 34-34: Delete ,
(prettier/prettier)
[error] 35-35: Delete ,
(prettier/prettier)
1-2: Verify package.json dependencies
The imports look correct. Let's verify the dependencies are properly configured in package.json.
✅ Verification successful
Dependencies are properly configured
Both winston (v3.17.0) and winston-timestamp-colorize (v1.1.0) are correctly listed in the package.json dependencies section. The imports in logger.js are properly supported by the project configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if winston dependencies are properly configured in package.json
# Expected: Both winston and winston-timestamp-colorize should be listed in dependencies
jq '.dependencies | has("winston") and has("winston-timestamp-colorize")' package.json
Length of output: 666
Script:
#!/bin/bash
# Let's check the actual content of package.json to verify the dependencies
cat package.json
Length of output: 5576
scripts/tools/tools-object.js (1)
11-11: Verify logger configuration.
Let's ensure the logger is properly configured with appropriate log levels and transports as mentioned in the PR objectives.
✅ Verification successful
Logger configuration is properly set up with comprehensive logging capabilities
The logger implementation is well-configured with:
- Appropriate log levels (default: 'info')
- Console transport with colorized output and timestamps
- File transports for both error-specific ('logs/error.log') and combined logs ('logs/combined.log')
- Required dependencies ([email protected] and [email protected]) are properly specified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check logger configuration
echo "Checking logger configuration..."
rg -A 10 "createLogger|configure" logger.js
# Verify Winston dependencies
echo "Verifying Winston dependencies..."
grep -A 5 '"winston"' package.json
grep -A 5 '"winston-timestamp-colorize"' package.json
Length of output: 1392
🧰 Tools
🪛 eslint
[error] 11-12: Replace "../../logger");⏎ with '../../logger');
(prettier/prettier)
package.json (3)
118-119: Dependencies organization looks good.
The addition of Winston and its colorize plugin aligns well with the PR objectives to implement custom logging with enhanced formatting. The dependencies are properly organized based on their usage.
Also applies to: 156-156
156-156: Verify the impact of moving fast-xml-parser to devDependencies.
The fast-xml-parser package has been moved from dependencies to devDependencies. This change implies it's only needed during development.
✅ Verification successful
Moving fast-xml-parser to devDependencies appears safe
The package is only referenced in package.json and package-lock.json files, with no usage found in the actual source code. This confirms it's safe to keep it in devDependencies since it's not being used in production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if fast-xml-parser is used in production code
# Search for fast-xml-parser imports in source files
echo "Checking for fast-xml-parser usage in source files..."
rg -g '!node_modules' -g '!test' -g '!*.test.*' -g '!*.spec.*' 'require.*fast-xml-parser|from.*fast-xml-parser'
Length of output: 403
Script:
#!/bin/bash
# Search for fast-xml-parser usage with different patterns and file types
echo "Searching for fast-xml-parser imports..."
rg -l "fast-xml-parser" --type-add 'src:*.{js,ts,jsx,tsx}' -g '!{test,tests,__tests__}/**' -g '!**/*.{test,spec}.*'
# Search for XML parsing related functionality
echo -e "\nSearching for XML parsing patterns..."
rg "XMLParser|parseXML|xml.*parse" --type-add 'src:*.{js,ts,jsx,tsx}' -g '!{test,tests,__tests__}/**' -g '!**/*.{test,spec}.*' -A 2
Length of output: 1122
118-119: Consider updating to the latest stable Winston version.
The PR introduces Winston for logging, but version 3.17.0 is ahead of the latest stable release (3.11.0). This could lead to stability issues.
| const customFormat = format.combine( | ||
| format.timestamp({ format: 'YYYY-MM-DD HH:mm:ss' }), | ||
| winstonTimestampColorize(), | ||
| format.colorize({all:true}), | ||
| format.printf(({ level, timestamp, message, stack, tool_name, url }) => { | ||
|
|
||
| let logMessage = `${level} at ${timestamp}`; | ||
| logMessage += `\n${message}`; | ||
| if (tool_name) { | ||
| logMessage += `\n Tool: ${tool_name}`; | ||
| } | ||
| if (url) { | ||
| logMessage += `\n URL: ${url}`; | ||
| } | ||
| if (stack) { | ||
| logMessage += `\n Stack: ${stack}`; | ||
| } | ||
| return logMessage; | ||
| }), | ||
|
|
||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-structured logging format with room for improvement
The custom format provides comprehensive logging with timestamps, colors, and structured output. However, there are a few improvements needed:
- Follow JavaScript naming conventions by using camelCase:
-format.printf(({ level, timestamp, message, stack, tool_name, url }) => {
+format.printf(({ level, timestamp, message, stack, toolName, url }) => {
let logMessage = `${level} at ${timestamp}`;
logMessage += `\n${message}`;
- if (tool_name) {
- logMessage += `\n Tool: ${tool_name}`;
+ if (toolName) {
+ logMessage += `\n Tool: ${toolName}`;
}- Remove unnecessary empty line at line 9
📝 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 customFormat = format.combine( | |
| format.timestamp({ format: 'YYYY-MM-DD HH:mm:ss' }), | |
| winstonTimestampColorize(), | |
| format.colorize({all:true}), | |
| format.printf(({ level, timestamp, message, stack, tool_name, url }) => { | |
| let logMessage = `${level} at ${timestamp}`; | |
| logMessage += `\n${message}`; | |
| if (tool_name) { | |
| logMessage += `\n Tool: ${tool_name}`; | |
| } | |
| if (url) { | |
| logMessage += `\n URL: ${url}`; | |
| } | |
| if (stack) { | |
| logMessage += `\n Stack: ${stack}`; | |
| } | |
| return logMessage; | |
| }), | |
| ); | |
| const customFormat = format.combine( | |
| format.timestamp({ format: 'YYYY-MM-DD HH:mm:ss' }), | |
| winstonTimestampColorize(), | |
| format.colorize({all:true}), | |
| format.printf(({ level, timestamp, message, stack, toolName, url }) => { | |
| let logMessage = `${level} at ${timestamp}`; | |
| logMessage += `\n${message}`; | |
| if (toolName) { | |
| logMessage += `\n Tool: ${toolName}`; | |
| } | |
| if (url) { | |
| logMessage += `\n URL: ${url}`; | |
| } | |
| if (stack) { | |
| logMessage += `\n Stack: ${stack}`; | |
| } | |
| return logMessage; | |
| }), | |
| ); |
🧰 Tools
🪛 eslint
[error] 5-5: Delete ·
(prettier/prettier)
[error] 6-6: Delete ·
(prettier/prettier)
[error] 7-7: Replace all:true}),· with ·all:·true·}),
(prettier/prettier)
[error] 8-8: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 8-9: Delete ⏎··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 12-12: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 13-13: Identifier 'tool_name' is not in camel case.
(camelcase)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Replace ········ with ······
(prettier/prettier)
[error] 17-17: Delete ··
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Delete ··
(prettier/prettier)
[error] 22-23: Delete ,⏎
(prettier/prettier)
| // console.error('Script is not failing, it is just dropping errors for further investigation'); | ||
| // console.error('Invalid .asyncapi-tool file.'); | ||
| // console.error(`Located in: ${tool.html_url}`); | ||
| // console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | ||
|
|
||
| //Created a logger function using Winston and tested also | ||
| logger.error('Error processing tool', { | ||
| message: err.message, | ||
| stack: err.stack, | ||
| tool_name: tool?.name, | ||
| url: tool?.url | ||
| }); |
There was a problem hiding this comment.
Fix validation error logging implementation.
There are several issues in this block:
- The
errvariable is undefined as this is the validation block - The validation errors from
validate.errorsare not being logged - The comment on line 105 is unnecessary
Apply this fix:
- // console.error('Script is not failing, it is just dropping errors for further investigation');
- // console.error('Invalid .asyncapi-tool file.');
- // console.error(`Located in: ${tool.html_url}`);
- // console.error('Validation errors:', JSON.stringify(validate.errors, null, 2));
-
- //Created a logger function using Winston and tested also
- logger.error('Error processing tool', {
- message: err.message,
- stack: err.stack,
- tool_name: tool?.name,
- url: tool?.url
- });
+ logger.error('Invalid .asyncapi-tool file', {
+ validation_errors: validate.errors,
+ tool_name: tool?.name,
+ url: tool?.html_url
+ });📝 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.
| // console.error('Script is not failing, it is just dropping errors for further investigation'); | |
| // console.error('Invalid .asyncapi-tool file.'); | |
| // console.error(`Located in: ${tool.html_url}`); | |
| // console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | |
| //Created a logger function using Winston and tested also | |
| logger.error('Error processing tool', { | |
| message: err.message, | |
| stack: err.stack, | |
| tool_name: tool?.name, | |
| url: tool?.url | |
| }); | |
| logger.error('Invalid .asyncapi-tool file', { | |
| validation_errors: validate.errors, | |
| tool_name: tool?.name, | |
| url: tool?.html_url | |
| }); |
🧰 Tools
🪛 eslint
[error] 104-104: Delete ··········
(prettier/prettier)
[error] 105-105: Expected exception block, space or tab after '//' in comment.
(spaced-comment)
[error] 105-105: Delete ·
(prettier/prettier)
[error] 106-106: Replace ············ with ··········
(prettier/prettier)
[error] 107-107: Delete ··
(prettier/prettier)
[error] 107-107: 'err' is not defined.
(no-undef)
[error] 108-108: Replace ·············· with ············
(prettier/prettier)
[error] 108-108: 'err' is not defined.
(no-undef)
[error] 109-109: Delete ··
(prettier/prettier)
[error] 110-110: Delete ··
(prettier/prettier)
[error] 111-111: Replace ············});·········· with ··········});
(prettier/prettier)
|
Hey @YashGupt29 , please take a look at Contributing.md and follow the guidline for PR title and description. |
YashGupt29
changed the title
|
Hey @SahilDahekar Thank you for sharing the info!! |
akshatnema
changed the title
| if (tool_name) { | ||
| logMessage += `\n Tool: ${tool_name}`; | ||
| } | ||
| if (url) { | ||
| logMessage += `\n URL: ${url}`; | ||
| } | ||
| if (stack) { | ||
| logMessage += `\n Stack: ${stack}`; | ||
| } |
There was a problem hiding this comment.
We need to setup custom logger for whole repo, not just for only one particular script
There was a problem hiding this comment.
okay @akshatnema it will take some time i will do it in the coming days.Is there anything which i need to take care of from your side.
Title:
Issue is #3358 Implement Custom LogService Using Winston for Enhanced Error Logging as suggest by the PR #3265
Description:
As per the discussion in PR #3265 and the request from @akshatnema, this PR implements a custom logging service using Winston to improve error logging throughout the application.
Key changes include:
Configured Winston with custom formatting to include timestamps, log levels, and colorization for better readability.
Introduced multiple transports for logging: console and file-based logs (error.log for errors and combined.log for general logs).
Enhanced log output with additional context, including tool names, URLs, and stack traces, where applicable.
Improved error tracking and visibility in the application.
This implementation ensures that error logs are captured consistently across the application, facilitating easier debugging and issue resolution.
Summary by CodeRabbit
New Features
Bug Fixes
Chores