Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use __android_log_is_loggable in AndroidLogger::enabled #84

Merged
merged 5 commits into from
Mar 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ jobs:
- armv7-linux-androideabi
- aarch64-linux-android
- i686-linux-android
features:
- ""
- --no-default-features
- --all-features

steps:
- uses: actions/checkout@v3
Expand All @@ -37,9 +41,9 @@ jobs:
components: rustfmt, clippy

- run: cargo fmt --check
- run: cargo clippy --target=${{ matrix.target }} -- -Dwarnings
- run: cargo build --target=${{ matrix.target }}
- run: cargo doc --target=${{ matrix.target }}
- run: cargo clippy --target=${{ matrix.target }} ${{ matrix.features }} -- -Dwarnings
- run: cargo build --target=${{ matrix.target }} ${{ matrix.features }}
- run: cargo doc --target=${{ matrix.target }} ${{ matrix.features }}
env:
RUSTDOCFLAGS: -Dwarnings
# Temporary test non-target only.
Expand Down
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ targets = [
[features]
default = ["regex"]
regex = ["env_filter/regex"]
android-api-30 = []

[dependencies.log]
version = "0.4"

[dependencies.android_log-sys]
version = "0.3"
version = "0.3.2"

[dependencies.env_filter]
version = "0.1"
Expand Down
42 changes: 42 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,48 @@ Therefore this library will only log a warning for subsequent `init_once` calls.
This library ensures that logged messages do not overflow Android log message limits
by efficiently splitting messages into chunks.

## Consistent log filtering in mixed Rust/C/C++ apps

Android's C logging API determines the effective log level based on [a
combination](https://cs.android.com/android/platform/superproject/main/+/main:system/logging/liblog/properties.cpp;l=243;drc=b74a506c1b69f5b295a8cdfd7e2da3b16db15934)
of a process-wide global variable, [system-wide
properties](https://cs.android.com/android/platform/superproject/main/+/main:system/logging/logd/README.property;l=45;drc=99c545d3098018a544cb292e1501daca694bee0f),
and call-specific default. `log` + `android_logger` crates add another layer of
log filtering on top of that, independent from the C API.

```
.-----.
| app |
'-----' Rust
C/C++ | '--------------.
| v
| .-----------. filter by log::STATIC_MAX_LEVEL +
| | log crate | - log::MAX_LOG_LEVEL_FILTER,
| '-----------' overrideable via log::set_max_level
| |
| v
| .----------------------.
| | android_logger crate | - filter by Config::max_level
| '----------------------'
| |
| .------------'
v v
.--------.
| liblog | - filter by global state or system-wide properties
'--------'
```

`liblog` APIs introduced in Android API 30 let `android_logger` delegate log
filtering decision to `liblog`, making the log level consistent across C, C++
and Rust calls.

If you build `android_logger` with `android-api-30` feature enabled, the logger
will consider the process-wide global state (set via
[`__android_log_set_minimum_priority`](https://cs.android.com/android/platform/superproject/main/+/main:prebuilts/runtime/mainline/runtime/sdk/common_os/include/system/logging/liblog/include/android/log.h;l=364;drc=4cf460634134d51dba174f8af60dffb10f703f51))
and Android system properties when deciding if a message should be logged or
not. In this case, the effective log level is the _least verbose_ of the levels
set between those and Rust log facilities.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we link Rust log facilities to (the documentation for) log::set_max_level() once again?


## License

Licensed under either of
Expand Down
60 changes: 57 additions & 3 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,56 @@ impl fmt::Debug for Config {
}
}

#[cfg(all(target_os = "android", feature = "android-api-30"))]
fn android_log_priority_from_level(level: Level) -> android_log_sys::LogPriority {
match level {
Level::Warn => android_log_sys::LogPriority::WARN,
Level::Info => android_log_sys::LogPriority::INFO,
Level::Debug => android_log_sys::LogPriority::DEBUG,
Level::Error => android_log_sys::LogPriority::ERROR,
Level::Trace => android_log_sys::LogPriority::VERBOSE,
}
}

/// Asks Android liblog if a message with given `tag` and `priority` should be logged, using
/// `default_prio` as the level filter in case no system- or process-wide overrides are set.
#[cfg(all(target_os = "android", feature = "android-api-30"))]
fn android_is_loggable_len(
prio: log_ffi::LogPriority,
tag: &str,
default_prio: log_ffi::LogPriority,
) -> bool {
// SAFETY: tag points to a valid string tag.len() bytes long.
unsafe {
log_ffi::__android_log_is_loggable_len(
prio as log_ffi::c_int,
tag.as_ptr() as *const log_ffi::c_char,
tag.len() as log_ffi::c_size_t,
default_prio as log_ffi::c_int,
) != 0
}
}

#[cfg(not(all(target_os = "android", feature = "android-api-30")))]
fn default_is_loggable(_tag: &str, record_level: Level, config_level: Option<LevelFilter>) -> bool {
record_level <= config_level.unwrap_or_else(log::max_level)
}

#[cfg(all(target_os = "android", feature = "android-api-30"))]
fn android_is_loggable(tag: &str, record_level: Level, config_level: Option<LevelFilter>) -> bool {
let prio = android_log_priority_from_level(record_level);
// Priority to use in case no system-wide or process-wide overrides are set.
let default_prio = match config_level {
Some(level_filter) => match level_filter.to_level() {
Some(level) => android_log_priority_from_level(level),
// LevelFilter::to_level() returns None only for LevelFilter::Off
None => android_log_sys::LogPriority::SILENT,
},
None => android_log_sys::LogPriority::INFO,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be log::max_level() to match what default_is_loggable does by default when no config_level is set?

Copy link
Author

Choose a reason for hiding this comment

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

The log level set in the log crate is used to filter early, where the log macros get expanded.

Using log::max_level here means global settings can make logs less verbose than the app initializes the log level to, but never raise it - and I personally find it very useful to have the default level be something not too verbose, but with the ability to raise the log level for temporary debugging.

And the only way I can see how that could be achieved is to:

  • set log::max_level to Trace unconditionally, so that logs don't get filtered immediately,
  • apply the requested log level in this branch.

In your opinion, what behavior would make the most sense?

Anyway, multiple layers of filters are extremely confusing. If only the log crate was completely stateless, we could leave the filtering to the Android platform and not worry about the weird interactions here. Maybe I should drop an RFC over there...

};
android_is_loggable_len(prio, tag, default_prio)
}

impl Config {
/// Changes the maximum log level.
///
Expand Down Expand Up @@ -64,9 +114,13 @@ impl Config {
}
}

pub(crate) fn is_loggable(&self, level: Level) -> bool {
// todo: consider __android_log_is_loggable.
level <= self.log_level.unwrap_or_else(log::max_level)
pub(crate) fn is_loggable(&self, tag: &str, level: Level) -> bool {
#[cfg(all(target_os = "android", feature = "android-api-30"))]
use android_is_loggable as is_loggable;
#[cfg(not(all(target_os = "android", feature = "android-api-30")))]
use default_is_loggable as is_loggable;

is_loggable(tag, level, self.log_level)
}

pub fn with_filter(mut self, filter: env_filter::Filter) -> Self {
Expand Down
3 changes: 2 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ const LOGGING_MSG_MAX_LEN: usize = 4000;

impl Log for AndroidLogger {
fn enabled(&self, metadata: &Metadata) -> bool {
self.config().is_loggable(metadata.level())
self.config()
.is_loggable(metadata.target(), metadata.level())
}

fn log(&self, record: &Record) {
Expand Down