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

feat(backend): allow to set log level via config file #182

Merged
merged 5 commits into from
Mar 12, 2022

Conversation

MichaIng
Copy link
Collaborator

@MichaIng MichaIng commented Mar 8, 2022

If the "RUST_LOG" environment variable is set, it overrides the value from the config file, which makes testing and debugging easier without the need to adjust the config file and hence without affecting the default service.

Also the conversion from String to toml::Value to str back to String, then in main.rs from String back to str to log::Level somehow looks like an unnecessary loop. Fascinating that searching around for an hour or so I couldn't find a single example of how a String or str is converted into a log::Level or alternatively log::LevelFilter directly. Probably we can also store the environment valuable || config value || default also directly as log::Level?

@MichaIng MichaIng added enhancement New feature or request rust Pull requests that update Rust code labels Mar 8, 2022
@MichaIng MichaIng linked an issue Mar 8, 2022 that may be closed by this pull request
@MichaIng MichaIng force-pushed the loglevel-config branch 11 times, most recently from d53ccae to 11f60cc Compare March 8, 2022 21:26
Making use of: https://docs.rs/simple_logger/2.1.0/simple_logger/fn.init_with_level.html

If the "RUST_LOG" environment variable is set, it overrides the value from the config file, which makes testing and debugging easier without the need to adjust the config file and hence without affecting the default service.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 8, 2022

@ravenclaw900
Okay I'm at the end of my trail&error attempts 😅. The issue is the different types of what these return:

  • std::env::var() => String
  • cfg.get() => &toml:Value
  • and the fallback/default.

Not sure how to convert them best. Converting the env var + default both into &toml:Value, which just needs to be converted back to string, seems somehow unreasonable. Generally &toml:Value implements to_string(), but currently it fails "due to unsatisfied trait bounds". I guess this would need to be done in let cfg above, but not sure how to do correctly and whether this is really wanted.

Best would be actually to pass it as str, so no further conversion is required in main.rs.

Otherwise we could simply ignore the environment variable for now, but actually I like the option to use it for quick debugging 😉.

Have `simple_logger` do the environment variable parsing for us, so that all we have to worry about is the config file
@ravenclaw900
Copy link
Owner

ravenclaw900 commented Mar 9, 2022

The problem was that you were trying to run to_string on an Option<&toml::Value>, not the &toml::Value itself. However, you're slightly overthinking it. No need for us to parse the environment variable, just let simple_logger do that for us. See the change I just pushed.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 9, 2022

Ah, so a simply unwrap() was missing 😅. And dammit yes, with_level() prefers RUST_LOG anyway, when present. I forgot about this as I wanted to use init_with_level() first, which doesn't do that. But that one does not take a LevelFilter but a Level instead, which cannot be off, making it more complicated to allow disabling logging completely, hence I switched back to with_level().

I'll test this tomorrow.

@ravenclaw900
Copy link
Owner

Also, once I figure some things out with serde, I'll probably switch to an actual configuration library like figment, rendering most of this boilerplate redundant and allowing us to set all config file settings with environment variables.

@MichaIng
Copy link
Collaborator Author

MichaIng commented Mar 9, 2022

There should be exclusions for login password and secret for security reasons, but otherwise sounds good.

Probably no performance difference, but doesn't allocate unless necessary
Signed-off-by: MichaIng <micha@dietpi.com>
@ravenclaw900 ravenclaw900 changed the title feat(backend): Allow to set log level via config feat(backend): allow to set log level via config file Mar 12, 2022
@ravenclaw900 ravenclaw900 merged commit 5643d51 into main Mar 12, 2022
@ravenclaw900 ravenclaw900 deleted the loglevel-config branch March 12, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to set log level via config file
2 participants