Remove max_level in favour of extracting it from the Filter

Documentation for `log` specifies that all implementations must have a
way to initialize `log::set_max_level()` (because it otherwise defaults
to `Off`), in our case via the `with_max_level()` builder - and by
calling `init_once()`.  Over time `android_logger` received `env_filter`
support for much more fine-grained control over filtering, making
`android_logger` similar to `env_logger` in its implementation.

With this change however, ambiguity arises when independently
configuring log levels through `.with_max_level()` and
`env_filter::Filter`.  The former acts as an early performance save
directly in `log::log!()` macros while the latter applies additional
filtering inside `android_logger`.  In short: `log::max_level()` must
be at least as high as what `Filter` can process, otherwise messages get
lost before they reach the `Filter`.

`env_logger` solved this ambiguity by hiding direct access to
`log::set_max_level()` as an implementation detail and initializing it
to the highest level that could possibly be processed by the `Filter`.
Mimick this inside `android_logger` by removing the `with_max_level()`
setter.  In addition all internal `is_enabled()` filtering and related
tests are removed because we rely on the `log` macros themselves to
filter, and no longer track `max_level` inside.  Keeping in mind that
any user can techincally change `log::set_max_level()` to increase or
decrease global verbosity.

The only complexity remains around `__android_log_is_loggable_len()`
on Android 30 and above (if enabled on the crate by the user - not
necessarily based on the actual minimum nor target SDK level yet!)
which asks the Android system for `env_filter`-like per-module (tag)
level overrides.

Author: MarijnS95

.github/workflows/ci.yml

@@ -41,6 +41,7 @@ jobs:
           components: rustfmt, clippy
 
       - run: cargo fmt --check
+      - 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:

examples/system_log_level_overrides.rs

@@ -68,16 +68,16 @@
 //! ```
 
 fn main() {
+    todo!("This test and its comments are no longer true after with_max_level is removed");
     android_logger::init_once(
-        android_logger::Config::default()
-            .with_tag("log_test")
-            // If set, this is the highest level to log unless overriddeby by the system.
-            // Note the verbosity can be *increased* through system properties.
-            .with_max_level(log::LevelFilter::Info),
+        android_logger::Config::default().with_tag("log_test"),
+        // If set, this is the highest level to log unless overridden by by the system.
+        // Note the verbosity can be *increased* through system properties.
+        // .with_max_level(log::LevelFilter::Info),
     );
     // The log crate applies its filtering before we even get to android_logger.
     // Pass everything down so that Android's liblog can determine the log level instead.
-    log::set_max_level(log::LevelFilter::Trace);
+    // log::set_max_level(log::LevelFilter::Trace);
 
     log::trace!("trace");
     log::debug!("debug");

src/config.rs

@@ -1,22 +1,22 @@
 use crate::{FormatFn, LogId};
-use log::{Level, LevelFilter, Record};
+#[cfg(all(target_os = "android", feature = "android-api-30"))]
+use log::LevelFilter;
+use log::{Level, Record};
 use std::ffi::CString;
 use std::fmt;
 
 /// Filter for android logger.
 #[derive(Default)]
 pub struct Config {
-    pub(crate) log_level: Option<LevelFilter>,
     pub(crate) buf_id: Option<LogId>,
-    filter: Option<env_filter::Filter>,
+    pub(crate) filter: Option<env_filter::Filter>,
     pub(crate) tag: Option<CString>,
     pub(crate) custom_format: Option<FormatFn>,
 }
 
 impl fmt::Debug for Config {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.debug_struct("Config")
-            .field("log_level", &self.log_level)
             .field("buf_id", &self.buf_id)
             .field("filter", &self.filter)
             .field("tag", &self.tag)
@@ -62,38 +62,44 @@ fn android_is_loggable_len(
 }
 
 #[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)
+pub(crate) fn is_loggable(_tag: &str, _record_level: Level) -> bool {
+    // There is nothing to test here, the `log` macros already checked the variable
+    // `log::max_level()` before calling into the implementation.
+    // The tests ensure this by creating and calling into `AndroidLogger::log()` without
+    // `set_max_level()` from `init_once()`, and expect the message to be logged.
+    true
 }
 
 #[cfg(all(target_os = "android", feature = "android-api-30"))]
-fn android_is_loggable(tag: &str, record_level: Level, config_level: Option<LevelFilter>) -> bool {
+pub(crate) fn is_loggable(tag: &str, record_level: Level) -> 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,
+    // WARNING: Reading live `log::max_level()` state here would break tests, for example when
+    // `AndroidLogger` is constructed and `AndroidLogger::log()` is called _without_ going through
+    // `init_once()` which would call `log::set_max_level()`, leaving this at `Off`.  Currently no
+    // tests exist that run on live Android devices and/or mock `__android_log_is_loggable_len()`
+    // such that this function can be called.
+    let default_prio = match log::max_level().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,
     };
     android_is_loggable_len(prio, tag, default_prio)
 }
 
 impl Config {
-    /// Changes the maximum log level.
-    ///
-    /// Note, that `Trace` is the maximum level, because it provides the
-    /// maximum amount of detail in the emitted logs.
-    ///
-    /// If `Off` level is provided, then nothing is logged at all.
-    ///
-    /// [`log::max_level()`] is considered as the default level.
-    pub fn with_max_level(mut self, level: LevelFilter) -> Self {
-        self.log_level = Some(level);
-        self
-    }
+    // /// Changes the maximum log level.
+    // ///
+    // /// Note, that `Trace` is the maximum level, because it provides the
+    // /// maximum amount of detail in the emitted logs.
+    // ///
+    // /// If `Off` level is provided, then nothing is logged at all.
+    // ///
+    // /// [`log::max_level()`] is considered as the default level.
+    // pub fn with_max_level(mut self, level: LevelFilter) -> Self {
+    //     self.log_level = Some(level);
+    //     self
+    // }
 
     /// Changes the Android logging system buffer to be used.
     ///
@@ -114,15 +120,8 @@ impl Config {
         }
     }
 
-    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)
-    }
-
+    // TODO: Replace this with env_logger-like constructors:
+    // https://docs.rs/env_logger/latest/env_logger/struct.Builder.html
     pub fn with_filter(mut self, filter: env_filter::Filter) -> Self {
         self.filter = Some(filter);
         self

src/lib.rs

@@ -66,7 +66,8 @@
 #[cfg(target_os = "android")]
 extern crate android_log_sys as log_ffi;
 
-use log::{Log, Metadata, Record};
+use config::is_loggable;
+use log::{LevelFilter, Log, Metadata, Record};
 use std::ffi::{CStr, CString};
 use std::fmt;
 use std::mem::MaybeUninit;
@@ -142,7 +143,7 @@ static ANDROID_LOGGER: OnceLock<AndroidLogger> = OnceLock::new();
 
 /// Maximum length of a tag that does not require allocation.
 ///
-/// Tags configured explicitly in [Config] will not cause an extra allocation. When the tag is
+/// Tags configured explicitly in [`Config`] will not cause an extra allocation. When the tag is
 /// derived from the module path, paths longer than this limit will trigger an allocation for each
 /// log statement.
 ///
@@ -151,9 +152,12 @@ const LOGGING_TAG_MAX_LEN: usize = 127;
 const LOGGING_MSG_MAX_LEN: usize = 4000;
 
 impl Log for AndroidLogger {
+    /// # Warning
+    /// This method relies on stateful data when `android-api-30` is enabled, including
+    /// [`log::max_level()`] which we only initialize when [`init_once()`] is called and which can
+    /// be changed by the user at any point via [`log::set_max_level()`].
     fn enabled(&self, metadata: &Metadata) -> bool {
-        self.config()
-            .is_loggable(metadata.target(), metadata.level())
+        is_loggable(metadata.target(), metadata.level())
     }
 
     fn log(&self, record: &Record) {
@@ -194,12 +198,10 @@ impl Log for AndroidLogger {
 
         // If a custom tag is used, add the module path to the message.
         // Use PlatformLogWriter to output chunks if they exceed max size.
+        use std::fmt::Write;
         let _ = match (&config.tag, &config.custom_format) {
             (_, Some(format)) => format(&mut writer, record),
-            (Some(_), _) => fmt::write(
-                &mut writer,
-                format_args!("{}: {}", module_path, *record.args()),
-            ),
+            (Some(_), _) => write!(&mut writer, "{}: {}", module_path, *record.args()),
             _ => fmt::write(&mut writer, *record.args()),
         };
 
@@ -213,7 +215,7 @@ impl Log for AndroidLogger {
 /// Send a log record to Android logging backend.
 ///
 /// This action does not require initialization. However, without initialization it
-/// will use the default filter, which allows all logs.
+/// will use the default filter, TODO Error default.
 pub fn log(record: &Record) {
     ANDROID_LOGGER
         .get_or_init(AndroidLogger::default)
@@ -228,12 +230,19 @@ pub fn log(record: &Record) {
 /// It is ok to call this at the activity creation, and it will be
 /// repeatedly called on every lifecycle restart (i.e. screen rotation).
 pub fn init_once(config: Config) {
-    let log_level = config.log_level;
+    // let log_level = config.log_level;
+    let log_level = config
+        .filter
+        .as_ref()
+        // Match the default Error level used by env_logger
+        .map_or(LevelFilter::Error, |f| f.filter());
     let logger = ANDROID_LOGGER.get_or_init(|| AndroidLogger::new(config));
 
     if let Err(err) = log::set_logger(logger) {
-        log::debug!("android_logger: log::set_logger failed: {}", err);
-    } else if let Some(level) = log_level {
-        log::set_max_level(level);
+        // TODO: Bubble up the error (try_init()) or panic (init()), as suggested
+        // by the `log` crate and as implemented by `env_logger`.
+        log::debug!("android_logger: log::set_logger failed: {err}");
+    } else {
+        log::set_max_level(log_level);
     }
 }

src/platform_log_writer.rs

@@ -192,12 +192,11 @@ pub mod tests {
     use crate::arrays::slice_assume_init_ref;
     use crate::platform_log_writer::PlatformLogWriter;
     use log::Level;
-    use std::ffi::CStr;
     use std::fmt::Write;
 
     #[test]
     fn platform_log_writer_init_values() {
-        let tag = CStr::from_bytes_with_nul(b"tag\0").unwrap();
+        let tag = c"tag";
 
         let writer = PlatformLogWriter::new(None, Level::Warn, tag);
 
@@ -318,10 +317,6 @@ pub mod tests {
     }
 
     fn get_tag_writer() -> PlatformLogWriter<'static> {
-        PlatformLogWriter::new(
-            None,
-            Level::Warn,
-            CStr::from_bytes_with_nul(b"tag\0").unwrap(),
-        )
+        PlatformLogWriter::new(None, Level::Warn, c"tag")
     }
 }

src/tests.rs

@@ -1,44 +1,47 @@
 use super::*;
-use log::LevelFilter;
 use std::sync::atomic::{AtomicBool, Ordering};
 
 #[test]
 fn check_config_values() {
     // Filter is checked in config_filter_match below.
     let config = Config::default()
-        .with_max_level(LevelFilter::Trace)
         .with_log_buffer(LogId::System)
         .with_tag("my_app");
 
-    assert_eq!(config.log_level, Some(LevelFilter::Trace));
     assert_eq!(config.buf_id, Some(LogId::System));
     assert_eq!(config.tag, Some(CString::new("my_app").unwrap()));
 }
 
 #[test]
 fn log_calls_formatter() {
     static FORMAT_FN_WAS_CALLED: AtomicBool = AtomicBool::new(false);
-    let config = Config::default()
-        .with_max_level(LevelFilter::Info)
-        .format(|_, _| {
-            FORMAT_FN_WAS_CALLED.store(true, Ordering::SeqCst);
-            Ok(())
-        });
+    let config = Config::default().format(|_, _| {
+        FORMAT_FN_WAS_CALLED.store(true, Ordering::SeqCst);
+        Ok(())
+    });
     let logger = AndroidLogger::new(config);
 
     logger.log(&Record::builder().level(log::Level::Info).build());
 
     assert!(FORMAT_FN_WAS_CALLED.load(Ordering::SeqCst));
 }
 
-#[test]
-fn logger_enabled_threshold() {
-    let logger = AndroidLogger::new(Config::default().with_max_level(LevelFilter::Info));
-
-    assert!(logger.enabled(&log::MetadataBuilder::new().level(log::Level::Warn).build()));
-    assert!(logger.enabled(&log::MetadataBuilder::new().level(log::Level::Info).build()));
-    assert!(!logger.enabled(&log::MetadataBuilder::new().level(log::Level::Debug).build()));
-}
+// TODO: How about deleting `fn enabled()` pretty much entirely?  It's only useful for implementing
+// the new `android-api-30` `__android_log_is_loggable_len()` and without tests mocking that,
+// bogus to call.  All filtering based on `log::max_level()` happens inside the `log::log!()`
+// macros already, and otherwise relies on the tests to call `init_once()` to ensure we call
+// `log::set_max_level()`.
+//
+// #[test]
+// fn logger_enabled_threshold() {
+//     let logger = AndroidLogger::new(
+//         Config::default().with_filter(FilterBuilder::new().filter_level(LevelFilter::Info).build()),
+//     );
+
+//     assert!(logger.enabled(&log::MetadataBuilder::new().level(log::Level::Warn).build()));
+//     assert!(logger.enabled(&log::MetadataBuilder::new().level(log::Level::Info).build()));
+//     assert!(!logger.enabled(&log::MetadataBuilder::new().level(log::Level::Debug).build()));
+// }
 
 // Test whether the filter gets called correctly. Not meant to be exhaustive for all filter
 // options, as these are handled directly by the filter itself.
@@ -47,7 +50,7 @@ fn config_filter_match() {
     let info_record = Record::builder().level(log::Level::Info).build();
     let debug_record = Record::builder().level(log::Level::Debug).build();
 
-    let info_all_filter = env_filter::Builder::new().parse("info").build();
+    let info_all_filter = FilterBuilder::new().parse("info").build();
     let info_all_config = Config::default().with_filter(info_all_filter);
 
     assert!(info_all_config.filter_matches(&info_record));

tests/config_log_level.rs

@@ -1,10 +1,14 @@
-extern crate android_logger;
-extern crate log;
+use android_logger::FilterBuilder;
+use log::LevelFilter;
 
 #[test]
 fn config_log_level() {
     android_logger::init_once(
-        android_logger::Config::default().with_max_level(log::LevelFilter::Trace),
+        android_logger::Config::default().with_filter(
+            FilterBuilder::new()
+                .filter_level(LevelFilter::Trace)
+                .build(),
+        ),
     );
 
     assert_eq!(log::max_level(), log::LevelFilter::Trace);

tests/default_init.rs

@@ -1,10 +1,8 @@
-extern crate android_logger;
-extern crate log;
-
 #[test]
 fn default_init() {
     android_logger::init_once(Default::default());
 
-    // android_logger has default log level "off"
-    assert_eq!(log::max_level(), log::LevelFilter::Off);
+    // android_logger has default log level of Error
+    // TODO: env_logger/env_filter have this too, but I cannot find it in the source code
+    assert_eq!(log::max_level(), log::LevelFilter::Error);
 }

tests/multiple_init.rs

@@ -1,16 +1,24 @@
-extern crate android_logger;
-extern crate log;
+use android_logger::FilterBuilder;
+use log::LevelFilter;
 
 #[test]
 fn multiple_init() {
     android_logger::init_once(
-        android_logger::Config::default().with_max_level(log::LevelFilter::Trace),
+        android_logger::Config::default().with_filter(
+            FilterBuilder::new()
+                .filter_level(LevelFilter::Trace)
+                .build(),
+        ),
     );
 
     // Second initialization should be silently ignored
     android_logger::init_once(
-        android_logger::Config::default().with_max_level(log::LevelFilter::Error),
+        android_logger::Config::default().with_filter(
+            FilterBuilder::new()
+                .filter_level(LevelFilter::Error)
+                .build(),
+        ),
     );
 
-    assert_eq!(log::max_level(), log::LevelFilter::Trace);
+    assert_eq!(log::max_level(), LevelFilter::Trace);
 }