search

estk / log4rs

A highly configurable logging framework for Rust
Apache License 2.0
1.01k stars 150 forks source link

Made Handle accessible on logger enabling multilog environments #393

Open izolyomi opened 2 months ago

izolyomi commented 2 months ago

Based on the discussion in issue https://github.com/estk/log4rs/issues/390 this is a proof-of-concept experiment that enables using log4rs in a multilog environment.

Probably naming, documentation and others can still be improved a lot, what do you think?

izolyomi commented 2 months ago

Can I please have an opinion whether the direction taken by these few lines of changes looks reasonable to any main contributor? Would a detailed rationale help the review?

estk commented 2 months ago

@izolyomi For context we have been slowly exposing more internals, this is not a use case that was considered.

estk commented 2 months ago

My only issue with this impl is the env mode, there are a number of other places that would also set the max log level and therefore it would be inconsistent. I think the only way we can get away with this without a major version or inconsistent api is either add a set_max_level param to the config or peer methods where set_max_level is called in log4rs, but without that functionality. (eg init_config_without_max_level(config))

As for pub fn handle I think there is some reason log does not recommend this. In lieu of this I would suggest just saving the handle somewhere accessible for later use.

izolyomi commented 2 months ago

My only issue with this impl is the env mode, there are a number of other places that would also set the max log level and therefore it would be inconsistent. I think the only way we can get away with this without a major version or inconsistent api is either add a set_max_level param to the config or peer methods where set_max_level is called in log4rs, but without that functionality. (eg init_config_without_max_level(config))

As for pub fn handle I think there is some reason log does not recommend this. In lieu of this I would suggest just saving the handle somewhere accessible for later use.

Thank you for the review and the reply, I agree that the environment mode was not fully consistent. Considering other possibilities some more, I've realized that whenever the logger is externally reconfigured then it can be the caller's responsibility to keep the global log level consistent by calling log::set_max_level after set_config(). Consequently, I've removed the environment mode from my suggested changes.

I think the PR became trivial now, is it acceptable in the current form? Frankly I couldn't fully understand your train of thoughts at pub fn handle, do you see a better way to make it accessible?

izolyomi commented 1 month ago

Is there any planned timeline to review and hopefully merge the dozen lines of the updated PR?

izolyomi commented 2 days ago

I guess it's a NO then.