rs / zerolog

Zero Allocation JSON Logger
MIT License
10.41k stars 567 forks source link

Hook Interface & Event Level #408

Open iaburton opened 2 years ago

iaburton commented 2 years ago

Hello!

I've developed a (soon to be OSS) "unified" logging interface/library somewhat analogous to database/sql where you import drivers; zerolog is one such driver. I'll be happy to add it to 'who uses zerolog' once it is available!

The interface defines some things that are not readily available in zerolog. Such as being able to change the event level after it is created; I know this is in contrast to zerolog that wants to define the level initially for reasons such as enabling/disabling/nil-ing out the event for performance and other reasons. The interface also allows you to delay when the event is written.

Both of these are primarily to support canonical log lines, and some other features.

Currently the driver gets around the event level issue by creating the initial event with 'NoLevel', this makes it valid from zerologs perspective and essentially disables zerolog from writing the level itself and allows me to write the level later when known.

This causes an issue with the hook interface, though. While the hooks will still be called they are all fed 'NoLevel' instead of the level that is known at the time Msg is called. I don't see a way to access the event level itself (#252) or access the hooks that have been set so I can call them with the correct level. I see that #255 hasn't been accepted, in part because it breaks code that writes the level first, but would also panic in code that had a disabled level (nil event) to begin with.

Considering using 'NoLevel' is a valid use case with zerolog, and writing the level yourself is easily accomplished with the same code found here I wonder if a modification of #255 would work. Allow changing the event as long as the event is non-nil and the event is coming from 'NoLevel' or the level changing remains 'enabled'. Meaning you can go from enabled->enabled but not disabled->enabled.

Something along the lines of

func (e *Event) Level(level Level) *Event {
    // perhaps leaving off the NoLevel check
    if e.Enabled() && e.level == NoLevel {
        e.level = level
    }
    return e
}

I would be happy to provide a PR for this, or one for an alternative solution (to the event level and/or hooks).

PS: I have a tangential issue regarding retaining zerologs performance in this driver that I'd like to discuss, I can create a new issue for it or discuss it here if you don't mind. Let me know.

Thank you!

rs commented 2 years ago

This feels like a hack. If you end up setting a level that should not be logged, the level check won't be made and the even will be wrongly logged. This code would also silently noop when the condition is not met, making it obscure to use.

iaburton commented 2 years ago

If you end up setting a level that should not be logged, the level check won't be made and the even will be wrongly logged.

I'm not sure I follow this. Would that not be covered if you only allowed enabled levels to be changed? Like the original comment mentions, no disabled->enabled changing.

This code would also silently noop when the condition is not met, making it obscure to use.

That's true. There would be appropriate documentation of course. Can't return a bool indicating it changed either or it breaks chaining.

I'm not bound to this approach, I'm just trying to find a solution to the problem 🙂 If there were a way to get & set hooks after they've been added that would work too. Are there alternative approaches you think might work?

iaburton commented 2 years ago

I had noticed this bit of code prior. I was thinking I had to use NoLevel to get around that. However it seems like if I set LevelFieldName to the empty string it would disable logging the level initially as well, meaning I wouldn't need to change the level.

The problem I have with that option though is it uses globals in zerolog which aren't concurrency safe. If that option, or the LevelFieldMarshalFunc could also be done per logger instead of only global that would also work for the issue I'm trying to resolve. Would you be open to a PR allowing changes in that regard?

mitar commented 1 year ago

I have made #583 with a more concrete proposal to support canonical log lines out of the box.