Repository navigation
fix(routers): keep the vitals an external pusher confirmed - #1724
Conversation
gnacho#441 nils CPU/RAM/temp for SNMP and external sources so the UI never paints false zeros, and gnacho#1569 revived the readings a source really confirmed - but only through the hasCpu/hasTemp flags, which just the SNMP poller sets (snmp_live.go). polledFromAgent copies System.CPU and System.Temp into plain ints and never sets the flags, so a pusher-fed unit loses exactly the vitals it did report: on this fleet crs305 (routeros-pusher) renders cpu/ram/temp = null with vitalsAvailable = false, while its own payload carries cpu-load 1 %, temperature 48 C and 446/536 MB free memory - and the same nulls are what reach the metrics history. Set the flags where the reading is taken, both in the payload branch and in the gnacho#441 anti-flicker cache branch, and give RAM the same contract with a hasRam flag: SysInfo.Memory.Total > 0 is a confirmed reading, so the memUsagePct derived from it is worth showing and recording. Sources that declare nothing keep their nulls: the beacon without a system section now has its own test, and the SNMP path still reports no RAM.
gnacho
left a comment
gnacho
left a comment
There was a problem hiding this comment.
Thanks for fixing the external-pusher vitals regression. The normal path looks good and make check passes on 972f11d, but I reproduced two edge cases that should be addressed before merging.
-
Cached vitals can cross the agent-source boundary (
server-go/internal/adapters/agent.go:696-718and906-909).extrasCacheis keyed only by router ID. After a native payload withSystem, an external payload withoutSystemfor the same ID enters thecached.systemfallback. The newhasCpu,hasTempandhasRamflags then expose those inherited readings for the external source and persist them in metrics. ThesysSnapfallback carries the same section forward again.I reproduced this with a native payload containing CPU, temperature and memory, followed by an external payload without
System: the resulting router had all three vitals non-nil. Reconfiguring a router does not necessarily recreateLive, so this transition is reachable without a restart.Please store the source kind alongside the cached
Systemsection and require it to match both when readingcached.systemand when carrying it intosysSnap. A native-full to external-empty regression test should cover both the router card and metrics rows. -
A total-only memory payload becomes 100% RAM (
server-go/internal/adapters/agent.go:712-718, together withmem.go:19-37).When
Memory.Total > 0andFree,Buffered,Cached,AvailableandUsedare all zero,memUsagePctreturns 100 and the newhasRamflag makes that value visible. These memory fields are not pointers and ingestion accepts this shape, so a partial or malformed pusher payload can fabricate full memory usage and persist it.I reproduced this with
Total = 100and every usage field left at zero: the resulting router reportedRAM = 100. Please treat a total-only payload as unknown memory before calculatingramPctand settinghasRam, with coverage for both the router card and metrics rows.
The two reproductions were temporary tests and are not included in this PR.
…med them
Both edge cases from the review.
- extrasCache is keyed only by the router id, so after a native payload that
carried a System section, an external payload without one read that native
cpu/temp and the new flags published them as the pusher's own confirmed
readings - on the card and in the metrics rows. The cached System now carries
the agent kind that wrote it, and both the recycle read and the sysSnap
carry-forward require that kind to match ("" and "native" are one source, per
the Payload.Kind contract). The gnacho#441 anti-flicker stays alive inside a
source: the new test walks native-full -> external-empty -> external-board ->
external-with-cpu -> external-empty and asserts the recycle only ever comes
from the same family. Reproduced before the guard: cpu=7 temp=61 ram=25
leaked into a payload that reported nothing.
- Mem.Total with no usage figure at all is unknown memory, not full memory.
memUsagePct falls through to "available = free + buffered = 0" and returns
100, and hasRam then publishes and persists that. ramPct and its flag now
require at least one usage value (memUsageKnown), covered on the card and in
the metrics rows. Reproduced before the guard: ram=100 from Total=100 alone.
Deliberately out of scope: the other memUsagePct caller (live.go:1293, the SSH
path) has the same fall-through, but its meminfo source always carries free or
available, so nothing there was fabricated by this change. Say the word and I
will apply the same guard there in its own commit.
|
Both edge cases fixed in f436d08, and both reproduced first so the tests are not vacuous. 1. Cross-source cache. 2. Total-only memory. One thing I deliberately did not touch: the other CI: eslint and gitleaks green, server/landing running ( |
…em (#1747) Two gates kept the 24h traffic line of a pusher-fed unit at zero: - `polledFromAgent` skipped `recordPortSamples` for every external payload. - The fallback in `buildOverview` only ran when `VitalsAvailable` was false, and #1724 leaves it true for a pusher that confirms CPU or temperature. The recording gate now looks at the data: an external payload that carries byte counters or per-port rates is recorded. A beacon that only reports link state and FDB still stays out, because `recordBeaconPortSamples` persists its own series. The fallback no longer depends on the vitals flag. It reads a new bps series first, since RouterOS and AsusWRT pushers send rates rather than frame counters, and keeps `HourlyFpsTotal` as the second attempt for sources that only have frames. Tests: an external payload with counters writes port samples, a beacon without them still writes none, `HourlyBpsTotal` aggregates per hour and per port, and the fallback fills the line with `VitalsAvailable` true while leaving units with their own series untouched. Closes #1741
Closes #1726 A unit can have `snmp_enabled` and also be fed by a pusher. `pollRouter` returns as soon as there is a fresh agent payload, so the SNMP branch never ran and the setting was a silent no-op. This adds SNMP as a second source in that path. It only fills what the payload cannot provide: - link speed per port (RouterOS does not expose it through the API, and the child switch reports `ifHighSpeed=0` in up ports) - error counters (`ifErrors`/`ifInErrors`, only available over SNMP) - LLDP neighbours (RouterOS 7.25 has no `/interface/lldp` menu), which replaces the FDB guess that labels `sfp-sfpplus1` as the wrong peer What it does not touch: CPU, RAM and temperature (the payload wins since #1724) and the FDB (the agent already sends it). A payload value is never overwritten: the merge only fills empty speeds and zero counters, and an `ifHighSpeed=0` from a down port is not propagated as a speed. The SNMP surface is cached per router with the same interval as the SNMP poller (`snmp_poll_interval`, 60 s by default), so a 30 s pusher does not turn into two SNMP walks per minute. It reuses `recordSnmpSuccess` and `recordSnmpFailure`, so the existing "SNMP polling failing" alert still fires on these units. It skips `recordPortSamples` and `portMon.Observe` on purpose: the pusher already persists its own series with its own port IDs, and duplicating them here would interleave `snmp-N` rows with the real ones. Five tests cover the merge rules, the down-link case, payload priority for LLDP, the fresh-cache path (no second walk) and the disabled case.
Problem
An external pusher that does report CPU, memory and temperature still renders all three as
null.#441 nils the vitals for SNMP and
externalsources so the UI never paints false zeros. #1569 added the rescue for the readings a source actually confirmed — but it keys offhasCpu/hasTemp, and those flags are only ever set by the SNMP poller (snmp_live.go).polledFromAgentcopiesSystem.CPUandSystem.Tempinto plain ints and never sets them, so a pusher-fed unit loses exactly the vitals it reports. The same nulls are what reach themetricshistory.Evidence (live fleet, 2026-10-31)
A CRS305-1G-4S+ on RouterOS 7.25, fed by the official
routeros-pusherv0.1.1 (kind=external,agent_only=1).What the device itself says:
What the pusher puts in the payload (
routeros.gofillsCPU,TempandSysInfo.Memory):cpu=1,temp=48,memory.total/freeas above.How the fleet renders it (
GET /api/routers):fn-switch-8is the case #441 was written for and this PR leaves it untouched.crs305is a different situation: its own source reports the numbers, and the core throws them away.The history is the more expensive half (query over the live
metricstable):The crs305 rows before that timestamp carry real values (15122 with
cpu > 0, 794 withcpu = 0): the unit is not "a device that cannot report vitals", it lost them by changing source. Every sample since then isNULL, so the detail chart of a RouterOS box fed by the vendor pusher is empty and stays empty.Fix
Set the flags where the reading is taken — both in the payload branch and in the #441 anti-flicker cache branch. RAM gets the same contract with a
hasRamflag:SysInfo.Memory.Total > 0is a confirmed reading, and thememUsagePctderived from it is a real percentage worth showing and recording. A payload that declares nothing keeps its nulls. The card and the history shareresolvePartialVitals, so both are covered by the same three flags.Tests
go test ./internal/adapters/ ./internal/snmp/ -count=1green onmain@135f4ba.TestBuildRouterKeepsConfirmedPusherVitals: a pusher payload withcpu/temp/memoryyields1 / 17 / 48on the card and in the persisted row.TestBuildRouterPusherSinSystemSigueSinVitals: a beacon without asystemsection still gives three nulls andvitalsAvailable=false— the Hide system vitals (CPU/RAM/temp) for devices whose data source cannot report them #441 contract, now pinned by a test instead of by convention.hasRamon that path.No
agent/change, so noagent/VERSIONbump.Related, deliberately not in this commit
The SNMP side of the same contract is its own gap and I will open a separate issue with the measurements:
OidMikrotikTemphardcodes the SwOS row (…14988.1.1.3.100.1.3.52.0), so a RouterOS box that answers the same table on row17getsNoSuchObject; andHOST-RESOURCES(hrProcessorLoad,hrStorage"main memory") is never read although this same CRS305 exposes CPU and RAM there. Same device, same evening, both confirmed withsnmpwalk.