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

Hook Interface & Event Level #408

Open
iaburton opened this issue Feb 3, 2022 · 4 comments
Open

Hook Interface & Event Level #408

iaburton opened this issue Feb 3, 2022 · 4 comments

Comments

@iaburton
Copy link

iaburton commented Feb 3, 2022

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
Copy link
Owner

rs commented Feb 6, 2022

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
Copy link
Author

iaburton commented Feb 7, 2022

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
Copy link
Author

iaburton commented Feb 7, 2022

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
Copy link
Contributor

mitar commented Aug 20, 2023

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

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

No branches or pull requests

3 participants