-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 input plugin for linux mdstat file #9061
Conversation
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.
🤝 ✅ CLA has been signed. Thank you!
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.
Thanks for the PR! I think mdstat would be a very useful input. I did a quick code review and found a couple things to work on. I can review a little more deeply to check the metric format and parsing once you get CI to pass.
go.sum
Outdated
@@ -24,6 +24,7 @@ code.cloudfoundry.org/clock v1.0.0 h1:kFXWQM4bxYvdBw2X8BbBeXwQNgfoWv1vqAk2ZZyBN2 | |||
code.cloudfoundry.org/clock v1.0.0/go.mod h1:QD9Lzhd/ux6eNQVUDVRJX/RKTigpewimNYBi7ivZKY8= |
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 PR changes a lot of lines in go.sum but the plugin itself only reads a file in proc. Why is go.sum changed so much? It seems unrelated.
CI dep checs aren't passing. You'll need to run 'go mod tidy' and commit the changes.
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.
You are right. I didn't do anything that would change the dependencies. go mod tidy
cleaned it up.
plugins/inputs/mdstat/mdstat.go
Outdated
// ############################################################################# | ||
// Telegraf plugin stuff | ||
// ############################################################################# | ||
type MDSTAT_PLUGIN struct { |
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.
Uppercase with underscore isn't go style. Identifiers (of types, variables, consts, funcs, etc) should be camelCase, with initial letter captialized only if they need to be exported.
CI runs a linter after dependency checks pass. I suspect the linter will not approve uppercase identifiers when it gets a chance to run.
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 have adjusted the identifiers to conform with the camelCase style.
plugins/inputs/mdstat/README.md
Outdated
```toml | ||
[[inputs.mdstat]] | ||
# The location of the mdstat file to read. | ||
# mdstat_file = /proc/mdstat |
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.
Other telegraf input plugins that read from proc look for an environment variable named HOST_PROC that allows the plugin to be used when telegraf is running in a container and the proc owned by the container host is somewhere other than /proc. Could you add this support? For examples, see inputs/bond, inputs/wireless, inputs/synproxy, inputs/linux_sysctl_fs.
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 went ahead and added this support. The main reason I had the mdstat_file config item was so I could throw arbitrary mdstat files at it for testing. You can do the same with the host_proc config item used in inputs/bond, so I replaced mine completely.
Thanks!. I will dig into these changes. |
… variable, replacing the mdstat_file config item in the process
…e if it fixes a ci failure affecting an entirely different plugin.
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
I noticed that we merged another mdstat input PR. @samckittrick could you take a look at #9101? Is there anything this mdstat plugin has that is missing in the merged one? |
Thanks for submitting this PR but I'm going to close this as #9101 is merged. If there are any enhancements that are missing from the new |
Required for all PRs:
Added an input plugin that parses the linux mdstat file and returns data on the status of raid devices managed by mdadm on the host.