Log stats-for-nerds stream statistics through the data lake - #2998
Log stats-for-nerds stream statistics through the data lake#2998cal-oriley wants to merge 12 commits into
Conversation
|
|
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.1 | Poll loop can fork into several copies | Opening the stats overlay at the wrong moment permanently doubles how often Cockpit interrogates the video service, and it keeps doubling every time it happens. | major | ❌ |
| 1.2 | Rename handover is wired to a stale list | After the user renames a camera, an active recording of that camera's stats silently stops recording and leaves dead duplicate entries behind. | major | ❌ |
| 1.3 | A hiccup talking to the video service is read as "all cameras gone" | A momentary blip makes the overlay report a video freeze that never happened, and writes a fake zero-bitrate reading into the recorded data. | major | ❌ |
| 1.4 | Fast/slow sampling is decided only when the overlay opens and closes | Switching which camera a video widget shows either leaves the fast polling running forever or leaves the overlay updating once every five seconds. | major | ❌ |
| 4.1 | Camera password can still end up in a recorded value's name | If Cockpit briefly loses track of a camera's friendly name, the camera's username and password appear as a row in the Data Lake list and in exported files. | major | ❌ |
| 2.1 | Old recording selections left behind with no cleanup and no notice | Users who had armed stream statistics keep silently-dead entries that show up as permanently empty columns in every export. | minor | ❌ |
| 5.1 | Camera-name lookup repeated 66 times per sample | Cockpit does hundreds of redundant list searches per second per camera, for a value that does not change between them. | minor | ❌ |
| 5.2 | Renaming an armed camera rewrites the saved selection dozens of times | Renaming a camera with many statistics armed briefly freezes the interface while the recorder tears itself down and rebuilds once per value. | minor | ❌ |
| 5.3 | Idle polling duplicates a poll that already exists | With nothing looking at the numbers, Cockpit asks the video service for the same information twice as often as the change description claims. | minor | ❌ |
| 8.1 | A commit fixes a slowdown that an earlier commit in the same branch created | A reviewer reads a performance problem being introduced and then repaired, and the commit message blames a state that never shipped. | minor | ❌ |
| 7.1 | A comment rewritten over unchanged code | Small deviation from the project's rule that existing comments stay put unless their code changes. | nit | ❌ |
| 7.2 | New value filed under the wrong heading in a list that has no headings | Cosmetic mis-grouping in a distinction nothing actually uses. | nit | ❌ |
| 11.1 | One of the two stats caches is never cleaned | A camera that goes away leaves its last WebRTC numbers cached for the rest of the session. | nit | ❌ |
| 11.2 | Recorder touches the browser window at import time without a guard | Loading this module outside a browser page would fail, unlike the guarded pattern used elsewhere. | nit | ❌ |
Change map — what was established before judging
Claims
| Claim (PR body / commit bodies) | Verdict |
|---|---|
| go2rtc rates were derived against a module-global previous-sample map, so the integration window depended on which caller polled last | Verified — src/electron/services/go2rtc.ts:342-343,369-381 is exactly that; both the panel's 100 ms poll and the video store's 5 s poll (src/stores/video.ts:178-181) feed the same map. |
| The API now exposes raw counters + sample epoch so each consumer differences its own window | Verified — src/types/video.ts:69-83 gains bytes/packets/sampleEpoch, and differenceGo2rtcSamples (src/libs/video/stream-stats.ts:34) is the only place rates are derived. No other consumer read bitrateKbps/packetsPerSec; getStreamDisplayInfo (src/stores/video.ts:239-247) only reads codec/width/height, so the type change is safe. |
| An open panel doubled the 100 ms sampling work; one collector per stream now fans out | Verified — base src/stores/omniscientLogger.ts:182-208 and base src/components/VideoPlayerStatsForNerds.vue:186,206-213 each built their own WebRTCStats({ getStatsInterval: 100 }) for the same stream. |
| WebRTC stat ids are now keyed by internal stream name | Verified for the go2rtc leg, partially for the WebRTC leg — internalStreamName() (src/composables/useStreamStats.ts:76) falls back to the external id; see 4.1. |
| The idle sampler "falls back to the video store's 5 s background cadence" | Contradicted — the composable runs its own go2rtcIdleSampleIntervalMs = 5000 timer (src/composables/useStreamStats.ts:64) in addition to src/stores/video.ts:178-181, which still polls the same IPC every 5 s. See 5.3. |
| "Renames after this change carry the recording over to the new ids automatically (local and BlueOS-synced renames alike)" | Contradicted — see 1.2. The watcher is detached by the first whole-array write, and a BlueOS sync-in is exactly a whole-array write (src/composables/settingsSyncer.ts:142). |
| "Up to 250 ms of buffered points was lost on crash or reload" | Half-verified — the 250 ms figure matches flushIntervalMs (src/libs/data-lake-logging.ts:21). beforeunload does not fire on a crash, so only the reload half holds. |
The sampler "ran at 10 Hz from boot for every Standalone user with an active RTSP stream" (commit d45d0cc) |
Contradicted as a description of master — on master the 10 Hz poll only ran while a panel was mounted (base src/components/VideoPlayerStatsForNerds.vue:219-222). That state was created two commits earlier in this same branch. See 8.1. |
| "RTSP stats are published only in Standalone" | Verified — isElectron() guard at src/composables/useStreamStats.ts:99. |
Failure sites
- The shared-window rate bug lives at
src/electron/services/go2rtc.ts:342-343(prevStreamStats/streamRatesmodule globals). In the diff, removed. - The doubled collector lives at base
src/stores/omniscientLogger.ts:182and basesrc/components/VideoPlayerStatsForNerds.vue:186. In the diff, both removed. - The 20 ms plot timer lives at base
src/components/VideoPlayerStatsForNerds.vue:246. In the diff, removed. - The missing unload flush and the unconditional session advance live at base
src/libs/data-lake-logging.ts:422-426. In the diff, both addressed. - Not in the diff:
getStreamsInfo'scatch { return {} }(src/electron/services/go2rtc.ts:394-396) conflates "the query failed" with "there are no streams". Master tolerated it because rates were cached across it; the new consumer acts on it. See 1.3.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
useStreamStats / initialize (useStreamStats.ts:72,299) |
src/main.ts:90 → omniscientLogger.ts setup; also each panel's <script setup> |
one-shot (init), per user action (panel) |
runGo2rtcSampler / sampleGo2rtcStreams / publishGo2rtcSample / isAnyActiveRtspStreamArmed (useStreamStats.ts:187,99,82,177) |
setTimeout chain started by the hasActiveRtspStreams watch |
per incoming message (10 Hz per active RTSP stream, 0.2 Hz idle) |
differenceGo2rtcSamples / go2rtcStreamStatVariableId (stream-stats.ts:34,125) |
sampleGo2rtcStreams |
per incoming message (10 Hz × 10 keys) |
pokeGo2rtcSampler / acquireGo2rtcSampling / releaseGo2rtcSampling (useStreamStats.ts:195,285,290) |
panel onMounted/onUnmounted |
per user action |
carryOverStreamStatRecording (useStreamStats.ts:138) |
watch(videoStore.streamsCorrespondency) |
never in practice — see 1.2 |
ensureWebRtcStatVariables / internalStreamName / streamStatVariableId (useStreamStats.ts:158,76, stream-stats.ts:115) |
activeStreams watch and the on('stats') handler |
per incoming message (10 Hz × 33 keys × 2 call sites per stream) |
on('stats') handler (useStreamStats.ts:226) |
@peermetrics/webrtc-stats internal 100 ms timer |
per incoming message (10 Hz per active stream) |
getStreamsInfo (go2rtc.ts:349) |
IPC go2rtcGetStreamsInfo, from both the sampler and src/stores/video.ts:178-181 |
per incoming message |
DataLakeLogger.constructor (data-lake-logging.ts:159) |
module import (main.ts chain, src/utils/migrations.ts) |
one-shot |
flushPendingPoints (changed) / rollbackSessionPointCount (data-lake-logging.ts:412,447) |
250 ms flush timer, teardownLogging, beforeunload / rejected setItem promise |
4 Hz while recording / rare |
panel update(), WebRTC-snapshot watch, rtspSample watch (VideoPlayerStatsForNerds.vue ~236-259) |
the two shared reactive maps | per incoming message (10 Hz) |
panel draw() |
requestAnimationFrame |
per frame |
Invariants
- "Persisted artifacts use the internal stream name; the credential-bearing external id never appears in a variable id or label." Two producers write stream stat variables.
sampleGo2rtcStreamsenforces it by skipping unmapped streams (useStreamStats.ts:116-117).ensureWebRtcStatVariables/ theon('stats')handler do not — they use?? streamName(useStreamStats.ts:76). One producer covered, one not. The chokepoint fix is to makeinternalStreamNamereturnundefinedand have both callers skip, mirroring the go2rtc path. → 4.1 - "
go2rtcSamplerTimer !== nullmeans a sampler timeout is pending." Broken byrunGo2rtcSampleritself, which never nulls the handle when its own timeout fires. → 1.1 - "
videoStore.streamsCorrespondencyis a stable reactive array." Broken bysrc/stores/video.ts:331,373,412,1539andsrc/composables/settingsSyncer.ts:142, all of which replace.valuewholesale. Every other in-tree watcher (VideoPlayer.vue:259-260,MiniVideoRecorder.vue:195-198,233-234) already uses the getter +deep: trueform that survives this; the new one does not. → 1.2 - "An empty
getStreamsInfo()result means no streams are registered." Broken bygo2rtc.ts:394-396, which also returns{}on any HTTP/parse failure and when the port is not yet known. → 1.3 - "
go2rtcPanelConsumerscounts currently-open RTSP panels." Broken whenever a mounted panel'sstreamNameprop changes protocol, whichVideoPlayer.vue:8,244allows. → 1.4
1. Correctness & Implementation Bugs — 4 findings
1.1 — major — the sampler timer handle is never cleared on fire, so the poll loop can fork
src/composables/useStreamStats.ts:187-199
const runGo2rtcSampler = async (): Promise<void> => {
await sampleGo2rtcStreams()
if (go2rtcSamplerTimer === null) return
...
go2rtcSamplerTimer = setTimeout(() => void runGo2rtcSampler(), intervalMs)
}
pokeGo2rtcSampler = (): void => {
if (go2rtcSamplerTimer === null) return
clearTimeout(go2rtcSamplerTimer)
go2rtcSamplerTimer = setTimeout(() => void runGo2rtcSampler(), 0)
}go2rtcSamplerTimer is only ever nulled by the hasActiveRtspStreams watch (:270-275). When a scheduled timeout fires, the variable keeps the now-dead handle, so for the whole duration of the await on line 188 it is non-null while nothing is pending. Anything that runs during that window and reads the handle as "a timer exists" forks a second chain:
acquireGo2rtcSampling→pokeGo2rtcSampler(:285-288) lands mid-poll:clearTimeouton the already-fired handle is a no-op, and it schedules chain B at 0 ms. The in-flight chain A then resumes at line 189, sees non-null, and schedules itself too. Two chains, one tracked handle.- The same happens when
hasActiveRtspStreamsflips false→true mid-poll: the watch starts a chain because the handle is null, and the resuming run starts another because it is not.
Every fork permanently doubles the IPC + HTTP rate against go2rtc for the rest of the session (a fork only dies at the next stream teardown, and only the untracked chains die). At the 10 Hz cadence the window is the IPC round trip against a 100 ms period, so this is not a rare interleaving.
Fix: track intent separately from the handle — null go2rtcSamplerTimer at the top of runGo2rtcSampler and keep a samplerEnabled boolean that the hasActiveRtspStreams watch sets, then reschedule only while samplerEnabled is true. pokeGo2rtcSampler then clears the handle only when one is genuinely pending.
Consequence: opening the stats overlay while a poll is in flight permanently doubles how often Cockpit interrogates the video service, and it compounds each time it happens.
1.2 — major — the correspondency watcher binds to one array object, so the rename handover never fires
src/composables/useStreamStats.ts:249-258
videoStore.streamsCorrespondency.forEach((corr) => { lastInternalNames[corr.externalId] = corr.name })
watch(videoStore.streamsCorrespondency, (corrs) => { ... carryOverStreamStatRecording(lastName, corr.name) ... })streamsCorrespondency is a useBlueOsStorage ref (src/stores/video.ts:71); reading it off the Pinia store unwraps it to the array object present at that moment. watch() on a reactive object tracks that object, not the ref, so the watcher dies the first time .value is replaced wholesale — which happens at src/stores/video.ts:331 (4K-cam remap), :373 and :412 (WebRTC/RTSP auto-discovery, which run within seconds of boot), :1539 (add RTSP stream), and src/composables/settingsSyncer.ts:142 (every BlueOS sync-in). Since initialize() runs from src/main.ts:90 before discovery, the watcher is detached almost immediately in a normal session, and a BlueOS-synced rename — the case the PR body explicitly claims works — can never fire it at all, because a sync-in is a replacement.
The consequence chain is concrete: ensureWebRtcStatVariables and publishGo2rtcSample immediately start writing under stream-<newName>-*, carryOverStreamStatRecording never runs, so the old ids stay in cockpit-data-lake-recorded-variables (armed, never emitting again), the old variables are never deleted from the Data Lake table, and lastInternalNames stays stale so no later rename recovers either.
Fix: use the getter form every other in-tree watcher on this ref already uses —
watch(() => videoStore.streamsCorrespondency, (corrs) => { ... }, { deep: true })as at src/components/widgets/VideoPlayer.vue:259-280 and src/components/mini-widgets/MiniVideoRecorder.vue:195-198. Seed lastInternalNames from inside the callback rather than once at init, so a correspondency that arrives after boot is seeded rather than treated as a rename. (watch(videoStore.activeStreams, …) at :203 is fine as written — activeStreams at src/stores/video.ts:77 is only ever mutated in place, never reassigned.)
Consequence: after a user renames a camera, an armed recording of that camera's statistics silently stops producing data and dead duplicate rows accumulate in the Data Lake table.
1.3 — major — a failed go2rtc query is indistinguishable from "no streams", and now fabricates a stall
src/composables/useStreamStats.ts:99-111 prunes on absence from allInfo:
const allInfo = await window.electronAPI.go2rtcGetStreamsInfo()
Object.keys(prevGo2rtcCounters).forEach((name) => { if (!(name in allInfo)) delete prevGo2rtcCounters[name] })
Object.keys(go2rtcStreamSamples).forEach((name) => { if (!(name in allInfo)) delete go2rtcStreamSamples[name] })But getStreamsInfo returns {} for three different situations (src/electron/services/go2rtc.ts:350 when the port is not yet known, and :394-396 for any HTTP or JSON failure) as well as for the legitimate "no streams registered". So one transient failure to reach the local go2rtc API:
- wipes
prevGo2rtcCountersfor every stream, so the next successful sample has no previous counters anddifferenceGo2rtcSamplesis skipped —bitrateKbpsandpacketsPerSecare published as the hard-coded0at:125-126; - that zero is then read by the panel as a stall (
VideoPlayerStatsForNerds.vue,const isStalled = !warmUp && sample.bitrateKbps === 0 ? 1 : 0), incrementing the Stalls counter and drawing a red spike; - and it is written to
stream-<name>-rtsp-bitrateKbps, which the PR body nominates as the series from which stalls are reconstructed offline. A false zero there becomes a false recorded freeze.
Master was insulated because streamRates (go2rtc.ts:343) carried the last rates across an empty response; that caching is what the PR correctly removed, which is why the hole is only reachable now.
Fix at the root: have getStreamsInfo distinguish failure from emptiness — return undefined/throw on the catch path and on the missing-port path — and have sampleGo2rtcStreams skip the whole tick (no pruning, no publishing) rather than treating it as a fleet-wide disappearance. Failing that, publish undefined rather than 0 when no previous counters exist, so a missing rate is not recorded as a real zero.
Consequence: a momentary failure to reach the local video service makes the stats overlay report a stream freeze that never happened and writes a false zero-bitrate reading into the user's recorded data.
1.4 — major — the fast-cadence acquire/release is decided by a protocol check at mount and unmount only
src/components/VideoPlayerStatsForNerds.vue (head ~261-271):
onMounted(() => { draw(); if (isRtspStream()) acquireGo2rtcSampling() })
onUnmounted(() => { cancelAnimationFrame(animationFrameId); if (isRtspStream()) releaseGo2rtcSampling() })isRtspStream() reads videoStore.getStreamProtocol(props.streamName), and props.streamName is externalStreamId, a computed bound at src/components/widgets/VideoPlayer.vue:8 over nameSelectedStream (:244-246). The panel is not keyed, so the prop changes in place whenever the user picks a different stream in that widget. Three unbalanced paths follow:
- RTSP at mount, WebRTC at unmount —
releaseGo2rtcSamplingis skipped,go2rtcPanelConsumersstays incremented, and the 10 Hz poll runs for the rest of the session with no panel open. That is precisely the cost commitd45d0ccset out to remove. - WebRTC at mount, RTSP later — no acquire ever happens, so with nothing armed the panel is fed by the 5 s idle cadence: the RTSP plot advances one point every 5 s and
bitrateStr/ppsStrsit at...for seconds at a time. - Cold boot —
externalStreamIdisundefineduntilstreamConnectionRoutine(VideoPlayer.vue:282-289) assigns a stream, so a widget that starts with the overlay already enabled can mount before the id resolves and never acquire.
Fix: hold a local let acquired = false and drive it from a watcher on isRtspStream() (or on props.streamName), acquiring on the false→true edge and releasing on the true→false edge; onUnmounted then releases iff acquired.
Consequence: changing which camera a video widget shows either leaves the fast polling running for the rest of the session or leaves the overlay updating once every five seconds.
2. Persistence & User Data — inventory, 1 finding
Inventory
| Key / store | Backend | What happened |
|---|---|---|
cockpit-data-lake-recorded-variables (src/libs/data-lake-logging.ts:13) |
machine-local (settingsManager) |
Not reshaped, but the ids it holds change meaning: stream-<externalId>-* → stream-<internalName>-*, plus 2 new WebRTC keys and 10 new rtsp- keys. Newly written by carryOverStreamStatRecording (useStreamStats.ts:145-149). No migration — decision stated in the PR body. |
cockpit-streams-correspondency (src/stores/video.ts:71) |
vehicle-synced (useBlueOsStorage) |
Read and watched only. Not written by this PR. |
Cockpit - Data Lake Logs IndexedDB store |
machine-local (IndexedDB) | Unchanged shape and key format (boot=…;epoch=…;seq=…). |
Data Lake sessions store (DataLakeSessionRecord) |
machine-local (IndexedDB) | Unchanged shape. dataPointCount semantics change: advanced synchronously before the write (data-lake-logging.ts:430-432) and decremented when the write rejects (:436-437, :447-454). |
New data lake variables (stream-<internalName>-*, stream-<internalName>-rtsp-*) |
not persisted | createDataLakeVariable is called without persistent/persistValue, so nothing lands in cockpit-persistent-*. Correct. |
The backend choices are right: the recorded-variable selection is a per-operator preference and stays machine-local; nothing machine-specific is pushed to useBlueOsStorage. All keys are cockpit--prefixed. No automatic migration is added, which matches the AGENTS.md preference for the non-destructive route. rollbackSessionPointCount (:449) can only decrement points it itself added, so the count cannot go negative.
2.1 — minor — dead recorded ids are left in the persisted selection with no prune and no user-facing note
src/composables/useStreamStats.ts:115-117,162 and the "Notes for reviewers" section of the PR body.
The id change is a deliberate, documented decision, and re-arming is a reasonable ask. What is missing is the second half of the AGENTS.md rule ("when you leave them, tell the user what changed", AGENTS.md:132): nothing in the UI says the entries went dead, so a user with an armed selection discovers it by exporting a session and finding the columns empty. Two knock-ons the note does not mention:
- In interval mode,
snapshotRecordedValues(src/libs/data-lake-logging.ts:460-467) iteratesrecordedVariableIdsand writesdata[id] = undefinedfor ids that no longer exist, andtoCsv/toJsonderive their columns from the stored points (:670-671,:691-692). So every dead id becomes a permanently empty column in every future export, not merely a no-op. - The dead ids are the old format, which for an RTSP stream is the credential-bearing URL. Those column headers therefore carry the camera username and password into every exported CSV/JSON until the user re-arms — a pre-existing leak (master keyed all stream variables this way) that this PR is well placed to sweep up and does not.
Both are cheap to close: prune ids matching ^stream-.*-(<known stat key>)$ that no longer resolve via getDataLakeVariableInfo when the logger next starts, or at minimum surface a one-time snackbar naming the affected stream(s).
Consequence: users who had armed stream statistics keep silently-dead entries that show up as permanently empty columns in every export, and for RTSP cameras those column names contain the camera's password.
4. Security — 1 finding
Checked and clean: no obfuscated code, no encoded blobs, no hidden Unicode or homoglyphs in the added identifiers, no new network destinations (the only new I/O is the existing go2rtcGetStreamsInfo IPC to 127.0.0.1), no new dependencies, no build-script/CI/Dockerfile changes, no eval/Function()/v-html, no environment variables or tokens. The Electron main-process change (src/electron/services/go2rtc.ts) only removes rate derivation and widens the returned record; it adds no new privileges or surface.
4.1 — major — the credential-bearing external id can still reach a variable name on the WebRTC leg
src/composables/useStreamStats.ts:76
const internalStreamName = (streamName: string): string =>
videoStore.internalStreamNameFromExternal(streamName) ?? streamNameThe invariant the PR sets for itself, in its own comment at :114-115, is that skipping unmapped streams "keeps the external id (an RTSP URL, credentials included) out of variable names". sampleGo2rtcStreams enforces it by returning early when internalName === undefined (:116-117). The WebRTC leg does not: ensureWebRtcStatVariables (:158-168) and the on('stats') handler (:238) both go through the ?? streamName fallback.
RTSP streams played through go2rtc do have a peer connection (src/stores/video.ts:767-780 returns go2rtcManager.peerConnection), so they are monitored by the WebRTC collector like any other stream. Whenever the correspondency entry for a live, monitored stream is missing — deleteStreamCorrespondency splices the entry at src/stores/video.ts:1460 while the collector is still monitoring the peer, and a BlueOS sync-in (src/composables/settingsSyncer.ts:142) can transiently hand over a list that does not yet contain a locally-discovered camera — the fallback creates 33 data lake variables with id: stream-rtsp://user:pass@host/path-<key> and name: Stream 'rtsp://user:pass@host/path' - <key>, both of which are rendered in the Data Lake table and exportable.
This is strictly better than master, which used the external id unconditionally for every stream (base src/stores/omniscientLogger.ts:189-197) — but it is an incomplete fix that leaves the reported problem reachable. Close it at the chokepoint rather than at N producers: make internalStreamName return string | undefined and have both ensureWebRtcStatVariables and the stats handler bail when it is undefined, exactly as the go2rtc path does.
Consequence: if Cockpit briefly loses track of a camera's friendly name, that camera's username and password appear as rows in the Data Lake list and in any exported log.
5. Performance — 3 findings
5.1 — minor — the internal-name lookup is recomputed inside both 33-key loops
src/composables/useStreamStats.ts:158-168 puts the lookup inside the loop, where it does not depend on key:
webRtcStreamStatKeys.forEach((key) => {
const internalName = internalStreamName(streamName) // ← invariant across the loop
const variableId = streamStatVariableId(internalName, key)and :238 repeats it once per key again in the stats handler:
setDataLakeVariableData(streamStatVariableId(internalStreamName(streamName), key), videoData[key])internalStreamName → internalStreamNameFromExternal (src/stores/video.ts:205-208) is an Array.find over the reactive streamsCorrespondency proxy. ensureWebRtcStatVariables is called on every stats event (:230), not only on activation, so per active stream that is 66 array scans per 100 ms sample — ~660 scans/second/stream, ~2 600/second on a four-camera vehicle — for a value that changes only on a rename. This sits directly on the dataLake:setVariable hot path the guidelines call out, and it runs unprompted from a library timer rather than from anything the user did.
Fix: hoist const internalName = internalStreamName(streamName) to the top of ensureWebRtcStatVariables and to the top of the stats handler, and pass it into the loop.
Consequence: Cockpit performs hundreds of redundant list searches per second per camera for a value that does not change between them.
5.2 — minor — the rename handover rewrites the persisted selection and rebuilds every listener twice per armed id
src/composables/useStreamStats.ts:145-149
oldIds.forEach((oldId, index) => {
if (!dataLakeLogger.recordedVariableIds.includes(oldId)) return
dataLakeLogger.setVariableRecorded(oldId, false)
dataLakeLogger.setVariableRecorded(newIds[index], true)
})Each setVariableRecorded (src/libs/data-lake-logging.ts:363-377) writes the whole id list back through settingsManager and, in raw mode while logging, calls applyLoggingMode() → teardownLogging() + a full re-registration of every raw listener. With a stream's 43 stat variables armed that is 86 settings writes and 86 complete listener teardown/rebuild cycles in one synchronous loop — on the order of a few thousand listenDataLakeVariable/unlistenDataLakeVariable calls — all on the main thread in response to a rename.
Fix: compute the new list once and assign it through the recordedVariableIds setter (:303-306), then let a single applyLoggingMode() run — or add a batched setVariablesRecorded(changes) to the logger, which is where the knowledge of "one settings write, one rebuild" belongs.
Consequence: renaming a camera that has many statistics armed briefly freezes the interface while the recorder tears itself down and rebuilds once per value.
5.3 — minor — the idle cadence is a second poll, not a fallback onto the existing one
src/composables/useStreamStats.ts:60-64 defines go2rtcIdleSampleIntervalMs = 5000 and the comment above it, along with commit d45d0cc's body, describes this as falling back to "the video store's 5 s background cadence". It is not a fallback: src/stores/video.ts:178-181 still runs its own unconditional setInterval(… fetchGo2rtcStreamInfo(), 5000), so an idle Standalone session with an active RTSP stream now makes two go2rtcGetStreamsInfo IPC round trips (each an HTTP request to go2rtc's /api/streams plus SDP parsing per stream, go2rtc.ts:349-397) per 5 s instead of one.
That is small in absolute terms, but it is the opposite of what the commit claims, and it is avoidable: the video store already holds the result in go2rtcStreamInfo (src/stores/video.ts:158). Either have the composable's idle path read that ref instead of polling, or drop the store's interval and let the store consume the composable's samples — one owner for the poll, which is what the PR's own framing promises.
Consequence: with nothing looking at the numbers, Cockpit asks the video service for the same information twice as often as the change description says it does.
7. Code Quality & Style — 2 findings
complexity-report.json (head 5157965, base 0eb78bd) reports 172 functions measured across 7 changed files with triggeredCount: 0 and truncated: false, so nothing in this PR crossed the complexity-12 or depth-4 thresholds. No complexity finding.
Otherwise the added code reads well and follows the project's structure: pure logic in src/libs/video/stream-stats.ts with no vue import, reactive orchestration in src/composables/, typed @param/@returns on every added block, no any, no scoped CSS duplicating a Tailwind utility, no long inline-expression strings, and omniscientLogger shrinks by 99 lines.
7.1 — nit — update()'s JSDoc is rewritten while its body is unchanged
src/components/VideoPlayerStatsForNerds.vue — the diff replaces "Draws the lines and updates the stats" with a three-line block, and every line of the function body below it is diff context. AGENTS.md:34 makes existing comments immutable "unless the code lines they document also change in the same diff". The mitigating case is AGENTS.md:35's exception — the old summary said the function draws, which it never did — and the calling contract genuinely moved from a 20 ms timer to the stats watch, so the rewrite is an improvement rather than churn. Worth noting only because the rule keys on the documented lines, not on the caller. AGENTS.md:40 also asks for one sentence as the target; this one is three.
Consequence: small deviation from the project's rule that existing comments stay put unless their code changes.
7.2 — nit — frameHeight and bitrate are filed under webRtcAverageStatKeys, in a split nothing consumes
src/libs/video/stream-stats.ts:51-90. webRtcCumulativeStatKeys and webRtcAverageStatKeys exist only to be concatenated on the next line into webRtcStreamStatKeys; neither is exported and no consumer distinguishes them, so the "cumulative"/"average" trailing comments now document nothing. The split was inherited from omniscientLogger, but the PR moved it into a new module and added frameHeight — a per-sample snapshot value, not an average — to the average bucket. Either collapse to one list while the module is being written, or keep the split and put frameHeight where it belongs.
Consequence: cosmetic mis-grouping in a distinction nothing actually uses.
8. Commit Hygiene — 1 finding
The 11 commits are individually well-scoped, each carries a real explanatory body, prefixes fit their changes (fix:/feat:/refactor:/perf: with an area), no wip/fixup!/squash! noise, no oversized commit, no AI-authorship trailers, and Closes #2947 correctly lives in the PR body only — no issue reference in any commit message. The sequencing (prerequisite fixes → shared collector → shared sampler → publish → on-demand cadence) is genuinely reviewable step by step.
8.1 — minor — d45d0cc repairs a regression 78a7d84 introduced two commits earlier, and misdescribes the baseline
d45d0cc ("perf: video: poll go2rtc stats at 10 Hz only on demand") opens with "The sampler ran at 10 Hz from boot for every Standalone user with an active RTSP stream, whether or not anything consumed the result." That was never true of master: on master the 100 ms poll lived inside the panel and ran only while it was mounted (base src/components/VideoPlayerStatsForNerds.vue:219-222), with the video store's 5 s interval as the only background cost. The state the commit describes was created by 78a7d84 in this same branch, which moved the sampler to a lifetime keyed on hasActiveRtspStreams.
AGENTS.md:191 asks for a commit that fixes an earlier commit on the same branch to be squashed into its target. The whole acquire/release mechanism (acquireGo2rtcSampling/releaseGo2rtcSampling and the demand-based interval) is intrinsic to the sampler 78a7d84 wrote, so folding d45d0cc into it — and rewording the body to describe the design rather than a regression that never shipped — leaves a branch where no commit introduces a slowdown for a later one to remove. As it stands a reviewer bisecting this branch sees a real 10 Hz-from-boot regression at 78a7d84, and a reader of d45d0cc is told master had a problem it did not have.
Consequence: a reviewer reads a performance problem being introduced and then repaired, and the commit message attributes it to a state that never reached users.
11. Nitpicks / Optional — 2 findings
11.1 — nit — only one of the two shared stats maps is pruned
src/composables/useStreamStats.ts:204-206 clears go2rtcStreamSamples for departed streams, with the comment "so consumers don't see stale data". webRtcStreamStatsSnapshots (:43) gets no equivalent: an entry written at :232 survives the stream's teardown for the rest of the session. The visible effect is limited (a newly mounted panel does not read the map until the next sample arrives), but the two maps are siblings with one stated rule between them, and the asymmetry will read as an oversight the next time someone adds a consumer. Add the matching delete in the same activeStreams watch.
Consequence: a camera that goes away leaves its last WebRTC numbers cached for the rest of the session.
11.2 — nit — the DataLakeLogger constructor reaches for window unguarded
src/libs/data-lake-logging.ts:159-161. dataLakeLogger is instantiated at module scope (:727) and the module is imported by src/utils/migrations.ts, so the constructor now runs window.addEventListener as an import side effect. That is fine in the renderer, which is the only place it currently loads, but the in-tree pattern for module-scope window access guards it — src/composables/useBaseStation.ts:206 uses typeof window !== 'undefined'. A one-line guard keeps the module importable from a non-DOM context (a node-environment unit test, or anything that ever pulls a type or a key constant out of it) without changing behaviour.
Related, on the same change: commit 226d8642's body says the flush recovers points lost "on crash or reload". beforeunload does not fire on a crash, and the listener is added at construction time, which for this singleton is import time rather than when recording starts. The reload half is real and matches the system-logging.ts:312-319 precedent; the crash half is not. Worth correcting in the commit body rather than in code.
Consequence: loading this module outside a browser page would throw at import, unlike the guarded pattern used elsewhere in the tree.
Sections with nothing to report (4)
3. AGENTS.md Adherence — ✅ (checked scope discipline against the diff — no unrelated renames, hook reorders or const/let swaps, and the streamRateVariableId → streamStatVariableId move is required by the two-consumer split; no new dependency in package.json; separation of concerns followed exactly, with src/libs/video/stream-stats.ts importing only types from @/types/video and no vue; isElectron() guard present at useStreamStats.ts:99; every added JSDoc has a non-empty summary and typed @param/@returns, including the property signatures jsdoc/require-jsdoc's TSPropertySignature context demands on the useStreamStats return type; grepped the added exports of stream-stats.ts and useStreamStats.ts and every one has a call site in this PR, so no groundwork)
6. UI / UX — ✅ (the only user-visible surface touched is the stats-for-nerds canvas overlay, whose template, layout, colours and copy are unchanged; no dialog, no overlay-teleporting Vuetify control, no button, no icon control and no new user interaction is added, so nothing for the dialog-anatomy, theme="dark", button-token, padding, glass, stacking, sentence-case or logUserAction clauses to bite on — traced VideoPlayer.vue:4-9 as the only mount site)
9. Tests — ✅ (no test file is added, removed or weakened — git-tracked specs under src/** are untouched by all seven changed files, and the guidelines do not ask for new coverage on added logic)
10. Documentation — ✅ (JSDoc added on every new exported symbol and on the changed getStreamsInfo; grepped README.md for the Lite/Standalone feature table AGENTS.md:118 refers to and found none in-tree — RTSP itself is already Standalone-only and undocumented there, so this PR does not widen an existing gap)
Generated by Claude. This is advisory; a human reviewer must still approve.
b1740e9 to
d402407
Compare
|
round 1 findings addressed in the rewritten branch:
also corrected the PR body's claim about spec files - src/tests exists; tests for this change remain waived per the plan. |
|
@cal-oriley nice PR! One tip: after each follow up round you need to call /review for the next one to start. |
d402407 to
9051f19
Compare
|
update: the Stream Statistics arming panel in the Data Logs view is removed - arming happens by ticking the stream's stat variables directly in the Data Lake table, same as any other variable. the readme's RTSP row now points there. no functional change to the publishing, sampling, or recording paths. |
|
@rafaellehmkuhl thank you so much! I'm still working on it a bit, just realized I should convert it to a draft. I'll call /review once it's all ready! |
3b409dd to
5157965
Compare
|
/review |
|
/review |
|
5157965 to
c946777
Compare
|
/review |
|
/review requires write access to this repository. @cal-oriley has |
getStreamsInfo computed bitrate/packet rates against a module-global previous-sample map, so the integration window was the gap since any caller last polled - about 100 ms with a stats panel open, 5 s without, interleaved when both ran. Expose the raw cumulative bytes/packets and the sample epoch on Go2RTCStreamInfo instead, so each consumer differences over its own known window.
The recorded-variables set persists the variable ids, and persisted artifacts use the internal stream name, not the external id. Ids recorded before this switch never emit again and are re-armed under the new ids; no migration.
The stats-for-nerds panel's Size row reads frameHeight but it was not in the published WebRTC stat set, so it could not be recorded.
omniscientLogger and the stats-for-nerds panel each constructed a WebRTCStats instance per stream, so an open panel doubled the 100 ms sampling work. A new useStreamStats composable owns the single collector per stream and fans each snapshot out to the panel and to the data lake publisher, which moves out of omniscientLogger with it.
The stats-for-nerds panel polled go2rtc itself while open. The sampler now lives in useStreamStats with its own lifetime (running only while an RTSP stream is active), differencing the raw counters over its own fixed window via the new pure src/libs/video/stream-stats.ts helper and fanning the samples out to the panel.
RTSP stream stats never reached the data lake - the stats-for-nerds panel was their only consumer. The shared sampler now publishes them (Standalone only): raw cumulative bytes/packets plus the sample epoch, so any rate over any window is derivable offline, bitrate/packet rates derived over the sampler's own window for live use, and the codec/width/height/fps/protocol metadata. Ids are keyed stream-<internalName>-rtsp-<key> so the ingest leg stays unmistakable from the WebRTC leg of the same stream. The stat key lists and variable id builders move into src/libs/video/stream-stats.ts, now that two consumers share them. Registration of a stream's stat variables now happens on every publish, so a rename re-registers under the new ids, and go2rtc streams with no correspondency are skipped, keeping the credential-bearing external id out of variable names.
Up to 250 ms of buffered points was lost on crash or reload for lack of a beforeunload flush like system-logging's.
A rejected write dropped the batch while the session point count still advanced, so the metadata overstated what was stored. The session still advances synchronously with the flush - its key range must cover the batch for export and deletion to find it, and the beforeunload path's promise callbacks may never run - so a rejected write rolls the count back instead.
The sampler ran at 10 Hz from boot for every Standalone user with an active RTSP stream, whether or not anything consumed the result. It now falls back to the video store's 5 s background cadence when idle, and runs at 10 Hz only while a stats-for-nerds panel is open (via acquire/release) or an active stream has recorded rtsp-* variables.
The WebRTC plot pushed a point every 20 ms from a timer while the underlying stats only update at 10 Hz, so it scrolled five times faster than the RTSP plot by re-plotting duplicate values and showed a 2 s window against the RTSP plot's 10 s. The plot now pushes one point per stats sample, so both panels show the same 10-second window; the updateInterval prop and its timer are gone with it.
The stats library already computes a per-sample bitrate from the bytesReceived deltas, and reconstructing it offline from the recorded counters is needlessly annoying, so the value is now published directly as stream-<internalName>-bitrate. The panel's smoothed display value stays unrecorded - it is just an EMA over this series.
c946777 to
f826512
Compare
|
/allow-extra-reviews |
|
Granted 3 extra. This PR has 4 left. |
@cal-oriley you can ask for reviews now. |
|
/review |
|
This PR has 3 left. A maintainer can comment |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.1 | Duplicate polling loops can fork | Opening the stats panel at the wrong moment starts a second copy of the same background polling loop, and from then on the bandwidth figures shown and recorded are wrong. | major | ❌ |
| 1.2 | Rename handover never runs | Renaming a video stream — or having another computer rename it — silently stops the recording of that stream's statistics, without telling anyone. | major | ❌ |
| 1.3 | A server hiccup is recorded as a real zero | If the video server briefly fails to answer, the recording shows the stream dropping to zero bandwidth, which is indistinguishable from the stream actually stalling. | major | ❌ |
| 1.4 | Panel accounting breaks on stream switch | Switching which camera a video widget shows leaves the fast polling either stuck on forever or switched off while the panel is still open. | major | ❌ |
| 4.1 | Camera passwords can end up in variable names | When a stream has not been mapped yet, the full camera address — username and password included — becomes part of the recorded variable names and the exported file. | major | ❌ |
| 1.5 | Closing the window can overstate the log | Closing Cockpit while recording can leave the session's saved point count higher than what was actually written, so an export silently misses the tail. | minor | ❌ |
| 2.1 | Stale recording entries are never cleaned up | Old recording selections for renamed streams stay switched on forever and show up as permanently empty columns in every export. | minor | ❌ |
| 5.1 | Name lookup repeated on the fast path | A list search runs several hundred times a second per stream to produce a value that never changes between searches. | minor | ❌ |
| 5.2 | Recording list saved twice per variable | Handing recording over to a renamed stream rewrites the saved settings and rebuilds every listener twice for each recorded value. | minor | ❌ |
| 5.3 | Two polls of the same server | The idle fallback is described as reusing an existing poll but is in fact a second one, so the video server is queried twice as often for the same data. | minor | ❌ |
| 8.1 | A commit repairs its own branch mate | One commit presents a slowdown as a pre-existing problem when an earlier commit in this same PR introduced it, which misleads anyone reading the history. | minor | ❌ |
| 7.1 | Comment rewritten with unchanged code | A comment was reworded although the code it describes was not touched, against the repository's comment rule. | nit | ❌ |
| 7.2 | Two values in the wrong group | Two statistics are grouped as averages although they are not, which will mislead the next person adding one. | nit | ❌ |
| 7.3 | Two bandwidth values, two different units | Two recorded bandwidth values use different units with nothing saying so, so plotting them together compares numbers a thousand times apart. | nit | ❌ |
| 8.2 | One change split across two distant commits | The same one-line change is made twice, eight commits apart, forcing a reviewer to read the same thing in two places. | nit | ❌ |
| 11.1 | One cache is pruned, its twin is not | Statistics for streams that no longer exist are kept in memory for as long as Cockpit runs. | nit | ❌ |
Since round 2 — 1 closed, comparing 5157965 → f826512
The comparison range is not usable this round. incremental.diff was generated for 515796575d44311eb723595a1d05cd12d73d2733...f8265123af56533289fbb34e5760f7484c91b7b0, but its contents are not an increment: it carries master-only files that this PR does not touch (.github/scripts/review-*.sh, .github/workflows/*, src/electron/main.ts, src/electron/services/config-store.ts, src/libs/joystick/protocols/predefined-resources.ts, src/utils/migrations.ts) and presents this PR's own files as freshly added. That is the signature of a rebase or history rewrite between the two heads, so the file cannot say what the author changed. Every status below is judged against pr.diff and the base checkout instead. Round 2 was also a short-circuit (no new commits at the time), so it carried the ledger without any finding bodies — the bodies below have all been re-derived from the current code, and the commit SHAs cited in finding 8.1 are the current ones, not the pre-rebase SHAs the round-1 ledger title recorded.
Closed this round (1)
- 11.2 — ⚪ No longer applicable. This finding was wrong when it was raised, and I am retracting it rather than reporting that the code changed. It claimed
DataLakeLogger's constructor should follow an in-treetypeof windowguard before callingwindow.addEventListenerat module-import time. There is no such norm:src/libs/system-logging.ts:312registers abeforeunloadlistener at module-import time with no guard at all, andvite.config.ts:72sets the test environment tojsdom, sowindowexists in every context this module is imported from. The premise was mine and it was mistaken.
Still open (13 carried, 3 new)
- 1.1, 1.2, 1.3, 1.4, 2.1, 4.1, 5.1, 5.2, 5.3, 7.1, 7.2, 8.1, 11.1 — ❌ Not addressed. Each still reproduces at the head revision; all thirteen are reprinted in full in their section blocks below.
- 1.5, 7.3, 8.2 — new this round. They are not regressions from a push (there was none to measure); they come from re-running the sections over the whole of
pr.diff, which the contract requires every round.
Commands and discussion. resolutions.json and decisions.json are both empty: no finding on this PR has been resolved by a maintainer or put to a vote, so nothing was closed on that basis and there is no open vote to report. new-comments.json holds four comments since round 2 — two /review invocations from cal-oriley, an /allow-extra-reviews from rafaellehmkuhl and a short maintainer note granting the extra rounds. None of them makes a claim about the code or disputes a finding, so none of them changed a status. No injected instructions were found in any of the untrusted inputs.
Change map — what was established before judging
Line numbers for added and changed lines are given at the head revision (computed from the diff hunks); quoted code from files this PR does not change is cited at the base checkout.
Claims (from the PR body and commit bodies, each checked against the code)
- "Stream statistics are published to the data lake so they can be recorded." — Verified.
src/composables/useStreamStats.ts:158-173arms the WebRTC variables and:226-243writes them;src/libs/video/stream-stats.ts:115and:125mint the ids. - "Variables are keyed by the internal stream name." — Contradicted in part.
useStreamStats.ts:76falls back to the external name whenever the correspondency has no entry, which is exactly the RTSP URL case (finding 4.1). - "Recording is carried over when a stream is renamed, including BlueOS-synced renames." — Contradicted. The watch that would do it (
useStreamStats.ts:252) is bound to the array object and is detached by the whole-array replacement every synced update performs (finding 1.2). - "go2rtc is polled at 10 Hz only on demand, falling back to the video store's 5 s background cadence when idle." — Contradicted in part. The 10 Hz gating is real (
useStreamStats.ts:187-193), but the idle path is a second independent 5 s poll, not the store's (finding 5.3). - "The sampler ran at 10 Hz from boot" (commit
ac3688fe) — Contradicted as history. True of the tree only afterc46692e2, four commits earlier in this same PR (finding 8.1).
Failure site — this PR carries three fixes alongside the feature.
- Panel plotted at the wrong cadence (
c051ab55): the misbehaving code is the panel's ownupdate()timer insrc/components/VideoPlayerStatsForNerds.vue; it is in the diff and the snapshot watch at head:201-208replaces it correctly. - Session point count counted unstored batches (
949f2ed3): the misbehaving code isupdateCurrentSessioninsrc/libs/data-lake-logging.ts:436-454(base); it is in the diff, and the rollback at head:436-437/:441-455addresses the batch-failure case but not the unload case (finding 1.5). - Pending points lost on unload (
0d60cb0d): addressed by the constructor listener at head:155-161, with the caveat above.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
useStreamStats.ts stats handler (:226-243) |
@peermetrics/webrtc-stats getStatsInterval: 100 timer |
per incoming message (10 Hz per stream) |
runGo2rtcSampler (:187) |
self-rescheduling setTimeout, seeded by the hasActiveRtspStreams watch (:270-281) |
per incoming message (10 Hz active / 0.2 Hz idle) |
pokeGo2rtcSampler (:195) |
acquireGo2rtcSampling (:285) and setVariableRecorded side effects |
per user action |
internalStreamName (:76) |
called inside both 33-key loops on the 10 Hz path | per incoming message |
ensureWebRtcStatVariables (:158-173) |
the same 10 Hz stats handler | per incoming message |
carryOverStreamStatRecording (:138) |
the streamsCorrespondency watch (:252) |
never — the watch is detached before it can fire (finding 1.2) |
differenceGo2rtcSamples (stream-stats.ts:35-49) |
sampleGo2rtcStreams |
per incoming message |
acquire/releaseGo2rtcSampling (:285, :290) |
onMounted/onUnmounted in VideoPlayerStatsForNerds.vue:261-271 |
per user action |
rollbackSessionPointCount (data-lake-logging.ts:441-455) |
the .catch on the batch write |
per incoming message (on failure only) |
DataLakeLogger beforeunload handler (:155-161) |
window unload | one-shot |
Invariants
- "A recorded stat variable id always contains the internal stream name, never the external one." Violated at
useStreamStats.ts:76(the?? streamNamefallback) and therefore at every consumer of it — the arming loop:162, the write path:238, and both id builders instream-stats.ts:115/:125. The chokepoint is that single helper; the PR guards nothing. (Findings 4.1, 2.1.) - "There is at most one live go2rtc poll chain." Violated because
go2rtcSamplerTimeris not cleared while the poll is in flight (:187-193), sopokeGo2rtcSamplercan schedule a second chain against the same sharedprevGo2rtcCounterswindow. (Finding 1.1.) - "A stream's data-lake ids follow it across renames." Relies on the
streamsCorrespondencywatch firing on replacement;src/composables/settingsSyncer.ts:142(refedValue.value = newValue as T) replaces the whole array on every BlueOS sync, andsrc/stores/video.ts:331,:373,:412and:1539do the same locally. None is covered. (Finding 1.2.) - "The persisted session point count never exceeds what was written." Relies on every increment having a matching rollback; the unload path has none, and the epoch guard at
:449skips the rollback once a new session has started. (Finding 1.5.)
1. Correctness & Implementation Bugs — 5 findings
1.1 — go2rtcSamplerTimer is not cleared while a poll is in flight, so a poke can fork a second poll chain (major, carried from round 1)
src/composables/useStreamStats.ts:187-199:
const runGo2rtcSampler = async (): Promise<void> => {
await sampleGo2rtcStreams()
if (go2rtcSamplerTimer === null) return
const intervalMs = go2rtcPanelConsumers > 0 || isAnyActiveRtspStreamArmed() ? 100 : 5000
go2rtcSamplerTimer = setTimeout(() => void runGo2rtcSampler(), intervalMs)
}
pokeGo2rtcSampler = (): void => {
if (go2rtcSamplerTimer === null) return
clearTimeout(go2rtcSamplerTimer)
go2rtcSamplerTimer = setTimeout(() => void runGo2rtcSampler(), 0)
}While await sampleGo2rtcStreams() is pending, go2rtcSamplerTimer still holds the handle of the timer that already fired. A pokeGo2rtcSampler in that window (mounting a stats panel, or arming an rtsp-* variable) clears a dead handle and schedules a fresh chain; when the in-flight poll resumes it sees a non-null timer and schedules its own successor as well. Two chains now interleave against the shared prevGo2rtcCounters window in stream-stats.ts:35-49, so each difference is taken over a fraction of the interval it assumes and every published rate is understated by roughly the split. The orphan chain does terminate when the last RTSP stream goes away (the === null check), so this is corrupted data while a stream is live rather than a permanent leak — but the corruption is silent and is what gets recorded.
Fix: set go2rtcSamplerTimer to a sentinel meaning "running" before the await (or track an inFlight flag and have pokeGo2rtcSampler set a pokeRequested flag instead of scheduling), so exactly one chain can exist.
1.2 — the streamsCorrespondency watch is bound to one array object, so the rename handover never fires (major, carried from round 1)
src/composables/useStreamStats.ts:252:
watch(videoStore.streamsCorrespondency, (corrs) => { … })Passing a reactive array (rather than a getter) binds the watcher to that specific array object. src/composables/settingsSyncer.ts:142 does refedValue.value = newValue as T on every BlueOS settings update, which replaces the array wholesale — and streamsCorrespondency is a useBlueOsStorage key (src/stores/video.ts:71), so this is the normal path, not an edge case. src/stores/video.ts:331, :373, :412 and :1539 replace it locally too. After the first replacement the watcher observes an orphaned array and never fires again.
The consequence is the opposite of what the PR advertises: carryOverStreamStatRecording (:138) never runs, so after a rename the old stream-<oldName>-* ids stay armed but stop receiving data, and the new ids are never armed. The user sees recording silently stop, with the settings still showing it as on. Note the local rename path at src/stores/video.ts:1437 mutates in place (streamCorr.name = newInternalName), so the local case survives until the next whole-array replacement — the synced case does not survive at all.
Fix: watch a getter, watch(() => videoStore.streamsCorrespondency, cb, { deep: true }), so the source is re-evaluated and survives replacement.
1.3 — getStreamsInfo returns {} for failure as well as for empty, so a transient error is recorded as a real stall (major, carried from round 1)
src/electron/services/go2rtc.ts (base :349-396, head ≈ :329-359) returns {} both when go2rtcPort is unset and from its catch. sampleGo2rtcStreams cannot distinguish that from "the server answered, no streams", so on a single failed poll the prune loop drops every entry and the zero-rate fallback at useStreamStats.ts:125-126 publishes zeros. Recorded, that is a genuine-looking bandwidth stall — the exact signature a user opens this data to investigate — produced by a hiccup in a status query. The next successful poll then differences against a missing previous sample.
Fix: have getStreamsInfo signal failure distinctly (null/undefined, or a { ok, streams } result) and have the sampler skip the cycle on failure, keeping the previous counters and publishing nothing, rather than treating it as data.
1.4 — acquire/releaseGo2rtcSampling are gated at mount and unmount only, so a stream switch mis-counts the consumer (major, carried from round 1)
src/components/VideoPlayerStatsForNerds.vue:261-271 calls acquireGo2rtcSampling() in onMounted and releaseGo2rtcSampling() in onUnmounted, each behind an isRtspStream() test on the stream that is current at that moment. src/components/widgets/VideoPlayer.vue:8 renders <statsForNerds :stream-name="externalStreamId" /> with no :key, and externalStreamId is a computed (:244), so changing the widget's stream re-renders the same component instance — no unmount, no remount. Switching from an RTSP stream to a WebRTC one therefore never releases (10 Hz polling stays on for the rest of the session), and switching the other way never acquires (the panel a user just opened shows 5 s-stale ingest numbers).
Fix: replace the lifecycle-hook pair with a watch on isRtspStream(streamName) that releases the old value and acquires the new one, keeping the onUnmounted release for the final teardown. Adding a :key on the component would also work but is the blunter of the two.
1.5 — the unload flush cannot uphold the point-count invariant it was added to protect (minor, new this round)
src/libs/data-lake-logging.ts:155-161 registers a beforeunload listener that flushes pending points, and :430-437 increments the session point count synchronously before the write and rolls it back in the write's .catch. On unload that pairing cannot hold: updateCurrentSession (base :436-454) persists through await sessionsDB.setItem(...) at :451, and neither that write nor the .catch that would undo the increment can complete after the document is gone. The count is raised, the write is abandoned, and the persisted session claims more points than sessionKeyRange (base :183-187) can actually export — silently truncating the tail of a recording at exactly the moment a user closes Cockpit expecting their data to be saved. rollbackSessionPointCount (:441-455) also refuses to act once the epoch has moved on (:449), so a rollback that is late for any reason is dropped rather than applied.
Fix: on the unload path, increment only after the batch write has resolved, or write the count and the batch in one transaction so a partial result is impossible. If neither is practical inside beforeunload, treat the persisted count as a ceiling and have the exporter derive the real count from the key range rather than trusting the field.
2. Persistence & User Data — inventory, 1 finding
Inventory
| Key | Backend | What happened |
|---|---|---|
cockpit-data-lake-recorded-variables (src/libs/data-lake-logging.ts:13) |
machine-local (settings-management.ts) |
Not reshaped, but the PR makes it accumulate a new family of ids (stream-<name>-*, stream-<name>-rtsp-*) whose names are derived from mutable stream names. |
Data-lake session records / point keys (sessionsDB, sessionKeyRange at :183-187) |
machine-local (IndexedDB) | Shape unchanged; the point-count field gains a rollback path (:430-455). |
cockpit-streams-correspondency (src/stores/video.ts:71) |
vehicle-synced (useBlueOsStorage) |
Read only. The PR adds a dependency on it, it does not write it. |
No key is added, no persisted shape is reshaped, no automatic migration is introduced, and no machine-specific value is put on a vehicle-synced key. The one issue is what the recorded-variables list accumulates:
2.1 — dead stream-<name>-* ids accumulate in the recorded-variables list with no prune and no user-facing note (minor, carried from round 1)
Variable ids are minted from the stream name (src/libs/video/stream-stats.ts:115, :125). Rename a stream — or let the handover fail as in finding 1.2 — and the old ids stay in cockpit-data-lake-recorded-variables forever: nothing removes an id whose variable no longer exists, applyLoggingMode (base :481-517) keeps rebuilding a listener for each one, and every subsequent export carries a permanently empty column per dead id. Combined with finding 4.1 those column headers can contain the RTSP URL, credentials included, so a stale entry is not merely noise in a file the user may share.
Fix: prune ids for streams that no longer appear in the correspondency when the list is loaded or when the handover runs, and/or surface the armed stat variables in the UI so a user can see and clear them. At minimum, the rename path must remove what it replaces.
4. Security — 1 finding
4.1 — the ?? streamName fallback lets a credential-bearing RTSP URL become a variable id and an export column header (major, carried from round 1)
src/composables/useStreamStats.ts:76:
const internalStreamName = (streamName: string): string =>
videoStore.internalStreamNameFromExternal(streamName) ?? streamNameThe fallback is to the external name whenever the correspondency has no entry. For an RTSP stream the external name is the URL, which in Cockpit routinely carries rtsp://user:password@host/path. That value then flows into ensureWebRtcStatVariables (:158-173), where it becomes part of 33 data-lake variable ids and their human-readable labels, into the write path at :238, and from there into cockpit-data-lake-recorded-variables and every CSV the user exports and shares. The unmapped case is not hypothetical — it is precisely the window between a stream appearing and the store mapping it, which is when a panel opened early will sample.
The invariant belongs at this single chokepoint, so the fix is local: when internalStreamNameFromExternal returns nothing, do not publish. Skip the sample (the sampler already skips unmapped go2rtc streams at :116-117 — apply the same rule here) rather than falling back to a value that can contain secrets. If a fallback really is needed, derive an opaque stable token from the URL rather than embedding it.
Every other sub-check in this section came back clean and is not written out.
5. Performance — 3 findings
5.1 — internalStreamName is recomputed inside both 33-key loops on the 10 Hz path (minor, carried from round 1)
src/composables/useStreamStats.ts:76 calls videoStore.internalStreamNameFromExternal, which is a linear find over the correspondency array (src/stores/video.ts:205-208). It is called inside the arming loop at :162 and again on the write path at :238, both of which iterate the full stat-key set. At the collector's 10 Hz cadence that is on the order of 660 array scans per second per stream, for a value that changes only when a stream is renamed.
Fix: hoist the lookup out of the loops — resolve it once per sample (or memoise it per external name, invalidated by the correspondency watch of finding 1.2) and pass the result down.
5.2 — carryOverStreamStatRecording calls setVariableRecorded twice per armed id (minor, carried from round 1)
src/composables/useStreamStats.ts:147-148 disarms the old id and arms the new one with two separate setVariableRecorded calls. Each one persists the whole recorded-variables list (src/libs/data-lake-logging.ts:292-306, :363-377) and, in raw mode, calls applyLoggingMode(), which tears down and rebuilds a listener for every recorded variable (base :481-517, :522+). With 33 keys per stream that is 66 full teardown/rebuild cycles and 66 writes for a single rename.
Fix: give the logger a batch operation that takes the whole new id list, persists once and reconciles listeners once, and call it from the handover.
5.3 — the idle cadence is a second poll of go2rtc, not the video store's (minor, carried from round 1)
useStreamStats.ts:61-62 and commit ac3688fe's body both describe the idle path as falling back to "the video store's 5 s background cadence". It does not: src/stores/video.ts:178-183 runs its own unconditional setInterval(..., 5000) calling fetchGo2rtcStreamInfo (:169-176), and runGo2rtcSampler schedules an independent 5 s chain of its own (:190). Standalone therefore queries go2rtc twice every five seconds, on two unsynchronised phases, for the same data.
Fix: either subscribe to the store's existing poll result for the idle case (making the composable's own timer exist only for the 10 Hz mode), or move the store's interval into the composable so there is one poll with two cadences. Whichever is chosen, correct the comment and the commit body, which currently describe code that does not exist.
7. Code Quality & Style — 3 findings
7.1 — update()'s JSDoc is rewritten while its body lines are unchanged (nit, carried from round 1)
src/components/VideoPlayerStatsForNerds.vue:185-189 reword the JSDoc block above update(), but the function body at :190-199 is untouched by the diff. AGENTS.md (comment policy, line 34) treats a comment whose code has not changed as immutable — the old wording was written by someone with context this PR does not have.
Fix: restore the original wording. If it is genuinely wrong now, that is a separate observation worth stating in the PR body.
7.2 — frameHeight and bitrate are placed in webRtcAverageStatKeys (nit, carried from round 1)
src/libs/video/stream-stats.ts:76-87 puts 'bitrate' (:77) and 'frameHeight' (:79) in the "average" set. Neither is an average of anything: one is a per-sample rate and the other is a frame dimension. Since both sets are concatenated at :88 and no consumer distinguishes them, the split is currently decorative — which is what makes it a trap for whoever adds the next key and reads the name as a specification.
Fix: rename the two sets for what they actually are (cumulative counters vs. instantaneous values), or drop the split and keep one list.
7.3 — two bandwidth variables published in different units, with nothing declaring either (nit, new this round)
f8265123 publishes stream-<internalName>-bitrate from the WebRTC stats library, which reports bits per second, while the go2rtc side publishes stream-<internalName>-rtsp-bitrateKbps in kilobits per second (stream-stats.ts:91-100, :125). Only one of the two names carries its unit, and the panel divides by 1000 at VideoPlayerStatsForNerds.vue:206-208 to reconcile them for display. In the data lake they sit side by side as bare numbers, so plotting both puts two series a factor of 1000 apart on one axis with no indication that this is expected.
Fix: name the WebRTC one for its unit too (-bitrateBps), or publish both in the same unit. The variable's display name is the only place a user will look for this.
8. Commit Hygiene — 2 findings
8.1 — ac3688fe repairs a regression introduced by c46692e2 in this same branch and describes it as pre-existing (minor, carried from round 1; SHAs restated for the current history)
c46692e2 ("refactor: video: share the go2rtc ingest sampler through useStreamStats") moves the sampler out of the panel and gives it a lifetime tied to any active RTSP stream — which is what makes it run at 10 Hz from boot for every Standalone user with such a stream, whether or not anything consumes it. Four commits later, ac3688fe ("perf: video: poll go2rtc stats at 10 Hz only on demand") fixes exactly that, and its body states it as a standing condition: "The sampler ran at 10 Hz from boot for every Standalone user with an active RTSP stream." On master before this PR it did not — the panel polled only while open. This is the self-correcting commit AGENTS.md (lines 188-199) rules out: a reviewer bisecting or backporting c46692e2 alone gets the regression, and the history attributes it to code that never had it.
Fix: squash ac3688fe into c46692e2 so the sampler lands already gated. If it must stay separate, the body has to say the previous commit in this branch introduced the behaviour.
8.2 — the same one-line change is split across two commits eight apart (nit, new this round)
aea166be ("feat: video: publish frameHeight to the data lake") and f8265123 ("feat: video: publish WebRTC bitrate to the data lake") each add a single string to webRtcAverageStatKeys in src/libs/video/stream-stats.ts:76-87, with eight commits between them. They are the same logical change to the same list, and reviewing the second means going back to the first. AGENTS.md (lines 188-199) calls out over-splitting alongside bundling.
Fix: squash the two into one commit that establishes the published key set.
11. Nitpicks / Optional — 1 finding
11.1 — webRtcStreamStatsSnapshots is never pruned while its sibling is (nit, carried from round 1)
src/composables/useStreamStats.ts:43 holds a per-stream snapshot map. The activeStreams watch at :203-207 prunes go2rtcStreamSamples for departed streams but leaves the snapshot map untouched, so entries for streams that no longer exist stay for the process lifetime. The amount is small and bounded by how many distinct streams a session sees, which is why this is a nit and not a leak worth blocking on — but the asymmetry between the two maps, three lines apart, reads as an oversight rather than a decision.
Fix: prune both maps in the same loop.
Sections with nothing to report (4)
3. AGENTS.md Adherence — ✅ (checked the added files against .eslintrc.cjs's jsdoc/require-jsdoc, jsdoc/require-returns, explicit-function-return-type and max-len: 180 — the added exports and interfaces in useStreamStats.ts and stream-stats.ts all carry typed, non-filler JSDoc; no dependency was added; src/electron/services/go2rtc.ts is a net deletion of 35 lines; every added export has a call site in this PR. The two AGENTS.md breaches found — the comment rewrite and the commit history — are raised in sections 7 and 8 where they belong.)
6. UI / UX — ✅ (the only component touched is VideoPlayerStatsForNerds.vue; its template is unchanged, no dialog, overlay-teleporting control, footer or icon-only control is added, and the change is a data-source swap behind an existing panel — the plotting cadence fix at :201-208 makes the displayed values track the 10 Hz samples rather than a slower timer, which is the user-visible improvement, not a new surface.)
9. Tests — ✅ (no file under src/**/*.test.ts or tests/ appears in pr.diff; no existing test is removed, weakened or made brittle. The new src/libs/video/stream-stats.ts is deliberately framework-agnostic and pure, which makes the extracted logic more testable than what it replaced.)
10. Documentation — ✅ (checked README.md for a Lite/Standalone feature table entry that would need updating: the go2rtc ingest statistics are Standalone-only, but that limitation is inherited from RTSP stream support itself, which the README already scopes — this PR adds no capability that differs between the builds beyond that. In-code JSDoc on the added public functions is present and typed.)
Generated by Claude. This is advisory; a human reviewer must still approve.
… and unmapped streams The sampler could fork when a panel opened mid-poll, rename handover detached on correspondency replacement, a failed go2rtc query published a fake zero-bitrate stall, and unmapped WebRTC stats fell back to the credential-bearing external id.
3 participants
Summary
frameHeightand the library-computedbitratenewly added); go2rtc/RTSP ingest stats are published for the first time: raw cumulativebytes/packetsplus the sample epoch, bitrate/packet rates derived over the sampler's own window, and codec/width/height/fps/protocol metadata, keyedstream-<internalName>-rtsp-<key>.useStreamStatscomposable) fans snapshots out to the panel and the data lake, so an open panel no longer doubles the 10 Hz sampling work. The go2rtc sampler lives there too, running only while an RTSP stream is active - at 10 Hz while a stats panel is open or an armed stream is being recorded, and at a 5 s idle cadence otherwise - and differencing the raw counters over its own fixed window.Notes for reviewers:
stream-<externalId>-*tostream-<internalName>-*. Anyone who armed stream variables before this change keeps deadstream-<externalId>-*entries in their machine-local recorded-variables set until they re-arm - no migration (the correspondency is vehicle-synced and not yet loaded at migration time, so an automatic migration could miss mappings and still mark itself run). The affected population is users who renamed a stream and had individually armed its variables in the Data Lake table. Renames after this change carry the recording over to the new ids automatically (local and BlueOS-synced renames alike).rtsp-bitrateKbpsseries. The panel's smoothed WebRTC bitrate is likewise just an EMA over the now-publishedbitrateseries.Test plan
yarn lint:fixpasses with no errors or warningsyarn buildpassesstream-<name>-*variables, set logging to Raw, record ~1 minute, export the session as JSON, and confirm the points land at ~10 Hz with changing values (this also sets the real churn figures)stream-<name>-rtsp-*variables in Standalone and confirm the raw counters, sampleEpoch, derived rates and metadata are recordedrtsp-*variables appear and WebRTC arming still worksCloses #2947