Conversation
📝 WalkthroughWalkthroughThis pull request adds comprehensive performance instrumentation to the LiveKit FFI server by integrating metrics logging across audio and video processing paths. It captures timing metrics for frame capture and forwarding operations, plus handle management, using histogram-based measurements without altering public APIs or core functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
ladvoc
force-pushed
the
ladvoc/ffi-perf
branch
from
aa937ee to
cb198f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
livekit-ffi/src/server/audio_source.rs (1)
77-121: Run rustfmt to fix the formatting failure.
CI reports a rustfmt error around Line 73; please runcargo fmtto resolve.
🤖 Fix all issues with AI agents
In `@livekit-ffi/Cargo.toml`:
- Line 40: The Cargo.toml adds metrics-logger = "0.22.1" which requires Rust
1.85+ but the crate lacks an explicit MSRV; update Cargo.toml to document the
requirement by adding rust-version = "1.85" in the package section, or
alternatively change the dependency to an older compatible metrics-logger
version (e.g., 0.21.x) to keep compatibility with older Rust toolchains—modify
either the package rust-version field or the metrics-logger version in
Cargo.toml accordingly.
In `@livekit-ffi/src/server/mod.rs`:
- Around line 15-29: The file has formatting issues flagged by CI; run rustfmt
(cargo fmt) on the file to normalize imports and spacing around the use
statements and items like AtomicU64, Ordering, DashMap, AudioProcessingModule,
AudioResampler, MetricsLogger and histogram; after running cargo fmt, re-check
that the use block and surrounding code (including any lines around the
referenced symbols) align with rustfmt’s output and commit the formatted file.
- Around line 104-109: The metrics recorder initialization in FfiServer::default
currently calls metrics::set_global_recorder(...).unwrap(), which panics if
invoked multiple times; wrap the recorder setup with a std::sync::Once (e.g., a
static ONCE) so the block that constructs
MetricsLogger::new(LogMode::Periodic(10), ...) and calls
metrics::set_global_recorder only runs once and ignores the "already set" error
instead of unwrapping; update FfiServer::default to use this Once guard when
initializing the global recorder to make repeated instantiation safe.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
livekit-ffi/Cargo.tomllivekit-ffi/src/server/audio_source.rslivekit-ffi/src/server/audio_stream.rslivekit-ffi/src/server/mod.rslivekit-ffi/src/server/video_source.rslivekit-ffi/src/server/video_stream.rs
🧰 Additional context used
🧬 Code graph analysis (2)
livekit-ffi/src/server/mod.rs (1)
livekit-ffi/src/server/logger.rs (2)
new(44-56)log(78-84)
livekit-ffi/src/server/audio_source.rs (3)
livekit-ffi/src/server/mod.rs (5)
std(230-230)std(256-256)handle(161-161)handle(229-229)handle(255-255)livekit-ffi/src/server/requests.rs (23)
std(831-831)std(848-848)std(870-870)std(909-909)std(919-919)std(941-941)std(951-951)server(70-70)server(87-87)server(98-98)server(110-110)server(121-121)server(133-133)server(144-144)server(159-159)server(174-174)server(189-189)server(207-208)server(219-219)server(228-229)server(239-240)server(250-251)server(261-262)libwebrtc/src/native/audio_source.rs (1)
std(130-130)
🪛 GitHub Actions: Rust Formatting
livekit-ffi/src/server/mod.rs
[error] 15-15: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
[error] 26-26: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
[error] 172-172: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
[error] 261-261: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
livekit-ffi/src/server/audio_source.rs
[error] 73-73: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
🔇 Additional comments (11)
livekit-ffi/src/server/video_source.rs (2)
15-20: No issues to call out here.
64-81: Timing instrumentation looks good.
Line 64 through Line 81 cleanly scopes the capture duration and reports it without altering control flow.livekit-ffi/src/server/audio_stream.rs (2)
16-23: No issues to call out here.
378-455: LGTM for the forward timing hook.
The added timing around frame handling is clean and doesn’t interfere with error paths.livekit-ffi/src/server/video_stream.rs (2)
15-23: No issues to call out here.
138-175: Timing instrumentation looks good.
Line 138 through Line 175 records forwarding duration without changing behavior.livekit-ffi/src/server/audio_source.rs (1)
15-18: No issues to call out here.livekit-ffi/src/server/mod.rs (4)
176-187: LGTM for send_event timing.
Line 176 through Line 187 adds clean instrumentation without side effects.
205-210: LGTM for store_handle timing.
The timing hook is minimal and safely scoped.
224-237: LGTM for retrieve_handle timing.
The instrumentation is correctly placed after validation.
265-272: LGTM for drop_handle timing.
No behavioral changes beyond metrics.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| console-subscriber = { version = "0.1", features = ["parking_lot"], optional = true } | ||
| bytes = "1.10.1" | ||
| from_variants = "1.0.2" | ||
| metrics-logger = "0.22.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
metrics-logger rust crate version 0.22.1 breaking changes MSRV
💡 Result:
metrics-logger v0.22.1: breaking changes?
No—0.22.1 is a patch release and (by the metrics ecosystem’s changelog) the corresponding metrics 0.22.1 change is a fix (pinning ahash to avoid an unintended MSRV bump), not an API break. [1]
If you’re upgrading from 0.21.x → 0.22.x, the breaking changes are effectively in 0.22.0, not 0.22.1 (e.g., API/behavior changes around recorder installation and related additions like local recorders). [1]
MSRV for metrics-logger v0.22.1
metrics-logger 0.22.1 uses Rust 2024 edition, which means you need Rust ≥ 1.85 (Rust 1.85.0 is what stabilized the 2024 edition). [2][3]
(And Rust’s own Rust-2024 guidance explicitly calls out using rust-version = "1.85" when setting edition = "2024".) [4]
Notes on metrics MSRV (if you care about the underlying facade too)
The metrics crate’s 0.22.0 release notes indicate MSRV was bumped to Rust 1.65.0 at that time. [1]
Sources:
[1] metrics CHANGELOG (0.22.0 / 0.22.1 entries) https://docs.rs/crate/metrics/latest/source/CHANGELOG.md
[2] metrics-logger package page (shows “Uses new Rust 2024”) https://lib.rs/crates/metrics-logger
[3] Rust 1.85.0 announcement (“stabilizes the 2024 edition”) https://blog.rust-lang.org/2025/02/20/Rust-1.85.0/
[4] Rust 2024 beta post (mentions rust-version = "1.85" for edition = "2024") https://blog.rust-lang.org/2025/01/22/rust-2024-beta/
🏁 Script executed:
cat livekit-ffi/Cargo.tomlRepository: livekit/rust-sdks
Length of output: 1670
🏁 Script executed:
cat Cargo.tomlRepository: livekit/rust-sdks
Length of output: 1534
Verify MSRV compatibility: metrics-logger 0.22.1 requires Rust 1.85+
metrics-logger 0.22.1 is a stable patch release with no breaking changes, but it uses Rust 2024 edition, which requires Rust ≥ 1.85.0. Since livekit-ffi has no explicit rust-version declared, this creates a silent MSRV requirement that consumers may not be aware of. Either explicitly set rust-version = "1.85" in Cargo.toml to document this requirement, or consider using an earlier metrics-logger version (e.g., 0.21.x) if targeting older Rust versions.
🤖 Prompt for AI Agents
In `@livekit-ffi/Cargo.toml` at line 40, The Cargo.toml adds metrics-logger =
"0.22.1" which requires Rust 1.85+ but the crate lacks an explicit MSRV; update
Cargo.toml to document the requirement by adding rust-version = "1.85" in the
package section, or alternatively change the dependency to an older compatible
metrics-logger version (e.g., 0.21.x) to keep compatibility with older Rust
toolchains—modify either the package rust-version field or the metrics-logger
version in Cargo.toml accordingly.
| use std::{ | ||
| error::Error, | ||
| sync::{ | ||
| atomic::{AtomicU64, Ordering}, | ||
| Arc, | ||
| Arc, atomic::{AtomicU64, Ordering} | ||
| }, | ||
| thread, | ||
| time::Duration, | ||
| time::{Duration, Instant}, | ||
| }; | ||
|
|
||
| use dashmap::{mapref::one::MappedRef, DashMap}; | ||
| use downcast_rs::{impl_downcast, Downcast}; | ||
| use livekit::webrtc::{ | ||
| native::apm::AudioProcessingModule, native::audio_resampler::AudioResampler, prelude::*, | ||
| }; | ||
| use metrics_logger::{LogMode, MetricsLogger, metrics::{self, histogram}}; |
There was a problem hiding this comment.
Run rustfmt for this file.
CI reports formatting issues at Lines 15/26/172/261; please run cargo fmt to address.
🧰 Tools
🪛 GitHub Actions: Rust Formatting
[error] 15-15: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
[error] 26-26: cargo fmt -- --check detected formatting issues in this file. Run 'cargo fmt' to fix code style.
🤖 Prompt for AI Agents
In `@livekit-ffi/src/server/mod.rs` around lines 15 - 29, The file has formatting
issues flagged by CI; run rustfmt (cargo fmt) on the file to normalize imports
and spacing around the use statements and items like AtomicU64, Ordering,
DashMap, AudioProcessingModule, AudioResampler, MetricsLogger and histogram;
after running cargo fmt, re-check that the use block and surrounding code
(including any lines around the referenced symbols) align with rustfmt’s output
and commit the formatted file.
| let recorder = MetricsLogger::new( | ||
| LogMode::Periodic(10), | ||
| |logs| log::info!(target: "metrics", "{}", logs), | ||
| |err| log::error!(target: "metrics", "{}", err), | ||
| ); | ||
| metrics::set_global_recorder(recorder).unwrap(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in the file
find . -name "mod.rs" -path "*/livekit-ffi/src/server/*" | head -5Repository: livekit/rust-sdks
Length of output: 133
🏁 Script executed:
# Find the file and examine the context around lines 104-109
find . -path "*/livekit-ffi/src/server/mod.rs" -type fRepository: livekit/rust-sdks
Length of output: 92
🌐 Web query:
metrics crate Rust set_global_recorder behavior multiple calls panic
💡 Result:
In the metrics crate, set_global_recorder is single-assignment for the lifetime of the process: only the first successful call installs the global recorder; any later call does not replace it and instead returns an error. (docs.rs)
- Behavior on multiple calls: the second (and subsequent) call returns
Err(SetRecorderError(recorder_you_tried_to_set)). (docs.rs) - Panic?
metrics::set_global_recorderitself does not panic; panics typically happen because caller code (or an exporter’sinstall()/build_and_install()helper) doesset_global_recorder(...).unwrap()/.expect(...). (So “panic on multiple calls” is a usage pattern, not the API contract.) (docs.rs) - Also true for
set_recorder(&'static dyn Recorder): it’s likewise “only once” and returns an error if already set. (prisma.github.io) - If you need different recorders in tests/scoped code: use
with_local_recorderinstead of trying to reset the global recorder. (docs.rs)
Citations:
- 1: https://docs.rs/metrics/latest/metrics/fn.set_global_recorder.html?utm_source=openai
- 2: https://docs.rs/metrics/latest/metrics/fn.set_global_recorder.html?utm_source=openai
- 3: https://docs.rs/metrics/latest/metrics/fn.set_global_recorder.html?utm_source=openai
- 4: https://prisma.github.io/prisma-engines/doc/metrics/fn.set_recorder.html?utm_source=openai
- 5: https://docs.rs/metrics/latest/metrics/?utm_source=openai
🏁 Script executed:
# View the actual code around lines 104-109
sed -n '100,115p' ./livekit-ffi/src/server/mod.rsRepository: livekit/rust-sdks
Length of output: 662
🏁 Script executed:
# Search for FfiServer definition and default implementation
rg "impl.*Default.*FfiServer|fn default" ./livekit-ffi/src/server/mod.rs -A 20Repository: livekit/rust-sdks
Length of output: 933
🏁 Script executed:
# Search for FfiServer::default() calls in the codebase
rg "FfiServer::default|FfiServer\(\)" --type rustRepository: livekit/rust-sdks
Length of output: 164
🏁 Script executed:
# Look for test files that might call FfiServer multiple times
fd "test|spec" --type f --path "*livekit-ffi*" | head -20Repository: livekit/rust-sdks
Length of output: 294
🏁 Script executed:
# Search for all FfiServer usage patterns
rg "FfiServer" --type rust -B 2 -A 2Repository: livekit/rust-sdks
Length of output: 47577
🏁 Script executed:
# Look for test files
find . -name "*.rs" -type f | xargs grep -l "mod test\|#\[test\]\|#\[cfg(test)\]" | grep -i ffiRepository: livekit/rust-sdks
Length of output: 126
🏁 Script executed:
# Check lib.rs to see the lazy_static usage
sed -n '1,50p' ./livekit-ffi/src/lib.rs | head -50Repository: livekit/rust-sdks
Length of output: 1481
Guard metrics recorder initialization against multiple calls to FfiServer::default().
metrics::set_global_recorder(...).unwrap() panics if called more than once. While the current codebase uses lazy_static! to ensure single initialization, the Default implementation is public and could be instantiated multiple times in tests or if the initialization pattern changes in the future. Use std::sync::Once to guard the initialization and ignore the "already set" error:
Suggested fix
-use std::{
+use std::{
error::Error,
sync::{
- Arc, atomic::{AtomicU64, Ordering}
+ Arc, atomic::{AtomicU64, Ordering}, Once
},
thread,
time::{Duration, Instant},
};
+
+static METRICS_INIT: Once = Once::new();
@@
- let recorder = MetricsLogger::new(
- LogMode::Periodic(10),
- |logs| log::info!(target: "metrics", "{}", logs),
- |err| log::error!(target: "metrics", "{}", err),
- );
- metrics::set_global_recorder(recorder).unwrap();
+ METRICS_INIT.call_once(|| {
+ let recorder = MetricsLogger::new(
+ LogMode::Periodic(10),
+ |logs| log::info!(target: "metrics", "{}", logs),
+ |err| log::error!(target: "metrics", "{}", err),
+ );
+ let _ = metrics::set_global_recorder(recorder);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let recorder = MetricsLogger::new( | |
| LogMode::Periodic(10), | |
| |logs| log::info!(target: "metrics", "{}", logs), | |
| |err| log::error!(target: "metrics", "{}", err), | |
| ); | |
| metrics::set_global_recorder(recorder).unwrap(); | |
| METRICS_INIT.call_once(|| { | |
| let recorder = MetricsLogger::new( | |
| LogMode::Periodic(10), | |
| |logs| log::info!(target: "metrics", "{}", logs), | |
| |err| log::error!(target: "metrics", "{}", err), | |
| ); | |
| let _ = metrics::set_global_recorder(recorder); | |
| }); |
🤖 Prompt for AI Agents
In `@livekit-ffi/src/server/mod.rs` around lines 104 - 109, The metrics recorder
initialization in FfiServer::default currently calls
metrics::set_global_recorder(...).unwrap(), which panics if invoked multiple
times; wrap the recorder setup with a std::sync::Once (e.g., a static ONCE) so
the block that constructs MetricsLogger::new(LogMode::Periodic(10), ...) and
calls metrics::set_global_recorder only runs once and ignores the "already set"
error instead of unwrapping; update FfiServer::default to use this Once guard
when initializing the global recorder to make repeated instantiation safe.
1 participant
Measures performance of the following in production:
This uses the metrics and metrics-logger crates. To see output, enable logging for metrics:
RUST_LOG=metrics=debug. This does not log every individual data point but rather calculates aggregate statistics and logs every 10 seconds.Note: this is intended to diagnose a specific performance issue and is not meant to be merged as-is.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.