Repository navigation
Conversation
…mall Record::timestamp comes from SystemTime (wall clock, not monotonic), so a clock adjustment between two consecutive records can make their time diff tiny. get_records_diff_power_microwatts() (Topology, CPUSocket and Domain) then divides a normal energy delta by that interval and reports a bogus power value. This is reproducible deterministically on real RAPL hardware: a real ~2s energy delta with the second record's timestamp corrupted to be 0.5ms after the first one's yields ~54.7 kW on the unpatched code. Discard the sample (return None) when time_diff is below a 10ms floor instead of dividing by it. The prometheus exporter's own scrape debounce already guarantees records are at least ~2s apart in normal operation, so the floor has ample margin without rejecting legitimate fast sampling in other exporters (stdout/json support a configurable, potentially sub-second step).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #441
What
get_records_diff_power_microwatts()(forTopology,CPUSocketandDomain) now discards the sample (returnsNone) when the interval between the two records is below a 10 ms floor (MIN_RECORD_TIME_DIFF_S), instead of dividing by it.Why
Record::timestampcomes fromSystemTime(wall clock, not monotonic), so a clock adjustment between two records can make the interval tiny while the energy delta stays normal, which produces a bogus power value. See #441 for the details and the reproduction.The prometheus exporter already debounces refreshes so records are at least ~2 s apart, so the floor does not affect normal operation. The stdout/json exporters accept a configurable, potentially sub-second step, which is why the floor is only 10 ms.
Test plan
cargo test --no-default-features --features "prometheus containers prometheuspush qemu json" --lib: all pass, including the two new unit tests (discards_power_sample_when_time_diff_too_small,computes_power_sample_when_time_diff_is_plausible).repro_bogus_power_spike_on_real_hardware(#[ignore], needs root and real RAPL): a real ~2 s delta with the second timestamp corrupted to +0.5 ms gave ~54.7 kW before the change; the sample is now discarded.cargo fmt --check