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

Improve error handling for log filter #13897

Conversation

LucDrenth
Copy link
Contributor

@LucDrenth LucDrenth commented Jun 17, 2024

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

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@LucDrenth LucDrenth changed the title Improve error handling for log plugin filter Improve error handling for log filter Jun 17, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 17, 2024
@alice-i-cecile
Copy link
Member

We may want to check if the maintainers are open to exposing the method.

Agreed, can you open an issue on their repo linking this PR?

@LucDrenth
Copy link
Contributor Author

We may want to check if the maintainers are open to exposing the method.

Agreed, can you open an issue on their repo linking this PR?

Here is the issue tokio-rs/tracing#3009. Shall I open an issue in this repo to follow on this once this PR is merged?

@alice-i-cecile
Copy link
Member

Yes please.

crates/bevy_log/src/lib.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 18, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2024
Merged via the queue into bevyengine:main with commit 45a5f66 Jun 19, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when log filters are incorrectly configured
4 participants