rest-api(fix): Denest keys for logs #3230
Conversation
WalkthroughThe changes in this pull request focus on enhancing the logging functionality within an Express application. The log message structure for incoming requests and outgoing responses has been updated to use an object format, encapsulating messages within an object that includes a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
Deploying sanguine-fe with
|
| Latest commit: |
5a6297f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://886d1e2f.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-denest-keys.sanguine-fe.pages.dev |
Deploying sanguine-fe with
|
| Latest commit: |
b1db615
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://90fcc8c8.sanguine-fe.pages.dev |
| Branch Preview URL: | https://rest-api-denest-keys.sanguine-fe.pages.dev |
![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 (2)
packages/rest-api/src/app.ts (2)
Line range hint
14-20: Improved logging structure for incoming requests.The change to an object-based logging format is a good improvement. It provides better structure and makes the logs easier to parse programmatically. The conditional inclusion of the request body for POST and PUT methods is also a good practice for data privacy.
Consider adding a
headersfield to the log object, excluding any sensitive information. This could provide valuable debugging information without compromising security. For example:headers: Object.entries(req.headers).reduce((acc, [key, value]) => { if (!['authorization', 'cookie'].includes(key.toLowerCase())) { acc[key] = value; } return acc; }, {}),This would log all headers except for 'authorization' and 'cookie'.
Line range hint
26-34: Enhanced logging structure for outgoing responses with size management.The change to an object-based logging format for outgoing responses is a good improvement, providing consistency with the incoming request logs and better structure for parsing. The conditional truncation of the response body is a smart approach to manage log size for potentially large responses.
Consider the following improvements:
- Instead of hardcoding the paths for truncation, create a configurable list of paths or a function to determine if a path should be truncated. This will make it easier to maintain as new routes are added. For example:
const shouldTruncateBody = (path: string) => { const truncatePaths = ['/', '/tokenlist']; return truncatePaths.some(p => path.toLowerCase().startsWith(p.toLowerCase())); }; // Then in the logging code: body: shouldTruncateBody(originalPath) ? '[truncated for size]' : body,
- To enhance security, consider implementing a function to sanitize the response body before logging, removing or masking sensitive information. This could be applied to all non-truncated responses:
const sanitizeBody = (body: any) => { // Implement logic to remove or mask sensitive fields // This is a simple example and should be adapted to your specific needs const sanitized = { ...body }; if (sanitized.password) sanitized.password = '[REDACTED]'; if (sanitized.token) sanitized.token = '[REDACTED]'; return sanitized; }; // Then in the logging code: body: shouldTruncateBody(originalPath) ? '[truncated for size]' : sanitizeBody(body),These changes will make the logging more flexible and secure as your API evolves.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rest-api/src/app.ts (2 hunks)
- packages/rest-api/src/middleware/logger.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/rest-api/src/middleware/logger.ts
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (1)
packages/rest-api/src/middleware/logger.ts (1)
23-30: 🛠️ Refactor suggestionRefactor the logger format function for improved robustness
The new format function successfully denests keys for logs, which aligns with the PR objective. However, there are a few areas where we can improve the implementation:
- Instead of modifying the
infoobject directly, consider creating a new object to avoid potential side effects.- Use a deep merge function instead of
Object.assignto handle nested objects correctly.- Consider keeping the original
messageproperty, as other format functions might expect it.Here's a suggested refactoring:
winston.format((info) => { const result = { ...info }; if (typeof info.message === 'object') { result = deepMerge(result, info.message); } result.msg = result.msg || result.message; return result; })(),This approach preserves the original object, handles nested objects correctly, and keeps the
messageproperty intact.To ensure this change doesn't break existing logging behavior, we should verify how the logger is used throughout the codebase. Run the following script:
Regarding the static analysis hint about using the
deleteoperator: While it's generally good to avoiddeletefor performance reasons, in this logging context, the performance impact is negligible. However, our refactored version addresses this concern by not usingdeleteat all.✅ Verification successful
Logger usage verified and refactoring is safe
The proposed refactoring of the logger format function has been reviewed against the codebase. Extensive usage of the logger across various modules was analyzed, and no conflicts or issues were found that would impede the suggested changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logger usage in the codebase rg --type typescript 'logger\.' -A 3 -B 1Length of output: 78
Script:
#!/bin/bash # Search for logger usage in TypeScript files rg 'logger\.' --glob '*.ts' --glob '*.tsx' -A 3 -B 1Length of output: 11725
🧰 Tools
🪛 Biome
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3230 +/- ##
============================================
Coverage ? 90.43902%
============================================
Files ? 54
Lines ? 1025
Branches ? 82
============================================
Hits ? 927
Misses ? 95
Partials ? 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Bundle ReportBundle size has no change ✅ |
Summary by CodeRabbit
Bug Fixes
New Features