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

Conditionally init of global logger when tracing-log feature is enabled #3075

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

Conversation

sdtnjung
Copy link

@sdtnjung sdtnjung commented Sep 6, 2024

Motivation

Solves #3074

Solution

I want to gather some feedback before putting more work into this PR.

There is a standard trait implementation of SubscriberInitExt. I cannot use a builder pattern for persisting the flag to signal whether a global logger should be initialized or not because I cannot introduce any trait fields.

My workaround is to add a feature gated parameter to init and try_init to then conditionally set the global logger.

I am not sure if you are OK with having feature gated parameters and with such a breaking change.

Regarding the update of the init methods I feel it is a good decision to give (and also enforce) more fine grained control during runtime. I have seen bugs in other repos where people were pulling in tracing-subscriber without being aware of the consequences (of having a global logger set) when tracing-log is enabled by default. Normally you would only have tracing-subscriber down the stream when letting another lib manage your tracing init logic. For such scenarios it makes sense to throw compile time errors in case that specific lib does not support the feature gated with_logger parameter. Imho that is better than unexpected behavior in terms of silently tracing logs.

fn try_init(
    self,
    #[cfg(feature = "tracing-log")] with_logger: bool,
) -> Result<(), TryInitError>

Another alternative could be a new method which then does not prevent existing users from thinking more about the impact of the default behavior

  • fn try_init_with(self, set_logger: bool)

@sdtnjung sdtnjung requested review from hawkw, davidbarsky and a team as code owners September 6, 2024 20:40
@sdtnjung sdtnjung changed the title Runtime configurable tracing-log / global logger Runtime configurable init of the tracing-log global logger Sep 6, 2024
@sdtnjung sdtnjung changed the title Runtime configurable init of the tracing-log global logger Conditionally init of global logger when tracing-log feature is enabled Sep 6, 2024
@sdtnjung sdtnjung changed the title Conditionally init of global logger when tracing-log feature is enabled Conditionally global logger init when tracing-log feature is enabled Sep 6, 2024
@sdtnjung sdtnjung changed the title Conditionally global logger init when tracing-log feature is enabled Conditionally init of global logger when tracing-log feature is enabled Sep 6, 2024
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.

1 participant