Add zerolog logging middleware #225
Conversation
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
==========================================
- Coverage 71.55% 70.31% -1.25%
==========================================
Files 37 38 +1
Lines 1392 1381 -11
==========================================
- Hits 996 971 -25
- Misses 347 363 +16
+ Partials 49 47 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey thanks for looking to add ZeroLog into the codebase 0️⃣ 📖
My main thing is I've not used ZeroLog before so would be good to get some more knowledge around it and how it is used. It looks to me that it is a strongly typed logging library and this is probably where its speed comes from.
I would like the addition of new loggers to be as similar as possible in approach as other loggers that exist in the code base this will make it much easier to make it generic and easier to refactor in the future.
Would you be able to do another pass and have a look if we can factor out the CtxLogger in favour fo adding the fields to the logger iteslef?
Also having a look at getting the DefaultDuration more inline with the others that are already in the codebase.
| "github.com/rs/zerolog" | ||
| "google.golang.org/grpc" | ||
| "path" | ||
| "time" |
There was a problem hiding this comment.
can you run goimports if you havnt already :)
| var ( | ||
| // ClientField is used in every client-side log statement made through grpc_zap. Can be overwritten before initialization. | ||
| //ClientField = zap.String("span.kind", "client") | ||
| ) |
There was a problem hiding this comment.
lets remove this var block and the commented code 👍
|
|
||
| func logFinalClientLine(o *options, logger *ctxzr.CtxLogger, startTime time.Time, err error, msg string) { | ||
| code := o.codeFunc(err) | ||
| var level = o.levelFunc(code) |
There was a problem hiding this comment.
| var level = o.levelFunc(code) | |
| level := o.levelFunc(code) |
| code := o.codeFunc(err) | ||
| var level = o.levelFunc(code) | ||
|
|
||
| clientLogger := logger.Logger.WithLevel(level).Err(err) |
There was a problem hiding this comment.
Not sure on the implementation of zero log but would hope this .Err(err) doesnt log it as an error log if the err is nil
| var level = o.levelFunc(code) | ||
|
|
||
| clientLogger := logger.Logger.WithLevel(level).Err(err) | ||
| args := make(map[string]interface{}) |
There was a problem hiding this comment.
could we create and assign values all in one for code, msg, duration?
| } | ||
|
|
||
| func logCall(ctx context.Context, options *options, msg string, code codes.Code, startTime time.Time) { | ||
|
|
| extractedLogger := ctxzr.Extract(ctx) | ||
|
|
||
| var level = options.levelFunc(code) | ||
| logger := extractedLogger.Logger.WithLevel(level) |
There was a problem hiding this comment.
could you inline level here?
| type CtxLogger struct { | ||
| Logger *zerolog.Logger | ||
| Fields map[string]interface{} | ||
| } |
There was a problem hiding this comment.
Is there anyway that we dont have to use our own CtxLogger? And instead add the fields onto the zerolog.Logger?
|
|
||
| var ( | ||
| ctxMarkerKey = &ctxMarker{} | ||
| nullLogger = &zerolog.Logger{} |
| @@ -1,7 +1,7 @@ | |||
| package ratelimit | |||
|
|
|||
| import ( | |||
| "golang.org/x/net/context" | |||
| "context" | |||
There was a problem hiding this comment.
can you rebase against master think this should have been fixed there already
|
Any update? |
|
I was looking for inspiration in other middlewares to implement a zerolog gRPC middleware - I'm happy that I've found this PR :) @Ahmet-Kaplan Is there anything we could do to help with this PR? Having this merged would be really helpful. |
|
@Ahmet-Kaplan could you have a look at the v2 logging interface and perhaps try to get it merged there? |
|
I only noticed this PR after I'd already submitted mine. :-( #297 follows the existing code structure more closely, FWIW. |
|
As #297 addresses the changes proposed by this PR, closing this. |
Inspired from adrien-f PR, I want to add Zerolog logging into that project.
For benchmarks, you can visit logbench site. As seen, Zerolog is much faster than Logrus and Zap.