plugins/filestat: Skip missing files #7316
Conversation
Previously, the filestat plugin would check for a file's existence, and then try to compute all sorts of things for the missing file, which failed and resulted in a log message about possible permission errors. Now, the plugin just skips the file if it exists. If an error is encountered probing an existing file, the plugin now logs that error, too, instead of hazarding a guess at permission problems.
There was a problem hiding this comment.
I'm not sure we want to ignore the files silently. What if the user misspelled the filename? He probably will notice quite late (when analyzing the data). So my guts-feeling is we should log at least once.
How about adding a map[string]bool to the struct for remembering which filename was logged already and output a warning on the first encounter of the missing file. Then set the map entry for the file to true and skip any further warning-outputs...
How does that sound to you?
|
@antifuchs any update? |
|
Ah, sorry I missed this! I will look into getting this implemented. |
|
@srebhan Updated - I hope this reflects what you had in mind. |
plugins/inputs/filestat/filestat.go
Outdated
| missingFiles map[string]struct{} | ||
| // files that had an error in Stat - we only log the first error. | ||
| filesWithErrors map[string]struct{} |
There was a problem hiding this comment.
How about
| missingFiles map[string]struct{} | |
| // files that had an error in Stat - we only log the first error. | |
| filesWithErrors map[string]struct{} | |
| missingFiles map[string]bool | |
| // files that had an error in Stat - we only log the first error. | |
| filesWithErrors map[string]bool |
plugins/inputs/filestat/filestat.go
Outdated
| if _, ok := f.missingFiles[fileName]; !ok { | ||
| f.Log.Warnf("File %q not found", fileName) | ||
| f.missingFiles[fileName] = struct{}{} | ||
| } | ||
| continue | ||
| } else { | ||
| delete(f.missingFiles, fileName) | ||
| } |
There was a problem hiding this comment.
I think you can simplify it to
| if _, ok := f.missingFiles[fileName]; !ok { | |
| f.Log.Warnf("File %q not found", fileName) | |
| f.missingFiles[fileName] = struct{}{} | |
| } | |
| continue | |
| } else { | |
| delete(f.missingFiles, fileName) | |
| } | |
| if ! f.missingFiles[fileName] { | |
| f.Log.Warnf("File %q not found", fileName) | |
| f.missingFiles[fileName] = true | |
| } | |
| continue | |
| } | |
| f.missingFiles[fileName] = false |
plugins/inputs/filestat/filestat.go
Outdated
| if _, ok := f.filesWithErrors[fileName]; !ok { | ||
| f.filesWithErrors[fileName] = struct{}{} | ||
| f.Log.Errorf("Unable to get info for file %q: %v", | ||
| fileName, err) | ||
| } | ||
| } else { | ||
| delete(f.filesWithErrors, fileName) |
There was a problem hiding this comment.
See above. Use a string/bool map.
If we encounter an error (file not found or otherwise), only log that error once, instead of every time the file gets stat'ed. * Introduce two new maps, for holding information on whether the last stat resulted in "file not found" or another error * Probe and set that value accordingly * Also fix a bug where the actual error encountered in Stat didn't get logged.
9486387 to
1b3fb2a
Compare
|
Restructured to use string/bool maps, hope that works for you |
| // files that were missing - we only log the first time it's not found. | ||
| missingFiles map[string]bool | ||
| // files that had an error in Stat - we only log the first error. | ||
| filesWithErrors map[string]bool |
There was a problem hiding this comment.
fyi, struct{} uses no memory.
| // files that were missing - we only log the first time it's not found. | |
| missingFiles map[string]bool | |
| // files that had an error in Stat - we only log the first error. | |
| filesWithErrors map[string]bool | |
| // files that were missing - we only log the first time it's not found. | |
| missingFiles map[string]struct{} | |
| // files that had an error in Stat - we only log the first error. | |
| filesWithErrors map[string]struct{} |
There was a problem hiding this comment.
Agreed that a struct{} map would use less memory, but then again it'll use about as much memory as the number of files configured to be checked, whose config structure already uses memory ... it comes out a wash, I feel like.
(cherry picked from commit b991aab)
This PR attempts to fix #7315, the same problem as #6940, but without adding a config flag.
Previously, the filestat plugin would check for a file's existence, and then try to compute all sorts of things for the missing file, which failed and resulted in a log message about possible permission errors.
Now, the plugin just skips the file if it exists. If an error is encountered probing an existing file, the plugin now logs that error, too, instead of hazarding a guess at permission problems.
Required for all PRs: