Conversation
`Telemetry` is the synchronous handle (emit, log, device state, flush, shutdown, purge, stats) behind a flood guard; `Scope` is one Room on it, with its own trace id, server URL and token, attributes, RTC stats and the `lk.subscribe` lifecycle; `Span` is one typed attempt (`SpanName`, `SpanStep`). The `Exporter` actor drains the queue and finished spans, encodes and gzips one batch per project into the cache before any network is involved, then uploads oldest first: it acts on every collector answer, pauses a destination with jittered backoff or for the delay the server asked for, splits a 413, holds uploads while a Room connects or the device asks for quiet, and meters requests while a Room is in a call.
pblazej
added
the
internal
463a051 to
c16912c
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 6 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| self.spans | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()) | ||
| .end(span, outcome, error_type, attributes); | ||
| } |
There was a problem hiding this comment.
🟡 Finished connections leave uploads waiting
When a connect span ends, end_span does not wake the exporter to lift its soft hold. Cached batches wait until the hold cap or next tick despite signaling having finished.
Learn more
The exporter checks open connect and reconnect spans in hold_reason. Once it has entered a soft hold, its next scheduled wake is the hold cap or export tick, unless another change explicitly wakes it. Ending a span only updates the registry, so cached work remains held even though the reason ended.
Example: A connect finishes at second 3 with cached batches waiting. With the default cadence, the exporter stays asleep until its 60-second hold deadline rather than starting an upload at second 3.
Recommended fix: Wake the exporter when a sensitive span ends, after releasing the spans mutex. Ensure the release wake also covers the subscribe and generic span-end paths without holding registry locks during notification.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.offline() { | ||
| self.log_hold(Some("offline"), backlog); | ||
| return (0, false); | ||
| } |
There was a problem hiding this comment.
🟡 Offline time exhausts the upload hold
When the device goes offline during a soft hold, budget retains held_since throughout the hard hold. Reconnection immediately releases a batch even if the constrained network or low battery still requires quiet.
Learn more
A soft hold uses held_since as its 60-second timer. Going offline is a hard hold and stops requests indefinitely, but the early offline return leaves that timer running. On reconnection, the next pass sees a cap that has already elapsed and sends a batch despite the continuing soft-hold condition.
Example: Low Data Mode starts holding at 12:00:00, the phone goes offline at 12:00:05 and reconnects at 12:02:00. The first pass sends a batch immediately rather than waiting under the still-active Low Data Mode hold.
Recommended fix: Suspend or reset the soft-hold timer when the hard offline hold begins, and resume a fresh or remaining timer when it lifts. Check the same behavior for destination/token hard holds so waiting for credentials does not consume the soft-hold allowance.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn in_call(&self) -> bool { | ||
| let scopes = self.shared.scopes.lock().unwrap_or_else(|e| e.into_inner()); | ||
| scopes.iter().any(|s| s.upgrade().is_some_and(|s| s.in_call())) | ||
| } |
There was a problem hiding this comment.
🟡 Dropped rooms keep backlog metered
After a room is dropped without disconnected, in_call still sees its server through the retained span session. Spans.sessions keeps that session alive, so backlog stays limited to four requests per tick.
Learn more
The exporter decides whether to meter backlog by inspecting live ScopeState values. The span registry remembers recent span-to-session associations with strong Arc<ScopeState> references, including finished spans, so a room scope can be gone while its session still upgrades here. Its server field remains set unless the caller invoked disconnected, making the exporter treat a vanished room as a call.
Example: A room sets its server, finishes one connect span, then its last Scope is dropped without calling disconnected. The process retains a 100-batch backlog; uploads stay at four per minute instead of draining freely after the room disappears.
Recommended fix: Track whether the actual Room scope remains alive independently of retained record/span associations, or make the span-to-session history weak and preserve needed queued data elsewhere. Clear the in-call state on room teardown without sacrificing the routing credentials required by cached batches.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if !self.guard.lock().unwrap_or_else(|e| e.into_inner()).admit() { | ||
| Counters::add(&self.shared.counters.rate_limited, 1); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Event quota drops warning logs
When warning logs arrive after 300 discrete events, emit_in applies the same flood guard to their empty-name records. Diagnostic logs disappear until the ten-minute window resets.
Learn more
The flood guard caps discrete events, while plain logs are distinguished by an empty event name. log_event creates those records, and they reach this same capture path. Sharing the quota means an unrelated burst of events can prevent important warning and error lines from being exported.
Example: After 300 lk.ping events in ten minutes, an SDK error log arrives with an empty name. It is counted as rate-limited and never enters the queue.
Recommended fix: Apply FloodGuard::admit only to named, discrete events; keep the existing severity floor for plain logs.
| if !self.guard.lock().unwrap_or_else(|e| e.into_inner()).admit() { | |
| Counters::add(&self.shared.counters.rate_limited, 1); | |
| return; | |
| } | |
| if !event.name.is_empty() && !self.guard.lock().unwrap_or_else(|e| e.into_inner()).admit() { | |
| Counters::add(&self.shared.counters.rate_limited, 1); | |
| return; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let valid = !name.is_empty() | ||
| && name.len() <= crate::event::MAX_NAME_BYTES | ||
| && attributes.len() <= crate::event::MAX_CUSTOM_ATTRIBUTES | ||
| && attributes.iter().all(|a| crate::event::valid_custom(&a.key, Some(&a.value))); |
There was a problem hiding this comment.
🟡 Custom event prefix exceeds name limit
A 128-byte name passes emit_custom, but TelemetryEvent::custom prepends custom. afterward. The exported name exceeds the 128-byte limit instead of being rejected and counted.
| let valid = !name.is_empty() | |
| && name.len() <= crate::event::MAX_NAME_BYTES | |
| && attributes.len() <= crate::event::MAX_CUSTOM_ATTRIBUTES | |
| && attributes.iter().all(|a| crate::event::valid_custom(&a.key, Some(&a.value))); | |
| let valid = !name.is_empty() | |
| && name.trim_start_matches("custom.").len() + "custom.".len() | |
| <= crate::event::MAX_NAME_BYTES | |
| && attributes.len() <= crate::event::MAX_CUSTOM_ATTRIBUTES | |
| && attributes.iter().all(|a| crate::event::valid_custom(&a.key, Some(&a.value))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn disconnected(&self, reason: DisconnectReason) { | ||
| let open: Vec<_> = | ||
| self.state.subscribes.lock().unwrap_or_else(|e| e.into_inner()).drain().collect(); | ||
| for (_, (span, since)) in open { | ||
| end_unfinished(&span, since); | ||
| } | ||
| self.telemetry.retire_stats(&self.state, None); | ||
| // Out of the call: uploads no longer yield to it. | ||
| self.state.server.lock().unwrap_or_else(|e| e.into_inner()).take(); |
There was a problem hiding this comment.
🟡 Repeated disconnects duplicate terminal events
Calling disconnected twice emits lk.room.disconnected twice because the method has no terminal-state check. A room's final disconnect is counted twice.
Learn more
The method clears pending subscribes and the server slot, then unconditionally emits a disconnect event. It never records whether it already emitted one for this scope. A repeated platform callback therefore produces duplicate final records rather than leaving the session terminal.
Example: The transport and Room cleanup paths both call disconnected(ClientInitiated) for the same scope. The queue receives two lk.room.disconnected records for one call.
Recommended fix: Add a per-scope terminal flag or equivalent one-time transition, set atomically before producing the event. Keep disconnect reporting available for failed connects that never had a server.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
The pipeline: the synchronous handle, one scope per Room, typed spans, and the exporter actor. The README becomes the crate docs (its usage example is a doc test) and
SPEC.mdis complete.Changes
Telemetry: the synchronous handle — flood guard, flush / shutdown / purge, statsScope: one Room — server URL and token, attributes, RTC stats, thelk.subscribelifecycleSpansExporter: the actor that encodes, caches and uploads under the upload policySPEC.mdcompleteWhy this stage is larger
The one stage over ~2k lines:
Telemetry ⇄ Shared ⇄ ExporterandScope ⇄ Span ⇄ Telemetryare one dependency cycle, so they land together. Their tests follow in #1484, together with the backend contract.Temporary
#[allow(dead_code)]onmod telemetry(weak_commandsservesglobal, #1485; test hooks serve #1484) and#[cfg_attr(test, allow(dead_code))]onmod span(open_count, #1485).Verification
At
c16912c3, 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 --all-features -- -D warningscargo check -p livekit-telemetry --all-targets --no-default-featureswith features[],[net],[uniffi],[net,uniffi]cargo test -p livekit-telemetry: 56 unit, 2 doc;--all-features: 57 unit, 2 doccargo doc -D warningsreports exactly what it reports on6aba1b68(private-item links,ExportError::from_response).