Add Chilli plugin for sessions state #751

Merged
merged 5 commits into from Jan 10, 2017

Projects

None yet

2 participants

@gmarsay
Contributor
gmarsay commented Sep 26, 2016

No description provided.

@sumpfralle

Your plugin looks good!

Maybe you want to change the name of the plugin to "chilli_sessions_"?

Please use double quotes for almost every variable access in order to avoid unwanted globbing or whitespace issues. Maybe shellcheck -s dash is of use for you.

plugins/chilli/chilli_
@@ -0,0 +1,118 @@
+#!/bin/bash
@sumpfralle
sumpfralle Oct 21, 2016 Contributor

Please change /bin/bash to /bin/sh, if possible. This will ease the usage of this plugin on platforms without a default installation of bash (embedded systems, some *BSD, AIX, ...).
The only required change below seems to be the rewriite of the tests:

  • replace double square brackets with single ones
  • replace == with =
plugins/chilli/chilli_
+ INSTANCES_LIST=$(ls /var/run/chilli_*.sock)
+
+ for file in $INSTANCES_LIST; do
+ echo $(basename $file .sock | cut -d _ -f 2)
@sumpfralle
sumpfralle Oct 21, 2016 Contributor

echo $(foo) is equivalent to foo

plugins/chilli/chilli_
+ config)
+ echo "graph_title Chilli $INSTANCE sessions"
+ echo "graph_args --base 1000 -l 0"
+ echo "graph_category chilli"
@sumpfralle
sumpfralle Oct 21, 2016 Contributor

Please pick a category from the list of common categories, if possible.

plugins/chilli/chilli_
+
+
+if [[ $INSTANCE == "total" ]]; then
+ STATE_PASS=$($CHILLI_PATH_BIN list | grep "pass" | wc -l)
@sumpfralle
sumpfralle Oct 21, 2016 edited Contributor

I do not know the output format of chilli. But maybe grep -w would make the matching safer.

@sumpfralle
Contributor

@gmarsay: what do you think?

@gmarsay
Contributor
gmarsay commented Jan 5, 2017

@sumpfralle : I updated the script by following your recommendations !
I would like to keep the category "chilli", other plugins will be added in this category.

@sumpfralle

Thank you!

I have a few last nitpicks. Afterwards it should be ready ...

+=head1 NAME
+
+chilli_sessions_ - Wildcard-plugin to monitor sessions state on Coova Chilli.
+
@sumpfralle
sumpfralle Jan 6, 2017 Contributor

Please add a short description (=head1 DESCRIPTION) of "chilli" (radius ...) and the scope of this plugin (number of active, blocked and unknown(?)) users/connections/devices(?) for each session (or is there a better word?).

plugins/chilli/chilli_sessions_
+ fi
+ ;;
+ suggest)
+ INSTANCES_LIST=$(ls /var/run/chilli_*.sock)
@sumpfralle
sumpfralle Jan 6, 2017 edited Contributor

This statement will not work with sockets containing a space. Additionally it neglects "CHILLI_PATH_SOCK".

How about this one?

find "$CHILLI_PATH_SOCK/" -name "chilli_*.sock" | while read file; do
    basename "$file" .sock | cut -d _ -f 2
done

This should also take care for shellcheck warnings regarding quoting.

plugins/chilli/chilli_sessions_
+
+ echo "total"
+
+ exit 0
@sumpfralle
sumpfralle Jan 6, 2017 Contributor

This indentation looks weird.

plugins/chilli/chilli_sessions_
+ STATE_DNAT=$("$CHILLI_PATH_BIN" list | grep -wc "dnat")
+ STATE_NONE=$("$CHILLI_PATH_BIN" list | grep -wc "none")
+else
+ STATE_PASS=$("$CHILLI_PATH_BIN" -s $CHILLI_PATH_SOCK/chilli_"$INSTANCE".sock list | grep -wc "pass")
@sumpfralle
sumpfralle Jan 6, 2017 Contributor

This would fail, if $CHILLI_PATH_SOCK contained whitespace. Just quote the full socket patch instead of only the INSTANCE variable. IMO, this also enhances readability.

@gmarsay
Contributor
gmarsay commented Jan 10, 2017

@sumpfralle : I updated the script by following your recommendations, it's ok ?

@sumpfralle
Contributor

Yes, I like it. Thank you!

@sumpfralle sumpfralle merged commit 6789469 into munin-monitoring:master Jan 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment