-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Remove unnecessary logging #35426
Remove unnecessary logging #35426
Conversation
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
772e453
to
5882ad6
Compare
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
The log line remove by this commit is unnecessary, it is not an actual error in the sense something will not work. It only means the harvester is already running for this file so another one does not need to be started. Keeping this log line will only spam our log files and make users worried.
5882ad6
to
b4398d1
Compare
// only thing it does is to log the error. So to avoid unnecessary errors, | ||
// we just return nil. | ||
//nolint: nilerr // Read the comment above | ||
return nil |
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 agree that it's not an error but we still should log it at least at the debug level.
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 path can be triggered by events watched by the file watcher, which can be a lot, even o debug level I believe it's too verbose.
I let it running on my machine for a while, in 2h there were about 2000 of those messages for a single active file (the Agent's log file). They're duplicated because the task.Group
also logs it, so about 500 messages per hour per file if it's logged on debug mode. I still believe it's too much.
What do you think?
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.
Being honest, I believe the best fix would for the newContext
method to just be a no-op on this case. However I did not want to change any implementation details as we already created the 8.8 branch.
beats/filebeat/input/filestream/internal/input-logfile/harvester.go
Lines 69 to 81 in ba662b8
func (r *readerGroup) newContext(id string, cancelation inputv2.Canceler) (context.Context, context.CancelFunc, error) { | |
r.mu.Lock() | |
defer r.mu.Unlock() | |
if _, ok := r.table[id]; ok { | |
return nil, nil, ErrHarvesterAlreadyRunning | |
} | |
ctx, cancel := context.WithCancel(ctxtool.FromCanceller(cancelation)) | |
r.table[id] = cancel | |
return ctx, cancel, nil | |
} |
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 think it's okay in the current form.
fec1420
to
ba662b8
Compare
* Remove unnecessary logging The log line remove by this commit is unnecessary, it is not an actual error in the sense something will not work. It only means the harvester is already running for this file so another one does not need to be started. Keeping this log line will only spam our log files and make users worried. * PR improvements * Restoring the error context. (cherry picked from commit a070ea1)
* Remove unnecessary logging The log line remove by this commit is unnecessary, it is not an actual error in the sense something will not work. It only means the harvester is already running for this file so another one does not need to be started. Keeping this log line will only spam our log files and make users worried. * PR improvements * Restoring the error context. (cherry picked from commit a070ea1) Co-authored-by: Tiago Queiroz <tiago.queiroz@elastic.co>
* Remove unnecessary logging The log line remove by this commit is unnecessary, it is not an actual error in the sense something will not work. It only means the harvester is already running for this file so another one does not need to be started. Keeping this log line will only spam our log files and make users worried. * PR improvements * Restoring the error context.
What does this PR do?
The log line remove by this commit is unnecessary, it is not an actual
error in the sense something will not work. It only means the
harvester is already running for this file so another one does not
need to be started. Keeping this log line will only spam our log files
and make users worried.
Why is it important?
It was spamming our logs and would make our uses worry for no good reason
Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files- [ ] I have added tests that prove my fix is effective or that my feature works- [ ] I have added an entry inCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.## Author's ChecklistHow to test this PR locally
Run Filebeat with the
filestream
input enabled, you should not see log errors like:error while adding new reader to the bookkeeper harvester is already running for file
harvester:: error while adding new reader to the bookkeeper harvester is already running for file
## Related issues## Use cases## Screenshots## Logs