Repository navigation
Conversation
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.
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 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.
e8facef to
2812b2d
Compare
`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.
A non-Cloud server URL now gets a routing key of its own (scheme, host and port), so a plaintext or userinfo URL on a Cloud host can no longer revive that project with its token or silence it. `alive` takes the session, so a Room without a server waits for its first project instead of being judged as process data. The test override swaps only a whole `logs` path segment. The project table drops entries that no live session or cached batch needs, except a project known to receive nothing (not Cloud, no grant, 404, disabled). A Cloud host must be a DNS hostname, and every other key escapes its host and port, so a key is a valid file name on every platform: it ends each cached batch's id. The `Server` and `set` docs describe both kinds of key. A Room's first project is recorded even when that project receives nothing, so what the Room captured before its first server is dropped with it instead of moving to a later project. The Cloud "disabled" phrase is matched on the whole error body, and only the logged reason is cut, to a named limit. `ExportError` loses `Disabled`, because a transport must not interpret the answer. `NetTransport::new` is crate-private, the public exports are listed, and `ExportResponse` and `Token` get `From` conversions. Doc comments now name the real classifier, the real body types and the real redirect behavior, and every public field has a doc line. The README names the transport and is now the crate doc, and a changeset records the stage. The `Retry-After` date parser checks the day against the length of the month, leap years included, and the retry verdict names 500 with `RetryInfo` as the only retryable 500. The native `livekit-net` HTTP client keeps its limit of 10 redirects but no longer follows one that changes the scheme, so `Authorization` never reaches a plaintext URL on the same host and port; a `livekit-net` changeset records it. In the code the crate first shipped with, the queue-full warning is logged after the queue lock, a pipeline attribute can no longer add a second `otel.event.name`, a record ships each attribute key once (of a key the caller repeats, the last value wins; the session's keys still replace the caller's), and report counts saturate at `i64::MAX` instead of wrapping. Finished spans wait in a `VecDeque`, `reserved` is `is_reserved`, the severity text is `Severity::as_str`, and `Spans::dropped` and `SpanKind` are crate-private. Every exported field and variant has a doc line, the limits and the size of a number are named constants, the duplicated wire values and the test hooks say why they exist, and the docs and SPEC state the real log floor, trace id, failure count and `queue_full` meaning.
2812b2d to
3e68d91
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| let mut safe = String::with_capacity(text.len()); | ||
| for byte in text.bytes() { | ||
| match byte { | ||
| b'a'..=b'z' | b'0'..=b'9' | b'.' | b'-' | b'_' => safe.push(byte as char), |
There was a problem hiding this comment.
🟡 Absolute hostnames break Windows cache filenames
For ws://localhost., file_safe leaves the routing key ending in a dot. Cached batch filenames end with that key, so Windows cannot store those batches.
Learn more
A trailing dot is valid in an absolute DNS hostname. Server::parse preserves it in a non-Cloud routing key, and the key ends the cached batch filename. Windows does not permit file names ending with a period, despite permitting periods elsewhere.
Example: set("ws://localhost.", token, "room") returns ws+localhost.. A batch named with that suffix cannot be written to the Windows cache.
Recommended fix: Escape or otherwise encode a terminal period in routing keys while keeping their server identity unambiguous. Test terminal periods as well as reserved characters in the filename check.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
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 credentialsExportErrorhas noDisabledvariantlivekit-netclient no longer follows a redirect that changes the scheme, soAuthorizationnever reaches a plaintext URLotel.event.name, and report counts saturate instead of wrappingVerification
At
3e68d917, from a clean checkout (CI's test workflow also runs on this PR because it targetsmain; 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: 56 unit, 0 doccargo test -p livekit-net --features native, and itscargo hack --feature-powerset --depth 2clippy runRUSTDOCFLAGS="-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