From aea04225620c0558501d91badab88b51a5bbcd56 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:58:43 +0100 Subject: [PATCH] fix(telemetry): count only rendered panels, and drop no-reply latencies Two defects reported against the validation harness. Both premises were correct, but neither suggested fix was, so the remedies differ. Dashboard panel count: `len(dashboard["panels"])` treated Grafana row objects as panels and skipped the panels nested inside collapsed rows, so every dashboard was over-reported by between 1 and 10 (`log-derived-insights` read 41 against a true 31). The check also passed unconditionally on HTTP 200, so a dashboard that renders nothing would still pass. `_leaf_panel_count` now walks row children and the result gates the verdict. Gating on the old top-level length, as suggested, would not have caught the case it was aimed at: a dashboard made only of collapsed rows counts its rows and reports a positive number while rendering nothing. RPC latency percentiles: `LoadStats.record` appended a latency for every outcome, including requests that never got a reply, where the value is a time-to-failure rather than a round trip. A timeout contributed the full receive timeout, and at the error rate a real run shows this reported p95 and p99 of 10000 ms where the true figure was 5 ms. `record` now takes an optional latency and the timeout path passes none. The suggestion to append only on success was not adopted: a reply carrying `status: error` is a completed, timely round trip whose latency is a genuine measurement, and discarding it would throw away real data. `per_command` is now keyed off the request counts rather than the latency map, so a command whose every request timed out still appears in the report instead of vanishing from it, and each entry carries a `latency_samples` count. --- .../telemetry/workload/rpc_load_generator.py | 37 ++++++++++++++----- .../telemetry/workload/validate_telemetry.py | 36 ++++++++++++++++-- 2 files changed, 61 insertions(+), 12 deletions(-) diff --git a/docker/telemetry/workload/rpc_load_generator.py b/docker/telemetry/workload/rpc_load_generator.py index 26962420f4..c6b07a6ac5 100644 --- a/docker/telemetry/workload/rpc_load_generator.py +++ b/docker/telemetry/workload/rpc_load_generator.py @@ -175,8 +175,10 @@ class LoadStats: total_success: Requests that returned a valid result. total_errors: Requests that returned an error or timed out. total_cancelled: Requests cancelled at teardown, never recorded. - latencies: Per-command list of round-trip times in seconds. - command_counts: Per-command request count. + latencies: Per-command round-trip times in seconds, for the + requests that got a reply. Requests that never got + one contribute no sample -- see record(). + command_counts: Per-command request count, replied or not. """ total_dispatched: int = 0 @@ -187,15 +189,27 @@ class LoadStats: latencies: dict[str, list[float]] = field(default_factory=dict) command_counts: dict[str, int] = field(default_factory=dict) - def record(self, command: str, latency: float, success: bool) -> None: - """Record the outcome of a single RPC call.""" + def record(self, command: str, latency: float | None, success: bool) -> None: + """Record the outcome of a single RPC call. + + Pass ``latency=None`` when no reply arrived, i.e. a timeout or a + transport failure. Such a request still counts as an error, but it + contributes no latency sample: time-to-failure is not a round-trip + time, and a timeout would inject RECV_TIMEOUT_S into the distribution + and dominate the percentiles. + + A reply carrying ``status: error`` is the opposite case. The round + trip completed and was timely, so its latency is a real measurement + and is kept even though the request is counted as an error. + """ self.total_sent += 1 if success: self.total_success += 1 else: self.total_errors += 1 - self.latencies.setdefault(command, []).append(latency) self.command_counts[command] = self.command_counts.get(command, 0) + 1 + if latency is not None: + self.latencies.setdefault(command, []).append(latency) def summary(self) -> dict[str, Any]: """Return a summary dict suitable for JSON serialization. @@ -208,11 +222,15 @@ class LoadStats: its load, and reporting 100% for it would be the same blind spot the key exists to close. """ + # Keyed off command_counts, not latencies: a command whose every + # request timed out has a count but no samples, and dropping it from + # the report would hide the command that failed worst. per_command: dict[str, Any] = {} - for cmd, lats in self.latencies.items(): - sorted_lats = sorted(lats) + for cmd in sorted(self.command_counts): + sorted_lats = sorted(self.latencies.get(cmd, [])) per_command[cmd] = { - "count": self.command_counts.get(cmd, 0), + "count": self.command_counts[cmd], + "latency_samples": len(sorted_lats), "p50_ms": round(_percentile(sorted_lats, 0.50) * 1000, 2), "p95_ms": round(_percentile(sorted_lats, 0.95) * 1000, 2), "p99_ms": round(_percentile(sorted_lats, 0.99) * 1000, 2), @@ -399,7 +417,8 @@ async def send_rpc( success = json.loads(raw).get("status") == "success" except REQUEST_FAILURES as exc: logger.debug("RPC %s failed: %s", command, exc) - stats.record(command, time.monotonic() - t0, False) + # No reply, so no latency sample -- see LoadStats.record(). + stats.record(command, None, False) return stats.record(command, latency, success) diff --git a/docker/telemetry/workload/validate_telemetry.py b/docker/telemetry/workload/validate_telemetry.py index 6f2c74ebfb..b7ac6f0ce7 100644 --- a/docker/telemetry/workload/validate_telemetry.py +++ b/docker/telemetry/workload/validate_telemetry.py @@ -910,6 +910,32 @@ async def validate_log_trace_correlation( # --------------------------------------------------------------------------- +def _leaf_panel_count(dashboard: dict[str, Any]) -> int: + """Count the panels a dashboard actually renders. + + Grafana models a row as an entry of ``type: "row"`` in the top-level + ``panels`` list, and a collapsed row carries its children in its own + nested ``panels`` list. So ``len(dashboard["panels"])`` counts rows as + though they were panels and misses everything inside a collapsed one -- + on ``node-health`` that reads 55 where the true figure is 51, and a + dashboard consisting only of collapsed rows would report a positive + count while rendering nothing. + + Args: + dashboard: The ``dashboard`` object from the Grafana API response. + + Returns: + The number of non-row panels, including those nested inside rows. + """ + total = 0 + for panel in dashboard.get("panels", []): + if panel.get("type") == "row": + total += len(panel.get("panels", [])) + else: + total += 1 + return total + + async def validate_dashboards( session: aiohttp.ClientSession, grafana_url: str, @@ -938,13 +964,17 @@ async def validate_dashboards( if resp.status == 200: data = await resp.json() dashboard = data.get("dashboard", {}) - panel_count = len(dashboard.get("panels", [])) + panel_count = _leaf_panel_count(dashboard) report.add( CheckResult( name=f"dashboard.{uid}", category="dashboard", - passed=True, - message=(f"{uid}: loaded ({panel_count} panels)"), + passed=panel_count > 0, + message=( + f"{uid}: loaded ({panel_count} panels)" + if panel_count + else f"{uid}: loaded but renders no panels" + ), details={"panel_count": panel_count}, ) )