Skip to content

plugins/filestat: Create option to disable logging of missing files#6940

Closed
nroach44 wants to merge 1 commit intoinfluxdata:masterfrom
nroach44:master
Closed

plugins/filestat: Create option to disable logging of missing files#6940
nroach44 wants to merge 1 commit intoinfluxdata:masterfrom
nroach44:master

Conversation

@nroach44
Copy link
Copy Markdown

@nroach44 nroach44 commented Jan 25, 2020 •
edited
Loading

Create configuration option NoLogFileMissing to prevent log spam when
the file is missing. For example, when using telegraf to check for
/var/run/reboot-needed (which is only present when the reboot is needed)
the agent will log the following each time the file is checked, and missing:

2020-01-25T15:23:20Z E! [inputs.filestat] Unable to get info for file "/var/run/reboot-required", possible permissions issue

This will allow us to disable this log alert.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@nroach44
plugins/filestat: Create option to disable logging of missing files
f2e80a2 
Create configuration option NoLogFileMissing to prevent log spam when
the file is missing. For example, when using telegraf to check for
/var/run/reboot-needed (which is only present when the reboot is needed)
the agent will log the following each time the file is checked, and missing:

2020-01-25T15:23:20Z E! [inputs.filestat] Unable to get info for file "/var/run/reboot-required", possible permissions issue

This will allow us to disable this log alert.
This was referenced Apr 11, 2020
Closed
Merged
@srebhan srebhan self-assigned this Nov 20, 2020
srebhan
srebhan requested changes Nov 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

I have two comments.

  1. The option you added is CamelCase while options usually are in snake_case. Please also change to snake_case.
  2. IMO you should output the information once to prevent unexpected behavior when specifying the option, the file is there but there is indeed a permission issue.

Speaking of this, maybe you could make this the new behavior without any option. If there is a problem stat-ing the file, output the error once and set a flag. Then do not output as long as the flag is set. Once you successfully accessed the file, unset the flag and next time you fail output once again a.s.o...

@sjwang90 sjwang90 added area/logging feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 24, 2020
@srebhan
Copy link
Copy Markdown
Member

srebhan commented Dec 3, 2020

@nroach44 any news? Are you still willing to carry on with this PR?

@nroach44
Copy link
Copy Markdown
Author

nroach44 commented Dec 13, 2020

@srebhan I can adjust the option casing but I don't have the go experience to perform the "output only once" change

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Dec 13, 2020

@nroach44 would appreciate the casing-changes. If you like we can work on the "output only once" together!?
The idea is that additional to adding NoLogFileMissing you also add a map (like a hashmap or maybe you know dict in python) that keeps the warning state for each file. That is add something like warnedFiles map[string]bool to the FileStat struct. Then in NewFileStat() you need to init that map by warnedFiles: make(map[string]bool). Now you do have the data-structure for doing the book-keeping. When you now warn you check if the file is already in warnedFiles and if so if the state is true e.g. if warned, ok := f.warnedFiles[fileName]; !ok || !warned { ... } then after warning you set the warning flag f.warnedFiles[fileName] = true. You also might want to reset that flag for files that were successfully queries to warn again...

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Jan 6, 2021

@nroach44 any chance you can adopt my suggestion? We can work this out together if you like!?

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Jan 26, 2021

@nroach44 any news on this one? Could you please at least fix the CamelCase issue!?

@sjwang90
Copy link
Copy Markdown
Contributor

sjwang90 commented Feb 9, 2021

Should we close this PR since we have this ready to be merged? #7316

@srebhan
Copy link
Copy Markdown
Member

srebhan commented Feb 10, 2021

I say yes on closing this. The PR you referenced does the same, is reviewed and doesn't add a config option.

@sjwang90 sjwang90 closed this Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srebhan srebhan srebhan requested changes

Assignees

@srebhan srebhan

Labels

area/logging feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nroach44 @srebhan @sjwang90