mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 22:30:57 +00:00
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.
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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},
|
||||
)
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user