-
Notifications
You must be signed in to change notification settings - Fork 11
feat(logger): log memory usage #1642
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
Conversation
WalkthroughThe updateTracker middleware was modified to log detailed memory usage statistics in megabytes when a response closes. Additionally, the logger service now serializes context values using JSON.stringify, ensuring accurate representation of complex values in log messages. No changes were made to the signatures of exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware (updateTracker)
participant Logger
Client->>Middleware (updateTracker): Send request
Middleware (updateTracker)->>Middleware (updateTracker): Process request
Middleware (updateTracker)->>Middleware (updateTracker): Collect memory usage stats
Middleware (updateTracker)->>Logger: Log response with memory_usage in context (JSON.stringify values)
Logger-->>Middleware (updateTracker): Log entry recorded
Middleware (updateTracker)-->>Client: Send response
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
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
🧹 Nitpick comments (2)
src/v3/services/logger/logger.ts (1)
19-19
: Good change for object serialization, but consider performance and security implications.Using
JSON.stringify
enables proper logging of complex objects like the newmemory_usage
data. However, be aware of potential impacts:
- Performance:
JSON.stringify
can be expensive for large objects on high-traffic endpoints- Security: This may expose more sensitive data in logs than the previous string interpolation
- Error handling: Consider wrapping in try-catch for circular reference protection
For safer implementation, consider:
-.map(([key, value]) => `${key}=${JSON.stringify(value)}`) +.map(([key, value]) => { + try { + return `${key}=${JSON.stringify(value)}`; + } catch (err) { + return `${key}=[Circular/Error]`; + } +})src/v3/middleware/update_tracker.ts (1)
51-59
: Memory tracking implementation looks correct, but consider performance impact.The memory usage collection logic is well-implemented with proper byte-to-megabyte conversion and consistent naming. However, consider the performance implications:
process.memoryUsage()
is called on every request completion- This could impact performance on high-traffic endpoints
- Consider sampling memory usage (e.g., every Nth request) for production environments
For better performance in high-traffic scenarios, consider sampling:
+// Sample memory usage every 100 requests +let requestCount = 0; res.on('close', () => { const [s, ns] = process.hrtime(start); const ms = Math.round(s * 1e3 + ns / 1e6); const message = (res.locals.message ?? '').replace(/\n|\r/g, ''); const user = res.locals.currentUser; + let memory_usage; + if (++requestCount % 100 === 0) { const memory = process.memoryUsage(); - - const memory_usage = { + memory_usage = { rssMB: +(memory.rss / 1024 / 1024).toFixed(2), heapTotalMB: +(memory.heapTotal / 1024 / 1024).toFixed(2), heapUsedMB: +(memory.heapUsed / 1024 / 1024).toFixed(2), externalMB: +(memory.external / 1024 / 1024).toFixed(2), arrayBuffersMB: +(memory.arrayBuffers / 1024 / 1024).toFixed(2), }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/v3/middleware/update_tracker.ts
(2 hunks)src/v3/services/logger/logger.ts
(1 hunks)
🔇 Additional comments (1)
src/v3/middleware/update_tracker.ts (1)
72-72
: Clean integration of memory usage into logging context.The addition of
memory_usage
to the context object is well-implemented and follows the existing pattern. This will be properly serialized by the updated logger service.
🎉 This PR is included in version 3.26.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
No description provided.