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

Module levels not actually getting sorted outside of #[cfg(test)] #90

Closed
nikarh opened this issue Dec 29, 2023 · 5 comments
Closed

Module levels not actually getting sorted outside of #[cfg(test)] #90

nikarh opened this issue Dec 29, 2023 · 5 comments

Comments

@nikarh
Copy link

nikarh commented Dec 29, 2023

Hi,

It seems that this commit 8cec833 introduced a bug - only the cloned Vec is now sorted (and never used later), leaving the original self.module_levels pristine.

@borntyping
Copy link
Owner

Can you describe the unexpected behaviour you're seeing? I can see that the code is doing something useless, but I can't work out if sorting module_levels is ever actually important in the code.

@nikarh
Copy link
Author

nikarh commented Dec 29, 2023

Honestly, I was pursuing an issue that turned out to be unrelated to this finding (I used indicatif_log_bridge which was overriding log::set_max_level to an incorrect value).

But theoretically, looking here, I would say it's possible to have something like:

    let log = simple_logger::SimpleLogger::new()
        .with_module_level("serde", log::LevelFilter::Error);
        .with_module_level("serde_json", log::LevelFilter::Trace);

And, without sorting, this would lead to all serde_json logs being treated as if they were configured to Error level instead of Trace (since to determine the logging level for target, the code finds first match in module_levels by a string prefix).

@borntyping
Copy link
Owner

Thanks - I ended up with a similar looking test debugging this so glad I understood correctly. It was a bit confusing to work out since there was some code that sorted the Vec in with_module_level but only when tests were running!

@borntyping
Copy link
Owner

Released in v4.3.3. Thanks!

@nikarh
Copy link
Author

nikarh commented Dec 29, 2023

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants