-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
logging: Switch from lumberjack
to timberjack
, add time-rolling options
#7244
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
base: master
Are you sure you want to change the base?
Conversation
Oh, fantastic! I didn't get the memo that a fork was worked on, we were waiting for someone to pick up the mantle. Will review soon. |
modules/logging/filewriter.go
Outdated
// Roll log file at fix time | ||
RollAt []string `json:"roll_at,omitempty` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand what the inputs are here. What are the values supposed to be? The godoc comment here should be fleshed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answering my own question, it looks something like this in timberjack
RotateAt: []string{"00:00", "12:00"}, // Also rotate at 00:00 and 12:00 each day
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc added for this config param in 6ff7126
// How many days to keep rolled log files. Default: 90 | ||
RollKeepDays int `json:"roll_keep_days,omitempty"` | ||
|
||
// Rotated file will have format <logfilename>-<format>-<criterion>.log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should explain that the default from timberjack is:
// Optional. If unset or invalid, defaults to 2006-01-02T15-04-05.000 (with fallback warning)
Also mention that the <format>
must be a Go time compatible format described here https://pkg.go.dev/time#pkg-constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add doc & link to go doc in 6ff7126
// Roll log file at fix minutes | ||
RollAtMinutes []int `json:"roll_at_minutes,omitempty"` | ||
|
||
// Roll log file at fix time | ||
RollAt []string `json:"roll_at,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These godoc comments should cover some of the caveats mentioned in the timberjack README https://github.com/DeRuina/timberjack#%EF%B8%8F-rotation-notes--warnings (and eventually also in the Caddy website docs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also subtleties like using either of RollAt/RollAtMinutes will use a background goroutine to trigger rotation, whereas RollInterval triggers a check on each log write whether the interval has been crossed and causes a rotation at that point (no goroutine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6ff7126
lumberjack
to timberjack
add time-rolling options
lumberjack
to timberjack
add time-rolling optionslumberjack
to timberjack
, add time-rolling options
This is looking great. I am happy to merge this, I just need to give it one closer look first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok yeah this looks great. Thank you!
I second Francis' comments -- if we can clarify some docs and address the things he brought up, I'd say let's merge it. 👍
👍 I try to fix docs & cie this week-end ! |
Thanks! FYI timberjack had a new release just now to add support for zstd compression (we should expose that option too) & adding a reason to manual rotation (so we could add support for rotating the files on config reload). Another requested nice-to-have, we could also add support for opt-in reopening of the log file on config reload, useful for when users use their own external log rolling instead of what we provide them, we should reopen the log file so it correctly points to the new file instead of the moved file. |
310f7df
to
6ff7126
Compare
Lumberjack is unmaintained since 2 years.
A fork exists, Timberjack to replace it with more features.
This PR migrate to Timberjack and add support newest features, specially
RotateAt
options to allow to rotate log file base on date.Assistance Disclosure
No AI was used.