rs / zerolog

Zero Allocation JSON Logger
MIT License
10.39k stars 566 forks source link

Add Logger.Enabled method #617

Closed mitar closed 9 months ago

mitar commented 10 months ago

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 commented 10 months ago

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

mitar commented 10 months ago

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 commented 10 months ago

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

mitar commented 10 months ago

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 commented 9 months ago

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

mitar commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

It seems I can do the same with:

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