diff --git a/.github/scripts/telemetry/check_regression_bounds.py b/.github/scripts/telemetry/check_regression_bounds.py index b07045ce70..4e89ac421c 100644 --- a/.github/scripts/telemetry/check_regression_bounds.py +++ b/.github/scripts/telemetry/check_regression_bounds.py @@ -87,6 +87,11 @@ UNIT_TO_MS = {"ms": 1.0, "s": 1000.0} REL_TOLERANCE = 1e-12 +def is_number(value): + """True for a real JSON number. bool is an int subclass, so exclude it.""" + return not isinstance(value, bool) and isinstance(value, (int, float)) + + def read_text_or_exit(path): """Read a required text input, or exit 1 naming the input that failed.""" try: @@ -96,11 +101,22 @@ def read_text_or_exit(path): def read_json_or_exit(path): - """Read and parse a required JSON input, or exit 1 naming what failed.""" + """Read and parse a required JSON object, or exit 1 naming what failed. + + All three JSON inputs are objects. A top-level null, list or number parses + fine and then dies on the first .get, so check the shape here rather than + report it as a traceback pointing into this script. + """ try: - return json.loads(read_text_or_exit(path)) + parsed = json.loads(read_text_or_exit(path)) except json.JSONDecodeError as exc: sys.exit(f"{path}: required input is not valid JSON -- {exc}") + if not isinstance(parsed, dict): + sys.exit( + f"{path}: required input is valid JSON but its top level is " + f"{type(parsed).__name__}, not an object -- nothing can be read from it" + ) + return parsed def span_edges_ms(): @@ -264,7 +280,7 @@ def _unusable_baseline(key, value, unit): 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)): + if not is_number(value): 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 " @@ -338,12 +354,24 @@ def check_key(key, entry, thresholds, ladders): if rule is None: failures.append( f"{key}: no per-metric override, so it falls back to the defaults and " - f"gates on the percentage bound alone. Add an override with " - f"max_abs_increase = {hi_next - value!r} (rule B)" + f"gates on the percentage bound alone. Add an override in " + f"{THRESHOLDS} with " + f"max_abs_increase_{unit} = {hi_next - value!r} (rule B)" ) return failures + if not isinstance(rule, dict): + return [ + f"{key}: threshold override {rule!r} is not an object carrying " + f"max_abs_increase_{unit} and max_pct_increase -- fix it in {THRESHOLDS}" + ] + bound = rule.get("max_abs_increase_ms", rule.get("max_abs_increase_us")) + if bound is not None and not is_number(bound): + return [ + f"{key}: max_abs_increase_{unit} is {bound!r}, not a number, so it " + f"cannot be compared with the derived bound -- fix it in {THRESHOLDS}" + ] expected = hi_next - value if bound is None or abs(bound - expected) > REL_TOLERANCE * expected: failures.append( @@ -354,6 +382,11 @@ def check_key(key, entry, thresholds, ladders): pct = rule.get("max_pct_increase") if pct is None: failures.append(f"{key}: no max_pct_increase, so the metric never gates") + elif not is_number(pct): + failures.append( + f"{key}: max_pct_increase is {pct!r}, not a number -- fix it in " + f"{THRESHOLDS}" + ) elif bound is not None and pct >= 100.0 * bound / value: failures.append( f"{key}: max_pct_increase {pct:g}% is at or above the absolute bound's " @@ -391,6 +424,13 @@ def main(): ladders = {"ms": span_edges_ms(), "us": microsecond_edges()} gated = baseline["metrics"] + # A list or a string is truthy, so it survives the placeholder test above + # and then either crashes or reports its characters as gated keys. + if not isinstance(gated, dict): + sys.exit( + f"{BASELINE}: 'metrics' is {type(gated).__name__}, not an object of " + f"key -> {{value, unit}} -- recapture the baseline from a CI run" + ) failures = [] declared = declared_keys(metrics_cfg) diff --git a/.github/scripts/telemetry/test_check_regression_bounds.py b/.github/scripts/telemetry/test_check_regression_bounds.py index ac66eb2f1b..44222e6576 100644 --- a/.github/scripts/telemetry/test_check_regression_bounds.py +++ b/.github/scripts/telemetry/test_check_regression_bounds.py @@ -156,6 +156,45 @@ class TestInputHandling(CheckerCase): self.assertEqual(code, 1, out) self.assertIn("valid JSON", out) + def test_non_object_top_level_fails_naming_the_input(self): + """Valid JSON of the wrong shape must be named, not raise a traceback. + + A top-level null, list or number parses, so it reaches the first .get + and dies pointing at a line in the checker rather than at the file the + operator has to fix. + """ + for rel, text in ( + (BASELINE, "null"), + (THRESHOLDS, "[]"), + (METRICS, "5"), + ): + with self.subTest(input=rel): + self.setUp() + (self.tree / rel).write_text(text) + code, out = self.run_checker() + self.assertEqual(code, 1, out) + self.assertIn(rel, out) + self.assertIn("not an object", out) + self.assertNotIn("Traceback", out) + + def test_non_object_metrics_map_fails_naming_the_input(self): + """A string 'metrics' is truthy, so it slips past the placeholder test. + + Left unchecked it reports the string's own characters as gated keys, + which is worse than a crash: the advice is wrong rather than absent. + An empty map still has to pass, because that is the bootstrap state. + """ + self.edit_json(BASELINE, lambda d: d.update(metrics="span.tx.process.p99")) + code, out = self.run_checker() + self.assertEqual(code, 1, out) + self.assertIn("'metrics' is str", out) + self.assertNotIn("Traceback", out) + + self.setUp() + self.edit_json(BASELINE, lambda d: d.update(metrics={})) + code, out = self.run_checker() + self.assertEqual(code, 0, out) + class TestRules(CheckerCase): """One case per rule, so a rule that stops flagging is caught.""" @@ -183,6 +222,40 @@ class TestRules(CheckerCase): self.assertEqual(code, 1, out) self.assertIn("(rule B)", out) + def test_rule_b_names_the_unit_suffixed_key(self): + """The key it tells the operator to add must be the key the code reads. + + The bound is stored as max_abs_increase_ms or _us. A message naming a + bare max_abs_increase sends the operator to add a key nothing reads, so + the gate keeps failing with no explanation. Both suffixes are covered, + because a test on the ms side alone passes on a hard-coded "_ms". + """ + for group, suffix in ( + ("span.ledger.build", "max_abs_increase_ms"), + ("job.transaction.queued", "max_abs_increase_us"), + ): + with self.subTest(group=group): + self.setUp() + self.edit_json(THRESHOLDS, lambda d: d["overrides"].pop(group)) + code, out = self.run_checker() + self.assertEqual(code, 1, out) + self.assertIn("(rule B)", out) + self.assertIn(suffix, out) + self.assertNotIn("max_abs_increase =", out) + + def test_non_numeric_threshold_is_reported_not_crashed(self): + """A hand-edited bound that is a string must be named, not raise.""" + self.edit_json( + THRESHOLDS, + lambda d: d["overrides"]["span.ledger.build"]["p99"].update( + max_abs_increase_ms="5.5" + ), + ) + code, out = self.run_checker() + self.assertEqual(code, 1, out) + self.assertIn("not a number", out) + self.assertNotIn("Traceback", 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") diff --git a/.github/workflows/telemetry-validation.yml b/.github/workflows/telemetry-validation.yml index e5b20de529..e60b5399b8 100644 --- a/.github/workflows/telemetry-validation.yml +++ b/.github/workflows/telemetry-validation.yml @@ -5,7 +5,7 @@ # # This is a separate workflow from the main CI. It runs: # - On manual dispatch (workflow_dispatch) -# - On pushes to telemetry-related branches +# - On any push that touches one of the paths globs below # # The workflow is intentionally heavyweight (builds rippled, starts Docker # services, runs a multi-node cluster) — it validates the full telemetry @@ -420,22 +420,51 @@ jobs: cat "$TIMINGS" >>"$GITHUB_STEP_SUMMARY" echo '```' >>"$GITHUB_STEP_SUMMARY" elif [ -f "$REGRESSION" ]; then - REGR_COUNT=$(jq -e '.summary.regressions' "$REGRESSION") || REGR_COUNT=0 - IMPR_COUNT=$(jq -e '.summary.improvements' "$REGRESSION") || IMPR_COUNT=0 - TOTAL=$(jq -e '.summary.total' "$REGRESSION") || TOTAL=0 - echo "| Stat | Count |" >>"$GITHUB_STEP_SUMMARY" - echo "|------|-------|" >>"$GITHUB_STEP_SUMMARY" - echo "| Metrics compared | $TOTAL |" >>"$GITHUB_STEP_SUMMARY" - echo "| Regressions | $REGR_COUNT |" >>"$GITHUB_STEP_SUMMARY" - echo "| Improvements | $IMPR_COUNT |" >>"$GITHUB_STEP_SUMMARY" - echo "" >>"$GITHUB_STEP_SUMMARY" - if [ "$REGR_COUNT" -gt 0 ]; then - echo "### Regressions" >>"$GITHUB_STEP_SUMMARY" + # Existence is not readability: a truncated report satisfies -f, + # and the `|| =0` fallbacks below would then render a clean table + # of zeros for a run that compared nothing. Same trap the note + # above the baseline parse warns about, so check the shape first. + SUMMARY_OK=$(jq -r 'if (.summary | type) == "object" then "yes" else "no" end' \ + "$REGRESSION" 2>/dev/null) || SUMMARY_OK=no + if [ "$SUMMARY_OK" != "yes" ]; then + echo "## Regression Gate: report unreadable" >>"$GITHUB_STEP_SUMMARY" echo "" >>"$GITHUB_STEP_SUMMARY" - echo "| Metric | Baseline | Current | Δ | % | Unit |" >>"$GITHUB_STEP_SUMMARY" - echo "|--------|---------:|--------:|--:|--:|------|" >>"$GITHUB_STEP_SUMMARY" - jq -r '.metrics[] | select(.regressed) | "| \(.key) | \(.baseline) | \(.current) | \(.delta) | \(.pct_change)% | \(.unit) |"' \ - "$REGRESSION" >>"$GITHUB_STEP_SUMMARY" + echo "\`$REGRESSION\` exists but carries no \`summary\` object, so no" \ + "table can be rendered. The pass/fail above still comes from the" \ + "comparator's exit code." >>"$GITHUB_STEP_SUMMARY" + echo "::error::Regression report is present but has no summary object" + else + # No `jq -e`: it exits non-zero when a field is legitimately 0 or + # false, which the fallbacks would silently turn into 0 as well. + REGR_COUNT=$(jq -r '.summary.regressions // 0' "$REGRESSION") + IMPR_COUNT=$(jq -r '.summary.improvements // 0' "$REGRESSION") + TOTAL=$(jq -r '.summary.total // 0' "$REGRESSION") + MISSING_COUNT=$(jq -r '.summary.missing_in_current // 0' "$REGRESSION") + COMPARED=$(jq -r '.summary.compared // 0' "$REGRESSION") + # `total` is every key in the report, i.e. the union of the + # baseline and this run, so it is not what was gated. The + # comparator reports `compared` for that; do not derive it from + # total minus missing, because a key can also be skipped for + # being new or for having no data on either side. + echo "| Stat | Count |" >>"$GITHUB_STEP_SUMMARY" + echo "|------|-------|" >>"$GITHUB_STEP_SUMMARY" + echo "| Metrics in report | $TOTAL |" >>"$GITHUB_STEP_SUMMARY" + echo "| Metrics compared | $COMPARED |" >>"$GITHUB_STEP_SUMMARY" + echo "| Not captured this run | $MISSING_COUNT |" >>"$GITHUB_STEP_SUMMARY" + echo "| Regressions | $REGR_COUNT |" >>"$GITHUB_STEP_SUMMARY" + echo "| Improvements | $IMPR_COUNT |" >>"$GITHUB_STEP_SUMMARY" + echo "" >>"$GITHUB_STEP_SUMMARY" + if [ "$MISSING_COUNT" -gt 0 ]; then + echo "::warning::$MISSING_COUNT baseline metric(s) were not captured this run, so they were not gated" + fi + if [ "$REGR_COUNT" -gt 0 ]; then + echo "### Regressions" >>"$GITHUB_STEP_SUMMARY" + echo "" >>"$GITHUB_STEP_SUMMARY" + echo "| Metric | Baseline | Current | Δ | % | Unit |" >>"$GITHUB_STEP_SUMMARY" + echo "|--------|---------:|--------:|--:|--:|------|" >>"$GITHUB_STEP_SUMMARY" + jq -r '.metrics[] | select(.regressed) | "| \(.key) | \(.baseline) | \(.current) | \(.delta) | \(.pct_change)% | \(.unit) |"' \ + "$REGRESSION" >>"$GITHUB_STEP_SUMMARY" + fi fi fi diff --git a/docker/telemetry/workload/compare_to_baseline.py b/docker/telemetry/workload/compare_to_baseline.py index 5619400858..4ae452a460 100644 --- a/docker/telemetry/workload/compare_to_baseline.py +++ b/docker/telemetry/workload/compare_to_baseline.py @@ -282,6 +282,29 @@ def compute_delta( current = current_entry.get("value") if current_entry else None unit = (baseline_entry or current_entry or {}).get("unit", "") + # A unit change makes the two numbers incomparable, so subtracting them is + # meaningless: us -> ms reads as a 99.9% improvement and the gate passes. + # Fail instead, and name both units so the baseline can be refreshed. + baseline_unit = (baseline_entry or {}).get("unit", "") + current_unit = (current_entry or {}).get("unit", "") + if baseline_unit and current_unit and baseline_unit != current_unit: + pct_threshold, abs_threshold = resolve_thresholds(key, thresholds) + return MetricDelta( + key=key, + baseline=baseline, + current=current, + delta=None, + pct_change=None, + unit=f"{baseline_unit}->{current_unit}", + threshold_pct=pct_threshold, + threshold_abs=abs_threshold, + regressed=True, + note=( + f"unit changed: baseline is {baseline_unit}, current run is " + f"{current_unit} -- refresh the baseline instead of comparing" + ), + ) + if baseline is None and current is None: return _skip_delta( key, None, None, unit, thresholds, "no data (neither baseline nor current)" @@ -358,6 +381,12 @@ def print_summary(deltas: list[MetricDelta]) -> None: "absolute bound alone where the baseline is not positive):" ) _print_table(regressions) + # A regression can also be recorded with no delta at all -- a unit + # change makes the two numbers incomparable. That row prints as dashes, + # so name the reason here or the table looks like a bug. + for d in regressions: + if d.delta is None: + print(f" {d.key}: {d.note}") if improvements: top = improvements[:5] @@ -401,7 +430,12 @@ def write_report( "window": timings.get("window"), "profile": timings.get("profile"), "summary": { + # total is every key in the report, which is the UNION of the + # baseline and the current run -- not the baseline count. "compared" + # is the only number that says how much was actually gated: a delta + # exists only when both sides had a value. "total": len(deltas), + "compared": sum(1 for d in deltas if d.delta is not None), "regressions": len(regressions), "improvements": sum( 1