-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(server): add process memory usage metrics #13148
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
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant CronScheduler as Cron Scheduler
participant MonitorService
participant MetricsSystem
participant Logger
CronScheduler->>MonitorService: Trigger monitor() every minute
MonitorService->>MonitorService: Collect process.memoryUsage()
MonitorService->>Logger: Log memory usage
MonitorService->>MetricsSystem: Record memory stats to gauges
Assessment against linked issues
Poem
✨ 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 (
|
affine-toeverythingBundle maindiff ------------------- Bundle Size Diff -------------------------
@@ EntryPoint: index @@
## canary …usage-metrics +/- ##
===================================================================
< Bundle 26.9 MB 26.9 MB +3.54 kB(+0.01%)
< Initial JS 11.6 MB 11.6 MB +813 B(+0.01%)
= Initial CSS 220 kB 220 kB
#~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~#
< Assets 88 89 +1
< Chunks 86 87 +1
= Packages 265 265
= Duplicates 5 5
#~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Warnings ~~~~~~~~~~~~~~~~~~~~~~~~~~~#
! Deduplicate versions of libraries
! Separate mixed content assets files
! Avoid cache wasting
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## canary #13148 +/- ##
==========================================
- Coverage 56.82% 56.81% -0.01%
==========================================
Files 2706 2708 +2
Lines 132147 132187 +40
Branches 20572 20570 -2
==========================================
+ Hits 75088 75105 +17
- Misses 54849 55408 +559
+ Partials 2210 1674 -536
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR introduces automated monitoring of the Node.js process’s memory usage by adding a scheduled service, expands the metric scope, and wires the service into the application module.
- Add
MonitorService
with a Cron job to log and emit process memory metrics every minute. - Extend the
KnownMetricScopes
union to include a new'process'
scope. - Register
MonitorModule
inAppModule
to enable global monitoring.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
packages/backend/server/src/core/monitor/service.ts | New MonitorService logs and records Node.js memory usage metrics |
packages/backend/server/src/core/monitor/index.ts | Created MonitorModule as a global Nest module |
packages/backend/server/src/base/metrics/metrics.ts | Added 'process' to the KnownMetricScopes union |
packages/backend/server/src/app.module.ts | Imported and registered MonitorModule within application modules |
Comments suppressed due to low confidence (1)
packages/backend/server/src/core/monitor/service.ts:11
- There are no unit tests covering the new MonitorService.monitor method. Consider adding a test to verify that memory usage metrics are correctly recorded and that the log statement fires as expected.
async monitor() {
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: 1
🧹 Nitpick comments (2)
packages/backend/server/src/core/monitor/service.ts (2)
10-10
: Consider adjusting the monitoring frequency.Running memory monitoring every minute might be too frequent for production environments. Consider using a configurable interval or a less frequent schedule like every 5-10 minutes.
- @Cron(CronExpression.EVERY_MINUTE) + @Cron(CronExpression.EVERY_5_MINUTES)
13-15
: Consider making monitoring configurable by environment.The monitoring runs unconditionally in all environments. Consider adding configuration to disable or adjust monitoring behavior based on environment (development, staging, production).
You could inject a configuration service to control monitoring behavior:
constructor( private readonly configService: ConfigService, ) {} @Cron(CronExpression.EVERY_MINUTE) async monitor() { if (!this.configService.get('ENABLE_MEMORY_MONITORING', true)) { return; } // ... rest of the method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/backend/server/src/app.module.ts
(2 hunks)packages/backend/server/src/base/metrics/metrics.ts
(1 hunks)packages/backend/server/src/core/monitor/index.ts
(1 hunks)packages/backend/server/src/core/monitor/service.ts
(1 hunks)
🔇 Additional comments (4)
packages/backend/server/src/base/metrics/metrics.ts (1)
63-64
: LGTM! Proper extension of metric scopes.The addition of 'process' to the
KnownMetricScopes
type union correctly supports the new process memory metrics functionality.packages/backend/server/src/app.module.ts (2)
39-39
: LGTM! Proper module import.The import follows the established pattern for core modules.
116-116
: LGTM! Correct module integration.Adding
MonitorModule
toFunctionalityModules
ensures the monitoring service is available application-wide.packages/backend/server/src/core/monitor/index.ts (1)
1-9
: LGTM! Well-structured global module.The
MonitorModule
is properly implemented as a global NestJS module, making theMonitorService
available throughout the application without explicit imports.
close CLOUD-235
PR Dependency Tree
This tree was auto-generated by Charcoal
Summary by CodeRabbit
New Features
Chores