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

Remove hard dependency on logrus for logging #42

Merged
merged 1 commit into from Jul 6, 2022

Conversation

nathanejohnson
Copy link
Contributor

Removes hard dependency on logrus for logging, while providing
backwards compatibility via the SetLogger method.

Introduces a Logger interface that others can implement for other
structured loggers such as zerolog.

Bumps the golang.org/x/sys dependency since tests fail to run on go 1.18 with the old version.

@nathanejohnson nathanejohnson force-pushed the feature/modular_logging branch 2 times, most recently from 73554cb to 7f484e3 Compare June 1, 2022 21:21
@nathanejohnson
Copy link
Contributor Author

nathanejohnson commented Jun 1, 2022

So this will slow logging down slightly if using the LogrusLogger versus native logrus, but from a quick POC doing a zerolog implementation as a Logger I get considerably better results:

% go test -bench=.
goos: darwin
goarch: amd64
pkg: github.com/mailgun/groupcache/v2
cpu: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
BenchmarkLogrusLogger-12          363612              3000 ns/op
BenchmarkLogrus-12                412762              2734 ns/op
BenchmarkZeroLogger-12           1000000              1100 ns/op
PASS
ok      github.com/mailgun/groupcache/v2        6.156s

Here is a link to the implementation if you're curious.

https://github.com/nathanejohnson/groupcache/blob/feature/zerolog/logger.go#L98

@nathanejohnson nathanejohnson force-pushed the feature/modular_logging branch 2 times, most recently from 41547e8 to 4e495db Compare June 1, 2022 22:02
Provides backwards compatibility with existing logrus via SetLogger() method

Introduces a Logger interface that others can implement for other
structured loggers such as zerolog.

Add SetLoggerFromLogger method that allows caller to pass in an implementation
of the new Logger interface.

Bumps the golang.org/x/sys dependency since tests fail to run on go 1.18 with the old version.

adding a test case for LogrusLogger

adding benchmark, add WithFields method because it's a lot faster apparently
@nathanejohnson
Copy link
Contributor Author

for what it's worth, I've opened a PR with upstream logrus around the x/sys dep:

sirupsen/logrus#1333

Copy link
Contributor

@thrawn01 thrawn01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to make this PR, I'll merge and tag a new release.

@thrawn01 thrawn01 merged commit 4d2d85e into mailgun:master Jul 6, 2022
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