test(telemetry): assert commit cost on allocation, not peak heap - #257
Merged
Merged
Conversation
The commit-memory gate added in #255 is flaky and its threshold does not separate the two encodings. It failed CI at 3.83x against a 3.75 limit for code that measured 2.79x-3.49x locally, turning main red. Peak live heap was the wrong signal. It carries whatever slack the collector happened to be holding when the sampler looked, which varies with GOMAXPROCS and machine speed, and the two encodings' peak-heap distributions overlap: dictionary encoding measured 3.21x-3.56x and plain 1.45x-2.52x once the sampling was averaged over several commits. A threshold between those is a coin flip, and the 4.0 I first reached for would have passed the very regression the test exists to catch. Total bytes allocated does separate them, and barely varies: 10.68x-10.90x with dictionary encoding against 4.40x-5.69x without. It also answers the question the test is actually asking -- how many copies does one commit make -- rather than how much the collector was holding at one instant. The limit moves to 7.0, in the middle of that gap, and is verified in both directions: restoring the dictionary tags fails it at 10.6x. Peak heap is still measured and logged, because it is what the kernel kills on, and the figure is now the minimum across three commits so the log is comparable between runs.
The race detector instruments every allocation, which inflated total allocation from roughly 5x payload to 33x on unchanged code and failed the race target in CI. Peak heap is distorted the same way. These figures are only meaningful in an uninstrumented build, so the file is excluded from -race rather than given a second, meaningless threshold.
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.
Fixes the flaky gate introduced in #255. main is currently red.
What went wrong
The commit-memory test failed CI at
ratio 3.83xagainst a 3.75 limit, for code that measured 2.79x–3.49x locally. I set that threshold from local runs on one machine and it did not survive a different one.Worse, when I stabilised the sampling by taking the minimum across several commits, I found the threshold was never sound in the first place:
A limit of 4.0 — which is where I would have landed to stop the flake — passes the regression the test exists to catch. The test would have looked green and gated nothing.
Fix
Assert on total bytes allocated instead of peak live heap.
No overlap, and the dictionary figure varies by about 2% across runs. Peak heap carries whatever slack the collector happened to be holding when the sampler looked, which varies with GOMAXPROCS and machine speed; total allocation does not, and it answers the question the test is actually asking — how many copies does one commit make.
Limit set to 7.0, in the middle of the gap, and verified in both directions: restoring the dictionary tags fails it at 10.6x.
Peak heap is still sampled and logged, because it is what the kernel kills on, and it is now reported as the minimum across three commits so the number is comparable between runs.
Note on method
I should have validated the original gate against the regression before merging #255 rather than only against the fix. A threshold that only ever passes proves nothing. This PR verifies both directions, and that check is cheap enough that it should be the habit.
just checkpasses.