Conversation
aclauer
changed the title
| try: | ||
| proc = _get_process(pid) | ||
| for child in proc.children(recursive=False): | ||
| child_proc = _get_process(child.pid) | ||
| try: | ||
| name = child_proc.name() | ||
| cpu = child_proc.cpu_percent(interval=None) | ||
| result.append(ChildProcessStats(pid=child.pid, name=name, cpu_percent=cpu)) | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass | ||
| return result |
There was a problem hiding this comment.
Presumably _get_process raises (psutil.NoSuchProcess, psutil.AccessDenied). Then surround just that function call with try-except. Nesting the try-excepts is confusing.
| parser.add_argument( | ||
| "--log", | ||
| nargs="?", | ||
| const=f"dtop_{time.strftime('%Y%m%d_%H%M%S')}.jsonl", |
There was a problem hiding this comment.
So it's automatically git-ignored.
| const=f"dtop_{time.strftime('%Y%m%d_%H%M%S')}.jsonl", | |
| const=f"dtop_{time.strftime('%Y%m%d_%H%M%S')}.ignore.jsonl", |
Greptile SummaryThis PR extends Confidence Score: 4/5Safe to merge; all findings are non-blocking P2 quality/robustness issues. No P0 or P1 issues found. The four P2 findings (unflushed log writes, file handle leak in init, pid variable shadowing, missing KeyError guard in dtop-plot) are style/robustness concerns that don't affect core dtop functionality. dimos/utils/cli/dtop.py — log file flush and handle lifecycle; dimos/utils/cli/dtop_plot.py — robustness of _load against malformed lines. Important Files Changed
Sequence DiagramsequenceDiagram
participant SM as StatsMonitor (daemon thread)
participant PS as psutil
participant RL as ResourceLogger (LCM)
participant DT as dtop TUI
participant LF as JSONL log file
SM->>PS: collect_process_stats(worker_pid)
SM->>PS: collect_children_stats(worker_pid)
PS-->>SM: ChildProcessStats[]
SM->>SM: add children cpu_percent to parent
SM->>RL: log_stats(coordinator, [WorkerStats+children])
RL-->>DT: LCM message (topic /dimos/resource_stats)
DT->>LF: write JSON line (--log)
DT->>DT: _refresh(): render worker rows + child rows
note over LF: dtop-plot reads JSONL → matplotlib plot
Reviews (1): Last reviewed commit: "Fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self._latest = msg | ||
| self._last_msg_time = time.monotonic() | ||
| if self._log_file: | ||
| self._log_file.write(json.dumps({"ts": time.time(), **msg}) + "\n") |
There was a problem hiding this comment.
Each _on_msg call writes a line to _log_file but never calls flush(). Because Python's file I/O is buffered by default, lines written near a crash or SIGKILL will silently stay in the OS/Python buffer and never reach disk. Adding a flush() after the write ensures each message is durable.
| self._log_file.write(json.dumps({"ts": time.time(), **msg}) + "\n") | |
| self._log_file.write(json.dumps({"ts": time.time(), **msg}) + "\n") | |
| self._log_file.flush() |
| ) -> None: | ||
| super().__init__() | ||
| self._topic_name = topic_name | ||
| self._log_file = open(log_path, "a") if log_path else None |
There was a problem hiding this comment.
__init__ raises after open()
_log_file is opened before autoconf, PickleLCM(), and subscribe(). If any of those subsequent calls throw, on_unmount is never called and the file handle is leaked. A try/except (or opening the file later, e.g. in on_mount) would prevent this.
| parts.append(Rule(title=title, style=border_style)) | ||
| parts.extend(self._make_lines(d, stale, ranges, self._cpu_history[role])) | ||
| for child in d.get("children", []): | ||
| pid = child.get("pid", 0) |
There was a problem hiding this comment.
pid variable shadowed by inner loop
The outer for tuple unpacks pid as a string (worker pid for display), but this inner assignment overwrites it with an integer child pid. The outer pid is rebound at the start of each outer iteration so there's no runtime bug, but the shadowing is confusing and could easily introduce a bug if code is added between the inner loop and the next outer iteration.
| pid = child.get("pid", 0) | |
| child_pid = child.get("pid", 0) | |
| if child_pid not in self._child_cpu_history: | |
| self._child_cpu_history[child_pid] = deque(maxlen=_SPARK_WIDTH * 2) | |
| if not stale: | |
| self._child_cpu_history[child_pid].append(child.get("cpu_percent", 0.0)) | |
| parts.append(self._make_child_line(child, stale, self._child_cpu_history[child_pid])) |
| rows = [] | ||
| for _, msg in raw.iterrows(): | ||
| ts = msg["ts"] | ||
| rows.append({"ts": ts, "role": _COORDINATOR, **msg[_COORDINATOR]}) |
There was a problem hiding this comment.
KeyError on malformed log lines
msg[_COORDINATOR] raises KeyError if any line in the JSONL file is missing the "coordinator" key (e.g., a truncated line written during an unclean shutdown). Wrapping the row processing in a try/except KeyError and skipping bad rows would make the tool more robust.
| rows.append({"ts": ts, "role": _COORDINATOR, **msg[_COORDINATOR]}) | |
| try: | |
| ts = msg["ts"] | |
| rows.append({"ts": ts, "role": _COORDINATOR, **msg[_COORDINATOR]}) | |
| except (KeyError, TypeError): | |
| continue |
2 participants
Problem
dtop only shows cpu usage for Python workers spawned by DimOS. Any native modules spawned by that worker don't show up in the cpu statistics. Also add --log flag to log dtop statistics and dtop-plot to generate plots of cpu usage.
Closes DIM-XXX
Solution
Read the pids of any processes spawned and include their cpu usage in a drop down of the main worker.
Breaking Changes
None
How to Test
dimos --dtop --replay --replay-db=go2_bigoffice run unitree-go2
and
dtop
When dimos spawns the viewer, it will show up as a subprocess of the rerun bridge worker.
Contributor License Agreement