Update wifi_clients graph #4846

Merged
merged 5 commits into from Nov 1, 2016

Projects

None yet

5 participants

@jquagga
Contributor
jquagga commented Oct 21, 2016

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

As part of my working on what ultimately became PR #4845, I updated the wifi_clients_graph code to use the generic multi line graph. It previously was a custom generated graph. This doesn't add a whole lot overall so it might not be worth committing. It does clean up the legend nicely. Since I built it anyway, I figure I might as well offer it up.

Here's a before (above) and after (below).
out-fs8

@jquagga jquagga Update wifi_clients to use generic_multi_line dc38f2d
@FTBZ
Contributor
FTBZ commented Oct 21, 2016

I prefer the values from the first screen shot, why using decimal, it's not possible to have a half of one Wi-Fi user.

@jquagga
Contributor
jquagga commented Oct 21, 2016 edited

The rrd graph graphs the average client count as did the previous code. So while the previous code may have truncated the output in the legend, it too had decimal client counts in the graph. I thought that was done to smooth changes in the graph?

Edit: Not sure that is accurate since it seems to converge to integers on either graph.

@jquagga
Contributor
jquagga commented Oct 21, 2016

I guess you could use the LAST graphing strategy which would eliminate the decimal client counts but could make a more jagged graph. That would be different than what was done before. Or you could just reject the PR and leave the legend truncated :-).

@jquagga
Contributor
jquagga commented Oct 21, 2016

Actually looking at the graphs, the values seems to converge on integers anyway other than for the average value which isn't in the original graph.

@jquagga jquagga Revert to existing wifi_clients.inc.php; now LAST.
The first commit of this PR moved to using generic_multiline to graph an
Average of wifi clients and have a legend showing average wifi clients.
This lead to the possiblity of decimal wifi_clients in the legend.

This commit goes the opposite direction.  It retains the original custom
graph (although freshens it up) and changes the graph strategy to LAST.

This means the legend will now match the graph (which it does NOT in the
present code which graphs and average and prints a LAST value).  The
cost is a somewhat more jagged graph since it jumps from integer to
integer value.
74da617
@jquagga jquagga changed the title from Update wifi_clients graph to use generic_multi_line to Update wifi_clients graph Oct 24, 2016
@jquagga jquagga Merge remote-tracking branch 'upstream/master' into wifi_graph
2272834
@jquagga
Contributor
jquagga commented Oct 24, 2016 edited

The first commit of this PR moved to using generic_multiline to graph an Average of wifi clients and have a legend showing average wifi clients. This lead to the possiblity of decimal wifi_clients in the legend.

This commit goes the opposite direction. It retains the original custom graph (although freshens it up) and changes the graph strategy to LAST.

This means the legend will now match the graph (which it does NOT in the current librenms code which graphs and average and prints a LAST value).

The cost is a somewhat more jagged graph since it jumps from integer to
integer value.

@laf
Member
laf commented Oct 24, 2016

Do you have a screenshot to share?

@laf laf added the Graphs label Oct 24, 2016
@jquagga
Contributor
jquagga commented Oct 24, 2016

Sure, here goes.

The 3 graphs below are the same time period, with the 3 graphing options. From Top to Bottom:

  • Existing LibreNMS code - Average Graph and Last Legend.
  • First Commit in this PR - Average Graph and Average Legend.
  • Second Commit - LAST Graph and Last Legend

out-fs8

I'm partial to the middle option but I understand why people could have issues with that (you can't have half a client). But you can have an average of half a client. I also think that the generic graph code generates a prettier graph than the custom graphs in 1 or 3 but I think that's minor.

@laf
Member
laf commented Oct 24, 2016

Can people use the 👍 / 👎 reaction to the first comment pls so we can take a vote on option 2 or option 3.

@laf
Member
laf commented Oct 30, 2016

@jquagga No further input provided. If you can put the code back to how it is for option 2 we can merge.

@jquagga jquagga Restore average wifi graph
This reverts commit 74da617.
1d4ad71
@jquagga
Contributor
jquagga commented Oct 30, 2016 edited

@laf Ok, we should now be back at "Option 2" code. The "pretty" average graph with average legend.

@laf
Member
laf commented Oct 31, 2016

Actually sorry I've just realised, the code now will only show radio2 graph or radio1. Before it showed radio1 and radio2 (if it existed). How come this has changed?

@jquagga
Contributor
jquagga commented Oct 31, 2016

Good day,

I believe this will graph both radios.

How this works is that if radio2 exists, it builds a DS array of radio1 AND 2 so it graphs both of them. If radio2 doesn't exist, then it elseif to see if radio1 exists. If it does, then it builds an array of only radio1.

When I did this originally I thought about doing a radio1 if and then a radio2 else if which array_pushed the radio2 ds information into the array. However I read that array_push had some overhead as it was a function call. This way was more pedantic, but goes in descending order (checks if radio2 exists and if so populates radio1 and radio2, and if not checks if radio1 exists and only populates radio1).

I think the long term solution would be to update both this graphing code and the wifi polling code to use loops to populate and then graph however many radios are polled. I thought about doing that, however I think that if you're polling / populating an arbitrary number of radios, you'd also want to set descriptions for those radios in the polling code. That became a bigger projects and at present, there aren't any devices being polled by LibreNMS which have more than the 2 radios.

This is the graph from the current librenms master, with this PR added on top:
graph-fs8

@jquagga jquagga Revise wifi_clients graph to loop
ab54889
@jquagga
Contributor
jquagga commented Oct 31, 2016

Ok, so this latest commit changes the polling strategy to a while loop.

Set up rrdfile, and counter and push that into while loop. while the rrdfile is a rrdfile, add to the rrd_list array the ds, description, and filename. Then update the counter and rrdfile and try again.

The big advantage here is that this should no longer be limited to just 2 radios. I copied over a rrd file from one of my APs to another and named it wificlients3.rrd and it added a 3rd line to the graph. It sets the stage for radios with more than 2 radios (but the polling code has to be updated to support that).

graph-fs8

@scrutinizer-notifier

The inspection completed: 2 new issues

@laf laf merged commit b4b1c8a into librenms:master Nov 1, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jquagga jquagga referenced this pull request Nov 1, 2016
Merged

Update wifi clients polling to support more than 2 radios #4913

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment