mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to 29673de531, three commits: a Loki-diagnostic count fix with
Tempo errors made visible, a baseline recapture that stops gating keys whose
run-to-run variance dominates their bound, and WebSocket reply correlation with
silent placeholder data removed.
Merged clean -- git reported no conflicts, so no resolution decisions were made
here. The 14 incoming files are all harness and docs: six .py, three .md, three
.json, two .sh. No runtime C++ and no include-line changes.
The baseline recapture resolves the regression gate that reddened the previous
run on this branch. That run reported span.consensus.ledger_close p95 1.62ms and
p99 7.14ms against a baseline of 0.49 and 0.93; the recaptured baseline puts p95
at 0.783 with a 5ms trip point and p99 at 2.03 with a 10ms trip point, so both
readings now sit inside the gate. That matches the diagnosis recorded at the
time: everything changed between the last green gate and that failure was a .py,
a .md and a GTest file, with zero runtime C++, and p50 had improved while only
the tail moved -- variance, not a slowdown. The incoming commit reaches the same
conclusion from the other side, excluding three p50 keys after measuring spreads
of 391.8x, 20.7x and 6.1x across three runs.
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