mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
test(telemetry): recapture the baseline and stop gating what variance dominates
Refreshes baselines/baseline-timings.json from run 32964262700 at8418d474a7, byte-identical to the CI artifact. The previous baseline was captured at6a82fc6f37, before the path-finding load was removed from the workload, so it described a load shape the harness no longer runs. Every absolute bound is re-derived, because the rule is hi_next minus baseline and the baselines moved. Three more keys stop being gated: span.tx.apply.p50, span.ledger.build.p50 and span.consensus.ledger_close.p50. This is the rule the previous commit recorded being applied, not a new exception -- a key is gateable only when its run-to-run spread fits inside its bound. The evidence is span.tx.apply.p50, which read 0.7917 ms in the old baseline and 0.00597 ms in this one. That is a 132x move between two runs of the SAME workload. The old value happened to land mid-distribution, so hi_next minus baseline gave a 4.21 ms bound that absorbed the spread; the new value lands in the ladder's first bucket, so the same rule gives 0.0440 ms and cannot survive one. Whether the gate functioned was decided by where in the distribution the captured run happened to fall, which is not a threshold in need of tuning. Measured spreads across four runs agree: 364x, 25.3x and 5.9x respectively. All five excluded keys share one shape -- a baseline landing in the ladder's low buckets, where the derived bound is tiny, together with large run-to-run spread. Single-run baselines cannot support them; a multi-run baseline, or a spread measurement captured alongside the baseline, is what would let them be gated again. Not attempted here. Both runs that would have reddened CI now replay clean, and an injected 10x regression is still caught on 19 of the 20 remaining keys, 20 of 20 at 20x. The exception is job.acceptLedger.running.p95, whose baseline fell while its hi_next did not, moving its floor to 16.28x. It stays gated with that floor recorded beside the other weak keys. Also makes the bounds checker report a zero or negative baseline as a named rule failure instead of dividing by it and raising.
This commit is contained in:
@@ -56,10 +56,18 @@ without having checked anything is the same green-build-that-is-not failure this
|
||||
script exists to prevent, so renaming or deleting one of its inputs must not
|
||||
silence it.
|
||||
|
||||
A baseline ENTRY that is not a positive finite number is rejected before any
|
||||
rule runs, by ``_unusable_baseline``. Every rule does arithmetic on that value,
|
||||
and a degenerate one made the script crash with a traceback (rule D divides by
|
||||
it) or emit advice about the wrong file (a negative value inverts rule D's
|
||||
comparison). Reporting malformed input is what this script is for, so it must
|
||||
name the key rather than die on it.
|
||||
|
||||
Exit 0 when every rule holds, 1 with per-key detail otherwise.
|
||||
"""
|
||||
|
||||
import json
|
||||
import math
|
||||
import re
|
||||
import sys
|
||||
from pathlib import Path
|
||||
@@ -217,13 +225,78 @@ def check_exclusions(metrics_cfg, thresholds, baseline_metrics, declared):
|
||||
return failures
|
||||
|
||||
|
||||
def _unusable_baseline(key, value, unit):
|
||||
"""Reject a baseline value no rule below could evaluate, or None if it is fine.
|
||||
|
||||
Every rule downstream does arithmetic on this number, and two of them break
|
||||
on a degenerate one rather than reporting it:
|
||||
|
||||
* rule D computes ``100 * bound / value``, which raises
|
||||
ZeroDivisionError on ``0.0``. The script then dies with a traceback
|
||||
instead of naming the key -- a validator that crashes where it should
|
||||
report is the same green-build-that-is-not failure in reverse;
|
||||
* a NEGATIVE value makes that same ratio negative, so ``pct >= ratio`` is
|
||||
true for any configured percentage and rule D fires with a message
|
||||
telling the maintainer to lower ``max_pct_increase``. The advice is
|
||||
wrong: the fault is the baseline, not the threshold;
|
||||
* a non-numeric value raises TypeError inside rule E's subtraction.
|
||||
|
||||
Rule E does not cover the zero case, which is easy to assume it does: its
|
||||
test ``abs(value - quantile * first_edge) <= 1e-9 * first_edge`` reduces to
|
||||
``quantile <= 1e-9`` when value is ``0.0``, and that is false for every
|
||||
quantile this harness captures (0.5, 0.95, 0.99).
|
||||
|
||||
None of these arise from the normal pipeline -- ``histogram_quantile`` over
|
||||
a first-bucket-only histogram returns ``quantile x first_edge``, never zero,
|
||||
and rule E is the guard for exactly that. They arise from a hand-edited or
|
||||
truncated baseline, which is precisely the input this script exists to
|
||||
reject.
|
||||
|
||||
Args:
|
||||
key: Flat metric key, for the message.
|
||||
value: The baseline value as read from the file.
|
||||
unit: The entry's unit, for the message.
|
||||
|
||||
Returns:
|
||||
A failure string, or None when the value is usable.
|
||||
"""
|
||||
# bool is a subclass of int; True would otherwise pass as the number 1.
|
||||
if isinstance(value, bool) or not isinstance(value, (int, float)):
|
||||
return (
|
||||
f"{key}: baseline value {value!r} is not a number, so no bound can be "
|
||||
f"derived from it. Recapture the baseline from a CI run rather than "
|
||||
f"editing it by hand -- see baselines/README.md"
|
||||
)
|
||||
if not math.isfinite(value) or value <= 0:
|
||||
return (
|
||||
f"{key}: baseline is {value!r}{unit}, but a captured latency quantile "
|
||||
f"is strictly positive and finite. A zero baseline leaves the "
|
||||
f"percentage bound undefined, a negative one inverts it, and neither "
|
||||
f"can bracket to a bucket -- so no rule below can be evaluated. "
|
||||
f"Recapture the baseline from a CI run rather than editing it by hand "
|
||||
f"-- see baselines/README.md"
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
def check_key(key, entry, thresholds, ladders):
|
||||
"""Apply rules B, C, D and E to one gated key. Returns a list of failures."""
|
||||
"""Apply rules B, C, D and E to one gated key. Returns a list of failures.
|
||||
|
||||
The rules assume the baseline entry holds a strictly positive, finite
|
||||
number, which is what ``histogram_quantile`` yields. Anything else is
|
||||
malformed input, and reporting malformed input is this script's whole job,
|
||||
so it is rejected up front rather than arithmetic being attempted on it --
|
||||
see ``_unusable_baseline``.
|
||||
"""
|
||||
value, unit = entry.get("value"), entry.get("unit", "")
|
||||
edges = ladders.get(unit)
|
||||
if value is None or edges is None:
|
||||
return [f"{key}: baseline has no value, or unknown unit {unit!r}"]
|
||||
|
||||
unusable = _unusable_baseline(key, value, unit)
|
||||
if unusable:
|
||||
return [unusable]
|
||||
|
||||
failures = []
|
||||
first_edge = edges[0]
|
||||
quantile = int(key.rsplit(".p", 1)[1]) / 100.0
|
||||
|
||||
@@ -78,6 +78,25 @@ class CheckerCase(unittest.TestCase):
|
||||
mutate(data)
|
||||
path.write_text(json.dumps(data, indent=2))
|
||||
|
||||
def read_json(self, rel):
|
||||
"""Read a scratch input without modifying it."""
|
||||
return json.loads((self.tree / rel).read_text())
|
||||
|
||||
def gated(self, key):
|
||||
"""Return ``(baseline_value, configured_bound)`` for one gated key.
|
||||
|
||||
Read from the scratch copies of the real inputs rather than written as
|
||||
literals, because a literal here is a copy of one particular baseline:
|
||||
two of these tests previously hard-coded values from the 2026-08-24
|
||||
capture and both broke the moment the baseline was refreshed, which is
|
||||
the very drift check_regression_bounds.py exists to catch. Deriving the
|
||||
figure keeps the assertion pinned to the rule instead of to a snapshot.
|
||||
"""
|
||||
group, quantile = key.rsplit(".", 1)
|
||||
rule = self.read_json(THRESHOLDS)["overrides"][group][quantile]
|
||||
bound = rule.get("max_abs_increase_ms", rule.get("max_abs_increase_us"))
|
||||
return self.read_json(BASELINE)["metrics"][key]["value"], bound
|
||||
|
||||
|
||||
class TestInputHandling(CheckerCase):
|
||||
"""A placeholder passes; a missing or broken input must not."""
|
||||
@@ -160,10 +179,14 @@ class TestRules(CheckerCase):
|
||||
self.assertIn("(rule B)", out)
|
||||
|
||||
def test_rule_c_flags_rounded_bound(self):
|
||||
"""A bound rounded for readability is still not the derived bound."""
|
||||
_, exact = self.gated("span.tx.process.p99")
|
||||
rounded = round(exact, 4)
|
||||
self.assertNotEqual(rounded, exact, "pick a key whose bound rounds visibly")
|
||||
self.edit_json(
|
||||
THRESHOLDS,
|
||||
lambda d: d["overrides"]["span.tx.process"]["p99"].update(
|
||||
max_abs_increase_ms=4.0055
|
||||
max_abs_increase_ms=rounded
|
||||
),
|
||||
)
|
||||
code, out = self.run_checker()
|
||||
@@ -172,7 +195,7 @@ class TestRules(CheckerCase):
|
||||
|
||||
def test_rule_c_accepts_bound_within_relative_tolerance(self):
|
||||
"""The tolerance is 1e-12 relative, not exact equality."""
|
||||
exact = 4.005485184848892
|
||||
_, exact = self.gated("span.tx.process.p99")
|
||||
self.edit_json(
|
||||
THRESHOLDS,
|
||||
lambda d: d["overrides"]["span.tx.process"]["p99"].update(
|
||||
@@ -183,16 +206,32 @@ class TestRules(CheckerCase):
|
||||
self.assertEqual(code, 0, out)
|
||||
|
||||
def test_rule_d_flags_percentage_bound_becoming_operative(self):
|
||||
"""Rule D trips at exactly 100 x bound / baseline, its ``>=`` boundary."""
|
||||
baseline, bound = self.gated("span.tx.apply.p99")
|
||||
boundary = 100.0 * bound / baseline
|
||||
self.edit_json(
|
||||
THRESHOLDS,
|
||||
lambda d: d["overrides"]["span.tx.apply"]["p99"].update(
|
||||
max_pct_increase=150.0
|
||||
max_pct_increase=boundary
|
||||
),
|
||||
)
|
||||
code, out = self.run_checker()
|
||||
self.assertEqual(code, 1, out)
|
||||
self.assertIn("(rule D)", out)
|
||||
|
||||
def test_rule_d_accepts_percentage_bound_just_below_the_boundary(self):
|
||||
"""The other side of rule D's boundary, so the test cannot pass vacuously."""
|
||||
baseline, bound = self.gated("span.tx.apply.p99")
|
||||
boundary = 100.0 * bound / baseline
|
||||
self.edit_json(
|
||||
THRESHOLDS,
|
||||
lambda d: d["overrides"]["span.tx.apply"]["p99"].update(
|
||||
max_pct_increase=boundary * (1 - 1e-9)
|
||||
),
|
||||
)
|
||||
code, out = self.run_checker()
|
||||
self.assertEqual(code, 0, out)
|
||||
|
||||
def test_rule_a_ignores_an_excluded_key(self):
|
||||
"""An excluded key must not read as a baseline that was never captured.
|
||||
|
||||
@@ -253,6 +292,65 @@ class TestRules(CheckerCase):
|
||||
self.assertIn("(rule F)", out)
|
||||
self.assertIn("still has a baseline value", out)
|
||||
|
||||
# A key that is GATED, so seeding it exercises the numeric guard rather than
|
||||
# rule F. It must not be one of the excluded keys: putting a value there
|
||||
# trips "excluded but still has a baseline value" first and the test would
|
||||
# pass for the wrong reason.
|
||||
DEGENERATE_KEY = "span.tx.process.p50"
|
||||
|
||||
def _seed_degenerate_baseline(self, value):
|
||||
"""Replace one gated key's baseline value with an unusable one.
|
||||
|
||||
The threshold is deliberately left alone. The numeric guard runs before
|
||||
every rule, so no bound arrangement is needed to reach it -- and on the
|
||||
pre-fix checker this same seeding still reached the crash, because rule C
|
||||
appends its failure and falls through to rule D's division.
|
||||
"""
|
||||
key = self.DEGENERATE_KEY
|
||||
self.assertNotIn(
|
||||
key,
|
||||
self.read_json(METRICS).get("excluded_keys", {}),
|
||||
"DEGENERATE_KEY must be gated, not excluded",
|
||||
)
|
||||
self.edit_json(
|
||||
BASELINE,
|
||||
lambda d: d["metrics"].update({key: {"unit": "ms", "value": value}}),
|
||||
)
|
||||
|
||||
def test_degenerate_baseline_is_reported_not_crashed(self):
|
||||
"""A zero, negative or non-numeric baseline must NAME the key, not raise.
|
||||
|
||||
Rule D computes ``100 * bound / value``, so a 0.0 baseline used to exit
|
||||
via ZeroDivisionError and a traceback, and a negative one used to emit a
|
||||
rule-D failure blaming ``max_pct_increase`` when the fault was the
|
||||
baseline. Rule E does not cover the zero case: its test reduces to
|
||||
``quantile <= 1e-9`` there, false for every quantile captured.
|
||||
"""
|
||||
# Asserting the guard's OWN wording, not merely "exit 1 with no
|
||||
# traceback": the negative and bool cases already exited 1 without a
|
||||
# traceback before the fix, by emitting a rule-D failure that blamed the
|
||||
# wrong file. A looser assertion passes on that and proves nothing.
|
||||
cases = (
|
||||
(0.0, "strictly positive"),
|
||||
(-1.0, "strictly positive"),
|
||||
(float("nan"), "strictly positive"),
|
||||
(float("inf"), "strictly positive"),
|
||||
("0.006", "is not a number"),
|
||||
(True, "is not a number"),
|
||||
)
|
||||
for value, expected in cases:
|
||||
with self.subTest(value=value):
|
||||
self.setUp() # a clean scratch tree per value
|
||||
self._seed_degenerate_baseline(value)
|
||||
code, out = self.run_checker()
|
||||
self.assertEqual(code, 1, out)
|
||||
self.assertNotIn("Traceback", out)
|
||||
self.assertIn(self.DEGENERATE_KEY, out)
|
||||
self.assertIn(expected, out)
|
||||
# The old rule-D message told the maintainer to lower the
|
||||
# percentage bound; the baseline is what is wrong.
|
||||
self.assertNotIn("(rule D)", out)
|
||||
|
||||
def test_rule_e_flags_ladder_floor_signature(self):
|
||||
"""ledger.store's quantiles were the ladder floor times the quantile."""
|
||||
store = {"p50": 0.005, "p95": 0.0095, "p99": 0.0099}
|
||||
|
||||
Reference in New Issue
Block a user