moq-ffi: route Android logs to logcat#1541
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds an 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rs/moq-ffi/src/log.rs (1)
53-53: ⚡ Quick winUse
try_init()instead ofinit()for the Android tracing subscriber to avoid panicking when a global subscriber is already set.
SubscriberInitExt::init()panics if a global subscriber is already installed;try_init()returns an error you can map intoMoqError::Log. The crate-levelAtomicBoolinmoq_log_levelonly prevents re-callingmoq_log_level, not initialization performed by the host before this function runs.♻️ Proposed change
- tracing_subscriber::registry().with(logcat_layer).init(); - - Ok(()) + tracing_subscriber::registry() + .with(logcat_layer) + .try_init() + .map_err(|err| MoqError::Log(format!("failed to install tracing subscriber: {err}")))?; + + Ok(())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-ffi/src/log.rs` at line 53, The call to tracing_subscriber::registry().with(logcat_layer).init() can panic if a global subscriber is already set; change this to use SubscriberInitExt::try_init() (i.e. tracing_subscriber::registry().with(logcat_layer).try_init()) and map/propagate the returned Err into MoqError::Log so initialization failures become a handled error instead of a panic; keep existing guard logic (moq_log_level AtomicBool) but do not rely on it to prevent host-installed subscribers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-ffi/src/log.rs`:
- Around line 20-26: The code sets INITIALIZED to true before calling
init_logging, which permanently blocks retries if init_logging fails; change the
flow so that either (a) set INITIALIZED only after init_logging returns Ok (use
INITIALIZED.store(true, Ordering::SeqCst) after successful init) or (b) if you
keep the current swap/check pattern, ensure you reset the flag on failure by
calling INITIALIZED.store(false, Ordering::SeqCst) before returning the Err from
init_logging; update the branch around INITIALIZED.swap(true, Ordering::SeqCst)
and the init_logging(level)? error path (referencing INITIALIZED, init_logging,
Ordering::SeqCst, and MoqError::Log) so the flag reflects the real initialized
state.
---
Nitpick comments:
In `@rs/moq-ffi/src/log.rs`:
- Line 53: The call to tracing_subscriber::registry().with(logcat_layer).init()
can panic if a global subscriber is already set; change this to use
SubscriberInitExt::try_init() (i.e.
tracing_subscriber::registry().with(logcat_layer).try_init()) and map/propagate
the returned Err into MoqError::Log so initialization failures become a handled
error instead of a panic; keep existing guard logic (moq_log_level AtomicBool)
but do not rely on it to prevent host-installed subscribers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1087238-d5d4-4266-9608-616e77a62fb9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rs/moq-ffi/Cargo.tomlrs/moq-ffi/src/log.rs
| #[cfg(all(target_os = "android", feature = "android-logcat"))] | ||
| fn init_logging(level: Level) -> Result<(), MoqError> { |
There was a problem hiding this comment.
Should this be in moq-native instead?
| let logcat_layer = tracing_android::layer("MoQNative") | ||
| .map_err(|err| MoqError::Log(format!("failed to initialize Android logcat layer: {err}")))? | ||
| .with_filter(filter); |
There was a problem hiding this comment.
If we move this to moq-native, this is the #[cfg] gate, while the rest is shared.
Per review, the Android-specific logging belongs in moq-native rather than moq-ffi. The EnvFilter setup is now shared, and only the layer (logcat on Android, stderr fmt elsewhere) is behind the cfg gate. moq-ffi's android-logcat feature forwards to moq-native. Log::init now returns a Result so the logcat layer's initialization error propagates instead of being swallowed, and moq_log_level resets its INITIALIZED guard when init fails so a transient failure doesn't permanently block retries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the directive .parse().unwrap() calls with ? and swap registry.init() for try_init(), so a parse error or an already-set global subscriber surfaces as an error instead of panicking. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 participants
While on iOS the logs can be inspected in xcode the same thing can't be said about android. For this particular usecase, this PR adds support for a android locat tracing subscriber so the logs are properly routed and can be inspected using ADB or Android Studio (both using logcat).