Skip to content

android tracing filter#3358

Open
insipx wants to merge 1 commit intomainfrom
insipx/android-trace
Open

android tracing filter#3358
insipx wants to merge 1 commit intomainfrom
insipx/android-trace

Conversation

@insipx
Copy link
Copy Markdown
Contributor

@insipx insipx commented Mar 26, 2026

Add Android-specific tracing filter for send_message and MLS sync paths

  • Replaces the single xmtp_api=debug directive with a multi-target filter defined in the new ANDROID_TRACING_FILTER constant in logger.rs, enabling TRACE for xmtp_mls::groups::send_message and send_message_optimistic, and DEBUG for xmtp_mls::mls_sync and xmtp_api_d14n.
  • Raises the compile-time tracing ceiling in bindings/mobile/Cargo.toml from release_max_level_debug to release_max_level_trace so TRACE-level events are not elided at compile time in release builds.
  • Behavioral Change: Android release builds will now emit TRACE-level log events for the send message code paths, which may increase log volume on device.
📊 Macroscope summarized a4d5b38. 1 file reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

@insipx insipx requested a review from a team as a code owner March 26, 2026 10:59
@claude
Copy link
Copy Markdown

claude bot commented Mar 26, 2026

Claude finished @insipx's task —— View job


Code Review — March 26, 2026 at 11:00 UTC

Summary

This PR adds Android-specific tracing filters to enable TRACE-level logging for specific critical paths (send_message, send_message_optimistic) and DEBUG logging for mls_sync and xmtp_api_d14n. The changes look good overall with proper test coverage.

Code Quality ✅

  • Clean implementation following existing patterns
  • Proper use of constants for the filter string (line 21)
  • Well-structured test added to validate the filter syntax (lines 405-409)
  • The changes to release_max_level_trace in Cargo.toml align with the new TRACE requirements

Potential Issues ⚠️

1. Error Handling Missing (bindings/mobile/src/logger.rs:33)

let api_calls_filter = EnvFilter::builder().parse(ANDROID_TRACING_FILTER);

The .parse() method returns a Result, but it's not being unwrapped or handled. This will cause a compilation error. Should be:

let api_calls_filter = EnvFilter::builder().parse(ANDROID_TRACING_FILTER).unwrap();

Or use .parse_lossy() like the old code to avoid panics on invalid filters.

2. Test Completeness (bindings/mobile/src/logger.rs:405-409)
The test validates that the filter string parses successfully, but it doesn't verify:

  • That the filter actually applies the correct log levels
  • That it targets the correct modules
    Consider adding assertions or integration tests to verify the filter behavior.

Performance Considerations ⚠️

  • Enabling TRACE-level logging for send_message paths will generate significantly more log data
  • This is Android-specific (behind tracing_android_trace::AndroidTraceAsyncLayer), which is good for performance isolation
  • The release_max_level_trace change in Cargo.toml means TRACE logs are compiled in for all platforms, potentially increasing binary size even if not used
  • Consider if this should be behind a feature flag to avoid including TRACE code in production builds

Security Concerns ✅

No security issues identified. The changes are limited to logging configuration.

Test Coverage ✅

  • New test android_tracing_filter_works validates the filter syntax
  • Existing test test_file_appender should cover the logging infrastructure

Workspace Hack Changes

The large changes to crates/xmtp-workspace-hack/Cargo.toml (+661/-129) are likely auto-generated dependency updates from the tracing feature change. This is expected when dependency features change.


Recommendation: Fix the error handling issue in line 33 before merging. Otherwise, the changes are well-implemented.

--- | Branch: insipx/android-trace

{
use tracing_subscriber::EnvFilter;
let api_calls_filter = EnvFilter::builder().parse_lossy("xmtp_api=debug");
let api_calls_filter = EnvFilter::builder().parse(ANDROID_TRACING_FILTER);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Critical src/logger.rs:33

EnvFilter::builder().parse(ANDROID_TRACING_FILTER) returns Result<EnvFilter, ParseError>, but api_calls_filter is passed directly to .with_filter() on line 42, which expects an EnvFilter. This causes a type mismatch — either a compilation error or silent misbehavior if coercion occurs. Either restore .parse_lossy() or unwrap the result (e.g., .expect("valid filter")) to match the type expected by with_filter().

-        let api_calls_filter = EnvFilter::builder().parse(ANDROID_TRACING_FILTER);
+        let api_calls_filter = EnvFilter::builder().parse_lossy(ANDROID_TRACING_FILTER);
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file bindings/mobile/src/logger.rs around line 33:

`EnvFilter::builder().parse(ANDROID_TRACING_FILTER)` returns `Result<EnvFilter, ParseError>`, but `api_calls_filter` is passed directly to `.with_filter()` on line 42, which expects an `EnvFilter`. This causes a type mismatch — either a compilation error or silent misbehavior if coercion occurs. Either restore `.parse_lossy()` or unwrap the result (e.g., `.expect("valid filter")`) to match the type expected by `with_filter()`.

Evidence trail:
bindings/mobile/src/logger.rs lines 33 and 42 at REVIEWED_COMMIT: line 33 `let api_calls_filter = EnvFilter::builder().parse(ANDROID_TRACING_FILTER);` and line 42 `.with_filter(api_calls_filter)`. Git diff MERGE_BASE..REVIEWED_COMMIT shows change from `.parse_lossy()` (returns `EnvFilter`) to `.parse()` (returns `Result<EnvFilter, ParseError>`). Test at line 405-407 uses `.unwrap()` confirming `parse()` returns a Result. docs.rs tracing_subscriber/filter/env/builder.rs lines 141 (parse_lossy returns EnvFilter) vs lines 156-157 (parse returns Result).

@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp bot commented Mar 26, 2026

Approvability

Verdict: Needs human review

1 blocking correctness issue found.

You can customize Macroscope's approvability policy. Learn more.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.02%. Comparing base (9a8d47f) to head (a4d5b38).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3358      +/-   ##
==========================================
+ Coverage   82.93%   83.02%   +0.09%     
==========================================
  Files         376      376              
  Lines       50896    50901       +5     
==========================================
+ Hits        42213    42263      +50     
+ Misses       8683     8638      -45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant