fix(json): stop truncating/colliding deep-percentile keys in JSON output

[slice4e]

Problem

--json-out-file truncates and collides percentile keys for --print-percentiles values with more than two decimal places. Both JSON blocks that emit percentile keys are affected, and both start failing at the same, unremarkable input — this is not a deep-tail-only bug.

Both sites build the key with a fixed-decimal format into an 8-byte buffer, but pass sizeof(buf) - 1 as the size, so only 6 characters of key survive. The longest key that fits is p99.99. That 6-char ceiling — not the format string — is what caps both blocks at two effective decimal places.

Percentile Latencies (result_print_to_json() in run_stats.cpp):

char quantile_header[8];
snprintf(quantile_header, sizeof(quantile_header) - 1, "p%.3f", quantile);

Time-Serie (per-second stats, same function):

char quantile_header[8];
snprintf(quantile_header, sizeof(quantile_header) - 1, "p%.2f", quantile);

The %.3f looks like it carries an extra decimal, but p99.991 is 7 characters and gets chopped back to p99.99, so in practice both blocks resolve to the same two decimals.

Where it starts

Any quantile with three or more decimal places collides with its two-decimal neighbour, in both blocks. --print-percentiles=99.99,99.991 emits p99.99 twice; the second write overwrites the first in the JSON object and one requested percentile disappears with no warning. --print-percentiles=99.9,99.99,99.999,100 is enough to lose a value.

The two blocks differ only in how they are wrong, not in when:

• Time-Serie rounds 99.995 and above up to 100.00, then truncates to p100.0 — mislabeling a deep percentile as the maximum and colliding with an explicit 100 request.
• Percentile Latencies truncates those same values to p99.99 — colliding with a genuine 99.99 request instead.

Either way, requested percentiles are silently dropped and the surviving key carries a value that does not match its label. Everything from roughly 99.999 upward — 99.9999, 99.99999, 99.999999, 100 — collapses onto a single key in both blocks.

The plain-text table is unaffected; only the JSON is wrong.

Fix

Both sites now use the format already used for the text-table column header (run_stats::print()), which solved this exact problem there:

char quantile_header[32];
snprintf(quantile_header, sizeof(quantile_header), "p%.10g", quantile);

%.10g varies the number of decimals shown instead of rounding to a fixed count, so every requested quantile gets a distinct, correctly-labeled key. The buffer is sized to hold the full key, and sizeof(buf) is no longer needlessly reduced by one.

⚠️ Breaking change — please read

This deliberately removes the backwards-compatibility guarantee that was documented in the code. The Time-Serie branch carried this comment, which this PR deletes:

// Backwards-compat JSON key shape "pNN.NN" (legacy
// consumers and tests expect this exact format).

Flagging that explicitly rather than letting it disappear in a diff: that promise is being broken on purpose, in both JSON blocks.

Block	Before	After
Percentile Latencies	p50.00, p99.00, p99.90	p50, p99, p99.9
Time-Serie	p50.00, p99.00, p99.90	p50, p99, p99.9

Consumers that look up a fixed key name — e.g. result["ALL STATS"]["Totals"]["Percentile Latencies"]["p99.00"] — will get a KeyError after this change. Consumers that iterate the dict are unaffected. Values are unchanged; only key spelling changes.

Why this can't be avoided: no fixed-decimal-place format can stay backward-compatible for the common case (p99.00) while also staying collision-free for deeper percentiles, since two decimals cannot represent 99.99 and 99.991 as distinct strings without rounding one onto the other. Varying the decimal count (%.10g) is the only way to fix the collision, and it necessarily changes the shallow keys too.

Why both blocks, rather than only fixing Percentile Latencies: an earlier revision of this PR left Time-Serie alone to limit the blast radius. That was the wrong call — it is the same defect, in the same function, with the same threshold, and leaving it would have shipped a JSON document keyed inconsistently with itself (Totals.Percentile Latencies.p99 next to Totals.Time-Serie.<ts>.p99.00) while Time-Serie still silently dropped percentiles. Since consumers have to be updated either way, it is better to break once, consistently, than to ship a half-fixed schema and a second breaking change later.

Testing

• make format-check passes; builds clean with no new warnings.
• Verified against a local Redis with --print-percentiles=50,99,99.9,99.99,99.99999,99.999999. Both blocks now emit six distinct, correctly-valued keys instead of collapsing the tail onto one:

  Percentile Latencies: ['p50', 'p99', 'p99.9', 'p99.99', 'p99.99999', 'p99.999999']
  Time-Serie:           ['p50', 'p99', 'p99.9', 'p99.99', 'p99.99999', 'p99.999999']
  

  Output validated with json.load().
• Updated the in-tree consumers of the old keys:
  • tests/test_monitor_cluster_backpressure.py, tests/differential_redis_benchmark.py (Percentile Latencies)
  • tests/tests_oss_simple_flow.py (4 sites), tests/test_monitor_input.py (2 sites) (Time-Serie)
  • tests_oss_simple_flow.py::test_default_set_get_with_print_percentiles needed no change — it already normalizes with key.split(".")[0].
• Repo-wide grep confirms no remaining references to p50.00 / p99.00 / p99.90.

Drive-by: unrelated flaky test fix

test_cpu_stats.py::test_cpu_warn_threshold_flag was failing CI on this branch. It is not related to this change — tests/test_cpu_stats.py is untouched here, and it has been flaky since #471.

The test counted the substring 'of a core' in stderr and compared it against the number of threads whose authoritative run-average cores_used exceeds 1.0. Two different warnings contain that substring:

memtier_benchmark.cpp:3059   warning: high CPU on thread %u: %.1f%% of a core (threshold %.1f%%) ...
run_stats.cpp:2435           warning: thread %u averaged %.1f%% of a core over the run (threshold %.1f%%) ...

Only the second is the end-of-run warning the assertion is about. With --cpu-warn-threshold=100 the live per-second warning is armed at 100% of a core too, and a 1-second sample can legitimately exceed that because the wall window and the CPU-clock sampling do not line up — the same file already tolerates this via PER_THREAD_PCT_CEILING = 110.0. When the live warning fires, end_warnings becomes 1 while over_one_core is 0 and the test fails, intermittently, depending on runner load.

Fixed by matching 'of a core over the run', which is unique to the end-of-run message. Confirmed against a live run — one invocation emits both warnings:

warning: high CPU on thread 0: 86.9% of a core (threshold 0.0%) ...
warning: thread 0 averaged 89.6% of a core over the run (threshold 0.0%) ...
count("of a core")             = 2   <- what the test used
count("of a core over the run")= 1   <- what it means

Happy to split this into its own PR if you'd prefer to keep this one single-purpose.

---

Note

^{Cursor Bugbot is generating a summary for commit 1dbb49e. Configure here.}

[fcostaoliveira]

Thanks for this — went through it, the root-cause diagnosis (the -1 on sizeof() plus a fixed-decimal format that rounds before it truncates) is precise and the writeup is genuinely one of the clearer bug reports I've seen come through here. A few things I'd want settled before this merges.

The big one: --realtime-latencies (#379) hit this exact class of bug in a different output channel — table headers, the rtl stderr block, statsd names — and that PR's own commit message says explicitly: "JSON keys keep their p%.2f / p%.3f shape for back-compat with existing consumers and the test suite" while everything else got switched to %.10g. That was a deliberate, reasoned decision to leave JSON alone. This PR reverses it without engaging with why. I may be wrong, but given the precedent (this is the same tradeoff oranagra reasoned through in #212 — pick the less-breaking of two viable fixes), shouldn't the PR at least say what changed since #379 to make the JSON break acceptable now?

And separately — does the fix actually need to touch every key, or only the ones that collide? The default config is 50,99,99.9; under the old %.2f/%.3f code those three never round or truncate, so p50.00/p99.00/p99.90 are correct today for anyone running with defaults. This PR still rewrites them to p50/p99/p99.9. That's breaking every default-config consumer to fix a bug that only manifests once someone asks for 3+ decimal places. Did you consider keeping the fixed-decimal shape for quantiles that already round-trip cleanly and only falling back to %.10g for the ones that'd otherwise collide? Feels closer to the "minimize blast radius" call yossigo made on #60.

On testing: per #364's own bar, a JSON-format fix needs a regression test that fails on master and passes here — not just a manual --print-percentiles=... run pasted into the description. The test diffs in this PR only update existing assertions from p99.00 to p99 (2-decimal quantiles, which never collided in the first place); nothing here actually exercises 99.99 vs 99.991 or 99.99999 vs 99.999999 and asserts six distinct keys. Can you add one that pins that specific invariant?

Nitpicking: run_stats.cpp:1931 — the text-table header you're citing as the template still does sizeof(average_header) - 1. Not this PR's problem to fix, but it's the same "wastes a byte" pattern you removed at both JSON sites, one function up.

The flaky-test fix looks fine and orthogonal — you already offered to split it, that's enough for me, no need to actually do it unless someone else feels strongly.

Buffer widening and the switch to sizeof(buf) without the -1 at both JSON sites look correct on their own. The core mechanics of the fix look solid, I'd just want the #379-reversal question answered and a real collision-repro test before this merges.

(would file this as a comment, not a formal block)