Log rotation by time interval #8349

danhermann · 2018-09-18T18:26:26Z

Adds optional log rotation by time interval in addition to size-based log rotation. It's modeled after ES's log rotation which defaults to daily log rotation in which the current log file is rotated to filename-yyyy-MM-dd-n where n is incremented if either the max file size is reached or a beat is started more than once per day. The rotation interval is configurable down to 1s. Values of 1m, 1h, 24h, 7*24h, 30*24h, and 365*24h are treated specially by being boundary-aligned with minutes, hours, days, weeks, months, and years as reported by the local system clock.

The keepfiles setting is also honored as the oldest backup files are removed during rotation.

Fixes #7990.

houndci-bot · 2018-09-18T18:26:40Z

libbeat/common/file/rotator.go

@@ -263,6 +293,81 @@ func (r *Rotator) closeFile() error {
 }

 func (r *Rotator) purgeOldBackups() error {
+	if (r.daily) {
+		return r.purgeOldDailyBackups()
+	} else {


if block ends with a return statement, so drop this else and outdent its block

vjsamuel · 2018-09-18T20:54:35Z

this can be time based instead of daily where the time is defaulted to 1d

kvch · 2018-09-19T10:59:57Z

auditbeat/auditbeat.reference.yml

@@ -1101,6 +1101,10 @@ logging.files:
  # Must be a valid Unix-style file permissions mask expressed in octal notation.
  #permissions: 0600

+  # Enable daily log file rotation in addition to size-based rotation. Defaults to
+  # disabled.
+  #daily: true


Please set this value to false as it is the default value.

kvch · 2018-09-19T11:05:07Z

libbeat/common/file/rotator.go

+		return errors.Wrap(err, "failed to rotate backups")
+	}
+
+	if r.log != nil {


Why not move logging in rotateByDay and rotateBySize?

kvch · 2018-09-19T11:08:27Z

libbeat/common/file/rotator.go

 type Rotator struct {
 	filename     string
 	maxSizeBytes uint
 	maxBackups   uint
 	permissions  os.FileMode
 	log          Logger // Optional Logger (may be nil).
+	daily        bool
+	lastYear     int


Why do you store date in separately instead of a time.Time struct? It would make easier to check if the log needs to be rotated or not by using Since.

kvch

Cool feature! However, I am not sure if it 100% addresses the initial issue. I would prefer to have time based log rotation where the rotation interval is configurable. For example adding a new option rotation_interval which is 0 by default, meaning there is no rotation. If someone wants to use it, he/she can specify an time interval. So the daily rotation would look like rotation_interval: 1d, weekly could be rotation_interval: 7d, and so on.

kvch · 2018-09-19T11:14:35Z

Also, please add a new entry to CHANGELOG.asciidoc.

houndci-bot · 2018-09-21T21:50:26Z

libbeat/common/file/rotator.go

@@ -108,13 +115,22 @@ func WithLogger(l Logger) RotatorOption {
 	}
 }

+// Daily enables or disables log rotation by time interval in addition to log


comment on exported function Interval should be of the form "Interval ..."

houndci-bot · 2018-09-21T21:50:26Z

libbeat/common/file/interval_rotator.go

+	return ""
+}
+
+// Given a log filename in the form [prefix]-[formattedDate]-n, returns n as int


comment on exported function IntervalLogIndex should be of the form "IntervalLogIndex ..."

houndci-bot · 2018-09-21T21:50:26Z

libbeat/common/file/interval_rotator.go

+	)
+}
+
+// Given a log filename in the form [prefix]-[formattedDate]-n, returns the filename after


comment on exported function OrderIntervalLogs should be of the form "OrderIntervalLogs ..."

houndci-bot · 2018-09-21T21:50:27Z

libbeat/common/file/interval_rotator.go

