Remove max_level in favour of extracting it from the Filter
#94
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@Nercury I'd love your input on the general direction (and specifically its justification in the description) of this PR removing |
MarijnS95
force-pushed
the
replace-max_level-with-env_filter-max
branch
from
83576aa to
75643d2
Compare
…tation (to also have it build-tested...)
MarijnS95
force-pushed
the
replace-max_level-with-env_filter-max
branch
from
75643d2 to
7f89554
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #80
Documentation for
logspecifies that all implementations must have a way to initializelog::set_max_level()(because it otherwise defaults toOff), in our case via thewith_max_level()builder - and by callinginit_once(). Over timeandroid_loggerreceivedenv_filtersupport for much more fine-grained control over filtering, makingandroid_loggersimilar toenv_loggerin its implementation.With this change however, ambiguity arises when independently configuring log levels through
.with_max_level()andenv_filter::Filter. The former acts as an early performance save directly inlog::log!()macros while the latter applies additional filtering insideandroid_logger. In short:log::max_level()must be at least as high as whatFiltercan process, otherwise messages get lost before they reach theFilter.env_loggersolved this ambiguity by hiding direct access tolog::set_max_level()as an implementation detail and initializing it to the highest level that could possibly be processed by theFilter. Mimick this insideandroid_loggerby removing thewith_max_level()setter. In addition all internalis_enabled()filtering and related tests are removed because we rely on thelogmacros themselves to filter, and no longer trackmax_levelinside. Keeping in mind that any user can techincally changelog::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 forenv_filter-like per-module (tag) level overrides.TODO
env_filternon-Optional?Builder-constructors likefilter_level()andfilter_module(), just likeenv_logger.env_logger/env_filter's environment variables? These are less common to be set for Android apps.Errormakes sense here.AndroidLoggerlevel.READMEin these tests, either:#![doc = include_str!("README.md")];