Conversation
tuyenpthust
force-pushed
the
rotation-zap-log
branch
from
5a40c92 to
a31d2ff
Compare
common/log/zap_logger.go
Outdated
| logger, _ := config.Build() | ||
| var logger *zap.Logger | ||
| if cfg.EnableRotation && len(cfg.OutputFile) > 0 { | ||
| core, _ := rotationZapCore(cfg, config) |
There was a problem hiding this comment.
We need to check and respond to this error, please don't swallow it
common/log/zap_logger.go
Outdated
| } | ||
|
|
||
| func newEncoder(name string, encoderConfig zapcore.EncoderConfig) (zapcore.Encoder, error) { | ||
| if name == "console" { |
There was a problem hiding this comment.
Would you mind replacing these with an enum containing something like ConsoleEncoding and JSONEncoding in the few places we use it?
Co-authored-by: Tim Deeb-Swihart <409226+tdeebswihart@users.noreply.github.com>
|
|
||
| func BenchmarkZapLoggerWithFields(b *testing.B) { | ||
| zLogger := buildZapLogger(Config{Level: "info"}, false) | ||
| zLogger, _ := buildZapLogger(Config{Level: "info"}, false) |
There was a problem hiding this comment.
Please check these errors and fail the benchmarks if these fail. It will help prevent problems when this code changes in the future
tdeebswihart
left a comment
tdeebswihart
left a comment
There was a problem hiding this comment.
This looks good over all but this PR should not change the version of the API repo used.
| assert.Less(t, currentFile.Size(), int64(1024*1024)) | ||
| } | ||
|
|
||
| func TestNoRotationLogger(t *testing.T) { |
There was a problem hiding this comment.
Nit: how about
| func TestNoRotationLogger(t *testing.T) { | |
| func TestLogger_FilesArentRotated_WhenDisabled(t *testing.T) { |
That's cleared about what you're testing (and the above could be TestLogger_FilesAreRotated_WhenEnabled or something)
There was a problem hiding this comment.
This shouldn't be updated as part of your PR. Please either rebase on main or check out the same version of the submodule as is on main
dnr
left a comment
dnr
left a comment
There was a problem hiding this comment.
I appreciate the contribution, but I'm wondering if this is really something that we want to build into the server, adding complexity and taking on yet another dependency.
In general modern infrastructure is moving in the direction of having server processes log to stdout, and having external systems accept logs and slice and dice and ship them in various ways, specifically so that every server doesn't have to have a full range of configuration options and log processing can be consolidated across services. This is true of both single-machine process managers like docker and systemd and of course distributed systems like k8s as well.
In that light, this change feels like a bit of a regression in infrastructure management. Could you provide a stronger justification for why this should go in the server itself?
|
This PR was marked as stale. Please update or close it. |
|
This PR was marked as stale. Please update or close it. |
3 participants
What changed?
add rotation zaplog based on file size
Why?
Currently all log are written in 1 file, it's easier to maintain, debug, backup if the log file is rotated
How did you test it?
Test locally and unittest