-
Notifications
You must be signed in to change notification settings - Fork 680
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
Added docker_ for containers and storage #888
base: master
Are you sure you want to change the base?
Conversation
Created by Are Tysland and Philip Gabrielsen
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.
Thank you for contributing this plugin!
Regarding the plugin name: there are already two docker-related plugins in the repository (see plugins/docker/
). Probably you should add your plugin to the same directory and find a more precise name for it (or extend/rewrite the existing plugins).
Regarding the overall structure of this plugin: maybe you want to turn it into a multigraph plugin. Simply output all configs/values at once and add a multigraph
line in between the sections.
This way you could also automatically generate graphs for all docker containers (instead of relying on the symlink configuration). This should make the usage of this plugin much more flexible (zero configuration).
What do you think?
@@ -0,0 +1,217 @@ | |||
#!/bin/bash | |||
# | |||
# Plugin to graph docker container and storage. |
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.
Please add a perldoc-style documentation to this plugin. Maybe take a look at plugins/munin/munin_events
for an example.
# | ||
# GNU GPL Are Tysland & Philip Gabrielsen | ||
|
||
TYPE=$( echo $0 | cut -f2 -d'_' ) |
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.
Please take care for proper quoting (here: echo "$0"
).
shellcheck
will show you all issues.
TYPE=$( echo $0 | cut -f2 -d'_' ) | ||
|
||
case $1 in | ||
autoconf) |
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 need to announce the autoconf
(and suggest
) capability in the documentation header.
Data_Space_Used) | ||
USED=$(numfmt --from=iec ${VAL}${UNT} ) | ||
TOTAL=$(numfmt --from=si $DATASPACETOTAL ) | ||
VALUE=`bc <<< "scale=2;$USED*100/$TOTAL"` |
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.
Maybe you want to use awk
for this calculation in order to avoid the dependency on bc
?
echo "$USED" "$TOTAL" | awk '{ print(($1 * 100) / $2);
What do you think? |
Ping? |
Stale pull request message |
Ping? |
Created by Are Tysland and Philip Gabrielsen