rs / zerolog

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

zerolog.Logger{} should behave the same as zerolog.Nop() logger #605

Closed mitar closed 11 months ago

mitar commented 11 months ago

It seems that in practice it behaves almost the same. zerolog.Logger{} does more work though, constructing whole event and then bailing out when it tries to write to nil. I think it would be nice if zerolog.Logger{} would behave exactly the same as zerolog.Nop. I think that Logger.should should probably check if writer is set to non-nil value. Currently it is:

func (l *Logger) should(lvl Level) bool {
    if lvl < l.level || lvl < GlobalLevel() {
        return false
    }
    if l.sampler != nil && !samplingDisabled() {
        return l.sampler.Sample(lvl)
    }
    return true
}

I propose:

func (l *Logger) should(lvl Level) bool {
    if l.w == nil {
        return false
    }
    if lvl < l.level || lvl < GlobalLevel() {
        return false
    }
    if l.sampler != nil && !samplingDisabled() {
        return l.sampler.Sample(lvl)
    }
    return true
}

Zero logger returns false on l.w == nil while nop logger return false on lvl < l.level (because l.level is set to Disabled which holds true for all levels).

rs commented 11 months ago

I agree with that. Do you want to propose a PR?

mitar commented 11 months ago

Done: #606.