-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[pkg/stanza] log when files are rotated/moved/truncated #33237
Conversation
08f16a6
to
b7343dd
Compare
b7343dd
to
147044a
Compare
147044a
to
8716076
Compare
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.
Should we add actual test cases to make sure we don't regress? We could use zaptest/observer
to validated the expected logs.
// the Validate method to ensure that the file has not been truncated. | ||
if !oldReader.Validate() { | ||
m.set.Logger.Debug("File has been rotated(truncated)", zap.String("path", newReader.GetFileName())) |
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 suppose it doesn't technically matter but do you think it would be slightly more readable to use oldReader
here, simply because we are communicating to the reader about the old file that is no longer here?
8716076
to
1f74fee
Compare
pkg/stanza/fileconsumer/design.md
Outdated
A) When a file it rotated out of pattern via move/create, we detect that | ||
our old handle is still valid and we attempt to read from it. | ||
B) When a file it rotated out of pattern via copy/truncate, we detect that | ||
our old handle is invalid and we do not attempt to read from it. | ||
C) When a file is copied within the pattern with unread logs on the end, then the original is truncated, | ||
we get the unread logs on the copy as well as any new logs written to the truncated file | ||
D) When a file is moved within the pattern with unread logs on the end, then the original is created again, | ||
we get the unread logs on the moved as well as any new logs written to the newly created file |
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.
Good explainations. Can I suggest we reverse the order so that we start with the simplest case (move/create within pattern) and progress to the most complex (copy/truncate out of pattern)?
pkg/stanza/fileconsumer/design.md
Outdated
In case of the file has been rotated with copy/truncate, the old handle will be pointing | ||
to the original file which is truncated. So we don't have a handle so as to consume any remaining | ||
logs from the moved file. This can cause data loss. | ||
In case of the file has been rotated with move/create, the old handle will be pointing | ||
to the moved file so we can still consume from it even if it's out of the pattern. |
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.
Here as well, I think move/create is simpler to understand since copy/truncate comes with a possible data loss scenario.
a698bda
to
5bd7b4d
Compare
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
5bd7b4d
to
78f738a
Compare
Description:
This PR adds the logging part from #31256.
With this addition every time that is identified that file is rotated
either by move/create or copy/truncate, proper logging takes place.
Link to tracking Issue: #31256
Testing:
Updated existing unit tests
How to test this manually
Using the following config:
Testing truncate (out of pattern)
Testing truncate (in pattern)
Testing move/create (out of pattern)
Testing move/create (in pattern)
Documentation:
Add some extra notes in the
design.md