Skip to content

fix(daemon): compute dora top disk-I/O rate from the real sampling window - #3109

Draft
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-xphg86-daemon-disk-io-rate
Draft

fix(daemon): compute dora top disk-I/O rate from the real sampling window#3109
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-xphg86-daemon-disk-io-rate

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

collect_and_send_metrics_bg (binaries/daemon/src/lib.rs) turns per-refresh disk byte deltas into a rate by dividing by a hard-coded constant:

disk_read_bytes: Some((disk_read as f64 / METRICS_INTERVAL_SECS) as u64),   // 2.0
disk_write_bytes: Some((disk_written as f64 / METRICS_INTERVAL_SECS) as u64),

disk_read/disk_written come from sysinfo's process.disk_usage(), which reports bytes since the previous refresh_processes_specifics call — not "over the last 2 s". The constant divisor is only correct when refreshes are exactly one interval apart, so the user-facing dora top "I/O READ"/"I/O WRITE" columns over-report in two real cases:

  • First sample. metrics_system is initialized empty (sysinfo::System::new()) and reset to a fresh empty System if a refresh ever panics. On the first refresh after that, sysinfo has no prior baseline, so read_bytes is effectively the process's total-since-start I/O. Divided by 2 s that is a large spurious spike for every freshly spawned (or post-panic) node.
  • Skipped/catch-up cycle. When a prior collection is still running, try_lock fails and the cycle is skipped. The next successful refresh then covers ~two intervals of I/O but is still divided by one interval, roughly doubling the reported rate.

Fix

Track the wall-clock Instant of the previous successful refresh (new metrics_last_refresh field) and divide by the measured window instead of the constant. A new pure helper does the arithmetic:

fn disk_rate_bytes_per_sec(bytes: u64, window: Option<Duration>) -> Option<u64>

It returns None — i.e. no rate reported for that sample — when there is no prior baseline (first refresh, or a poisoned lock) or the window is implausibly short (< 0.1 s), mirroring how CPU% is meaningless on a first observation. disk_read_bytes/disk_write_bytes are already Option<u64>, so None flows through cleanly.

The now-unused METRICS_INTERVAL_SECS constant is removed (METRICS_INTERVAL itself is still used to drive the interval timer).

Scope note

This fixes the divisor and the daemon's first-sample case. A process appearing for the very first time in an already-warm System (or a newly spawned descendant folded into the aggregate) can still momentarily reflect sysinfo's total-since-start delta for that single sample — that is an inherent sysinfo behavior, out of scope here.

Validation

cargo test -p dora-daemon disk_rate      # 3 passed (new)
cargo clippy -p dora-daemon -- -D warnings
cargo fmt --all -- --check

New unit tests cover: no baseline → None; near-zero window → None; 8 MB over a 4 s window → 2 MB/s (the old constant divisor would have reported 4 MB/s); steady-state 2 s window unchanged.


⚠️ This is a machine-generated PR authored by Claude (Claude Code) as part of an automated codebase review. The finding was verified by hand against origin/main before opening. Please review carefully before merging.

🤖 Generated with Claude Code


Generated by Claude Code

@trunk-io

trunk-io Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@phil-opp
phil-opp force-pushed the claude/dreamy-bardeen-xphg86-daemon-disk-io-rate branch from ce49a6e to 0100960 Compare August 10, 2026 23:37
…ndow

`collect_and_send_metrics_bg` divided the per-refresh disk byte deltas
(from sysinfo's `disk_usage()`, which report bytes since the previous
`refresh_processes_specifics` call) by a hard-coded `METRICS_INTERVAL_SECS`
(2 s). That constant is only correct when refreshes are exactly one
interval apart, so `dora top`'s "I/O READ"/"I/O WRITE" columns over-reported
in two cases:

- First sample: the metrics `System` starts empty (and is reset to empty
  if a refresh ever panics), so sysinfo has no baseline and reports the
  process's total-since-start I/O. Divided by 2 s that is a large spurious
  spike for every freshly spawned node.
- Skipped/catch-up cycle: when a prior collection still holds the lock the
  cycle is skipped, so the next refresh covers ~two intervals of I/O but is
  still divided by one — roughly doubling the reported rate.

Track the wall-clock instant of the previous successful refresh and divide
by the measured window instead. A new `disk_rate_bytes_per_sec` helper
returns `None` (no rate reported) when there is no prior baseline or the
window is implausibly short, mirroring how CPU% is meaningless on first
observation. Unit-tested for the no-baseline, near-zero-window, and
actual-window cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PYiYgEo2WKBXfGooqP2WG5

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this is a fully automated review with no human in the loop. Treat it as advisory.

Reviewed the diff — no issues found. The new metrics_last_refresh timestamp and the sysinfo System baseline stay in sync: the timestamp is set to Some only on the successful-refresh path that also restores the refreshed System (carrying the per-process baseline), and the try-lock-skip / no-running-nodes / panic paths all leave it consistent — the last two clear it to None so the next sample is suppressed rather than producing a spike. First sample correctly yields None, and windows shorter than 0.1s are rejected. The unit tests exercise the no-baseline, near-zero-window, and real-window division cases (8 MB / 4 s = 2 MB/s) with concrete assertions that distinguish the measured window from the old constant divisor.


Generated by Claude Code

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.

2 participants