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

[libbeat] Add flag for rotating log files on startup #11953

Merged
merged 10 commits into from
May 6, 2019

Conversation

faec
Copy link
Contributor

@faec faec commented Apr 26, 2019

Adds the logging.files.rotateonopen file and corresponding FileConfig.RotateOnOpen flag, controlling whether existing log files are rotated on startup or appended to.

Resolves #11537.

@faec faec requested review from a team as code owners April 26, 2019 16:08
@faec faec assigned faec and kvch and unassigned faec Apr 26, 2019
@faec faec requested a review from a team as a code owner April 29, 2019 15:08
@kvch kvch requested review from kvch and a team April 29, 2019 16:18
@faec faec added the docs label Apr 29, 2019
@faec faec force-pushed the 11537-log-rotation branch from 4534807 to 35bfdc7 Compare April 29, 2019 17:48
@kvch
Copy link
Contributor

kvch commented Apr 30, 2019

The failing test on Jenkins seems related to this PR.

Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

Nice PR @faec, I really like your clarifying comments.

I left a couple of minor questions / suggestions

@@ -1173,6 +1173,10 @@ logging.files:
# Unix epoch. Defaults to disabled.
#interval: 0

# Rotate existing logs on startup rather than appending to the existing
# file. Defaults to true.
# rotateonopen: true
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this setting would be clearer using start or startup instead of open. I don't think it's obvious to the user what open means here.

I'm OK with using open internally (rotator already uses open for this concept).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, changed open -> startup everywhere (I'd rather use the same name internally so it's easy to find)

libbeat/logp/config.go Outdated Show resolved Hide resolved
@faec
Copy link
Contributor Author

faec commented Apr 30, 2019

The failing test on Jenkins seems related to this PR.

that push was a squashed merge of all the original commits (because some of them were committed without the right email address, so the CLA checker wouldn't let them through)... it looks like the build itself is segfaulting, it may be refusing to rerun on something with no binary changes from previous run? is there a way to try it again?

@adriansr
Copy link
Contributor

adriansr commented Apr 30, 2019

it looks like the build itself is segfaulting, it may be refusing to rerun on something with no binary changes from previous run?

I squash all the time and the build kicks again just fine. There might be a problem with this particular test under Windows. Can you check? There's a couple of Windows boxes in {beats}/Vagrantfile.

is there a way to try it again?

You comment:

jenkins, test this

@faec faec unassigned kvch May 3, 2019
@faec faec added the libbeat label May 3, 2019
@faec faec changed the title Add flag for rotating log files on startup [libbeat] Add flag for rotating log files on startup May 3, 2019
@faec
Copy link
Contributor Author

faec commented May 3, 2019

I (believe I) fixed the problem with the windows unit test. travis-ci is still failing, but it's been failing on everything for the last couple days (and none of the failures look real); beats-ci is happy now, so this is ready for final review :-)

@faec faec merged commit c4d580e into elastic:master May 6, 2019
@faec faec deleted the 11537-log-rotation branch May 6, 2019 13:41
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
* Add the `logging.files.rotateonopen` option (elastic#11537)

* Rename rotateonopen -> rotateonstartup

* Commenting out part of the rotator test to troubleshoot on Windows

* test troubleshooting

* updating x-pack/winlogbeat docs

* troubleshooting unit test on windows

* troubleshooting windows unit tests

* Candidate fix for unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Libbeat.rotator: enable flag for rotating log files on startup
3 participants