-
Notifications
You must be signed in to change notification settings - Fork 574
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
Hook Interface & Event Level #408
Comments
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. |
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.
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 🙂 |
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? |
I have made #583 with a more concrete proposal to support canonical log lines out of the box. |
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
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!
The text was updated successfully, but these errors were encountered: