Conversation
`TelemetryTransport` moves encoded batches off the device (`NetTransport` over `livekit-net` behind the `net` feature) and returns status, headers and body; the core classifies every answer (partial success, 4xx, 413, 401/403, 404, 429/503 `Retry-After` and `RetryInfo`, 5xx, no answer). Destinations turn a Room's server URL and token into the ingest URL (LiveKit Cloud hosts only, parsed, never string-matched), read the token's unverified grant and expiry, and key credentials by project and session. `LK_TELEMETRY_ENDPOINT` points every upload at a local collector.
pblazej
added
the
internal
e87c77f to
e8facef
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 5 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| [features] | ||
| # Default HTTP transport over the pluggable `livekit-net` client (native backend, or one | ||
| # the host registered with `livekit_net::set_http_client`). | ||
| net = ["dep:livekit-net"] |
There was a problem hiding this comment.
🔴 Default network transport cannot start
With only net enabled, NetTransport::from_registry() returns None unless the host registers a client. livekit-net has no native backend enabled by default, leaving these builds unable to export telemetry.
Learn more
The net feature activates the dependency but not its native backend. In a build that selects only this crate's net feature, http_client returns None unless an application called set_http_client, because native is disabled by default. NetTransport::from_registry therefore cannot construct the transport promised by this feature.
Example: An application enables livekit-telemetry/net on a native target without registering an HTTP client. from_registry() returns None instead of a built-in client, so no default exporter can send its batches.
Recommended fix: Arrange for the native livekit-net backend and an appropriate TLS backend to be enabled for native default-transport builds, while retaining an explicit registered-client path for host-provided builds. Validate the standalone net feature without another dependency enabling livekit-net/native.
Was this helpful? React with 👍 or 👎 to provide feedback.
| pub fn alive(&self, host: Option<&str>) -> bool { | ||
| self.endpoint_override.is_some() || !matches!(self.route(host, None), Route::Drop) | ||
| } |
There was a problem hiding this comment.
🔴 New Room loses pre-connection records
When all known projects are dead, alive(None) rejects records from a new Room before it sets a server. route treats the missing host as process data, so those records cannot reach the Room's eventual project.
Learn more
A Room captures records before its server is known; those records must wait for that Room's first project. route distinguishes an unconnected Room from process data by its owner, but alive supplies neither host nor owner. When all previously seen projects are dead, the process-data route returns Drop. Applying this result at collection time discards the new Room's records before it can connect.
Example: Room A connects to ws://localhost:7880, leaving the only known project dead. Room B emits an event before connecting to wss://b.livekit.cloud; alive(None) returns false and the event is lost instead of waiting for B's first project.
Recommended fix: Make the collection-time check owner-aware, or treat an unassigned Room route as waiting rather than using the process-level fallback. Keep the existing process-level dead-project behavior separately.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let endpoint = endpoint.trim_end_matches('/'); | ||
| let (logs, traces) = match endpoint.rsplit_once("logs") { | ||
| Some((before, after)) => (endpoint.to_owned(), format!("{before}traces{after}")), | ||
| None => (format!("{endpoint}/v1/logs"), format!("{endpoint}/v1/traces")), | ||
| }; |
There was a problem hiding this comment.
🟡 Collector override rewrites unrelated URLs
When a base URL contains logs in its hostname, override_target mistakes it for a logs endpoint. Both signal URLs become wrong; the traces URL even points to a different hostname.
Learn more
The override accepts either a base URL or a full logs URL. rsplit_once("logs") searches the entire string, not the URL's final path segment, so a base URL with logs in its hostname never gets /v1/logs or /v1/traces appended. The traces replacement can also change the hostname.
Example: LK_TELEMETRY_ENDPOINT=http://logs-collector:4318 yields logs at http://logs-collector:4318 and traces at http://traces-collector:4318, rather than two /v1/ paths on logs-collector.
Recommended fix: Parse the override as a URL and recognize a full logs endpoint only when its final path segment is logs; otherwise append the two OTLP paths to the base URL.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if !(1970..=9999).contains(&year) | ||
| || hms.next().is_some() | ||
| || !(1..=31).contains(&day) | ||
| || !(0..24).contains(&h) | ||
| || !(0..60).contains(&m) | ||
| || !(0..61).contains(&s) | ||
| { | ||
| return None; |
There was a problem hiding this comment.
🟡 Invalid retry dates delay uploads
When Retry-After names February 30, imf_fixdate_secs converts it into a valid later date. A malformed 429 response then pauses uploads until that date instead of using the default delay.
Learn more
retry_after_ms passes an IMF-fixdate to this parser, and Verdict::of honors the resulting delay for a 429. The parser bounds the day to 31 without checking its month or leap year. Its civil-date arithmetic then silently rolls an impossible date forward, treating malformed server input as a real pause. It also accepts hour 24 and minute 60.
Example: On February 1, Retry-After: Mon, 30 Feb 2027 12:00:00 GMT resolves to a date in March, rather than being ignored in favor of the 429 default delay.
Recommended fix: Validate the day against the selected month's length and leap-year rules, and require hours 0–23 and minutes 0–59 before computing Unix seconds.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if project.dead.is_none() && !cloud { | ||
| project.dead = Some(Dead::NotCloud); | ||
| log::warn!("{host} is not LiveKit Cloud: client telemetry stays on this device"); | ||
| } | ||
| if matches!(project.dead, Some(Dead::NotCloud | Dead::Disabled)) { | ||
| return Some(host); |
There was a problem hiding this comment.
🟥 Plaintext server URLs inherit Cloud eligibility
After a secure connection registers a Cloud host, set accepts a ws:// URL for that host without revoking its Cloud status. That room's token can then be sent to the Cloud ingest despite its server URL failing TLS validation.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
How batches leave the device and where they go. The core classifies every answer itself; the backend contract, row by row with its tests, is in #1484.
Changes
TelemetryTransport: moves encoded batches and returns status, headers and bodyNetTransportoverlivekit-net(featurenet)LK_TELEMETRY_ENDPOINTpoints uploads at a local collectorSPEC.md: Destination and credentialsVerification
At
e8facef6, 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: 26 unit;--all-features: 27 unitcargo doc -D warningsreports unresolved links toTelemetry::stats(#1483 adds it) andExportError::from_response(unresolved on6aba1b68too).