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

Add Logger.Enabled method #617

Closed
wants to merge 1 commit into from
Closed

Add Logger.Enabled method #617

wants to merge 1 commit into from

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Nov 15, 2023

This works with Nop and zero Logger. It allows one to skip some initialization if logger is disabled (e.g., skip installing hlog middleware).

@rs
Copy link
Owner

rs commented Nov 19, 2023

You already have Enabled at the event level. Adding another similar method at the logger level will be confusing.

@mitar
Copy link
Contributor Author

mitar commented Nov 19, 2023

Are you suggesting to name it differently? Because I think that is a plus to have the same name, because it is semantically the same: at the event level Enabled says if logger is enabled at level, and at logger level Enabled says if logger is enabled at all, at any level.

@rs
Copy link
Owner

rs commented Nov 19, 2023

I'm hesitant to add it as it creates multiple ways to do a similar thing.

@mitar
Copy link
Contributor Author

mitar commented Nov 19, 2023

So how does can one check if Logger is a Nop or zero logger, so in practice a disabled logger, in any other way? (If this would introduce multiple ways to do it?) One does not have access to Logger's writer. So you cannot check if writer is nil from outside.

@rs
Copy link
Owner

rs commented Nov 20, 2023

The idea is that you get this via the event's Enabled().

@mitar
Copy link
Contributor Author

mitar commented Nov 20, 2023

But I need before. To even know if I should install hlog middleware. If logger is Nop there is no point installing hlog middleware.

@rs
Copy link
Owner

rs commented Nov 20, 2023

Handlers are generally installed unconditionally so they can start logging if a logger is enabled at runtime. If you want to conditionally install them, you could use the same condition used to disable your logger.

@mitar
Copy link
Contributor Author

mitar commented Nov 20, 2023

Handlers are generally installed unconditionally so they can start logging if a logger is enabled at runtime. If you want to conditionally install them, you could use the same condition used to disable your logger.

I mean, I allow users to pass zerolog.Logger to my app server. And I am not sure why I would install a bunch of handlers (they add a lot to the stack) if the Logger is Nop or zero. This is a already "at runtime" through configuration. The issue is that I do not know what was the condition that the user of my library picked to pass zerolog.Nop to me (or maybe they just didn't initialize that field). Anyway, it feels to me silly to install all those handlers.

@rs
Copy link
Owner

rs commented Nov 20, 2023

The logger is passed to handlers via context. The state of the logger during app init might not be its state for the lifetime of the app. This is sort of app dependent so the mechanism to decide how to install those handlers should probably be as well.

@mitar
Copy link
Contributor Author

mitar commented Nov 20, 2023

I disagree for my particular case, my library controls both the context creation and middleware installation.

But I will not push this further. Thanks for keeping arguing your point.

@mitar mitar closed this Nov 20, 2023
@mitar
Copy link
Contributor Author

mitar commented Nov 20, 2023

It seems I can do the same with:

if l := logger.Sample(nil); l.Log().Enabled() {
    // ...
}

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