From c1505804bdbac5607f6eac2165a41a2691cf2d25 Mon Sep 17 00:00:00 2001 From: Sven van Ginkel Date: Sun, 27 Sep 2026 17:08:50 +0200 Subject: [PATCH] fix(site): show gaps in network monitor charts for failed probes and disconnects (#2428) Co-authored-by: henrygd --- .../site/src/components/charts/line-chart.tsx | 24 +++++++- .../network-monitors-table.tsx | 1 + .../routes/system/charts/monitors-charts.tsx | 30 ++++++---- .../site/src/lib/network-monitor-utils.ts | 56 +++++++++++++++++-- internal/site/src/lib/use-network-monitors.ts | 16 ++++-- internal/site/src/types.d.ts | 7 ++- .../site/tests/network-monitor-utils.test.ts | 45 +++++++++++++-- 7 files changed, 150 insertions(+), 29 deletions(-) diff --git a/internal/site/src/components/charts/line-chart.tsx b/internal/site/src/components/charts/line-chart.tsx index 97927526b..211c1f482 100644 --- a/internal/site/src/components/charts/line-chart.tsx +++ b/internal/site/src/components/charts/line-chart.tsx @@ -22,12 +22,34 @@ export type DataPoint = { order?: number strokeOpacity?: number activeDot?: boolean - dot?: boolean + dot?: boolean | typeof isolatedDot /** Which Y axis this series plots against. Defaults to "left". */ yAxisId?: "left" | "right" strokeDasharray?: string } +type IsolatedDotProps = { + key: string + cx: number + cy: number + stroke: string + index: number + points: { value: unknown }[] +} + +const hasValue = (point?: { value: unknown }) => typeof point?.value === "number" + +/** + * Dot renderer that only draws points with no value on either side. Without connectNulls + * those points have no line segment, so they would otherwise only be visible on hover. + */ +export function isolatedDot({ key, cx, cy, stroke, index, points }: IsolatedDotProps) { + if (!hasValue(points[index]) || hasValue(points[index - 1]) || hasValue(points[index + 1])) { + return + } + return +} + export default function LineChartDefault({ chartData, customData, diff --git a/internal/site/src/components/network-monitors-table/network-monitors-table.tsx b/internal/site/src/components/network-monitors-table/network-monitors-table.tsx index 50a46a9c3..52bf131a6 100644 --- a/internal/site/src/components/network-monitors-table/network-monitors-table.tsx +++ b/internal/site/src/components/network-monitors-table/network-monitors-table.tsx @@ -706,6 +706,7 @@ function NetworkMonitorSheetContent({ const monitorStats = useNetworkMonitorStats({ systemId: monitor.system, monitorId: monitor.id, + interval: monitor.interval, chartTime, enabled: open, }) diff --git a/internal/site/src/components/routes/system/charts/monitors-charts.tsx b/internal/site/src/components/routes/system/charts/monitors-charts.tsx index 0602611c5..287b56faf 100644 --- a/internal/site/src/components/routes/system/charts/monitors-charts.tsx +++ b/internal/site/src/components/routes/system/charts/monitors-charts.tsx @@ -1,5 +1,5 @@ -import { getMonitorTarget } from "@/lib/network-monitor-utils" -import LineChartDefault from "@/components/charts/line-chart" +import { getMonitorTarget, monitorGapRecord } from "@/lib/network-monitor-utils" +import LineChartDefault, { isolatedDot } from "@/components/charts/line-chart" import type { DataPoint } from "@/components/charts/line-chart" import { decimalString, formatMicroseconds, matchesFilterGroups, parseFilterGroups, toFixedFloat } from "@/lib/utils" import { $monitorFilter } from "@/lib/stores" @@ -77,10 +77,14 @@ function MonitorChart({ return { dataPoints: points, visibleKeys: visibleIDs } }, [monitors, filter, metric, chartData.chartTime, color]) + // Monitors with different intervals don't share timestamps, so multiple lines need connectNulls. + // A single monitor's stats already contain empty records at real gaps, so the line breaks there. + const multipleMonitors = visibleKeys.length > 1 + const filteredMonitorStats = useMemo(() => { - if (!visibleKeys.length) return monitorStats + if (!multipleMonitors) return monitorStats return monitorStats.filter((record) => visibleKeys.some((id) => record.stats?.[id] != null)) - }, [monitorStats, visibleKeys]) + }, [monitorStats, visibleKeys, multipleMonitors]) const legend = dataPoints.length < 10 && showFilter @@ -99,7 +103,7 @@ function MonitorChart({ customData={filteredMonitorStats} dataPoints={dataPoints} domain={domain ?? ["auto", "auto"]} - connectNulls + connectNulls={multipleMonitors} tickFormatter={tickFormatter} contentFormatter={contentFormatter} legend={legend} @@ -125,9 +129,10 @@ export function AvgMinMaxResponseChart({ monitorStats, monitor, chartData, empty // only one monitor is relevant for this chart const dataPoints: DataPoint[] = useMemo(() => { const dataFn = (metric: keyof MonitorStats) => (record: NetworkMonitorStatsRecord) => - record.stats?.[monitor?.id ?? ""]?.[metric] ?? "-" + record.stats?.[monitor?.id ?? ""]?.[metric] ?? null const avgPoint = { label: "Avg", + dot: isolatedDot, dataKey: dataFn("res_avg"), color: 1, order: 0, @@ -139,6 +144,7 @@ export function AvgMinMaxResponseChart({ monitorStats, monitor, chartData, empty return [ { label: "Max", + dot: isolatedDot, dataKey: dataFn("res_max"), color: 3, order: 0, @@ -146,6 +152,7 @@ export function AvgMinMaxResponseChart({ monitorStats, monitor, chartData, empty avgPoint, { label: "Min", + dot: isolatedDot, dataKey: dataFn("res_min"), color: 2, order: 2, @@ -153,10 +160,14 @@ export function AvgMinMaxResponseChart({ monitorStats, monitor, chartData, empty ] }, [chartTime, hasLongInterval, monitor?.id]) + // Replace records where every probe failed with gap markers, so the line breaks there without + // leaving points that have no response time for the tooltip to show. const data = useMemo(() => { - if (!monitor) return [] - return monitorStats.filter((record) => record.stats && monitor.id in record.stats) - }, [monitor, monitorStats]) + const id = monitor?.id ?? "" + return monitorStats.map((record) => + record.stats?.[id] && record.stats[id].res_avg == null ? monitorGapRecord : record + ) + }, [monitorStats, monitor?.id]) const legend = dataPoints.length > 1 @@ -174,7 +185,6 @@ export function AvgMinMaxResponseChart({ monitorStats, monitor, chartData, empty customData={data} dataPoints={dataPoints} domain={["auto", "auto"]} - connectNulls legend={legend} tickFormatter={(value) => formatMicroseconds(value, false)} contentFormatter={({ value }) => { diff --git a/internal/site/src/lib/network-monitor-utils.ts b/internal/site/src/lib/network-monitor-utils.ts index dcbf2dffc..79f460368 100644 --- a/internal/site/src/lib/network-monitor-utils.ts +++ b/internal/site/src/lib/network-monitor-utils.ts @@ -1,12 +1,19 @@ -import type { MonitorCertInfo, MonitorStats, NetworkMonitorRecord, RawMonitorStatsRecord } from "@/types" +import type { + MonitorCertInfo, + MonitorStats, + NetworkMonitorRecord, + NetworkMonitorStatsRecord, + RawMonitorStatsRecord, +} from "@/types" import { toFixedFloat } from "./utils" /** Derive chart metrics from the counts and response sum stored at every retention tier. */ export function getMonitorStats(record: RawMonitorStatsRecord): MonitorStats { + const success = record.success_count > 0 return { - res_avg: record.success_count > 0 ? toFixedFloat(record.res_sum / record.success_count, 2) : 0, - res_min: record.res_min, - res_max: record.res_max, + res_avg: success ? toFixedFloat(record.res_sum / record.success_count, 2) : null, + res_min: success ? record.res_min : null, + res_max: success ? record.res_max : null, loss: record.total_count > 0 ? toFixedFloat(((record.total_count - record.success_count) / record.total_count) * 100, 2) @@ -14,6 +21,47 @@ export function getMonitorStats(record: RawMonitorStatsRecord): MonitorStats { } } +/** + * Realtime stats come from the agent without counts and report 0 response times when every + * probe failed; clear them to match stored stats. + */ +export function clearFailedResponse(stats: MonitorStats): MonitorStats { + if (stats.loss < 100) return stats + return { ...stats, res_avg: null, res_min: null, res_max: null } +} + +/** + * Gap marker in the same form appendData uses. Without a timestamp it can't become the active + * tooltip point, which would otherwise have no values and make the tooltip jump to the corner. + */ +export const monitorGapRecord = { created: null, stats: null } as unknown as NetworkMonitorStatsRecord + +/** + * Return the records that have stats for one monitor, with a gap marker inserted wherever + * consecutive records are further apart than expected (e.g. while the agent was disconnected), + * so charts break the line there instead of drawing across the missing time. + */ +export function withMonitorGaps( + records: NetworkMonitorStatsRecord[], + monitor: Pick, + expectedInterval: number +): NetworkMonitorStatsRecord[] { + // long-interval monitors only get a record when a new probe completes + const maxGap = Math.max(expectedInterval, monitor.interval * 1000) * 1.5 + const result: NetworkMonitorStatsRecord[] = [] + let prevTime = 0 + for (const record of records) { + // skip appendData's gap markers (created: null) and records without this monitor + if (record.created == null || !record.stats?.[monitor.id]) continue + if (prevTime && record.created - prevTime > maxGap) { + result.push(monitorGapRecord) + } + prevTime = record.created + result.push(record) + } + return result +} + export function getMonitorTarget(monitor: Pick) { if (monitor.protocol !== "tcp") return monitor.target const host = monitor.target.includes(":") && !monitor.target.startsWith("[") ? `[${monitor.target}]` : monitor.target diff --git a/internal/site/src/lib/use-network-monitors.ts b/internal/site/src/lib/use-network-monitors.ts index be3e1456b..747a739e3 100644 --- a/internal/site/src/lib/use-network-monitors.ts +++ b/internal/site/src/lib/use-network-monitors.ts @@ -1,5 +1,5 @@ import { chartTimeData } from "@/lib/utils" -import { getMonitorStats } from "@/lib/network-monitor-utils" +import { clearFailedResponse, getMonitorStats, withMonitorGaps } from "@/lib/network-monitor-utils" import type { ChartTimes, MonitorStats, @@ -7,7 +7,7 @@ import type { NetworkMonitorStatsRecord, RawMonitorStatsRecord, } from "@/types" -import { useEffect, useRef, useState } from "react" +import { useEffect, useMemo, useRef, useState } from "react" import { appendData } from "@/components/routes/system/chart-data" import { pb, getPbTimestamp } from "@/lib/api" import { toast } from "@/components/ui/use-toast" @@ -157,12 +157,15 @@ export function useNetworkMonitors(props: UseNetworkMonitorsProps) { interface UseNetworkMonitorStatsProps { systemId: string monitorId: string + /** Monitor probe interval in seconds, used to tell missing data apart from slow probes */ + interval: number chartTime: ChartTimes enabled?: boolean } +/** Returns the monitor's stats with empty records inserted where data is missing (see withMonitorGaps). */ export function useNetworkMonitorStats(props: UseNetworkMonitorStatsProps) { - const { systemId, monitorId, chartTime, enabled = true } = props + const { systemId, monitorId, interval, chartTime, enabled = true } = props const [monitorStats, setMonitorStats] = useState([]) // pending raw events to be merged (keyed by monitor+created) const pendingRaw = useRef(new Map()) @@ -275,7 +278,7 @@ export function useNetworkMonitorStats(props: UseNetworkMonitorStatsProps) { (data: { Monitors: NetworkMonitorStatsRecord["stats"] }) => { const monitorStats = data.Monitors?.[monitorId] if (cancelled || !monitorStats) return - const stats = { created: Date.now(), stats: { [monitorId]: monitorStats } } + const stats = { created: Date.now(), stats: { [monitorId]: clearFailedResponse(monitorStats) } } const newStats = appendCacheValue(monitorId, "rt", [stats], 120) setMonitorStats(newStats) }, @@ -291,7 +294,10 @@ export function useNetworkMonitorStats(props: UseNetworkMonitorStatsProps) { } }, [chartTime, systemId, monitorId, enabled]) - return monitorStats + return useMemo( + () => withMonitorGaps(monitorStats, { id: monitorId, interval }, chartTimeData[chartTime].expectedInterval), + [monitorStats, monitorId, interval, chartTime] + ) } async function fetchMonitors(system?: string) { diff --git a/internal/site/src/types.d.ts b/internal/site/src/types.d.ts index a67201f0f..ceb9868af 100644 --- a/internal/site/src/types.d.ts +++ b/internal/site/src/types.d.ts @@ -677,9 +677,10 @@ export interface MonitorCertInfo { /** Response times in microseconds and packet loss percentage (0-100). */ export interface MonitorStats { - res_avg: number - res_min: number - res_max: number + /** null when no probe succeeded, so there is no response time */ + res_avg: number | null + res_min: number | null + res_max: number | null loss: number } diff --git a/internal/site/tests/network-monitor-utils.test.ts b/internal/site/tests/network-monitor-utils.test.ts index f58dd38eb..46fa7cbcc 100644 --- a/internal/site/tests/network-monitor-utils.test.ts +++ b/internal/site/tests/network-monitor-utils.test.ts @@ -5,7 +5,7 @@ mock.module("@lingui/core/macro", () => ({ plural: (_count: number, forms: { other?: string }) => forms.other ?? "", })) -const { getMonitorStats } = await import("../src/lib/network-monitor-utils") +const { getMonitorStats, withMonitorGaps } = await import("../src/lib/network-monitor-utils") describe("monitor stats derived from stored counts", () => { test("retains probe weights and response precision", () => { @@ -39,10 +39,10 @@ describe("monitor stats derived from stored counts", () => { }) test.each([ - { total_count: 3, success_count: 0, loss: 100 }, - { total_count: 0, success_count: 0, loss: 0 }, - { total_count: 1, success_count: 1, loss: 0 }, - ])("handles zero sums with $total_count attempts and $success_count successes", ({ loss, ...counts }) => { + { total_count: 3, success_count: 0, loss: 100, res: null }, + { total_count: 0, success_count: 0, loss: 0, res: null }, + { total_count: 1, success_count: 1, loss: 0, res: 0 }, + ])("handles zero sums with $total_count attempts and $success_count successes", ({ loss, res, ...counts }) => { const stats = getMonitorStats({ monitor: "monitor1", created: 1000, @@ -51,6 +51,39 @@ describe("monitor stats derived from stored counts", () => { res_sum: 0, ...counts, }) - expect(stats).toEqual({ res_avg: 0, res_min: 0, res_max: 0, loss }) + expect(stats).toEqual({ res_avg: res, res_min: res, res_max: res, loss }) + }) +}) + +describe("monitor gaps", () => { + const monitor = { id: "m1", interval: 30 } + const stats = { res_avg: 1, res_min: 1, res_max: 1, loss: 0 } + const record = (created: number | null, id = monitor.id) => ({ created, stats: { [id]: stats } }) + + test("does not insert markers at the expected cadence", () => { + const records = [record(0), record(60_000), record(120_000)] + expect(withMonitorGaps(records, monitor, 60_000)).toEqual(records) + }) + + test("inserts a marker between records further apart than expected", () => { + const records = [record(60_000), record(300_000)] + expect(withMonitorGaps(records, monitor, 60_000)).toEqual([records[0], { created: null, stats: null }, records[1]]) + }) + + test("uses the monitor interval when it is longer than the tier interval", () => { + const slowMonitor = { id: monitor.id, interval: 300 } + const records = [record(300_000), record(600_000), record(900_000)] + expect(withMonitorGaps(records, slowMonitor, 60_000)).toEqual(records) + expect(withMonitorGaps([records[0], record(1_200_000)], slowMonitor, 60_000)).toHaveLength(3) + }) + + test("skips records for other monitors and existing gap markers", () => { + const records = [ + record(60_000), + record(90_000, "m2"), + { created: null, stats: null }, + record(120_000), + ] as Parameters[0] + expect(withMonitorGaps(records, monitor, 60_000)).toEqual([records[0], records[3]]) }) })