fix(bench): fail loud on a missing latency percentile instead of defaulting to 0#767
Merged
Conversation
…ulting it to 0 The last c7g #676 run was wasted by an "empty p999" and the harness did not say so -- it reported a result anyway. Root cause: the extraction used a silent default, p999="$(json_field "${open_json}" p999_us)" [[ -n "${p999}" ]] || p999="0" but `jq -r .missing_key` prints the STRING "null", which is non-empty, so the `-n` guard never fires. A missing percentile therefore became the literal "null" (or, via the sed fallback, ""->0) and flowed into the report and the moat ratio as a plausible-looking 0us tail. On a paid bench host that is a silently invalidated run -- the worst failure mode, because the number LOOKS real. Adds `json_num FILE KEY LABEL`: requires a non-negative integer and otherwise aborts naming the file, the key, the offending value, and dumping the first 20 lines of the results file. The four open-loop percentiles (p50/p99/p999/p9999) now use it -- the p99.9 IS the #518 moat metric and the #676 A/B verdict, so it must never be defaulted. Unit-checked standalone against all five shapes: a good file extracts 30000 (rc=0); a missing key, a JSON null, a non-numeric value, and an empty file each exit 1 with a FATAL diagnostic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FfFZ8gkkNhDBASuntB72HR
perf-gate (A5)Same-runner ratchet of HEAD against the merge-base (both rebuilt and measured in this job).
Overall: PASS
|
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.
Why
The last c7g #676 run was wasted by an "empty p999" -- and the harness reported a result anyway.
Root cause is a silent default that can't fire:
jq -r .missing_keyprints the string"null", which is non-empty, so the-nguard never triggers. A missing percentile became"null"(or0via the sed fallback) and flowed into the report and the moat ratio as a plausible-looking0ustail.That's the worst failure mode: the number looks real, so a paid bench run is silently invalidated.
What
Adds
json_num FILE KEY LABEL-- requires a non-negative integer, else aborts naming the file, key, offending value, and dumping the first 20 lines of the results file. The four open-loop percentiles now use it: the p99.9 is the #518 moat metric and the #676 A/B verdict, so it must never be defaulted.Verified
Unit-checked standalone against all five shapes: good file ->
30000(rc=0); missing key / JSONnull/ non-numeric / empty file -> each exits 1 with a FATAL diagnostic.bash -nclean.Pre-flight for the sanctioned c7g run, so a bad extraction aborts in seconds instead of producing a garbage tail.