feat(container-runner): setup instance memory and cpu usage logger - #5549
Conversation
ReviewOverall this is a clean, well-documented addition (nice doc comments explaining the cgroup v2 vs /proc fallback and the sampling math). A few things worth addressing: Correctness / design actor.rs:88-97 - the new "resource monitor status" log in on_start calls crate::monitor::sampling_source() unconditionally, which calls detect_source() and does blocking reads of /sys/fs/cgroup/memory.current, cpu.stat, and potentially /proc/meminfo /proc/stat as a fallback - on every single actor start, regardless of whether RIVET_LOG_RESOURCE_USAGE is set. This contradicts the module doc comment on monitor.rs ("Disabled by default so nothing is logged unless explicitly turned on"): with the monitor disabled (the default), every actor placement still does synchronous fs I/O on the async on_start hot path and adds a permanent extra log line to every actor's logs forever, just to report resource_monitor_enabled=false. Since enabled()/sampling_source() reflect process-wide, static-for-the-life-of-the-instance state (not something that varies per actor), consider either:
Code quality main.rs:120-131 - active_actor_ids() duplicates the identical CHILDREN.retain_async(|actor_id, _| { ids.push(...); true }) scan that's already inlined in spawn_signal_handler at main.rs:456-464 for the SIGTERM path. Now that the helper exists, spawn_signal_handler could call active_actor_ids() instead of keeping two copies of the same read-only scan. Test coverage monitor.rs adds several small parsing functions (read_meminfo_kb, read_proc_cpu_busy_usec, read_proc_cpu_count, read_cgroup_cpu_limit_cores, read_cgroup_memory_max) with no tests. This crate already has a convention for this: tests/inline/input.rs and tests/inline/boot_id.rs, wired via a '#[path]' shim per CLAUDE.md's guidance on keeping inline tests out of src/. As written, each parsing function directly calls std::fs::read_to_string(PATH) inline, so none of them can be unit tested without touching the real filesystem. Splitting "read the file" from "parse the string" (e.g. have read_meminfo_kb take the file contents as a &str parameter) would make the cgroup v2 / /proc parsing logic (the part most likely to have an off-by-one or wrong-field bug, e.g. /proc/stat field ordering or the cpu.max "max" sentinel) directly testable and consistent with the rest of the crate. Minor
Nothing else stood out - the cgroup v2//proc fallback logic, the Skip-on-missed-tick handling, and the opt-in env var parsing all look correct, and the feature is safely disabled by default (aside from the point above). |
No description provided.