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

Add logr implementation to README and json.RawMessage to Fields() #337

Merged
merged 2 commits into from Aug 3, 2021
Merged

Add logr implementation to README and json.RawMessage to Fields() #337

merged 2 commits into from Aug 3, 2021

Conversation

hn8
Copy link
Contributor

@hn8 hn8 commented Jul 30, 2021

Add the fastest (fewest allocation) logr implementation using zerolog, much faster than zapr. Use type switch instead Fields() to avoid map overhead (even with sync.Pool).

Also split appendFields type switch into concrete vs interface type, and add type assert of json.RawMessage
@hn8 hn8 changed the title Add logr implementation Add logr implementation and optimize Fields() type switch Jul 31, 2021
@rs
Copy link
Owner

rs commented Jul 31, 2021

What is the advantage of having this implementation inside zerolog instead of as a separate project like all other implementations of this interface? I'm generally against adding dependencies, unnecessary complexity or any code that could be implemented outside the library unless there is a good reason.

As I would not be the maintainer of this implementation, I believe it would be better living in a repository you own.

@hn8
Copy link
Contributor Author

hn8 commented Jul 31, 2021

@rs No problem. The existence of log/journald/pkgerrors was my motivation to contribute to zerolog, to have simple subpackages to integrate with other popular libraries. I was trying to optimize go-logr/zapr first, and it turns out zerlog is the fastest implementation for logr after some tuning. IMHO, many Go projects want to keep their logging interface generic (like logr) for library consumers.

@rs
Copy link
Owner

rs commented Jul 31, 2021

To be honest, I should have refused journald and pkgerrors.

I'm all for pointing to other projects like logr wrapper in the README. I prefer people composing the right set of libraries for their needs rather than big libraries with hundreds of dependencies to fulfill all the possible needs they will probably never have :)

Added to README instead
@hn8 hn8 changed the title Add logr implementation and optimize Fields() type switch Add logr implementation to README and json.RawMessage to Fields() Jul 31, 2021
@hn8
Copy link
Contributor Author

hn8 commented Jul 31, 2021

@rs I reverted logr subpackage and added to README instead as you suggested. Also revert type switch but only add json.RawMessage to Fields() as the minimal change for you to review. I really admire your work and hope that you change your mind one day on hosting logr wrapper since zerolog is definitely the best one. It is funny that neither zerolog nor logr is willing to host the zerolog-logr wrapper, but it is understandable. Just a little bit more hassle for developers like us to use a third-party like mine (I have zero intention for self-promotion). Thanks for your clear communication, appreciate your time and great work!

@rs rs merged commit 164ec91 into rs:master Aug 3, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants