Skip to content

Commit

Permalink
Improve error handling for log filter (#13897)
Browse files Browse the repository at this point in the history
# Objective
This PR aims to improve error handling for log filters.

Closes #13850

## Solution
I changed the parsing of LogPlugin its filter to lossy, so that it
prints the directives with an error but does not skip them. I decided on
letting it gracefully handle the error instead of panicking to be
consistent with the parsing from an environment variable that it tries
to do before parsing it from the LogPlugin filter.

If the user decides to specify the filter by an environment variable, it
would silently fail and default to the LogPlugin filter value. It now
prints an error before defaulting to the LogPlugin filter value.

Unfortunately, I could not try and loosely set the filter from the
environment variable because the `tracing-subscriber` module does not
expose the function it uses to get the environment variable, and I would
rather not copy its code. We may want to check if the maintainers are
open to exposing the method.


## Testing
Consider the following bevy app, where the second of the 3 filters is
invalid:
```
use bevy::{log::LogPlugin, prelude::*};

fn main() {
    App::new().add_plugins(DefaultPlugins
        .set(LogPlugin {
            filter: "wgpu=error,my_package=invalid_log_level,naga=warn".into(),
            ..default()
        })
    ).run();
}
```
In the previous situation, it would panic with a non-descriptive error:
"called `Result::unwrap()` on an `Err` value: ParseError { kind:
Other(None) }", while only 1 of the 3 filters is invalid. When running
`cargo run`, it will now use the two valid filters and print an error on
the invalid filter.
> ignoring `my_package=invalid_log_level`: invalid filter directive

This error comes from `tracing-subscriber` and cannot be altered as far
as I can see.

To test setting the log filter through an environment variable, you can
use `RUST_LOG="wgpu=error,my_package=invalid_log_level,naga=warn" cargo
run` to run your app. In the previous situation it would silently fail
and use the LogPlugin filter. It will now print an error before using
the LogPlugin filter.
> LogPlugin failed to parse filter from env: invalid filter directive


## Changelog
- Added warning when using invalid filter in the RUST_LOG environment
variable
- Prevent the app from panicking when setting an invalid LogPlugin
filter

---------

Co-authored-by: Luc Drenth <luc.drenth@ing.com>
  • Loading branch information
LucDrenth and Luc Drenth committed Jun 19, 2024
1 parent 0a003ea commit 45a5f66
Showing 1 changed file with 18 additions and 2 deletions.
20 changes: 18 additions & 2 deletions crates/bevy_log/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
//! For more fine-tuned control over logging behavior, set up the [`LogPlugin`] or
//! `DefaultPlugins` during app initialization.

use std::error::Error;
#[cfg(feature = "trace")]
use std::panic;

Expand Down Expand Up @@ -52,7 +53,12 @@ use bevy_app::{App, Plugin};
use tracing_log::LogTracer;
#[cfg(feature = "tracing-chrome")]
use tracing_subscriber::fmt::{format::DefaultFields, FormattedFields};
use tracing_subscriber::{prelude::*, registry::Registry, EnvFilter, Layer};
use tracing_subscriber::{
filter::{FromEnvError, ParseError},
prelude::*,
registry::Registry,
EnvFilter, Layer,
};
#[cfg(feature = "tracing-chrome")]
use {bevy_ecs::system::Resource, bevy_utils::synccell::SyncCell};

Expand Down Expand Up @@ -166,7 +172,17 @@ impl Plugin for LogPlugin {

let default_filter = { format!("{},{}", self.level, self.filter) };
let filter_layer = EnvFilter::try_from_default_env()
.or_else(|_| EnvFilter::try_new(&default_filter))
.or_else(|from_env_error| {
_ = from_env_error
.source()
.and_then(|source| source.downcast_ref::<ParseError>())
.map(|parse_err| {
// we cannot use the `error!` macro here because the logger is not ready yet.
eprintln!("LogPlugin failed to parse filter from env: {}", parse_err);
});

Ok::<EnvFilter, FromEnvError>(EnvFilter::builder().parse_lossy(&default_filter))
})
.unwrap();
let subscriber = subscriber.with(filter_layer);

Expand Down

0 comments on commit 45a5f66

Please sign in to comment.