[Fix][Core/Dashboard] component_cpu_percentage metric always zero for dashboard subprocess modules - #65055
Conversation
… dashboard subprocess modules Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
afe3086 to
7a81cd7
Compare
7a81cd7 to
4553db5
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces caching of psutil.Process handles for dashboard subprocess modules to ensure accurate CPU usage reporting, as psutil.Process.cpu_percent() requires a persistent handle to measure relative CPU usage. It also adds comprehensive unit tests to verify this behavior. The reviewer's feedback recommends handling potential exceptions like psutil.NoSuchProcess or psutil.AccessDenied when instantiating psutil.Process for a PID, and checking for None before recording metrics to prevent the metric recording loop from crashing.
| self._record_cpu_mem_metrics_for_proc( | ||
| proc, subprocess_module_handle.module_cls.__name__ | ||
| self._get_subprocess_module_proc(pid), | ||
| subprocess_module_handle.module_cls.__name__, | ||
| ) |
There was a problem hiding this comment.
Since _get_subprocess_module_proc can return None if the process is missing or inaccessible, we should check if the returned process is not None before attempting to record its metrics.
| self._record_cpu_mem_metrics_for_proc( | |
| proc, subprocess_module_handle.module_cls.__name__ | |
| self._get_subprocess_module_proc(pid), | |
| subprocess_module_handle.module_cls.__name__, | |
| ) | |
| proc = self._get_subprocess_module_proc(pid) | |
| if proc is not None: | |
| self._record_cpu_mem_metrics_for_proc( | |
| proc, | |
| subprocess_module_handle.module_cls.__name__, | |
| ) |
There was a problem hiding this comment.
Looks like a valid suggestion.
| def _get_subprocess_module_proc(self, pid: int) -> psutil.Process: | ||
| """Return the cached psutil.Process for pid, creating it if needed. | ||
|
|
||
| Reusing the handle is what gives cpu_percent() a baseline to measure | ||
| against. See the comment on self._subprocess_module_procs. | ||
| """ | ||
| proc = self._subprocess_module_procs.get(pid) | ||
| # is_running() is False once the process is gone, and also once the pid | ||
| # has been reused by an unrelated process, in which case the cached | ||
| # handle's baseline no longer describes the process being measured. | ||
| if proc is None or not proc.is_running(): | ||
| proc = psutil.Process(pid) | ||
| self._subprocess_module_procs[pid] = proc | ||
| return proc |
There was a problem hiding this comment.
If a subprocess module exits or restarts, or if there are permission restrictions, psutil.Process(pid) can raise psutil.NoSuchProcess or psutil.AccessDenied. Wrapping the instantiation in a try-except block and returning None prevents these exceptions from crashing the periodic metric recording loop.
def _get_subprocess_module_proc(self, pid: int) -> Optional[psutil.Process]:
"""Return the cached psutil.Process for pid, creating it if needed.
Reusing the handle is what gives cpu_percent() a baseline to measure
against. See the comment on self._subprocess_module_procs.
"""
proc = self._subprocess_module_procs.get(pid)
# is_running() is False once the process is gone, and also once the pid
# has been reused by an unrelated process, in which case the cached
# handle's baseline no longer describes the process being measured.
if proc is None or not proc.is_running():
try:
proc = psutil.Process(pid)
self._subprocess_module_procs[pid] = proc
except (psutil.NoSuchProcess, psutil.AccessDenied):
return None
return proc
Description
The
component_cpu_percentagemetric is currently always zero for dashboard subprocess modules. This PR fixes this.Related issues
Similar symptom with #29848
Additional information
Before
After