Skip to content
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

Add filecount input plugin #4363

Merged
merged 6 commits into from
Jul 31, 2018
Merged

Conversation

sometimesfood
Copy link
Contributor

This pull request introduces a new input plugin named filecount. The filecount plugin counts files in a directory that match certain criteria (e.g. "name", "size", "mtime" etc.).

While filecount is modeled after the collectd filecount plugin, the two plugins do not share any code.

Required for all PRs:

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

@danielnelson
Copy link
Contributor

This will be handy, but I think the implementation should use filepath.Walk to reduce the amount of code.

@sometimesfood
Copy link
Contributor Author

You are right of course. Sorry, I sat on this code for a while and completely forgot that I had planned on refactoring it to use filepath.Walk before submitting a pull request.

I'll clean up the required changes later this week.

@danielnelson danielnelson added this to the 1.8.0 milestone Jul 11, 2018
@phemmer
Copy link
Contributor

phemmer commented Jul 18, 2018

Since this plugin uses things like "size" and "mtime" as filter parameters, should the related metrics be exported as fields? For example: "min size found", "max size found", "total size", "oldest age", "newest age".
And then if this is done, maybe the postfix plugin could be re-written as a wrapper around this one.

@danielnelson
Copy link
Contributor

This seems nice, I was a little uneasy with a one field plugin. Here are some proposed field names for the metrics you mentioned, but I think it would be fine to add these either before or after merging this PR.

  • count
  • size_bytes
  • file_size_bytes_min
  • file_size_bytes_max
  • file_mtime_min
  • file_mtime_max

@phemmer
Copy link
Contributor

phemmer commented Jul 18, 2018

Is the proposed file_mtime_min (and _max) an absolute timestamp, or an offset? I don't know that an absolute time would be of use, but maybe.

@sometimesfood
Copy link
Contributor Author

I'm not a huge fan of adding hardcoded min/max fields. The original collectd filecount plugin has an additional aggregate file size field (equivalent to @danielnelson's size_bytes field) and I think that would make sense here as well.

Regarding the postfix plugin, I think it would be a better idea to extract shared code and move it into a separate helper that could then be used by both plugins rather than adding all the fields needed by the postfix plugin to the filecount plugin. I could open another feature branch so we can give this a try and see how it would improve the existing codebase.

However, if it's OK with you, I think it would be a good idea to merge this pull request first and add new fields/extract shared code/do whatever you decide to do in a separate pull request. None of the proposed changes would break any existing interfaces and it would be nice to keep the scope of this pull request as small as possible.

@sometimesfood
Copy link
Contributor Author

Btw, the "extract shared code" route would have another benefit: I am fairly certain that there is a number of input plugins that would profit from extracting the aforementioned helper. The filestat input plugin would be an obvious first candidate, but I'm sure other plugins share similar code as well.

@danielnelson
Copy link
Contributor

@sometimesfood Can you fix the conflict or is it alright if I push to your branch?

@sometimesfood
Copy link
Contributor Author

@danielnelson Should be fixed now. Sorry, I didn't see your thumbs up reaction; GitHub doesn't send out notifications for those.

@sometimesfood
Copy link
Contributor Author

Yikes. Somehow the merge got mangled. Is it okay if I fix this, rebase to master and do a force push?

@danielnelson
Copy link
Contributor

Yes, that will work great.

@sometimesfood
Copy link
Contributor Author

Thanks, done.

@danielnelson danielnelson requested a review from glinton July 30, 2018 22:24
@glinton glinton merged commit 228efe9 into influxdata:master Jul 31, 2018
@sometimesfood sometimesfood deleted the filecount-plugin branch August 4, 2018 22:05
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants