Refuse a telemetry snapshot whose latency buckets mean something else - #46
Merged
Conversation
I wrote a version field into the persisted telemetry and never read it. That is the same failure this repository keeps producing: a fact recorded, its consequence not drawn. The consequence is silent corruption. A bucket stores only a COUNT, and its meaning lives entirely in LATENCY_BUCKET_BOUNDS_MICROS, which is not written to the file. Change those bounds and every historical count is reattributed without a word. I measured the damage rather than describing it. 100 probes recorded at 64 ms sit in the bucket whose bound is 100 ms. Under a plausible refinement of the same LENGTH, finer around 50 to 200 ms, which is exactly the region that collapsed identical percentiles on live data hours ago, that same index means 60 ms. So the daemon would report a 60 ms latency for samples that were all 64 ms. Nothing errors, because a same-length change never triggers the resize. load now refuses a snapshot whose version is not the current one and starts the counts over, saying why. Counters are not state the product depends on, so dropping them is the cheap and honest outcome; reporting a latency that never happened is not. THE REAL GUARD IS THE TEST, because the coupling is otherwise invisible. One test pins the bucket bounds AND the version together and says, if this fails you changed one of the two, change the other. I verified it fires: I made exactly the refinement above without bumping the version and watched it fail with that message. The current version still loads, which a second test asserts, so this cannot quietly discard every restart. The live file on this machine is version 1 and reloads unchanged. 204 lib tests green.
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
1 participant
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.
Latent bug in my own shipped code, found by drawing the consequence of a fact I had already recorded.
The fact I recorded and never used
I wrote a
versionfield into the persisted telemetry in #36 and never read it on load.The consequence
A bucket stores only a count. Its meaning lives entirely in
LATENCY_BUCKET_BOUNDS_MICROS, which is not written to the file. Change those bounds and every historical count is silently reattributed.I measured the damage rather than describing it:
100 probes recorded at 64ms sit in the bucket bounded at 100ms. Under a plausible refinement of the same length — finer around 50–200ms, which is exactly the region that printed identical percentiles for different distributions on live data in #45 — that index means 60ms. The daemon would report a latency that never happened, with no error, because a same-length change never triggers the resize in
record.That refinement is not hypothetical. It is the change #45 made me want.
The fix
loadrefuses a snapshot whose version is not current and starts the counts over, saying why. Counters are not state the product depends on, so dropping them is the cheap and honest outcome; reporting a latency that never happened is not.The real guard is the test
The coupling between bounds and version is invisible in the code, so one test pins both together and says: if this fails you changed one of the two — change the other.
I verified it fires. I made exactly the refinement above without bumping the version and watched it fail with that message. A second test asserts the current version still loads, so this cannot quietly discard every restart — the live file on this machine is version 1 and reloads unchanged.
Why this was worth picking
cos noted that my README already said the reconnect percentiles are bucketed and approximate while max is exact — the fact was written down, and nobody drew the consequence that percentiles therefore cannot compare two similar distributions. This is the same shape: a version field recorded, its purpose never wired up.
Tests
204 lib tests green (3 new). Changed file passes rustfmt. No behaviour change for a current-version file.