fix(telemetry): stop gating ledger.validate p95 and p99, which vary too much

The regression gate has been red on runs with no code change. Only two of
the 25 gated keys ever tripped, both on the same span and never together:
run 32862589645 failed p99 at 25.8750 ms against a 1.0600 ms baseline
(+2341%), run 32867433073 failed p95 at 0.7500 ms against 0.2404 ms
(+212%), and in each run the other quantile sat well inside its own bound.
A real slowdown would move both. This is variance, not a defect.

Measured across four CI runs:

  span.ledger.validate.p50   0.0484 to 0.0778 ms    1.6x spread   kept
  span.ledger.validate.p95   0.1281 to 0.7500 ms    5.9x spread   excluded
  span.ledger.validate.p99   0.3875 to 25.8750 ms  66.8x spread   excluded

Both excluded quantiles reach past their trip point on a healthy run. The
mechanism is arrival timing, not slow code: the span opens only once a
quorum-completing validation arrives (LedgerMaster.cpp:987, inside
checkAccept, past the early return) and wraps the promotion work that
follows, so one slow consensus round dominates the tail of a 3m rate
window and which round that is differs every run.

Widening is not available and must not be attempted later: tolerating
25.8750 ms against a 1.0600 ms baseline needs a bound of about 24.8 ms,
which gates nothing. A bound admitting every healthy run's worst case
admits every regression too. p50 stays gated; it is stable.

THE GENERAL RULE, recorded so this does not recur: an absolute bound
derived as hi_next minus baseline comes from the histogram ladder, so it
budgets for quantization noise and for nothing else. It knows nothing about
how far a metric moves between runs on identical code. Before gating any
key, check its observed maximum across several runs against its trip point
and gate it only with margin. Spread alone proves nothing: tx.apply.p50
swings 364x and never fires, because its 5 ms trip point absorbs the range.
Of the 23 keys still gated the worst reaches 0.67 of its trip point.

Mechanism: spans.names lists span names while _quantiles is shared, so
dropping two quantiles of one span cannot be expressed by deleting a name.
regression-metrics.json gains an excluded_keys map from a flat key to the
reason it is not gated, subtracted by both prom_queries.py (so the key is
never queried) and check_regression_bounds.py rule A. A per-name quantile
override was rejected: a typo there leaves the key gating, whereas a typo
in an exclusion subtracts nothing and new rule F rejects it, along with an
empty reason, a leftover threshold override and a leftover baseline value.

Derived figures recomputed from the committed baseline: 25 gated keys to
23, detection floor 2.02x-9.43x to 2.02x-9.42x, weakly guarded keys ten to
nine, bound over baseline 102%-843% to 102%-842%. The baseline edit is a
deletion of two entries only, with no value rewritten.

Verified: both previously failing runs replay to zero regressions and exit
0; a tenfold increase injected into each of the 23 remaining keys in turn
is still caught in all 23 cases; rule F was confirmed load-bearing by
stubbing it out, which lets a stale exclusion pass.
This commit is contained in:
Pratik Mankawde
2026-08-25 18:21:35 +01:00
parent c65cb0e2a8
commit 3836078a78
9 changed files with 268 additions and 41 deletions

View File

