[web-console] Fix metrics graphs incorrectly drops excessive data points too early - #6935
[web-console] Fix metrics graphs incorrectly drops excessive data points too early#6935Karakatiza666 wants to merge 2 commits into
Conversation
…nts too early This drops the expectation that the metrics time series has a certain number of data points per second Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
| * One second wider than the plotted window, because the throughput series | ||
| * derives a rate from each pair of samples and so cannot plot the oldest one. | ||
| */ | ||
| const RETAIN_MS = GRAPH_WINDOW_MS + 1000 |
There was a problem hiding this comment.
The + 1000 re-introduces the very assumption this PR removes: it only works if the newest sample interval is exactly 1s. I simulated it against staleSampleCount + calcPipelineThroughput: at a 2s effective interval (stats thread lagging under load) the throughput line ends 2000ms short of the left edge; even ~1.05s jitter leaves a 150ms gap. The previous 3s cushion absorbed that. Consider deriving the cushion from the observed gap between the two newest samples, or keeping the wider one — the surplus is clipped by the axis anyway.
| * @param samples - Series ordered oldest first. | ||
| * @param windowMs - How far back from the newest sample to retain. | ||
| */ | ||
| export const staleSampleCount = (samples: TimeSeriesEntry[], windowMs: number): number => { |
There was a problem hiding this comment.
Anchoring the window on the last sample makes a single bogus timestamp destructive: I fed a full 60-sample window one sample with t far in the future and the series collapsed from 60 entries to 1, then needed a full minute of samples to refill the graph. The old count-based buffer could not do that. Multihost is exactly where this can happen — each host stamps with its own Utc::now(), so any host whose clock is more than windowMs ahead permanently evicts every other host's samples. Worth ignoring samples that sit implausibly far ahead of the previous newest one (or capping how much a single append may drop).
| it('retains the same time span whatever the sample rate', () => { | ||
| // A multihost pipeline reports one sample per host per tick, so a count-based | ||
| // buffer would shrink the retained span in proportion to the host count. | ||
| const spanOf = (hosts: number) => { | ||
| const samples = seriesAt(hosts, 90) | ||
| const kept = samples.slice(staleSampleCount(samples, 60_000)) | ||
| return kept.at(-1)!.t - kept[0]!.t | ||
| } | ||
| expect(spanOf(1)).toBe(60_000) | ||
| expect(spanOf(2)).toBe(60_000) | ||
| expect(spanOf(4)).toBe(60_000) |
There was a problem hiding this comment.
This test varies only the timestamps (r stays 0), so it proves the retained span but not that the graphs then read correctly. Replaying the same 4-host interleave with distinct r values through calcPipelineThroughput gives [97, 1, 1, 1, 97, 1, 1, 1, …]: the derivative pairs samples from different hosts, so both the plotted rate and the "Throughput: N records/s" readout are wrong — and this PR now keeps 60s of that instead of 15s. If samples really are per-host, they probably need merging by tick before the derivative; a test with per-host r would pin that down.
|
Ran Two things before merge:
Uncovered cases I exercised by hand (all inline): non-1s sample interval, far-future/skewed timestamp, per-host |
mihaibudiu
left a comment
There was a problem hiding this comment.
Fred seems to have some good questions too
| * @param samples - Series ordered oldest first. | ||
| * @param windowMs - How far back from the newest sample to retain. | ||
| */ | ||
| export const staleSampleCount = (samples: TimeSeriesEntry[], windowMs: number): number => { |
There was a problem hiding this comment.
wouldn't a binary search be faster? this is sorted.
This drops the expectation that the metrics time series has a certain number of data points per second
Also: reduce buffer for kept datapoints - extra 3 seconds of data are not needed because no smoothing over multiple datapoints is performed (an old change).
Testing: added unit tests
Fix #6816