fix(site): show gaps in network monitor charts for failed probes and disconnects (#2428)

Co-authored-by: henrygd <hank@henrygd.me>

internal/site/src/components/charts/line-chart.tsx

@@ -22,12 +22,34 @@ export type DataPoint<T = SystemStatsRecord> = {
 	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 <g key={key} />
+	}
+	return <circle key={key} cx={cx} cy={cy} r={2} fill={stroke} />
+}
+
 export default function LineChartDefault({
 	chartData,
 	customData,

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,
 	})

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<NetworkMonitorStatsRecord>[] = 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 }) => {

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<NetworkMonitorRecord, "id" | "interval">,
+	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<NetworkMonitorRecord, "target" | "protocol" | "port">) {
 	if (monitor.protocol !== "tcp") return monitor.target
 	const host = monitor.target.includes(":") && !monitor.target.startsWith("[") ? `[${monitor.target}]` : monitor.target

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<NetworkMonitorStatsRecord[]>([])
 	// pending raw events to be merged (keyed by monitor+created)
 	const pendingRaw = useRef(new Map<string, RawMonitorStatsRecord>())
@@ -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) {

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
 }
 

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<typeof withMonitorGaps>[0]
+		expect(withMonitorGaps(records, monitor, 60_000)).toEqual([records[0], records[3]])
 	})
 })