Repository navigation
Conversation
pblazej
added
the
internal
622c20e to
fe4f566
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 4 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let nominated = | ||
| matches!(stat.members.get("nominated"), Some(AttributeValue::Bool(true))) | ||
| || text(stat, "state").as_deref() == Some("succeeded"); | ||
| if nominated { | ||
| index.pair_rtt_ms = num(stat, "currentRoundTripTime").map(|s| s * 1000.0); |
There was a problem hiding this comment.
🟡 Inbound RTT comes from an inactive pair
If an old succeeded candidate pair precedes the nominated pair, Index::new selects its RTT. Every inbound sample then reports the old path's RTT.
Learn more
A WebRTC report can contain multiple successful candidate pairs after connectivity checks or an ICE switch. state = succeeded does not mean that the pair carries traffic. Because the index takes the first qualifying entry, the RTT depends on report ordering rather than the active path.
Example: Pair A is succeeded with RTT 300 ms and pair B is nominated with RTT 40 ms. If A appears first, inbound samples carry 300 ms.
Recommended fix: Resolve the selected pair from transport's selectedCandidatePairId when present; otherwise prioritize nominated pairs and use a succeeded pair only as a documented fallback.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 4c3e79d: inbound RTT now comes from the pair that the transport names as selected. A stale succeeded pair listed first no longer wins, and inbound_rtt_comes_from_the_selected_candidate_pair covers it.
| #[derive(Default)] | ||
| pub(crate) struct StatsWindows { |
There was a problem hiding this comment.
🔴 RTC samples cannot enter telemetry
SDKs can construct RtcStatsSample, but StatsWindows is private and no public API accepts samples. SDK-provided stats cannot become telemetry events.
Learn more
The accumulator is not exposed to callers, and neither a pipeline nor a scope ingestion method exists in the current crate. Thus RtcStatsSample is only a data type in the public API; external users cannot record its readings or obtain windowed events.
Example: An SDK creates an inbound audio sample with bytes = Some(1000). It cannot submit that sample to a public telemetry object to produce lk.rtc.stats.sample.
Recommended fix: Provide a public ingestion method that routes the sample to the owning session's StatsWindows, and wire closing and retirement into the pipeline lifecycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This stage adds only the windowing, and #1483 adds the public calls that feed it (Scope::record_stats, record_stats_report and record_peer_stats).
fe4f566 to
ec765e3
Compare
ec765e3 to
4c6c812
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| /// The closed window as a record captured now (see `QueuedEvent::new`). | ||
| fn into_queued(self, end_ns: u64) -> QueuedEvent { | ||
| let session = Arc::clone(&self.session); | ||
| QueuedEvent::new(self.into_event(end_ns), session) |
There was a problem hiding this comment.
🔴 RTC windows inherit later session ownership
When a session changes project or attributes before a stats window closes, into_queued captures its new ownership. Earlier readings ship with the wrong project or correlation attributes.
Learn more
A stats window collects readings over time and carries a session reference. QueuedEvent::new captures the session's current route and custom attributes when called, not when the window opened. If those values change before closure, the whole window is assigned the later values. This is especially consequential for a room that switches Cloud projects: an already collected window can be routed to the new project.
Example: A room records stats under project A, then reconnects to project B before the window closes. Closing the window now assigns it project B and attaches B-era correlation attributes, although its samples were taken under A.
Recommended fix: Capture route and correlation attributes in Window::open and apply those snapshots when creating QueuedEvent. Preserve the SDK-owned session attributes at the appropriate capture boundary, and ensure StatsWindows::split closes windows before any intentional ownership changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
`DeviceState` is what the host observes (thermal, power, memory, battery, network, lifecycle); the core derives the `lk.device.*.changed` events, a cadence factor of up to 4× under pressure, and soft and hard upload holds. `DeviceEvent` files audio route changes, interruptions and capture failures with shared enums. RTC stats map one raw `getStats()` report (per track or per peer connection) to samples, fold simulcast layers into one series per track, and window them on device into `lk.rtc.stats.sample`, per track, direction and session. A window closes into a captured record that carries its session's attributes and `session.id` as of that moment.
… tracks Inbound RTT now comes from the candidate pair the transport names as selected, else the first pair that is selected, or nominated and succeeded, as the JS port does; a stale succeeded pair listed first no longer wins. A reading without the CPU limitation counter keeps the previous baseline instead of resetting it to 0, which made the next reading look like CPU pressure. A track keeps its layer and CPU state through a close that follows a split, because state now follows the tracks that read since the last close, not the open windows. A NaN or infinite stats number counts as no reading. A report gets one timestamp, and each poll is read as a whole: each field of its one window reading is the largest value its streams report, so a stream without a value never erases another's and the order of a report changes nothing. The display bitrate spans the window's first and last reading and is left out without two. A layered outbound track's bytes and packets (its readings name their RTP streams) are a running total of every stream's growth since the previous poll; any other track, inbound included, passes its counters through as reported. A stream's first poll only sets its starting point, so a stream that comes and goes is never counted twice, and a track keeps only the previous poll's streams. Clearing counts a first poll that has not opened its window yet as one purged record. `network.connection.type` is reserved for the core. SPEC names the battery attributes the code emits (`lk.device.battery.level`, `lk.device.battery.charging`), now pinned by a test, and the `lk.rtc.stats.sample` contract moves into this stage. A closed window is captured through `QueuedEvent::new`, so its owner and attributes are taken at one moment. Device events convert with `From`, variant names are documented as the wire contract and pinned to SPEC, the cadence cap and battery buckets are named, the cadence helpers are crate-private, and every public field and variant has a doc line: the ones this commit adds name the value or attribute they are sent as. A changeset records the stage.
4c6c812 to
4c3e79d
Compare
Summary
What the host reports and what the core does with it. The device contract, row by row with its tests, is in #1485.
Changes
DeviceState(thermal, power, memory, battery, network, lifecycle): thelk.device.*.changedevents, a cadence factor of up to 4×, and soft and hard upload holdsDeviceEvent: audio route changes, interruptions and capture failuresgetStats()report per track or peer connection mapped to samples, simulcast layers folded into one series, windowed on device intolk.rtc.stats.sampleSPEC.md: the cadence policyclearcounts a first poll that has not opened its window yet as one purged recordSPEC.mdnames the battery attributes the code sends (pinned by a test), and thelk.rtc.stats.samplecontract moves to this stageVerification
At
4c3e79da, from a clean checkout (CI's test workflow runs only for PRs intomain, so these were run locally; there is no clippy job in CI):cargo fmt --checkcargo clippy -p livekit-telemetry --all-targets -- -D warnings, also with--all-featuresand as acargo hack --feature-powerset --depth 2runcargo test -p livekit-telemetry: 110 unit, 0 docRUSTDOCFLAGS="-D warnings" cargo doc -p livekit-telemetry --no-depsreports only the unresolved link toTelemetry::statsinstats.rs, whichmain(6473ec17) has too; feat(telemetry): add the pipeline, sessions, spans and exporter #1483 adds the item