diff --git a/.github/scripts/telemetry/check_regression_bounds.py b/.github/scripts/telemetry/check_regression_bounds.py index 5454d5dd29..b07045ce70 100644 --- a/.github/scripts/telemetry/check_regression_bounds.py +++ b/.github/scripts/telemetry/check_regression_bounds.py @@ -43,10 +43,12 @@ baseline's own. Six rules are checked: 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. +Rule A subtracts ``excluded_keys`` from BOTH sides of its comparison, so a +quantile removed from the gated set neither reads as a missing baseline nor as +an undeclared one -- an exclusion left in the baseline is one failure, rule F's, +which names the file to edit. 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 @@ -56,8 +58,9 @@ 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, +A baseline ENTRY that is not an object, or whose value is not a positive finite +number, is rejected before any rule runs -- see ``check_key`` and +``_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 @@ -287,7 +290,20 @@ def check_key(key, entry, thresholds, ladders): 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``. + + The entry's SHAPE is checked first, for the same reason. A hand edit that + writes the bare number instead of the ``{"value": .., "unit": ..}`` object + leaves no ``.get`` to call, and the script died with an AttributeError + traceback naming a line in itself rather than the key at fault. """ + if not isinstance(entry, dict): + return [ + f"{key}: baseline entry {entry!r} is not an object carrying value and " + f"unit, so no bound can be derived from it. Recapture the baseline " + f"from a CI run rather than editing it by hand -- see " + f"baselines/README.md" + ] + value, unit = entry.get("value"), entry.get("unit", "") edges = ladders.get(unit) if value is None or edges is None: @@ -381,8 +397,16 @@ def main(): 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): + # + # The same keys come off rule A's over-coverage side too. An excluded key + # left in the baseline is rule F's finding, reported with the file to edit; + # rule A would add a second failure for the same single mistake, saying the + # key is not declared -- which is not even true, it is declared and then + # excluded. Only exclusions the surface really declares are subtracted, so a + # misspelt exclusion naming a stale baseline key still reaches rule A. + excluded_declared = set(metrics_cfg.get("excluded_keys", {})) & declared + declared -= excluded_declared + for key in sorted(set(gated) - declared - excluded_declared): failures.append( f"{key}: in the baseline but not declared by {METRICS}, so it is " f"reported every run and can never gate -- remove it (rule A)" diff --git a/.github/scripts/telemetry/test_check_regression_bounds.py b/.github/scripts/telemetry/test_check_regression_bounds.py index ba3da1848c..5e7a4b7811 100644 --- a/.github/scripts/telemetry/test_check_regression_bounds.py +++ b/.github/scripts/telemetry/test_check_regression_bounds.py @@ -46,7 +46,12 @@ class CheckerCase(unittest.TestCase): def setUp(self): self.tree = Path(tempfile.mkdtemp()) - self.addCleanup(self._cleanup) + # Bound to THIS tree, not read off self.tree when the cleanup finally + # runs. A test that calls setUp again for a fresh tree (see the + # degenerate-baseline subTests) rebinds self.tree, and a late read would + # make every registered cleanup remove the LAST tree, leaving each + # earlier one behind in /tmp. + self.addCleanup(self._cleanup, self.tree) for rel in INPUTS: dest = self.tree / rel dest.parent.mkdir(parents=True, exist_ok=True) @@ -55,11 +60,12 @@ class CheckerCase(unittest.TestCase): script.parent.mkdir(parents=True, exist_ok=True) shutil.copy(CHECKER, script) - def _cleanup(self): - for path in self.tree.rglob("*"): + def _cleanup(self, tree): + """Remove one scratch tree, restoring permissions rmtree needs first.""" + for path in tree.rglob("*"): if path.is_file(): path.chmod(stat.S_IRUSR | stat.S_IWUSR) - shutil.rmtree(self.tree, ignore_errors=True) + shutil.rmtree(tree, ignore_errors=True) def run_checker(self): """Run the checker in the scratch tree, returning (code, stdout+stderr).""" diff --git a/docker/telemetry/workload/capture_timings.py b/docker/telemetry/workload/capture_timings.py index 3cd4bb8cd2..9c90ac5f30 100644 --- a/docker/telemetry/workload/capture_timings.py +++ b/docker/telemetry/workload/capture_timings.py @@ -122,10 +122,17 @@ def _capture_status(metrics: dict, min_ratio: float) -> dict: keys on, and it is the same predicate that decides this script's exit code — see the module docstring for why it lives in the artifact. - An empty surface is vacuously complete: there is nothing for the capture - to have fallen short of, and that is the case the exit-code check has - always passed. Defining it any other way here would make the flag and the - exit code disagree, which is the drift this block exists to remove. + An empty surface is NOT complete. ``declared == 0`` means the capture asked + Prometheus for nothing: ``build_query_plan`` returns an empty plan, without + complaining, for any config that yields no ``spans``/``rpc_methods``/ + ``job_queue`` entries -- a ``--metrics`` path pointing at the wrong file, a + truncated one, or every key excluded. Nothing about that run is evidence + the pipeline works, so treating it as vacuously complete would exit 0 and + hand the paste-me path a ``metrics: {}`` artifact to offer as baseline + material. Pasted in, it still reads as a placeholder, so the gate stays off + while the workflow reports the baseline as activated -- the silent-green + outcome the whole ``capture`` block exists to prevent. The exit code reads + this same flag, so the two still cannot disagree. """ declared = len(metrics) captured = sum(1 for entry in metrics.values() if entry["value"] is not None) @@ -133,7 +140,7 @@ def _capture_status(metrics: dict, min_ratio: float) -> dict: "declared": declared, "captured": captured, "min_ratio": min_ratio, - "complete": declared == 0 or (captured / declared) >= min_ratio, + "complete": declared > 0 and (captured / declared) >= min_ratio, } @@ -236,16 +243,28 @@ def main() -> int: logger.info("Wrote %s (%d/%d metrics captured)", args.output, captured, total) if not status["complete"]: - logger.error( - "Only %d/%d (%.0f%%) metrics captured — below the %.0f%% minimum. " - "Is Prometheus reachable at %s? The file is marked " - "capture.complete=false and must not be pasted into the baseline.", - captured, - total, - captured / total * 100, - args.min_capture_ratio * 100, - args.prometheus, - ) + if total == 0: + # No ratio to report: nothing was asked for, so the shortfall is the + # declared surface, not Prometheus. Named separately because the + # percentage below would divide by zero. + logger.error( + "No metrics were declared, so nothing was captured. Does %s " + "declare spans/rpc_methods/job_queue names, and does " + "excluded_keys leave any of them gated? The file is marked " + "capture.complete=false and must not be pasted into the baseline.", + args.metrics, + ) + else: + logger.error( + "Only %d/%d (%.0f%%) metrics captured — below the %.0f%% minimum. " + "Is Prometheus reachable at %s? The file is marked " + "capture.complete=false and must not be pasted into the baseline.", + captured, + total, + captured / total * 100, + args.min_capture_ratio * 100, + args.prometheus, + ) return 1 return 0