-
Notifications
You must be signed in to change notification settings - Fork 499
fix: add signature for tail.File so we can handle atomic writes #5143
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: main
Are you sure you want to change the base?
Conversation
| // signatureThresholds defines the byte offsets at which we should recompute the signature | ||
| // as the file grows. This allows us to progressively build a more complete signature. | ||
| var signatureThresholds = []int{64, 128, 256, 512} |
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.
Why add this complication? could we wait for the signatureSize and assume it's 0 at first? or unknown?
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.
It kinda depends on when you start tailing a file.
If we start tailing before the file is signatureSize we take whatever is in there. So if it's 0 then that is our current signature.
If we only update the signature again when we reach signatureSize there is are some problems:
- If we started at 0 we asume it's not a match causing rereads.
- The smaller the signature is the higher likely hood of collision causing missed logs.
And that is what we are trying to combat with this. But this is for sure a edge case with rotation happening on really small files. In real workloads I don't think this should cause problems
| if _, err := f.file.Seek(f.lastOffset, io.SeekStart); err != nil { | ||
| return nil, err | ||
| } |
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 seeking back be in defer? in case we exit at line 133?
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.
We don't expect it to fail at 133 and if it does the tailer will stop, so it would not do anything for us.
|
|
||
| // Recompute signature if we've crossed a threshold and haven't reached it yet. | ||
| // This progressively builds a more complete signature as the file grows. | ||
| if f.signature.shouldRecompute(f.lastOffset) { |
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 wonder if we could put some of this logic somewhere else, these function is already pretty big
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.
Where would you put it?
1678109 to
6585c27
Compare
PR Description
Noticed when doing some manual testing. Our tailer do not handle atomic writes, e.g. neovim will perform these kind of writes on every save.
Not sure how big of a problem this is in "normal" log rotation environments. What happens before this pr is that if I have a log file in neovim and save
:wwe will detect a delete event, reopen the file and consume it fully.If this is something we want to handle we can compute a signature. The signature consist of currently hard coded number of bytes (512).
This pr handle incomplete signatures and recompute it once we have read past desired size.
Which issue(s) this PR fixes
Notes to the Reviewer
PR Checklist