feat(rest-api): Adds winston logger [SLT-271] #3216
Conversation
abtestingalpha
requested review from
trajan0x,
ChiTimesChi,
bigboydiamonds and
Defi-Moses
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 WalkthroughThe pull request introduces several changes primarily focused on enhancing logging capabilities and modifying ESLint configurations for TypeScript files. The ESLint configuration now allows empty functions and unused variables in TypeScript files. Additionally, a new logging middleware using Winston is added, and various controllers are updated to incorporate structured logging for both successful responses and error handling. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
abtestingalpha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (31)
packages/rest-api/src/controllers/tokenListController.ts (2)
5-5: Logging statement added, with room for improvementThe logging statement is correctly placed and uses an appropriate log level. It provides basic information about the successful operation of the controller.
However, consider enhancing the log message with more details:
- logger.info(`Successful tokenListController response`) + logger.info(`Successful tokenListController response. Returning ${Object.keys(tokenList).length} tokens.`)This change would provide more context about the response, making it easier to monitor and debug the application.
1-7: Consider ensuring logging consistency across controllersThe changes successfully integrate logging into this controller, which aligns well with the PR objectives. To maintain consistency and maximize the benefits of the new logging system:
- Verify that similar logging practices are applied to other controllers in the application.
- Consider creating logging standards or guidelines for the team to follow.
- Ensure that the log levels (info, error, debug, etc.) are used consistently across the application.
Would you like assistance in creating a checklist or guidelines for consistent logging practices?
packages/rest-api/src/middleware/logger.ts (3)
5-17: Good configuration, but consider logging errors in test environment.The transport configuration based on
NODE_ENVis well-structured and follows good practices. It ensures no logs are output during tests, which is generally desirable. However, consider logging errors even in the test environment to catch and debug potential issues during testing.Consider modifying the test environment configuration to log errors:
if (process.env.NODE_ENV === 'test') { transports.push( new winston.transports.Stream({ + level: 'error', stream: new Writable({ - write: () => {}, + write: (chunk, encoding, next) => { + console.error(chunk.toString()); + next(); + }, }), }) ) } else { transports.push(new winston.transports.Console()) }This change will allow error logs to be visible during tests while still suppressing other log levels.
19-26: Good logger configuration, but consider environment-specific settings.The logger configuration is well-structured for JSON logging, which aligns with the PR objective of streaming logs to Railway. However, there are a few points to consider:
- The 'info' log level might be too high for development environments. Consider using environment variables to set different log levels for different environments.
- There's no explicit configuration for different environments (apart from 'test'). It might be beneficial to have more granular control over logger settings based on the environment.
Consider modifying the logger configuration to be more environment-aware:
const getLogLevel = () => { switch (process.env.NODE_ENV) { case 'production': return 'info'; case 'test': return 'error'; default: return 'debug'; } }; export const logger = winston.createLogger({ level: getLogLevel(), format: winston.format.combine( winston.format.timestamp(), winston.format.json() ), transports, });This change allows for more flexible logging configurations across different environments.
1-26: Overall good implementation with room for minor improvements.The logger implementation successfully integrates Winston and aligns with the PR objective of enabling JSON logging for Railway. The environment-specific configurations are a good start, but could be further refined as suggested in previous comments.
One area that hasn't been addressed is error handling. While Winston is generally robust, it's good practice to implement error handling for logger initialization.
Consider wrapping the logger creation in a try-catch block:
let logger; try { logger = winston.createLogger({ // ... existing configuration ... }); } catch (error) { console.error('Failed to initialize logger:', error); // Fallback to a basic console logger logger = { error: console.error, warn: console.warn, info: console.info, debug: console.debug, }; } export { logger };This ensures that even if logger initialization fails, the application will still have a basic logging capability.
packages/rest-api/src/controllers/synapseTxIdController.ts (1)
28-31: Good addition: Error logging implemented.The error logging implementation is well done. It uses the appropriate log level (error) and includes both the error message and stack trace, which will be invaluable for debugging. The log message clearly identifies the controller where the error occurred.
Consider adding request details (like
originChainId,bridgeModule, andtxHash) to the error log. This would provide more context for debugging:logger.error(`Error in synapseTxIdController`, { error: err.message, stack: err.stack, + request: { originChainId, bridgeModule, txHash }, })packages/rest-api/src/controllers/destinationTokensController.ts (3)
20-20: Approved: Variable renamed for clarity.The renaming of
optionstopayloadimproves code readability and aligns better with API terminology.Consider updating any related comments or documentation that might still reference
optionsto maintain consistency.
22-23: LGTM: Success logging implemented.The addition of success logging enhances the monitoring capabilities of the application.
Consider a minor improvement to the log message for consistency:
- logger.info(`Successful destinationnTokensController response`, { payload }) + logger.info(`Successful destinationTokensController response`, { payload })This corrects the typo in "destinationnTokensController".
26-29: Approved: Comprehensive error logging implemented.The enhanced error logging provides valuable information for debugging, capturing both the error message and stack trace.
For consistency with the success log, consider including the controller name in the error message:
- logger.error(`Error in destinationTokensController`, { + logger.error(`Error in destinationTokensController`, {packages/rest-api/src/controllers/indexController.ts (2)
17-24: Improved response structure, consider updating API documentation.The restructured response using a
payloadobject improves code organization and provides a more informative API output. This change enhances readability and maintainability.Consider updating the API documentation to reflect this change in response structure, as it might affect clients consuming this API.
Also applies to: 25-25, 27-27
26-26: Success logging added, consider including response details.The addition of success logging is beneficial for monitoring and debugging.
Consider enhancing the log message by including some key details from the response, such as the number of available chains and tokens. This could provide more context in the logs without being overly verbose.
packages/rest-api/src/app.ts (4)
13-15: LGTM: Server listening log updated to use logger.The change from console.log to logger.info is appropriate and aligns with the integration of the winston logger. The log message is clear and informative.
Consider adding more context to the log message, such as the environment or application name. For example:
logger.info(`Server is listening on port ${port}`, { env: process.env.NODE_ENV, app: 'rest-api' })This additional context can be valuable when aggregating logs from multiple services.
35-38: Good addition of error handling middleware, with room for improvement.The error handling middleware is a good addition, using the logger to record errors and providing a safe, generic response to the client.
Consider the following improvements:
- Add more context to the error log, such as request details:
logger.error(`Express error: ${err.message}`, { stack: err.stack, method: _req.method, url: _req.url, body: _req.body, headers: _req.headers })
- Differentiate between different types of errors:
let statusCode = 500; if (err instanceof SomeSpecificError) { statusCode = 400; } else if (err instanceof AnotherTypeOfError) { statusCode = 404; } res.status(statusCode).json({ error: 'Something went wrong', details: err.message })
- In production, consider not sending the error.message to the client for security reasons:
const details = process.env.NODE_ENV === 'production' ? 'An error occurred' : err.message; res.status(statusCode).json({ error: 'Something went wrong', details })These changes would provide more detailed logging for debugging, better error handling, and improved security in production.
40-43: Good addition of uncaught exception handler, with a suggestion for improvement.The uncaught exception handler is a good addition, properly logging the error before exiting the process.
Consider implementing a graceful shutdown process:
process.on('uncaughtException', (err) => { logger.error(`Uncaught Exception: ${err.message}`, { stack: err.stack }) // Attempt to close servers, finish remaining requests, etc. if (server) { server.close(() => { logger.info('Server closed') process.exit(1) }) // If server hasn't finished in 5s, shut down process setTimeout(() => { process.exit(1) }, 5000) } else { process.exit(1) } })This approach allows for a more graceful shutdown, attempting to close the server and any other resources before exiting. It also sets a timeout to ensure the process exits even if the server close operation hangs.
45-47: Good addition of unhandled rejection handler, with a suggestion for improvement.The unhandled rejection handler is a good addition, properly logging the rejection details.
Consider structuring the log message for easier parsing and analysis:
process.on('unhandledRejection', (reason, promise) => { logger.error('Unhandled Rejection', { reason: reason instanceof Error ? reason.message : reason, stack: reason instanceof Error ? reason.stack : undefined, promise: promise.toString() }) })This approach provides a more structured log entry, which can be easier to parse and analyze in log management systems. It also handles cases where the reason might not be an Error object.
packages/rest-api/package.json (1)
33-34: LGTM! Consider adding types for winston.The addition of winston logger (version 3.14.2) aligns well with the PR objective of integrating the winston logging library. The use of the caret (^) notation is appropriate for allowing compatible updates.
Consider adding
@types/winstonto thedevDependenciessection to improve TypeScript support:"devDependencies": { ... + "@types/winston": "^3.0.0", ... }packages/rest-api/src/controllers/bridgeTxInfoController.ts (2)
56-56: Approve with minor correction: Fix typo in log message.The logging statement is well-placed and provides valuable context. However, there's a small typo in the message.
Please correct the typo in the log message:
- logger.info(`Succesful bridgeTxInfoController response`, { txInfoArray }) + logger.info(`Successful bridgeTxInfoController response`, { txInfoArray })
Line range hint
6-62: Overall implementation aligns well with PR objectives.The integration of the Winston logger has been successfully implemented, meeting the PR objectives:
- Logging is added for both success and error scenarios.
- The logs provide valuable context for monitoring and debugging.
- The use of structured logging (JSON format) is implicit in the Winston logger setup.
To further enhance the logging capabilities, consider:
- Adding a log at the beginning of the controller function to track incoming requests.
- Logging intermediate steps, such as the retrieval of quotes, to provide more granular insights into the function's execution.
These additions would provide a more comprehensive view of the controller's operation, further improving monitoring and debugging capabilities.
packages/rest-api/src/controllers/bridgeController.ts (3)
58-58: Consider refining the logged information.The logging statement for successful responses is a good addition. However, consider the following suggestions:
- Instead of logging the entire payload, which might lead to verbose logs, consider logging only essential information such as request identifiers, fromChain, toChain, and perhaps a hash of the payload.
- Ensure that no sensitive information (e.g., user addresses, exact amounts) is being logged to comply with data protection regulations.
Here's a suggested refinement:
logger.info(`Successful bridgeController response`, { requestId: req.id, // Assuming you have request IDs fromChain, toChain, payloadHash: crypto.createHash('sha256').update(JSON.stringify(payload)).digest('hex') });This approach provides necessary context without potentially exposing sensitive data.
61-64: LGTM: Comprehensive error logging implemented.The error logging implementation is well-placed and captures essential information for debugging. Great job including both the error message and stack trace.
Consider adding more context to the error log, such as the request parameters, to make debugging easier:
logger.error(`Error in bridgeController`, { error: err.message, stack: err.stack, requestParams: { fromChain, toChain, amount, fromToken, toToken, originUserAddress } });This addition would provide more context about the state of the request when the error occurred, potentially speeding up the debugging process.
7-7: Overall: Logging implementation aligns with PR objectives.The changes successfully integrate the winston logger into the
bridgeController. The implementation covers both success and error scenarios, which will greatly enhance monitoring and debugging capabilities.A few suggestions for further improvement:
- Refine the success log to be more concise and avoid potential sensitive data exposure.
- Enhance the error log with additional context (e.g., request parameters).
- Consider implementing log rotation and retention policies to manage log file sizes and comply with data retention regulations.
To fully leverage the new logging capabilities, consider implementing centralized log management and analysis tools. This will help in aggregating logs from different parts of the application and provide better insights into system behavior and potential issues.
Also applies to: 58-58, 61-64
packages/rest-api/src/controllers/destinationTxController.ts (3)
58-69: Improved response structure and added logging.The changes enhance the response clarity by including a status field and improve debugging capabilities with the added logging. Good job on formatting the value using token decimals for better readability.
Consider destructuring
tokenInfoto directly access thesymbolproperty:- tokenSymbol: tokenInfo ? tokenInfo?.symbol : null, + tokenSymbol: tokenInfo?.symbol ?? null,This change simplifies the code and uses the nullish coalescing operator for a more concise expression.
80-83: Enhanced error logging implemented.The addition of error logging, including both the error message and stack trace, significantly improves the ability to debug and monitor the application.
Consider adding more context to the error log by including the request parameters:
logger.error(`Error in destinationTxController`, { error: err.message, stack: err.stack, + query: req.query, })This additional information can help in reproducing and diagnosing issues more effectively.
Line range hint
1-91: Overall improvements in logging and response structure.The changes successfully integrate the winston logger and improve the response structure, aligning well with the PR objectives. The consistent use of logging throughout the controller enhances debuggability and monitoring capabilities.
Consider the following suggestions for further improvements:
Implement type safety:
- Use TypeScript interfaces or types for the request query parameters and the response payload.
- This will improve code reliability and provide better IDE support.
Enhance error handling:
- Implement custom error classes for different types of errors (e.g., ValidationError, GraphQLError).
- Use these custom errors to provide more specific error responses to the client.
Consider moving the GraphQL query to a separate file or constant:
- This would improve maintainability and allow for easier testing of the query independently.
Implement request tracing:
- Add a unique identifier to each request and include it in all log messages related to that request.
- This will make it easier to trace the flow of a single request through the system.
These improvements will further enhance the robustness and maintainability of the code.
packages/rest-api/src/controllers/bridgeTxStatusController.ts (5)
60-69: LGTM: Payload object and logging implemented effectively.The introduction of the payload object and logging statement improves code structure and traceability. The use of structured logging is a good practice.
Consider adding more context to the log message, such as including
synapseTxIdordestChainId:-logger.info(`Successful bridgeTxStatusController response`, { payload }) +logger.info(`Successful bridgeTxStatusController response for txId: ${synapseTxId}`, { payload, destChainId })
71-77: LGTM: Consistent implementation for null toInfo case.The structure is consistent with the previous case, which is good for maintainability.
For consistency with the previous suggestion, consider adding more context to the log message:
-logger.info(`Successful bridgeTxStatusController response`, { payload }) +logger.info(`Successful bridgeTxStatusController response for txId: ${synapseTxId} (null toInfo)`, { payload, destChainId })
80-83: LGTM: Consistent implementation for status-only case.The structure remains consistent with the previous cases, which is excellent for maintainability.
For consistency with the previous suggestions, consider adding more context to the log message:
-logger.info(`Successful bridgeTxStatusController response`, { payload }) +logger.info(`Successful bridgeTxStatusController response for txId: ${synapseTxId} (status only)`, { payload, destChainId })
86-89: LGTM: Comprehensive error logging implemented.The error logging includes both the error message and stack trace, which is excellent for debugging purposes. The use of structured logging is consistent with other logging statements.
Consider adding more context to the error log, such as including
synapseTxIdordestChainId:logger.error(`Error in bridgeTxStatusController`, { + synapseTxId, + destChainId, error: err.message, stack: err.stack, })
Line range hint
6-89: Overall improvements with logging implementation, consider further refactoring.The addition of the logger significantly enhances the traceability and debugging capabilities of the
bridgeTxStatusController. The consistent use of structured logging and payload objects improves code readability and maintainability.Consider refactoring the repeated logging pattern into a separate function to reduce code duplication:
function logAndRespond(res, payload, context = '') { logger.info(`Successful bridgeTxStatusController response${context ? ` ${context}` : ''}`, { payload, synapseTxId, destChainId }); res.json(payload); } // Usage: logAndRespond(res, payload, `for txId: ${synapseTxId}`); logAndRespond(res, payload, `for txId: ${synapseTxId} (null toInfo)`); logAndRespond(res, payload, `for txId: ${synapseTxId} (status only)`);This refactoring would make the code more DRY and easier to maintain.
packages/rest-api/src/controllers/bridgeLimitsController.ts (2)
103-103: Good addition of success logging, but there's a typo.Adding a log for successful responses is beneficial for monitoring and debugging. However, there's a typo in the log message.
Please correct the typo in the log message:
- logger.info(`Succesful bridgeLimitsController response`, { payload }) + logger.info(`Successful bridgeLimitsController response`, { payload })
Line range hint
8-109: Overall, excellent implementation of logging functionality.The changes successfully integrate the winston logger, improving the application's monitoring and debugging capabilities. The structured payload and comprehensive logging of both successful responses and errors align well with the PR objectives.
To further enhance the implementation, consider the following suggestion:
Add a constant or configuration value for the log level (e.g., 'info', 'error') to make it easier to adjust log verbosity in different environments. For example:
const LOG_LEVEL = process.env.LOG_LEVEL || 'info'; // Then use it in your logging calls logger[LOG_LEVEL](`Successful bridgeLimitsController response`, { payload });This approach allows for more flexible control over logging output across different deployment environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
- packages/rest-api/.eslintrc.js (1 hunks)
- packages/rest-api/package.json (1 hunks)
- packages/rest-api/src/app.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeController.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeLimitsController.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeTxInfoController.ts (2 hunks)
- packages/rest-api/src/controllers/bridgeTxStatusController.ts (2 hunks)
- packages/rest-api/src/controllers/destinationTokensController.ts (2 hunks)
- packages/rest-api/src/controllers/destinationTxController.ts (2 hunks)
- packages/rest-api/src/controllers/indexController.ts (2 hunks)
- packages/rest-api/src/controllers/swapController.ts (2 hunks)
- packages/rest-api/src/controllers/swapTxInfoController.ts (2 hunks)
- packages/rest-api/src/controllers/synapseTxIdController.ts (2 hunks)
- packages/rest-api/src/controllers/tokenListController.ts (1 hunks)
- packages/rest-api/src/middleware/logger.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rest-api/src/controllers/swapTxInfoController.ts (1)
Learnt from: Defi-Moses PR: synapsecns/sanguine#3133 File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14 Timestamp: 2024-09-17T23:09:44.463Z Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
🔇 Additional comments (32)
packages/rest-api/src/controllers/tokenListController.ts (1)
2-2: LGTM: Logger import added correctlyThe import statement for the logger is correctly added, which aligns with the PR objective of integrating the winston logger. This promotes better logging practices and modularity in the codebase.
packages/rest-api/.eslintrc.js (1)
15-16:⚠️ Potential issueReconsider disabling these TypeScript ESLint rules
The changes to disable
@typescript-eslint/no-empty-functionand@typescript-eslint/no-unused-varsfor TypeScript files may lead to potential code quality issues:
- Allowing empty functions might result in overlooking unintentionally empty implementations.
- Permitting unused variables can lead to code clutter and confusion.
These rules are generally beneficial for maintaining clean and efficient code. Consider the following alternatives:
- Instead of disabling
no-empty-function, use// eslint-disable-next-linecomments for specific instances where empty functions are intentional.- For
no-unused-vars, consider using the"args": "none"option to allow unused function parameters while still flagging other unused variables.If these rules are being disabled to address specific issues related to the winston logger integration, please provide more context so we can explore more targeted solutions.
To understand the impact of these changes, let's check for empty functions and unused variables in the TypeScript files:
packages/rest-api/src/middleware/logger.ts (1)
1-3: LGTM: Imports are correct and necessary.The imports for
Writablefrom 'stream' andwinstonare appropriate for the logger configuration. They follow best practices and there are no unused imports.packages/rest-api/src/controllers/synapseTxIdController.ts (3)
4-4: LGTM: Logger import added successfully.The addition of the logger import aligns with the PR objective of integrating the winston logger. Importing from a middleware file is a good practice for maintaining separation of concerns.
21-24: Good improvement: Response encapsulated in a payload object.Encapsulating the
synapseTxIdin a payload object is a good practice. It enhances API consistency and allows for easier addition of metadata or additional fields in the future. This change is also backwards compatible.
25-26: Well implemented: Success logging added.The addition of success logging is well implemented. It uses the appropriate log level (info) and includes the entire payload, which will be helpful for debugging. The log message is clear and includes the controller name for easy identification.
packages/rest-api/src/controllers/destinationTokensController.ts (2)
5-5: LGTM: Logger import added successfully.The addition of the logger import aligns with the PR objective of integrating the winston logger into the project.
Line range hint
1-36: Summary: Winston logger successfully integrated.The changes in this file successfully implement the winston logger as per the PR objectives. The logging has been added for both successful operations and error scenarios, enhancing the monitoring and debugging capabilities of the application. The implementation streams logs in JSON format, which aligns with the goal of improving log streaming to Railway.
A few minor suggestions have been made for consistency and clarity, but overall, the implementation is solid and achieves the intended purpose.
packages/rest-api/src/controllers/indexController.ts (2)
3-3: LGTM: Logger import added successfully.The addition of the logger import aligns with the PR objective and follows good practices by using a dedicated middleware for logging.
29-32: Comprehensive error logging implemented.The addition of error logging, including both the error message and stack trace, significantly improves debugging capabilities. This change aligns well with the PR objective of enhancing logging functionality.
The error response to the client remains unchanged, which maintains backwards compatibility for API consumers.
packages/rest-api/src/app.ts (2)
6-6: LGTM: Logger import added correctly.The import statement for the logger middleware is correctly placed and aligns with the PR objective of integrating the winston logger.
Line range hint
1-47: Overall, excellent integration of winston logger with room for minor enhancements.The changes successfully integrate the winston logger into the application, covering server startup, error handling, uncaught exceptions, and unhandled rejections. This aligns well with the PR objective and significantly improves the application's logging capabilities.
The suggestions provided for each segment aim to enhance the logging further by adding more context, improving error handling, and ensuring graceful shutdowns. These improvements would make the logs more useful for debugging and monitoring purposes.
Great job on this implementation! The suggested enhancements are optional and can be considered for future iterations if not addressed in this PR.
packages/rest-api/src/controllers/swapTxInfoController.ts (5)
6-6: LGTM: Logger import added correctly.The import statement for the logger is correctly added and follows the project's import style.
39-42: LGTM: Comprehensive error logging implemented.The error logging implementation is well done. It logs both the error message and stack trace, which will be valuable for debugging. The error response to the client appropriately excludes sensitive details.
Line range hint
1-48: Overall: Good implementation of logging, with minor points to address.The changes successfully integrate the winston logger into the
swapTxInfoController, aligning with the PR objectives. The logging implementation covers both successful operations and error scenarios, which will enhance monitoring and debugging capabilities.Key points to address:
- Verify that the renaming of
txInfotopayloaddoesn't break any existing code.- Review the contents of the
payloadto ensure no sensitive information is being logged.These changes will significantly improve the application's observability when deployed to Railway.
28-28: Approve renaming, but verify impact on codebase.The renaming from
txInfotopayloadimproves clarity. However, ensure that this change doesn't break any code that might be relying on thetxInfovariable name.Let's verify the impact of this change:
#!/bin/bash # Search for any remaining usage of 'txInfo' in the codebase rg 'txInfo' --type typescript
36-36: Approve logging, but review for sensitive data.The addition of logging for successful responses is good for monitoring and debugging. However, please review the
payloadcontent to ensure no sensitive information is being logged.Let's check the contents of the payload:
packages/rest-api/src/controllers/swapController.ts (5)
7-7: LGTM: Logger import added successfully.The import of the
loggerfrom the middleware aligns with the PR objective of integrating the winston logging library.
33-36: LGTM: Improved response structure.The creation of a separate
payloadobject with formattedmaxAmountOutimproves code readability and ensures consistent data representation in the response.
38-38: LGTM: Informative logging added.The addition of
logger.infofor successful responses enhances the application's observability. The log message is clear and includes the full payload, which will be valuable for debugging and monitoring.
41-44: LGTM: Comprehensive error logging implemented.The addition of
logger.errorfor error cases significantly improves the ability to debug issues. Including both the error message and stack trace provides valuable information for troubleshooting.
Line range hint
7-44: Great job implementing winston logger!The changes successfully integrate the winston logger into the
swapController, enhancing the application's observability and debuggability. The modifications include:
- Proper import of the logger
- Improved response structure with formatted data
- Informative logging for successful responses
- Comprehensive error logging
These changes align perfectly with the PR objectives and follow best practices for logging. They will significantly improve monitoring and debugging processes.
packages/rest-api/package.json (1)
Line range hint
1-54: Changes align well with PR objectives.The addition of the winston logger dependency is the only change in this file, which aligns perfectly with the PR objective of integrating the winston logging library for improved logging capabilities. The change is minimal and focused, maintaining the integrity of the existing project setup.
packages/rest-api/src/controllers/bridgeTxInfoController.ts (2)
6-6: LGTM: Logger import added correctly.The import statement for the logger is appropriately placed and aligns with the PR objective of integrating the Winston logger.
59-62: LGTM: Error logging implemented correctly.The error logging is well-implemented:
- Appropriate log level (error) is used.
- Comprehensive error information is logged, including both the error message and stack trace.
- The logging statement is correctly placed in the catch block.
This implementation will greatly aid in debugging and aligns well with the PR objective of improving monitoring capabilities.
packages/rest-api/src/controllers/bridgeController.ts (1)
7-7: LGTM: Logger import added successfully.The import of the logger from the middleware aligns with the PR objective of integrating the winston logger into the project.
packages/rest-api/src/controllers/destinationTxController.ts (2)
6-6: LGTM: Logger import added successfully.The addition of the logger import aligns with the PR objective of integrating the winston logger into the project.
71-77: Consistent pending response structure and logging added.The changes maintain consistency with the successful response structure, which is a good practice for API design. The addition of logging for the pending response enhances monitoring capabilities.
packages/rest-api/src/controllers/bridgeTxStatusController.ts (1)
6-6: LGTM: Logger import added correctly.The logger import is appropriately placed and follows good practices for organizing custom middleware.
packages/rest-api/src/controllers/bridgeLimitsController.ts (3)
8-8: LGTM: Logger import added correctly.The logger import is appropriately placed and consistent with the project's import style.
98-101: Good: Response data structured into a payload object.Creating a
payloadobject improves the response structure and makes it easier to add or modify response fields in the future.
106-109: Excellent: Comprehensive error logging added.The addition of error logging with both the error message and stack trace is a great improvement. This will significantly aid in debugging and monitoring the application.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3216 +/- ##
============================================
Coverage ? 90.56974%
============================================
Files ? 54
Lines ? 1018
Branches ? 82
============================================
Hits ? 922
Misses ? 93
Partials ? 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying sanguine-fe with
|
| Latest commit: |
be91356
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d264d7fc.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-logging.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Gave an approval/tested but couple of qs
-
Do we not want to log the request along with whatever is returned ? Would be a lot more information logged but may be easier to read/see errors
-
We dont log errors caught by /routes whats the logic here ? Not sure how helpful this would be but would be interesting to see around what type of requests people make that are failing.
Just trying to think a bit about what we want to accomplish here, how to make this useful
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/rest-api/src/app.ts (3)
13-38: Effective logging middleware implemented, with room for improvementThe middleware successfully logs both incoming requests and outgoing responses, with good practices such as:
- Only logging request body for POST and PUT methods
- Truncating large response bodies
However, consider the following improvements:
- Separate the request and response logging into two middleware functions for better separation of concerns.
- Move the response logging logic into a separate function for improved readability.
- Consider using environment variables or configuration files for paths that require truncation, making it more maintainable.
Here's a suggested refactoring:
const logRequest = (req, res, next) => { logger.info('Incoming request', { method: req.method, path: req.path, query: req.query, body: ['POST', 'PUT'].includes(req.method) ? req.body : undefined, }); next(); }; const logResponse = (req, res, next) => { const originalJson = res.json; res.json = function (body) { const truncatedPaths = ['/', '/tokenlist']; logger.info('Outgoing response', { method: req.method, path: req.path, statusCode: res.statusCode, body: truncatedPaths.includes(req.path.toLowerCase()) ? '[truncated for size]' : body, }); return originalJson.call(this, body); }; next(); }; app.use(logRequest); app.use(logResponse);This refactoring improves readability and maintainability while preserving the original functionality.
62-65: Error handling middleware added, but could be improvedThe new error handling middleware successfully logs errors and provides a response. However, consider the following improvements:
- Use appropriate status codes based on the error type, not always 500.
- Avoid sending error details in the response for security reasons.
- Consider adding more context to the logged error, such as request details.
Here's a suggested improvement:
app.use((err, req, res, _next) => { logger.error(`Express error: ${err.message}`, { stack: err.stack, method: req.method, path: req.path, query: req.query }); const statusCode = err.statusCode || 500; const message = statusCode === 500 ? 'Internal Server Error' : err.message; res.status(statusCode).json({ error: message }); });This version provides more context in the logs, uses appropriate status codes, and avoids exposing sensitive error details in the response.
72-73: Unhandled rejection handler added, but could be improvedThe unhandled rejection handler is implemented to log the rejection, which is good. However, consider the following improvements:
- Consider treating unhandled rejections similarly to uncaught exceptions, potentially exiting the process or attempting graceful shutdown.
- Add more context to the logged information, such as the current date and time.
- Use a more structured logging format for easier parsing and analysis.
Here's a suggested improvement:
process.on('unhandledRejection', (reason, promise) => { logger.error('Unhandled Rejection', { reason: reason instanceof Error ? reason.stack : reason, promise: promise.toString(), timestamp: new Date().toISOString() }); // Consider adding process.exit(1) here if you want to treat unhandled rejections like uncaught exceptions });This version provides more structured and detailed logging, which can be beneficial for debugging and monitoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rest-api/src/app.ts (2 hunks)
🔇 Additional comments (3)
packages/rest-api/src/app.ts (3)
6-6: LGTM: Logger import added correctlyThe import statement for the logger is correctly placed and follows good practices for modularity.
40-42: LGTM: Server listening log updated to use loggerThe server listening log has been correctly updated to use the new logger instead of console.log. The log message is informative and includes the port number, which is helpful for debugging and monitoring.
67-70: Uncaught exception handler addedThe uncaught exception handler is correctly implemented to log the error. However, consider the following:
- Exiting the process immediately might not be suitable for all production environments. In some cases, you might want to attempt graceful shutdown or recovery.
- Consider adding more context to the logged error, such as the current date and time.
To ensure this handler doesn't conflict with any existing error handling, please run the following command:
This will help verify if there are any other uncaught exception handlers in the codebase that might conflict with this new implementation.
✅ Verification successful
Uncaught exception handler verification
No additional uncaught exception handlers found in the codebase. The handler in
packages/rest-api/src/app.tsis the only one, ensuring no potential conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg -i "uncaughtException" --type tsLength of output: 107
Bundle ReportChanges will decrease total bundle size by 11.22kB (-0.03%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
Description
Adds
winstonfor logging. Logs will be streamed as json to Railway.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
winston).