fix(health): report CPU as machine-relative, not single-core - #1286
fix(health): report CPU as machine-relative, not single-core#1286DanielCarmingham wants to merge 2 commits into
Conversation
…0#1235) process.cpuUsage() deltas are single-core time, so 2.44 cores of work read as 244% while thresholds.ts treated 90 as 90% of the machine. Every multi-core host tripped cpu_critical and /health returned 503. Divide by os.cpus().length at the producer and record the divisor on the snapshot.
…cer (rohitg00#1235) Round-1 review findings: cpu.cores was storing the raw os.cpus().length instead of the clamped divisor actually used, so an empty cpus() array recorded cores:0 while still (correctly) dividing by 1 -- self- contradicting telemetry in exactly the edge case the clamp exists for. Clamp before assigning to coreCount in collectHealth so the stored value and the divisor are provably the same number. Also add a test that drives registerHealthMonitor/collectHealth end to end with mocked process.cpuUsage()/Date.now()/os.cpus(), reads the persisted snapshot back out of a mock KV, and asserts the normalized percent -- verified to fail (500 vs 62.5) if the normalizeCpuPercent call is bypassed, so removing the fix trips this test even though the full suite would otherwise stay green.
|
@DanielCarmingham is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe health monitor now reports CPU usage relative to total machine capacity. It records the clamped core count used for normalization. Tests cover helper behavior, persisted snapshots, and CPU severity thresholds. ChangesCPU health normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to This change makes CPU health reporting machine-relative, preventing false 503 responses on multi-core hosts while retaining critical status for genuine machine-wide saturation. It is mergeable with owner awareness that existing external consumers of cpu.percent may need to account for the changed meaning. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Problem
process.cpuUsage()deltas measure single-core time, so 2.44 cores' worth of work reads as 244%, whilethresholds.tstreats itscpuCriticalPercent(90) as a machine-relative percentage. On effectively every multi-core host doing real work,cpu_criticaltrips and/healthreturns 503 for a process using a fraction of the machine (#1235).Fix
Normalize at the producer:
collectHealthdivides the single-core percentage by the core count (normalizeCpuPercent, exported for direct unit testing), clamping a zero/bogusos.cpus().lengthto 1 rather than dividing by zero. The snapshot records the divisor actually used ascpu.cores— the clamped value, not rawos.cpus().length, so the stored telemetry and the divisor are provably the same number even in the empty-cpus()edge case the clamp exists for.Tests
Unit tests on
normalizeCpuPercent(division, clamp, real-core default), threshold-side tests for machine-relative interpretation (15.25% healthy where the old reading of 244% would have been critical; genuine 95% machine-wide saturation still critical), and a producer-wiring test drivingregisterHealthMonitor/collectHealthend to end with mockedprocess.cpuUsage()/Date.now()/os.cpus(), reading the persisted snapshot from a mock KV — verified to fail (500 vs 62.5) if thenormalizeCpuPercentcall is bypassed.Full suite: 1717 passed / 1 skipped.
tsc --noEmitunchanged at the 30 pre-existing errors (none in touched files).Sibling of #1285 (heap denominator) — the two
/health503 false-positive sources; independent, either merges first. A follow-up PR makes the thresholds themselves configurable (#226) and depends on both.Closes #1235.
Summary by CodeRabbit
Bug Fixes
Tests