feat(dashmate): collect docker stats in the doctor command #2180
Conversation
Fixes #2178 Add docker container stats collection and analysis to the doctor command. * **Collect Docker Stats**: Modify `collectSamplesTaskFactory.js` to collect docker container stats using `si.dockerContainerStats` and store them in `Samples`. * **Analyze Docker Stats**: Modify `analyseServiceContainersFactory.js` to analyze docker container stats and report problems if any container consumes too many resources. * **Update Imports**: Add `systeminformation` import in `collectSamplesTaskFactory.js`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/dashpay/platform/issues/2178?shareId=XXXX-XXXX-XXXX-XXXX).
WalkthroughThe changes introduce functionality to collect and analyze Docker container statistics within the Dashmate application. The Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (2)
330-331: LGTM: Container ID extraction with a minor suggestion.The container ID extraction is correctly implemented using optional chaining. This is necessary for the subsequent Docker stats collection.
Consider removing the empty line 331 for better code compactness:
const containerId = inspect?.Id; -
Line range hint
1-372: Overall assessment: Implementation aligns with PR objectives.The changes in this file successfully implement the collection of Docker stats as requested in the PR objectives. The code is well-structured and follows good practices. There are only minor suggestions for code style improvements.
To ensure the reliability of this new feature, consider adding unit tests for the Docker stats collection functionality. This will help maintain the stability of the codebase as it evolves.
Consider the following architectural improvements:
- Error handling: Add try-catch blocks around the Docker stats collection to handle potential errors gracefully.
- Logging: Implement logging for the Docker stats collection process to aid in debugging and monitoring.
- Configuration: Consider making the Docker stats collection configurable, allowing users to enable/disable this feature or adjust its behavior (e.g., collection frequency) through configuration settings.
packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (2)
Line range hint
85-89: Fix logical error: Incorrect variable used in OOMKilled services handlingIn the block handling services that were OOMKilled, the code mistakenly uses
servicesNotStartedinstead ofservicesOOMKilledwhen constructing the description. This results in incorrect service information being reported.Apply this diff to correct the variable references:
if (servicesOOMKilled.length > 0) { let description; - if (servicesNotStarted.length === 1) { - description = chalk`Service ${servicesNotStarted[0].service.title} was killed due to a lack of memory.`; + if (servicesOOMKilled.length === 1) { + description = chalk`Service ${servicesOOMKilled[0].service.title} was killed due to a lack of memory.`; } else { - description = chalk`Services ${servicesNotStarted.map((e) => e.service.title).join(', ')} were killed due to lack of memory.`; + description = chalk`Services ${servicesOOMKilled.map((e) => e.service.title).join(', ')} were killed due to lack of memory.`; } const problem = new Problem( description, 'Make sure you have enough memory to run the node.', SEVERITY.HIGH, ); problems.push(problem); }
123-127: Improve user message for clarityThe suggestion message in
problemcould be made more actionable and user-friendly. Instead of "report in case of misbehaviour," consider providing specific steps the user can take.Apply this diff to enhance the message:
const problem = new Problem( description, - 'Consider upgrading your system resources or report in case of misbehaviour.', + 'Consider optimizing service configurations, checking for issues causing high resource usage, or upgrading your system resources.', SEVERITY.MEDIUM, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (3 hunks)
- packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (3 hunks)
🔇 Additional comments (4)
packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (2)
12-12: LGTM: Import statement for systeminformation.The import statement for the 'systeminformation' library is correctly placed and uses the conventional alias 'si'. This import is necessary for the new Docker stats collection functionality.
332-333: LGTM: Docker stats collection and storage with suggestions.The Docker stats collection and storage are correctly implemented. The conditional execution based on containerId presence is a good practice.
Consider the following improvements:
- Remove the empty line 333 for better code compactness.
- Fix the indentation of line 362 to align with other setServiceInfo calls.
const dockerStats = containerId ? await si.dockerContainerStats(containerId) : undefined; - // ... (other code) - ctx.samples.setServiceInfo(service.name, 'dockerStats', dockerStats); + ctx.samples.setServiceInfo(service.name, 'dockerStats', dockerStats);Let's verify the potential performance impact of this new feature:
This script will help us understand the time complexity of the
dockerContainerStatsfunction and check for any known performance issues with the systeminformation package.Also applies to: 362-362
packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (2)
23-23: LGTM!Initialization of
servicesHighResourceUsagearray is appropriate.
27-27: LGTM!Retrieving Docker stats using
samples.getServiceInfo(service.name, 'dockerStats')is correct.
| if (dockerStats) { | ||
| const cpuUsage = dockerStats.cpuStats.cpuUsage.totalUsage / dockerStats.cpuStats.systemCpuUsage; | ||
| const memoryUsage = dockerStats.memoryStats.usage / dockerStats.memoryStats.limit; | ||
|
|
||
| if (cpuUsage > 0.8 || memoryUsage > 0.8) { | ||
| servicesHighResourceUsage.push({ | ||
| service, | ||
| cpuUsage, | ||
| memoryUsage, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent possible division by zero and handle missing properties
In the calculations of cpuUsage and memoryUsage, there is a risk of division by zero if dockerStats.cpuStats.systemCpuUsage or dockerStats.memoryStats.limit are zero or undefined. Additionally, accessing nested properties without verifying their existence may lead to runtime errors.
Apply this diff to handle these cases:
if (dockerStats) {
+ if (
+ dockerStats.cpuStats &&
+ dockerStats.cpuStats.cpuUsage &&
+ dockerStats.cpuStats.systemCpuUsage > 0 &&
+ dockerStats.memoryStats &&
+ dockerStats.memoryStats.limit > 0
+ ) {
const cpuUsage = dockerStats.cpuStats.cpuUsage.totalUsage / dockerStats.cpuStats.systemCpuUsage;
const memoryUsage = dockerStats.memoryStats.usage / dockerStats.memoryStats.limit;
if (cpuUsage > 0.8 || memoryUsage > 0.8) {
servicesHighResourceUsage.push({
service,
cpuUsage,
memoryUsage,
});
}
+ } else {
+ // Handle cases where stats are unavailable or denominators are zero
+ console.warn(`Docker stats for service ${service.name} are incomplete or invalid.`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (dockerStats) { | |
| const cpuUsage = dockerStats.cpuStats.cpuUsage.totalUsage / dockerStats.cpuStats.systemCpuUsage; | |
| const memoryUsage = dockerStats.memoryStats.usage / dockerStats.memoryStats.limit; | |
| if (cpuUsage > 0.8 || memoryUsage > 0.8) { | |
| servicesHighResourceUsage.push({ | |
| service, | |
| cpuUsage, | |
| memoryUsage, | |
| }); | |
| } | |
| } | |
| if (dockerStats) { | |
| if ( | |
| dockerStats.cpuStats && | |
| dockerStats.cpuStats.cpuUsage && | |
| dockerStats.cpuStats.systemCpuUsage > 0 && | |
| dockerStats.memoryStats && | |
| dockerStats.memoryStats.limit > 0 | |
| ) { | |
| const cpuUsage = dockerStats.cpuStats.cpuUsage.totalUsage / dockerStats.cpuStats.systemCpuUsage; | |
| const memoryUsage = dockerStats.memoryStats.usage / dockerStats.memoryStats.limit; | |
| if (cpuUsage > 0.8 || memoryUsage > 0.8) { | |
| servicesHighResourceUsage.push({ | |
| service, | |
| cpuUsage, | |
| memoryUsage, | |
| }); | |
| } | |
| } else { | |
| // Handle cases where stats are unavailable or denominators are zero | |
| console.warn(`Docker stats for service ${service.name} are incomplete or invalid.`); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (2)
137-149: LGTM: Reporting high CPU usage implemented correctly.The implementation for reporting high CPU usage is well done. It correctly iterates through the
servicesHighCpuUsagearray, creates a problem description with formatted CPU usage percentage, and adds a newProbleminstance with appropriate severity.Consider adding more specific advice in the problem description. For example:
- 'Consider upgrading CPU or report in case of misbehaviour.', + 'Consider upgrading CPU, optimizing the service, or report to support if you suspect misbehavior.',This provides users with more actionable steps.
151-163: LGTM: Reporting high memory usage implemented correctly.The implementation for reporting high memory usage is well done and consistent with the CPU usage reporting. It correctly iterates through the
servicesHighMemoryUsagearray, creates a problem description with formatted memory usage percentage, and adds a newProbleminstance with appropriate severity.Similar to the CPU usage reporting, consider adding more specific advice in the problem description. For example:
- 'Consider upgrading RAM or report in case of misbehaviour.', + 'Consider upgrading RAM, optimizing the service's memory usage, or report to support if you suspect misbehavior.',This provides users with more actionable steps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
.yarn/cache/fsevents-patch-19706e7e35-10.zipis excluded by!**/.yarn/**
📒 Files selected for processing (2)
- packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (3 hunks)
- packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dashmate/src/listr/tasks/doctor/collectSamplesTaskFactory.js
🔇 Additional comments (3)
packages/dashmate/src/doctor/analyse/analyseServiceContainersFactory.js (3)
23-24: LGTM: New arrays for tracking high resource usage.The addition of
servicesHighCpuUsageandservicesHighMemoryUsagearrays aligns well with the PR objectives. These will be used to track services exceeding CPU or memory usage thresholds.
Line range hint
28-80: Suggestion: Enhance error handling and consider parameterizing the threshold.The implementation of Docker stats collection and analysis looks good overall. However, there are a few points to consider:
The code doesn't handle the case where
dockerStatsis undefined or null. Consider adding a check at the beginning of this block.The 80% threshold for high usage is hardcoded. It might be beneficial to make this configurable.
While there's proper handling of potential division by zero, the code could be more concise using optional chaining and nullish coalescing operators.
Consider applying these changes:
+ if (!dockerStats) continue; - const cpuSystemUsage = dockerStats?.cpuStats?.system_cpu_usage; - const cpuServiceUsage = dockerStats?.cpuStats?.cpu_usage?.total_usage; + const cpuSystemUsage = dockerStats.cpuStats?.system_cpu_usage ?? 0; + const cpuServiceUsage = dockerStats.cpuStats?.cpu_usage?.total_usage ?? 0; - if (Number.isInteger(cpuServiceUsage) && cpuSystemUsage > 0) { - const cpuUsagePercent = cpuServiceUsage / cpuSystemUsage; + const cpuUsagePercent = cpuSystemUsage > 0 ? cpuServiceUsage / cpuSystemUsage : 0; - if (cpuUsagePercent > 0.8) { + const HIGH_USAGE_THRESHOLD = 0.8; // Consider making this configurable + if (cpuUsagePercent > HIGH_USAGE_THRESHOLD) {Apply similar changes to the memory usage calculation as well.
Could you clarify if the 80% threshold for high resource usage is intentionally hardcoded or if it should be configurable?
164-164: LGTM: Improved code readability.The addition of an empty line before the return statement improves the code's readability by clearly separating the problem reporting logic from the function's return.
Fixes #2178
Add docker container stats collection and analysis to the doctor command.
collectSamplesTaskFactory.jsto collect docker container stats usingsi.dockerContainerStatsand store them inSamples.analyseServiceContainersFactory.jsto analyze docker container stats and report problems if any container consumes too many resources.systeminformationimport incollectSamplesTaskFactory.js.For more details, open the Copilot Workspace session.
Summary by CodeRabbit
New Features
Bug Fixes