-
Notifications
You must be signed in to change notification settings - Fork 1
Add process leaderboard to system watcher notifications #11
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -68,6 +68,93 @@ def get_system_metrics(self) -> tuple[float, float, Any]: | |||||||||||||||||||||||||||||||||||||
| ram_percent = memory.percent | ||||||||||||||||||||||||||||||||||||||
| return cpu_percent, ram_percent, memory | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def get_top_processes_by_cpu(self, top_n: int = 5) -> list[dict[str, Any]]: | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| Get top N processes consuming the most CPU. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||
| top_n: Number of top processes to return | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||
| List of dictionaries with process information (pid, name, cpu_percent) | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| processes = [] | ||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||
| for proc in psutil.process_iter(['pid', 'name', 'cpu_percent']): | ||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||
| pinfo = proc.info | ||||||||||||||||||||||||||||||||||||||
| if pinfo['cpu_percent'] is not None: | ||||||||||||||||||||||||||||||||||||||
| processes.append(pinfo) | ||||||||||||||||||||||||||||||||||||||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| pass | |
| # The process may have terminated or become inaccessible between | |
| # iteration and inspection; skip it and continue with the next one. | |
| continue |
Copilot
AI
Feb 6, 2026
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.
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: |
Copilot
AI
Feb 6, 2026
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.
'except' clause does nothing but pass and there is no explanatory comment.
Copilot
AI
Feb 6, 2026
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.
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.
Copilot
AI
Feb 6, 2026
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.
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.
Copilot
AI
Feb 6, 2026
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.
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.
Copilot
AI
Feb 6, 2026
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.
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.
Copilot
AI
Feb 6, 2026
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.
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 |
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.
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 usingcpu_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.