feat(uniffi): add logging callback for mobile observability #627
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
🔍 SDK Breaking Change Detection ResultsSDK Version:
Breaking change detection completed. View SDK workflow |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
==========================================
+ Coverage 78.87% 79.06% +0.18%
==========================================
Files 283 285 +2
Lines 29680 29810 +130
==========================================
+ Hits 23410 23569 +159
+ Misses 6270 6241 -29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
addisonbeck
force-pushed
the
PM-27800-expose-sdk-tracing-flight-recorder
branch
6 times, most recently
from
cee9fbf to
b072562
Compare
addisonbeck
force-pushed
the
PM-27800-expose-sdk-tracing-flight-recorder
branch
2 times, most recently
from
2efef50 to
26a4f0a
Compare
Adds an optional LogCallback trait that mobile clients can implement to receive SDK log events for forwarding to Flight Recorder or other observability systems. The callback integrates with the existing tracing infrastructure from PM-26930 using a custom Layer that forwards INFO+ events to registered callbacks. The implementation includes CallbackLayer for event forwarding, MessageVisitor for extracting log data, and error handling to ensure callback failures don't crash the SDK. The callback parameter is optional to maintain backward compatibility with existing clients. Comprehensive test coverage includes unit tests and integration tests in separate processes to validate thread safety, error handling, and multi-level log forwarding without Once initialization conflicts.
Adds a reusable GitHub action that removes unused toolchains and packages before running tests on ubuntu-24.04 runners. The cleanup frees approximately 30GB by removing .NET SDKs, Android SDK, Swift toolchain, Java JDKs, and other pre-installed software not needed for Rust compilation. This prevents "No space left on device" errors that occur when compiling the integration test suite alongside the full SDK workspace. The action runs only on Linux runners since macOS and Windows have sufficient disk space.
addisonbeck
force-pushed
the
PM-27800-expose-sdk-tracing-flight-recorder
branch
from
26a4f0a to
68a6590
Compare
addisonbeck
changed the title
|
Claude finished @addisonbeck's task in 5m 14s —— View job Overall Assessment: REQUEST CHANGES Important Issues:
Questions for clarification:
See inline comments for details. The implementation demonstrates solid engineering with comprehensive tests, excellent documentation, and proper thread safety. The main issue is the message formatting that will affect mobile display. The questions raised are about intended behavior rather than bugs. Strengths:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| impl tracing::field::Visit for MessageVisitor { | ||
| fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { | ||
| if field.name() == "message" { | ||
| self.message = format!("{:?}", value); |
There was a problem hiding this comment.
❌ CRITICAL: Debug formatting adds extra quotes to log messages
Details and fix
The format!("{:?}", value) call wraps string messages in double quotes due to Debug formatting. For example, tracing::info!("User logged in") produces "\"User logged in\"" instead of "User logged in".
This impacts mobile teams consuming these logs - they'll need to strip quotes or the logs will display incorrectly in Flight Recorder.
Recommended fix:
fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) {
if field.name() == "message" {
self.message = format!("{}", value); // Use Display instead of Debug
}
}However, this requires that all logged values implement Display, which may not always be true. A more robust approach:
use tracing::field::Visit;
impl Visit for MessageVisitor {
fn record_str(&mut self, field: &tracing::field::Field, value: &str) {
if field.name() == "message" {
self.message = value.to_string();
}
}
fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) {
if field.name() == "message" && self.message.is_empty() {
// Fallback for non-string types
self.message = format!("{:?}", value);
}
}
}This prioritizes record_str for string messages (avoiding quotes) while falling back to record_debug for other types.
Testing impact: All test assertions checking message contents may need updating if the format changes.
Reference: tracing::field::Visit documentation
There was a problem hiding this comment.
This doesn't match what I've seen running the tests and demo, but @dereknance and @dani-garcia have a look at this one and let me know if you think it is useful.
| impl From<uniffi::UnexpectedUniFFICallbackError> for BitwardenError { | ||
| fn from(_: uniffi::UnexpectedUniFFICallbackError) -> Self { | ||
| Self::CallbackError |
There was a problem hiding this comment.
Details and fix
The From<UnexpectedUniFFICallbackError> implementation discards the underlying error details from the mobile callback. UnexpectedUniFFICallbackError contains a reason field describing what went wrong, but it's dropped when converting to CallbackError.
Current behavior:
impl From<uniffi::UnexpectedUniFFICallbackError> for BitwardenError {
fn from(_: uniffi::UnexpectedUniFFICallbackError) -> Self {
Self::CallbackError // Loses error details
}
}Impact: When debugging callback failures, the platform logger at log_callback.rs:48 only shows "CallbackError" without indicating whether it was a panic, exception type mismatch, or other UNIFFI bridging issue.
Recommended fix:
impl From<uniffi::UnexpectedUniFFICallbackError> for BitwardenError {
fn from(e: uniffi::UnexpectedUniFFICallbackError) -> Self {
Self::Conversion(format!("Callback invocation failed: {}", e.reason))
}
}This preserves diagnostic information while maintaining security (the reason field is SDK-generated, not from mobile code, so no sensitive data risk).
Alternative: If CallbackError should remain as-is for API stability, at least log the details before conversion:
if let Err(e) = self.callback.on_log(level, target, message) {
tracing::error!(target: "bitwarden_uniffi::log_callback",
"Logging callback failed: {:?}", e); // This captures it
}The current code at line 48 already does this, so the information isn't completely lost—it just doesn't propagate through the error type.
There was a problem hiding this comment.
We'll let mobile decide if they need this right now.
|
@dereknance I added @dani-garcia as a reviewer because this is the first rust I've committed and it ended up being more complex than I was feeling comfortable with. I'd appreciate double review coverage if y'all don't mind. After reviews from Platform I'll include mobile to make sure this design and its current limitations meet their needs. |
🎟️ Tracking
📔 Objective
This PR introduces an optional UNIFFI logging callback that enables mobile clients to receive SDK trace events and forward them to Flight Recorder observability systems.
The changes add a new
LogCallbacktrait that allows mobile teams to register a callback duringClient::new()initialization. When registered, the SDK forwards all INFO+ log events through this callback alongside existing platform loggers (oslog, android_logger). The callback is completely optional - existing SDK clients continue functioning without code changes.Documentation
Please review
crates/bitwarden-uniffi/docs/logging-callback.mdfor comprehensive details including:The documentation is the authoritative reference for this feature. You can also find a tech breakdown linked in the Tracking section.
Side Effect: CI Disk Cleanup
This PR also adds a disk cleanup action for Ubuntu test runners (
.github/actions/free-ubuntu-runner-disk-space/). The integration test suite compiles 5 separate test binaries alongside the full SDK workspace, requiring a lot of disk space. Without cleanup, Ubuntu runners exhaust available disk during compilation.Demo
Also included is an example showing how to use the callback. It, alongside the tests, are the assertions that the whole thing works. Here is a screenshot showing the output of the example:
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes