Add process leaderboard to system watcher notifications - #11
Conversation
- Add get_top_processes_by_cpu() to fetch top CPU-consuming processes - Add get_top_processes_by_memory() to fetch top RAM-consuming processes - Add format_process_leaderboard() to format process info as markdown - Update send_cpu_alert() to include top 5 CPU-consuming processes - Update send_ram_alert() to include top 5 memory-consuming processes This helps system admins quickly identify which processes are causing high resource usage when alerts are triggered. Fixes #10 Co-authored-by: Yikun Ji <Gennadiyev@users.noreply.github.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
This PR adds process leaderboard functionality to the system watcher's alert notifications, helping administrators quickly identify resource-consuming processes when CPU or RAM usage exceeds configured thresholds. The feature addresses issue #10 by providing real-time diagnostic information alongside alerts.
Changes:
- Added
get_top_processes_by_cpu()andget_top_processes_by_memory()methods to retrieve top 5 resource-consuming processes - Added
format_process_leaderboard()to format process information as markdown - Integrated process leaderboards into CPU and RAM alert notifications
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if metric_type == 'cpu': | ||
| usage = proc.get('cpu_percent', 0) | ||
| leaderboard += f"{i}. `{name}` (PID: {pid}) - {usage:.1f}%\n" | ||
| elif metric_type == 'memory': | ||
| usage_pct = proc.get('memory_percent', 0) | ||
| usage_mb = proc.get('memory_mb', 0) | ||
| leaderboard += f"{i}. `{name}` (PID: {pid}) - {usage_pct:.1f}% ({usage_mb:.0f} MB)\n" |
There was a problem hiding this comment.
The format_process_leaderboard method does not handle cases where metric_type is neither 'cpu' nor 'memory'. When an invalid metric type is provided, the method will silently add empty entries to the leaderboard (just the numbering without any usage information). Consider adding an else clause that either raises a ValueError for invalid metric types or logs a warning and returns an error message.
| # Get top CPU-consuming processes | ||
| top_processes = self.get_top_processes_by_cpu(top_n=5) | ||
| process_leaderboard = self.format_process_leaderboard(top_processes, 'cpu') |
There was a problem hiding this comment.
Calling get_top_processes_by_cpu() and get_top_processes_by_memory() during an alert adds computational overhead at a time when the system is already under stress (CPU or RAM usage has exceeded thresholds). Iterating through all processes with psutil.process_iter() can be resource-intensive on systems with many processes. Consider whether this additional load during high-usage scenarios could exacerbate the problem. An alternative approach would be to collect process information periodically in the background and cache it, then use cached data when sending alerts.
| # Get top memory-consuming processes | ||
| top_processes = self.get_top_processes_by_memory(top_n=5) | ||
| process_leaderboard = self.format_process_leaderboard(top_processes, 'memory') | ||
|
|
There was a problem hiding this comment.
Calling get_top_processes_by_memory() during an alert adds computational overhead at a time when the system is already under stress (RAM usage has exceeded the threshold). Iterating through all processes with psutil.process_iter() can be resource-intensive on systems with many processes. Consider whether this additional load during high-usage scenarios could exacerbate the problem. An alternative approach would be to collect process information periodically in the background and cache it, then use cached data when sending alerts.
| # Get top memory-consuming processes | |
| top_processes = self.get_top_processes_by_memory(top_n=5) | |
| process_leaderboard = self.format_process_leaderboard(top_processes, 'memory') | |
| # Get top memory-consuming processes, with simple caching to reduce | |
| # overhead when the system is already under RAM pressure. | |
| cache_ttl_seconds = 10 | |
| now = time.time() | |
| cached_leaderboard = getattr(self, "_last_ram_process_leaderboard", None) | |
| cached_time = getattr(self, "_last_ram_process_leaderboard_time", 0.0) | |
| if cached_leaderboard is None or (now - cached_time) > cache_ttl_seconds: | |
| top_processes = self.get_top_processes_by_memory(top_n=5) | |
| process_leaderboard = self.format_process_leaderboard(top_processes, 'memory') | |
| # Update cache | |
| self._last_ram_process_leaderboard = process_leaderboard | |
| self._last_ram_process_leaderboard_time = now | |
| else: | |
| process_leaderboard = cached_leaderboard |
| for proc in psutil.process_iter(['pid', 'name', 'memory_percent', 'memory_info']): | ||
| try: | ||
| pinfo = proc.info | ||
| if pinfo['memory_percent'] is not None: |
There was a problem hiding this comment.
If pinfo['memory_info'] is None, accessing .rss will raise an AttributeError, which is not caught by the inner exception handler that only catches psutil.NoSuchProcess and psutil.AccessDenied. This will cause the outer exception handler to catch it and return an empty list. Consider adding a null check for pinfo['memory_info'] before accessing its rss attribute, similar to how memory_percent is checked on line 113.
| if pinfo['memory_percent'] is not None: | |
| if pinfo['memory_percent'] is not None and pinfo['memory_info'] is not None: |
| name = proc.get('name', 'Unknown') | ||
| pid = proc.get('pid', '?') | ||
|
|
||
| if metric_type == 'cpu': | ||
| usage = proc.get('cpu_percent', 0) | ||
| leaderboard += f"{i}. `{name}` (PID: {pid}) - {usage:.1f}%\n" |
There was a problem hiding this comment.
Process names obtained from psutil could potentially contain backticks or other markdown special characters that could break the markdown formatting. Consider escaping or sanitizing the process name before including it in backticks to prevent markdown rendering issues. For example, a process named "my`process" would result in malformed markdown.
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass | ||
| except Exception as e: | ||
| logger.error(f"Error getting top memory processes: {e}") |
There was a problem hiding this comment.
Error logging should include exc_info=True parameter to capture stack traces for debugging. This is the consistent pattern used throughout the codebase for exception logging.
| for proc in psutil.process_iter(['pid', 'name', 'cpu_percent']): | ||
| try: | ||
| pinfo = proc.info | ||
| if pinfo['cpu_percent'] is not None: | ||
| processes.append(pinfo) |
There was a problem hiding this comment.
The per-process CPU percentage retrieved via psutil.process_iter(['cpu_percent']) returns an instantaneous value that may be 0.0 for many processes, especially on the first call or for processes that haven't been actively running. This could result in misleading or empty leaderboards. Consider using cpu_percent(interval=0.1) on each process object to get more accurate CPU usage, or document that the values represent instantaneous CPU usage and may not reflect overall resource consumption.
| for proc in psutil.process_iter(['pid', 'name', 'cpu_percent']): | |
| try: | |
| pinfo = proc.info | |
| if pinfo['cpu_percent'] is not None: | |
| processes.append(pinfo) | |
| # Use a short interval to get a more accurate CPU usage sample per process. | |
| for proc in psutil.process_iter(['pid', 'name']): | |
| try: | |
| cpu_percent = proc.cpu_percent(interval=0.1) | |
| if cpu_percent is None: | |
| continue | |
| pinfo = proc.info | |
| pinfo['cpu_percent'] = cpu_percent | |
| processes.append(pinfo) |
| if pinfo['cpu_percent'] is not None: | ||
| processes.append(pinfo) | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| pass | |
| # The process may have terminated or become inaccessible between | |
| # iteration and inspection; skip it and continue with the next one. | |
| continue |
| # Add memory in MB for better readability | ||
| pinfo['memory_mb'] = pinfo['memory_info'].rss / (1024 * 1024) | ||
| processes.append(pinfo) | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Adds a process leaderboard to system watcher notifications to help admins quickly identify which processes are consuming the most resources.
Changes
Fixes #10
Generated with Claude Code