@@ -26,7 +26,7 @@ bucket ``(lo, hi]`` of its ladder, with ``hi_next`` the next edge above ``hi``,
max_abs_increase_* == hi_next - baseline
so the gate trips only when the reading clears the bucket *above* the
baseline's own. Five rules are checked:
baseline's own. Six rules are checked:
A the baseline's key set equals the surface ``regression-metrics.json``
declares (a stale key left behind reads as covered but never gates);
@@ -38,7 +38,15 @@ baseline's own. Five rules are checked:
stops being true;
E no baseline carries the ladder-floor signature ``quantile x first_edge``,
which means every sample landed in the first bucket and the number is
interpolation arithmetic rather than a latency.
interpolation arithmetic rather than a latency;
F every entry in ``excluded_keys`` names a key the surface would otherwise
declare, carries a reason, and has neither a threshold override nor a
baseline value left behind.
Rule A subtracts ``excluded_keys`` before comparing, so a quantile removed from
the gated set does not read as a missing baseline. Rule F is what keeps that
subtraction honest: an exclusion is the one edit here that makes the gate cover
LESS, so a stale or misspelt entry must fail rather than silently widen itself.
A PLACEHOLDER baseline -- ``"placeholder": true`` or an empty ``metrics``
object -- exits 0, because that is the documented bootstrap state and CI has to
@@ -119,6 +127,10 @@ def declared_keys(metrics_cfg):
Deliberately reimplemented rather than imported from ``prom_queries.py``,
which pulls in aiohttp; CI telemetry checks stay dependency-free. The key
format is fixed by that file's own ``_key_format`` field.
``excluded_keys`` is NOT subtracted here: rule F needs the full product to
tell a real exclusion from a misspelt one. Callers that want the gated
surface subtract it themselves.
"""
keys = set()
spans = metrics_cfg.get("spans", {})
@@ -154,6 +166,57 @@ def resolve_override(key, thresholds):
return thresholds.get("overrides", {}).get(group, {}).get(quantile)
def check_exclusions(metrics_cfg, thresholds, baseline_metrics, declared):
"""Apply rule F to every entry in ``excluded_keys``.
An exclusion is the only edit to this config that makes the gate cover
LESS, so each entry has to prove it is deliberate and complete:
* it names a key the names x quantiles product would otherwise declare,
so a typo or a stale entry surviving a surface change is caught rather
than silently subtracting nothing;
* it carries a non-empty reason, because "why is this not gated" is the
question a future maintainer will ask and prose is the only answer;
* no threshold override and no baseline value are left behind, since
either would read as gated to anyone grepping for the key.
Args:
metrics_cfg: Parsed regression-metrics.json.
thresholds: Parsed regression-thresholds.json.
baseline_metrics: The baseline's ``metrics`` map.
declared: Output of declared_keys(), before exclusions come off.
Returns:
A list of failure strings, empty when every entry is well formed.
"""
failures = []
for key, reason in sorted(metrics_cfg.get("excluded_keys", {}).items()):
if key not in declared:
failures.append(
f"{key}: listed in excluded_keys but not produced by the "
f"names x quantiles product, so it subtracts nothing -- fix the "
f"spelling or drop the entry (rule F)"
)
continue
if not isinstance(reason, str) or not reason.strip():
failures.append(
f"{key}: excluded with no reason. Record why it is not gated, "
f"with the measurement behind it (rule F)"
)
if resolve_override(key, thresholds) is not None:
failures.append(
f"{key}: excluded but still has a threshold override, which "
f"reads as gated -- remove it from {THRESHOLDS} (rule F)"
)
if key in baseline_metrics:
failures.append(
f"{key}: excluded but still has a baseline value, so rule A "
f"would pass while nothing gates it -- remove it from "
f"{BASELINE} (rule F)"
)
return failures
def check_key(key, entry, thresholds, ladders):
"""Apply rules B, C, D and E to one gated key. Returns a list of failures."""
value, unit = entry.get("value"), entry.get("unit", "")
@@ -242,6 +305,10 @@ def main():
failures = []
declared = declared_keys(metrics_cfg)
failures.extend(check_exclusions(metrics_cfg, thresholds, gated, declared))
# Rule A compares against the GATED surface, so a deliberately excluded
# quantile is not reported as a baseline that was never captured.
declared -= set(metrics_cfg.get("excluded_keys", {}))
for key in sorted(set(gated) - declared):
failures.append(
f"{key}: in the baseline but not declared by {METRICS}, so it is "
@@ -258,11 +325,19 @@ def main():
failures.extend(check_key(key, gated[key], thresholds, ladders))
if not failures:
excluded = metrics_cfg.get("excluded_keys", {})
print(
f"OK: {len(gated)} gated key(s); every absolute bound equals "
f"hi_next - baseline, every key has an override, and the absolute "
f"bound is the operative half of the AND for all of them"
)
# Printed, not silent: an exclusion narrows the gate, so the count
# belongs in the CI log where a reviewer sees it without opening a file.
if excluded:
print(
f" {len(excluded)} declared key(s) deliberately not gated: "
f"{', '.join(sorted(excluded))} (see excluded_keys in {METRICS})"
)
return 0
print(

View File

@@ -13,7 +13,7 @@ Two groups:
is the documented bootstrap state, while a missing, unreadable or malformed
input must FAIL. A checker that returns success without having checked
anything is the failure this whole gate exists to prevent;
* one case per rule (A to E), so a rule that stops flagging is caught.
* one case per rule (A to F), so a rule that stops flagging is caught.
stdlib unittest only; the repo installs no third-party runner for CI.
"""
@@ -193,6 +193,66 @@ class TestRules(CheckerCase):
self.assertEqual(code, 1, out)
self.assertIn("(rule D)", out)
def test_rule_a_ignores_an_excluded_key(self):
"""An excluded key must not read as a baseline that was never captured.
The unmodified tree already exercises this — span.ledger.validate p95
and p99 are declared by the names x quantiles product, excluded, and
absent from the baseline — so this asserts the subtraction is what makes
it pass, by naming the keys in the reported exclusion line.
"""
code, out = self.run_checker()
self.assertEqual(code, 0, out)
self.assertIn("span.ledger.validate.p95", out)
self.assertIn("deliberately not gated", out)
def test_rule_f_flags_exclusion_that_subtracts_nothing(self):
"""A misspelt or stale exclusion silently narrows nothing — catch it."""
self.edit_json(
METRICS,
lambda d: d["excluded_keys"].update({"span.ledger.validate.p97": "typo"}),
)
code, out = self.run_checker()
self.assertEqual(code, 1, out)
self.assertIn("(rule F)", out)
self.assertIn("subtracts nothing", out)
def test_rule_f_flags_exclusion_without_a_reason(self):
self.edit_json(
METRICS,
lambda d: d["excluded_keys"].update({"span.ledger.validate.p95": " "}),
)
code, out = self.run_checker()
self.assertEqual(code, 1, out)
self.assertIn("(rule F)", out)
self.assertIn("no reason", out)
def test_rule_f_flags_override_left_behind(self):
"""An excluded key still carrying a bound reads as gated."""
self.edit_json(
THRESHOLDS,
lambda d: d["overrides"]["span.ledger.validate"].update(
{"p95": {"max_pct_increase": 50.0, "max_abs_increase_ms": 0.25}}
),
)
code, out = self.run_checker()
self.assertEqual(code, 1, out)
self.assertIn("(rule F)", out)
self.assertIn("still has a threshold override", out)
def test_rule_f_flags_baseline_value_left_behind(self):
"""Excluded but still in the baseline: rule A passes, nothing gates."""
self.edit_json(
BASELINE,
lambda d: d["metrics"].update(
{"span.ledger.validate.p95": {"unit": "ms", "value": 0.24}}
),
)
code, out = self.run_checker()
self.assertEqual(code, 1, out)
self.assertIn("(rule F)", out)
self.assertIn("still has a baseline value", 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}