+	return time.Now()
+}
+
+func NewIntervalRotator(interval time.Duration) (*intervalRotator, error) {


exported function NewIntervalRotator should have comment or be unexported
exported func NewIntervalRotator returns unexported type *file.intervalRotator, which can be annoying to use

danhermann · 2018-09-21T22:04:29Z

@kvch, thanks for the review. I've updated the PR to allow for rotation on arbitrary time intervals as explained in the edited PR description above.

ruflin · 2018-09-24T11:42:33Z

My concern about this feature is that we add more logging logic to our code but are in the progress of more moving to popular libraries at least for the internal logging. I wonder if we could do the same for writing to file/stdout etc. too? @andrewkroh Thoughts?

andrewkroh · 2018-09-25T01:19:20Z

If there is some popular well-tested library for this sort of thing then I would consider using it, but I didn't find much last time I looked. So I think we have to roll our own here.

Is there a use-case for enabling it on the file output? I've never used the file output for anything other than testing and debugging.

danhermann · 2018-09-25T02:13:47Z

I also looked around for a library that did this and didn't see one. The https://github.com/natefinch/lumberjack library seems to be the most popular library for log rotation, but it does not support time-interval rotation and will not based on the authors comments in issue 17. This PR doesn't suffer from his main criticisms of time-interval rotation in that both size limits and time intervals are considered for rotation, so running out of disk space wouldn't be an issue.

kvch · 2018-09-25T12:16:45Z

libbeat/common/file/interval_rotator.go

+	newInterval func(lastTime time.Time, currentTime time.Time) bool
+}
+
+type clock interface {


Why does this interface exists? What's the reason of having a realClock? Why not simply call time.Now directly instead of through an interface?

I assume it's used during unit testing.

It's only for testing. I'm happy to do it differently if there's a better or more idiomatic way in Go.

kvch · 2018-09-25T12:27:09Z

libbeat/common/file/interval_rotator.go

+	return u64, i, err
+}
+
+func newSecond(lastTime time.Time, currentTime time.Time) bool {


Nit1: It would be more readable if you called these interval function from each other.
Example:

func newSecond(lastTime, currentTime time.Time) bool { return lastTime.Second() != currentTime.Second || newMinute(lastTime, currentTime) }

Nit2: It's a bit confusing/requires a bit more effort to read the code, because the order of the parameters are differs from the order during comparison.

func ...(lastTime, currentTime time.Time) bool { return lastTime.Seconds() != currentTime.Seconds() }

I am willing to let the second nit go, because it might be a personal preference.

kvch · 2018-09-25T12:30:31Z

libbeat/common/file/interval_rotator.go

+	return u64, i, err
+}
+
+func newSecond(lastTime time.Time, currentTime time.Time) bool {


To be sure that nothing can go wrong here, you could add a sanity check to see if currentTime is indeed later in time than lastTime. Otherwise the checks only check if the two times are different, not later.

That was to accommodate cases where the system clock goes backward in calendar time which may occur during daylight savings transitions or when the clock is updated. If that is not desirable, I can change it.

Yes, I also had concerns about that. I'm fine with this.

danhermann · 2018-09-28T18:08:34Z

The unrelated failing tests are all due to an updated geoip database.

(cherry picked from commit 7b4cb29)

houndci-bot reviewed Sep 18, 2018

View reviewed changes

danhermann added in progress Pull request is currently in progress. libbeat labels Sep 18, 2018

danhermann added review and removed in progress Pull request is currently in progress. labels Sep 18, 2018

kvch reviewed Sep 19, 2018

View reviewed changes

danhermann added 3 commits September 21, 2018 16:39

daily log rotation

efc073e

adding rotation for arbitrary time intervals

ad72453

update docs

a3b9859

danhermann force-pushed the log_rotation_by_day branch from 1f1cf94 to a3b9859 Compare September 21, 2018 21:50

houndci-bot reviewed Sep 21, 2018

View reviewed changes

danhermann changed the title ~~Daily log rotation~~ Log rotation by time interval Sep 21, 2018

fix hound violations

71a450e

fix style

59da0d7

kvch reviewed Sep 25, 2018

View reviewed changes

simplify new* functions per review comments

ee4a3c8

kvch approved these changes Sep 26, 2018

View reviewed changes

danhermann added v6.5.0 v7.0.0 labels Sep 26, 2018

danhermann added the needs_backport PR is waiting to be backported to other branches. label Sep 28, 2018

danhermann merged commit 7b4cb29 into elastic:master Sep 28, 2018

danhermann added a commit to danhermann/beats that referenced this pull request Sep 28, 2018

Log rotation by time interval (elastic#8349)

f7a352c

(cherry picked from commit 7b4cb29)

danhermann removed the needs_backport PR is waiting to be backported to other branches. label Sep 28, 2018

danhermann mentioned this pull request Sep 28, 2018

(Backport to 6.x) Log rotation by time interval (#8349) #8489

Merged

danhermann added a commit that referenced this pull request Oct 1, 2018

Log rotation by time interval (#8349) (#8489)

12fc9ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log rotation by time interval #8349

Log rotation by time interval #8349

danhermann commented Sep 18, 2018 •

edited

Loading

houndci-bot Sep 18, 2018

vjsamuel commented Sep 18, 2018

kvch Sep 19, 2018

kvch Sep 19, 2018

kvch Sep 19, 2018

kvch left a comment

kvch commented Sep 19, 2018

houndci-bot Sep 21, 2018

houndci-bot Sep 21, 2018

houndci-bot Sep 21, 2018

houndci-bot Sep 21, 2018

danhermann commented Sep 21, 2018

ruflin commented Sep 24, 2018

andrewkroh commented Sep 25, 2018

danhermann commented Sep 25, 2018 •

edited

Loading

kvch Sep 25, 2018

kvch Sep 25, 2018

danhermann Sep 26, 2018

kvch Sep 25, 2018

kvch Sep 25, 2018

danhermann Sep 26, 2018

kvch Sep 26, 2018

danhermann commented Sep 28, 2018

Log rotation by time interval #8349

Log rotation by time interval #8349

Conversation

danhermann commented Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

vjsamuel commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvch left a comment

Choose a reason for hiding this comment

kvch commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhermann commented Sep 21, 2018

ruflin commented Sep 24, 2018

andrewkroh commented Sep 25, 2018

danhermann commented Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhermann commented Sep 28, 2018

danhermann commented Sep 18, 2018 •

edited

Loading

danhermann commented Sep 25, 2018 •

edited

Loading