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

Allow to override log level for specific target #4305

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Timshel
Copy link
Contributor

@Timshel Timshel commented Feb 1, 2024

Useful for either silencing some target when running in debug or switching debug only for a specific target,

With this a good chunk of the default overrides could be moved to the conf, cf:

        // Hide unknown certificate errors if using self-signed
        .level_for("rustls::session", log::LevelFilter::Off)
        // Hide failed to close stream messages
        .level_for("hyper::server", log::LevelFilter::Warn)
        // Silence Rocket `_` logs
        .level_for("_", rocket_underscore_level)
        .level_for("rocket::response::responder::_", rocket_underscore_level)
        .level_for("rocket::server::_", rocket_underscore_level)
        .level_for("vaultwarden::api::admin::_", rocket_underscore_level)
        .level_for("vaultwarden::api::notifications::_", rocket_underscore_level)
        // Silence Rocket logs
        .level_for("rocket::launch", log::LevelFilter::Error)
        .level_for("rocket::launch_", log::LevelFilter::Error)
        .level_for("rocket::rocket", log::LevelFilter::Warn)
        .level_for("rocket::server", log::LevelFilter::Warn)
        .level_for("rocket::fairing::fairings", log::LevelFilter::Warn)
        .level_for("rocket::shield::shield", log::LevelFilter::Warn)
        .level_for("hyper::proto", log::LevelFilter::Off)
        .level_for("hyper::client", log::LevelFilter::Off)
        // Filter handlebars logs
        .level_for("handlebars::render", handlebars_level)
        // Prevent cookie_store logs
        .level_for("cookie_store", log::LevelFilter::Off)
        // Variable level for trust-dns used by reqwest
        .level_for("trust_dns_resolver::name_server::name_server", trust_dns_level)
        .level_for("trust_dns_proto::xfer", trust_dns_level)
        .level_for("diesel_logger", diesel_logger_level)

@Timshel Timshel force-pushed the feature/log_override branch 3 times, most recently from a951845 to 7df4dd6 Compare February 6, 2024 14:15
@Timshel Timshel force-pushed the feature/log_override branch 2 times, most recently from 7bd00a9 to 77ba826 Compare March 19, 2024 15:02
@Timshel
Copy link
Contributor Author

Timshel commented May 13, 2024

Any interest in the feature ?

@BlackDex
Copy link
Collaborator

It sounds like a nice feature. The only issue i have is, moving all defaults to config also means that either someone has to add all the defaults them self, or we someone need to provide a default and allow them to be overridden via the settings.

I'm also not sure on the multi-line formatting of the config. I would then rather use the standard of Rust it self.
https://rust-lang-nursery.github.io/rust-cookbook/development_tools/debugging/config_log.html

That allows for a general log_level settings as first item (or key less item) and are all comma separated.
That would allow you to keep using the LOG_LEVEL env for example and that will not break other instances.
It would then look like LOG_LEVEL=info,hyper::client=info,handlebars::render=debug for example.
We could have all defaults in a HashMap, and add or overwrite this with the configured items.

What do you think?

I btw also think this is very useful during development and troubleshooting issues reported by users :).

@Timshel
Copy link
Contributor Author

Timshel commented May 18, 2024

I think it all depends on if it's used for the current defaults.

Thinking on it, I believe it might be acceptable if all defaults are set (in config.rs) and in env.template.
I would expect that if someone then start customizing then they would copy and paste from the template; and if they don't do it then it's not really complicated for them to replicate it by adding the lines that generate too many logs. Additionally, a warning could be added to the release notes.

If used this way then it means it would contain at least 10 values and that's why I decided to make it multiline because I believe it makes it way more legible than a comma separated list.

If it's decided not to use it for the defaults then I believe your solution is better since with less values the comma separated solution would make more sense and risk less conflict due to the lines break.

But since I wrote it initially ^^ I believe the multiline is better since it allows the user to easily override the defaults while keeping a very simple logic in the rust code.

@Timshel
Copy link
Contributor Author

Timshel commented May 18, 2024

I was a bit fast with my response, the Hashmap you suggest would keep things quite simple while allowing override.

But I think I still prefer the multiline and default included in env.template because it makes those default more accessible and visible, as opposed at having to check in the code.

Edit: But it might be a more sensible change since I expect that the multiline will have issues with things like docker-compose env.
I'll test settings the defaults using this solution to see how it would look.

@BlackDex
Copy link
Collaborator

The new line has some strange behavior in some environments, that is why I'm a bit against that way. While dotenvy does it correctly now, it might be systemd or other OS environments make it more error prone.

@Timshel
Copy link
Contributor Author

Timshel commented May 21, 2024

Made the change additionally made it to fallback to info if it fails to parse LOG_INFO.
Could make modifying the value in the admin console viable (did not make the change).

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice :).
Just a few questions

Cargo.toml Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

None yet

2 participants