Skip to content

feat(container-runner): setup instance memory and cpu usage logger - #5549

Open
abcxff wants to merge 1 commit into
stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvntfrom
stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy
Open

feat(container-runner): setup instance memory and cpu usage logger#5549
abcxff wants to merge 1 commit into
stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvntfrom
stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy

Conversation

@abcxff

@abcxff abcxff commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@claude

claude Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review

Solid, well-scoped addition: an opt-in (RIVET_LOG_RESOURCE_USAGE), disabled-by-default resource sampler with a sensible cgroup-v2 to /proc fallback for gVisor sandboxes. I read through monitor.rs and the wiring in actor.rs/main.rs. No security or correctness bugs found in the sampling/parsing logic (cumulative-counter deltas, /proc/stat field indexing, div-by-zero guards, and the MissedTickBehavior::Skip handling all look correct). A few things worth considering before merge:

Test coverage (moderate).
monitor.rs adds several pure parsing functions (read_cgroup_cpu_limit_cores, read_proc_cpu_busy_usec, read_proc_cpu_count, bytes_to_mib, etc.) but ships with zero tests. This repo already has precedent for exactly this kind of unit test: container-runner/tests/inline/boot_id.rs tests the pure base64url_nopad helper in main.rs the same way. As written, though, the parsing functions read directly from hardcoded path constants (CPU_STAT, PROC_STAT, ...), so they are not unit-testable without a refactor to accept the file content as a parameter (e.g. fn parse_cpu_max(raw: &str) -> Option<f64>) with the constants only used at the call site. Worth doing given how easy it would be to get the /proc/stat field offsets or the cpu.max/memory.max unlimited sentinel handling wrong silently.

Minor: sampling_source() cost on every actor start (low).
actor.rs's new on_start log unconditionally calls crate::monitor::sampling_source(), which runs detect_source() (up to 4 blocking file reads) even when the monitor is disabled (resource_monitor_enabled = false, the default) and even on the idempotent duplicate-start retry path (before the existing-child check). The reads are cheap pseudo-file reads, so this is not a real perf problem, but it is unconditional work done for a status log that is mostly informative when the flag is off. Consider short-circuiting: only call sampling_source() when enabled() is true.

Log volume with multiple actors per instance (low, already acknowledged).
Per main.rs's doc comment, one instance can host multiple concurrent actors. The monitor's counters are instance-wide, so with N actors running it logs the identical "instance resource usage" line N times every 500ms. The code comment already calls this out, so it is a conscious tradeoff, just flagging it since it could meaningfully multiply log volume when the flag is enabled on a densely-packed instance.

Nit: Option<f64> fields via ? (very low).
mem_used_mib = ?mem_used_mib etc. log as Some(123.45) / None under the current plain-text formatter. If this ever moves to JSON-formatted log output for aggregation, these fields will serialize as strings like "Some(123.45)" rather than numbers/null, which is awkward to query. Not a problem today, just worth being aware of.

Nothing here blocks merging. The feature is opt-in, defaults to off, and the core sampling logic is sound.

@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from 886a208 to 33d70a3 Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 40d03bf to 13cd549 Compare August 10, 2026 17:16
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 13cd549 to fbd0e22 Compare August 10, 2026 19:03
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch 2 times, most recently from 45f065f to c0e196d Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from fbd0e22 to 7df3e38 Compare August 10, 2026 20:35
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from c0e196d to 8d63009 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 7df3e38 to 5c46256 Compare August 10, 2026 22:09
@abcxff
abcxff force-pushed the stack/feat-container-runner-setup-instance-memory-and-cpu-usage-logger-xumpnzoy branch from 8d63009 to cabfb3c Compare August 11, 2026 13:39
@abcxff
abcxff force-pushed the stack/fix-rivetkit-keep-actor-event-loop-alive-through-shutdown-state-serialization-losmyvnt branch from 5c46256 to b8b0fd3 Compare August 11, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant