Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
Add `livekit-telemetry`: the records SDKs push in (events, log records, attributes), the bounded in-memory queue, health counters and the `lk.telemetry.report` event, the span store, a session's identity (trace id, SDK and app attributes, route), and the OTLP/HTTP protobuf encoding of logs and traces on `opentelemetry-proto` message types.
ddc7bc2 to
fc0ef58
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.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| name = "livekit-telemetry" | ||
| description = "Client telemetry core for LiveKit: buffers events on-device and exports them as OTLP" | ||
| version = "0.1.0" | ||
| readme = "README.md" |
There was a problem hiding this comment.
🔴 Missing README blocks crate publishing
When packaging livekit-telemetry, Cargo cannot find the declared README.md. The crate cannot be published.
Learn more
Cargo uses the package's readme entry to include its README in the published archive. The new crate declares README.md, but its directory contains only CHANGELOG.md, SPEC.md, Cargo.toml, src, and uniffi.toml. Packaging therefore fails before a release can publish the crate.
Example: A release job packages livekit-telemetry at version 0.1.0. Cargo looks for livekit-telemetry/README.md, fails to find it, and does not produce a package.
Recommended fix: Add the declared README to the crate, or remove the explicit readme setting if the crate intentionally ships without one. Validate with cargo package -p livekit-telemetry.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| fn log_record(Queued { mut event, session, .. }: Queued, global: &[Attribute]) -> LogRecord { | ||
| session.decorate(&mut event.attributes, global); | ||
| let time_unix_nano = event.timestamp_ns.unwrap_or_else(now_unix_nanos); |
There was a problem hiding this comment.
🔴 Buffered events acquire upload timestamps
When timestamp_ns is absent, log_record timestamps the event during encoding, not capture. Buffered events appear at upload time instead of emission time.
Learn more
Events enter the bounded queue through Queued::new, which preserves an absent timestamp. log_record runs only when the queue is encoded for export. Any buffering interval therefore shifts event timestamps, even though TelemetryEvent::timestamp_ns promises an emit-time stamp for None.
Example: An event emitted at 12:00 remains queued during a network outage until 12:30. Its exported timestamp is 12:30, so it appears after operations that actually occurred later.
Recommended fix: Fill a missing timestamp_ns when capturing the event in Queued::new, while preserving caller-provided timestamps. Encoding must use that stored capture time.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.session_order.len() > REMEMBERED_SPANS { | ||
| if let Some(old) = self.session_order.pop_front() { | ||
| self.sessions.remove(&old); | ||
| } |
There was a problem hiding this comment.
🔴 Long-lived spans lose session identity
After 1,024 newer spans begin, begin_in evicts an open span from sessions. A later scope_of lookup loses that span's room identity.
Learn more
The registry stores span handles in both open and sessions; scope_of reads only sessions to identify the owning room. The 1,024-entry FIFO discards the oldest session mapping without checking whether its span remains open. The open span still accepts checkpoints and can end normally, but subsequent logs can no longer resolve its session.
Example: Room A starts a long connect span. Other rooms collectively start 1,024 more spans before A logs a connect error. scope_of returns None for A's still-open span instead of Room A.
Recommended fix: Retain mappings for all open spans. Apply the fixed retention limit only to ended spans, removing open mappings after end only when they expire from the recent-span cache.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn set_custom(&self, key: &str, value: Option<AttributeValue>) -> bool { | ||
| if !self.accepts_custom(key, value.as_ref()) { | ||
| return false; | ||
| } | ||
| let mut custom = self.custom.lock().unwrap_or_else(|e| e.into_inner()); | ||
| custom.retain(|a| a.key != key); | ||
| if let Some(value) = value { | ||
| custom.push(Attribute::new(key, value)); | ||
| } |
There was a problem hiding this comment.
🟡 Concurrent updates exceed custom attribute cap
When two threads add distinct keys at 63 attributes, set_custom checks capacity before taking its insertion lock. Both can succeed, leaving 65 attributes.
Learn more
accepts_custom takes and releases the custom-attribute mutex before set_custom takes it to insert. Two callers can both observe available space and each insert a different key. The limit is intended to bound attributes copied into every queued record.
Example: A scope holds 63 attributes. Threads A and B both pass accepts_custom for different new keys before either inserts; both return true, and the scope now holds 65 attributes.
Recommended fix: Validate the value and check for an existing key or remaining capacity while holding the same mutex used for the mutation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if self.finished.len() >= self.finished_capacity { | ||
| self.finished.remove(0); | ||
| self.dropped += 1; |
There was a problem hiding this comment.
🟡 Zero span capacity panics on completion
With finished_capacity set to zero, end calls remove(0) on an empty vector. The first completed span panics.
Learn more
Spans::new accepts any usize capacity. For zero, an empty finished vector already meets the >= guard, so remove(0) panics rather than discarding the completed span.
Example: Construct Spans::new(0), begin one span, then end it. The registry panics while trying to evict a nonexistent previous span.
Recommended fix: Handle zero capacity explicitly in end, counting the just-finished span as dropped without indexing the empty buffer, or reject zero capacity when constructing the registry.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let mut attributes: Vec<KeyValue> = record.attributes.iter().map(KeyValue::from).collect(); | ||
| attributes.extend(record.outcome_attributes().iter().map(KeyValue::from)); |
There was a problem hiding this comment.
🟡 Duplicate span outcome corrupts rollups
When end receives an lk.outcome attribute, otlp_span exports that value alongside the computed outcome. Backends can read the wrong outcome.
Learn more
Spans::end accepts caller attributes without removing lk.outcome or error.type. otlp_span converts those attributes before adding the authoritative outcome attributes, creating duplicate OTLP keys. Backends that select the first value can treat a failed span as successful.
Example: End an error span with Attribute::new("lk.outcome", "ok"). The exported span contains both lk.outcome=ok and lk.outcome=error, so an outcome rollup can count it as successful.
Recommended fix: Remove caller-supplied reserved outcome keys before encoding, then append exactly one lk.outcome and, when present, one authoritative error.type.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Adds
livekit-telemetry, the shared client-telemetry core, starting with its data model and the OTLP encoding.Changes
lk.telemetry.reporteventScopeState(trace id, SDK and app attributes, route)opentelemetry-prototypesSPEC.md(resource attributes, events, log records, spans) and the crate changeset — the only PR the changeset check seesTemporary
Until #1483: a crate-level
#![allow(dead_code)]for internals the pipeline consumes, andScopeStatewithout subscribe tracking (#1483 completesscope.rs).Architecture
lk.outcomecarriesok | error | cancelled.getStats()report per peer connection) and polling cadence.TelemetryTransportforeign trait (Swift/Kotlin), a bounded pull queue (Dart, whose callbacks are isolate-bound: polled withtry_next()from a timer, no instruments passed, so Rust never calls into Dart), orNetTransportover the unmodifiedlivekit-netregistry.Verification
At
fc0ef589, 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[],[uniffi]cargo test -p livekit-telemetry: 6 unit;--all-features: 6 unitcargo doc -D warningsreports one unresolved link, toTelemetry::stats, which #1483 adds.