-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(rest-api): adds path to route request #3231
Conversation
WalkthroughThe changes in this pull request focus on enhancing the logging functionality within an Express application. The log messages for incoming requests and outgoing responses have been modified to include the request path dynamically. Additionally, the outgoing response logs are now conditionally truncated for specific paths to avoid logging sensitive or large payloads. The overall middleware structure remains unchanged. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rest-api/src/app.ts (1)
Line range hint
27-33
: Enhanced logging for outgoing responses with payload size considerationGreat improvements to the outgoing response logging:
- Including
${originalPath}
in the log message enhances traceability.- Conditionally truncating the response body for specific paths ('/' and '/tokenlist') is a good practice to prevent logging sensitive or large payloads.
These changes align well with the PR objective and improve the overall logging strategy.
Minor suggestion: Consider using case-insensitive comparison for the '/tokenlist' path to ensure consistent behavior. You can modify the condition as follows:
- originalPath === '/' || originalPath.toLowerCase() === '/tokenlist' + originalPath === '/' || originalPath.toLowerCase() === '/tokenlist'.toLowerCase()This change ensures that variations like '/TokenList' or '/TOKENLIST' are also captured.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/rest-api/src/app.ts (2 hunks)
🔇 Additional comments (2)
packages/rest-api/src/app.ts (2)
15-15
: Improved logging for incoming requestsThe addition of
${req.path}
to the log message enhances the logging detail, making it easier to track and debug incoming requests. This change aligns well with the PR objective of adding path to route requests.
Line range hint
1-78
: Summary: Improved request and response loggingThe changes in this PR successfully enhance the logging capabilities of the Express application by including the request path in both incoming and outgoing log messages. This improvement aligns well with the PR objective of adding path to route requests.
Key improvements:
- More detailed logging for incoming requests
- Enhanced traceability for outgoing responses
- Consideration for payload size in response logging
These changes will significantly aid in debugging and monitoring the application's behavior. No major issues were found, and the modifications are focused without introducing any breaking changes or security risks.
Great job on improving the logging functionality!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3231 +/- ##
=============================================
Coverage 90.43902% 90.43902%
=============================================
Files 54 54
Lines 1025 1025
Branches 82 82
=============================================
Hits 927 927
Misses 95 95
Partials 3 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 Cloudflare Pages
|
Bundle ReportBundle size has no change ✅ |
Summary by CodeRabbit