Conversation
|
Welcome @noncrab! It looks like this is your first PR to tikv/rust-prometheus 🎉 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
|
||
| // cpu | ||
| let total = (stat.utime + stat.stime) / *CLK_TCK as u64; | ||
| let total = (stat.utime + stat.stime) as f64 / *CLK_TCK as f64; |
There was a problem hiding this comment.
As noted in the description, I'm not quite sure how to represent the risks of precision loss here. Is it worth noting?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/process_collector.rs (1)
9-9: Remove unusedIntCounterimport.
IntCounteris no longer used in this file after the refactoring.Proposed fix
-use crate::counter::{Counter, IntCounter}; +use crate::counter::Counter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/process_collector.rs` at line 9, Remove the unused IntCounter import from the use statement in process_collector.rs: update the existing use crate::counter::{Counter, IntCounter}; to only import the symbols actually used (e.g., Counter) so the IntCounter identifier is no longer referenced in the import list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/process_collector.rs`:
- Around line 166-168: The update to cpu_total can underflow because total -
past may be negative; restore underflow protection by comparing total and past
before calling self.cpu_total.inc_by(...): compute total as (stat.utime +
stat.stime) as f64 / *CLK_TCK as f64, read past via self.cpu_total.get(), and
only call self.cpu_total.inc_by(total - past) when total > past (otherwise skip
or treat the delta as 0) to prevent negative increments and preserve counter
semantics of the cpu_total counter and inc_by usage.
---
Nitpick comments:
In `@src/process_collector.rs`:
- Line 9: Remove the unused IntCounter import from the use statement in
process_collector.rs: update the existing use crate::counter::{Counter,
IntCounter}; to only import the symbols actually used (e.g., Counter) so the
IntCounter identifier is no longer referenced in the import list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
noncrab
force-pushed
the
issue-560-fractional-cpu-seconds
branch
from
89dc0d7 to
f82fa41
Compare
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff: DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
I clearly opened this prematurely. |
1 participant
Fixes #560.
The one thing i'm not sure about is trading the well-defined wrap-around semantics for the risks of running out of bits in the mantissa. But then again, this uses an AtomicF64 under the hood, whose mantissa is 53 bits wide (can accurately hold an integer of up to ~9e+15).
Practically, by my (hand-wavey) reckoning, that'll mean we'll not be able to accurately represent times at ~1µs of precision after ~285 CPU millenia. I'm not sure how much of an issue that might be, or how best to record that concern.