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

Made Handle accessible on logger enabling multilog environments #393

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

izolyomi
Copy link

Based on the discussion in issue #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?

…x() functions, enabling multilog environments.
@izolyomi
Copy link
Author

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
Copy link
Owner

estk commented Sep 26, 2024

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

@estk
Copy link
Owner

estk commented Sep 26, 2024

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
Copy link
Author

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
Copy link
Author

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

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.

2 participants