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

feature: Wireless Sensors Overhaul #6471

Merged
merged 19 commits into from May 2, 2017

Conversation

Projects
None yet
7 participants
@murrant
Member

murrant commented Apr 20, 2017

Includes client counts for ios and unifi
Graphing could use some improvement.
Alerting and threshold ui not implemented

DO NOT DELETE THIS TEXT

Please note

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

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

TODO:

  • Discovery
  • Polling
  • UI (Graphs and Custom thresholds)
  • Alerting
  • Documentation
  • Testing - Deferred too much is needed still
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 20, 2017

Thank you for submitting a PR @murrant! We have found the following @jquagga, @SaaldjorMike and @guillemmateos based on the history of these files to review this PR.

mention-bot commented Apr 20, 2017

Thank you for submitting a PR @murrant! We have found the following @jquagga, @SaaldjorMike and @guillemmateos based on the history of these files to review this PR.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 20, 2017

Member

I've not checked through the code much but 2 things:

  1. I think we should do all 'vendor' work in one file (we should do this in sensors) so includes/discovery/wireless/unifi.inc.php would contain all wireless sensor work rather than splitting it out (imho).

  2. The code that's commented out for alerting on eventlog, in the sensors code that's a hang up from the old alerting system, should we use it still as rules are defined by the user so they may not care about this info, if they do, they create an alert rule?

Member

laf commented Apr 20, 2017

I've not checked through the code much but 2 things:

  1. I think we should do all 'vendor' work in one file (we should do this in sensors) so includes/discovery/wireless/unifi.inc.php would contain all wireless sensor work rather than splitting it out (imho).

  2. The code that's commented out for alerting on eventlog, in the sensors code that's a hang up from the old alerting system, should we use it still as rules are defined by the user so they may not care about this info, if they do, they create an alert rule?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf
Member

laf commented Apr 20, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 20, 2017

Member

Indeed, these will all be alertable. With default rules and user overrides hopefully.

Member

murrant commented Apr 20, 2017

Indeed, these will all be alertable. With default rules and user overrides hopefully.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Apr 21, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Apr 21, 2017

CLA assistant check
All committers have signed the CLA.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 22, 2017

Member

If anyone has an snmpwalk of the data this would use so I can test that would be ace.

Member

laf commented Apr 22, 2017

If anyone has an snmpwalk of the data this would use so I can test that would be ace.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 22, 2017

Member

The Cisco walk I have is from Rosiak it is not annonymized. Unified is from a live device.

Member

murrant commented Apr 22, 2017

The Cisco walk I have is from Rosiak it is not annonymized. Unified is from a live device.

Show outdated Hide outdated sql-schema/187.sql
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 23, 2017

Member

I'm reorganizing the code structure of this, so hold tight.

Member

murrant commented Apr 23, 2017

I'm reorganizing the code structure of this, so hold tight.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 25, 2017

Member

Updated!

Left to implement:
UI (Graphs and Custom thresholds)
Alerting
Documentation
Testing - Deferred too much is needed still

Member

murrant commented Apr 25, 2017

Updated!

Left to implement:
UI (Graphs and Custom thresholds)
Alerting
Documentation
Testing - Deferred too much is needed still

@FTBZ

This comment has been minimized.

Show comment
Hide comment
@FTBZ

FTBZ Apr 26, 2017

Contributor

This is a really good news and I'm available to help and test if needed (Cisco WLC here).

  • Can this refactor used to add users by SSID like Cacti does?
    #4294

  • The graph for AP count isn't placed on the correct page now and it's impossible to get notification when an AP is down: #4295

Contributor

FTBZ commented Apr 26, 2017

This is a really good news and I'm available to help and test if needed (Cisco WLC here).

  • Can this refactor used to add users by SSID like Cacti does?
    #4294

  • The graph for AP count isn't placed on the correct page now and it's impossible to get notification when an AP is down: #4295

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 26, 2017

Member

@FTBZ Mikrotik displays user counts by SSID. Unifi, I'm considering adding both radio and ssid counts for clients. Basically, we can break them out as much as the snmp data allows :)

I haven't delved into controllers yet, but I've made affordances for that. We already have the ability to collect APs, and I've added the ability to reference those APs in wireless sensors. Controllers may need to have AP polling added, not sure about that yet. Can't make this PR too big :)

Member

murrant commented Apr 26, 2017

@FTBZ Mikrotik displays user counts by SSID. Unifi, I'm considering adding both radio and ssid counts for clients. Basically, we can break them out as much as the snmp data allows :)

I haven't delved into controllers yet, but I've made affordances for that. We already have the ability to collect APs, and I've added the ability to reference those APs in wireless sensors. Controllers may need to have AP polling added, not sure about that yet. Can't make this PR too big :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 26, 2017

Member

@murrant Feel free to merge this when you're happy.

Member

laf commented Apr 26, 2017

@murrant Feel free to merge this when you're happy.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Apr 27, 2017

Member

After Sunday release sometime.

Member

murrant commented Apr 27, 2017

After Sunday release sometime.

@murrant murrant added the Enhancement label Apr 28, 2017

@murrant murrant added this to the May 2017 Release milestone Apr 28, 2017

@murrant murrant changed the title from feature: Wireless Sensors to feature: Wireless Sensors Overhaul Apr 28, 2017

@murrant murrant added the Schema label Apr 28, 2017

murrant added some commits Apr 20, 2017

feature: Wireless Sensors
Includes client counts for ios and unifi
Graphing could use some improvement.
Alerting and threshold ui not implemented

WIP: starting OO based wireless sensors.

Class based functionality working
remove old functional files
add schema file

discovery needs to be enabled, not polling

fix up schema

fix Unifi discovery not returning an array

Add some debug when discovering a sensor.
Fix style.

Add missing semicolin

Add a null object (Generic) for OS.
Fill out some phpdocs

Re-organized code
Each sensor type now has it's own discovery and polling interface
Custom polling tested with Unifi CCQ

Left to do:
Implement UI (Graphs and Custom thresholds)
Alerting
Testing

Fix event message text

Remove runDiscovery and runPolling from OS, they are unused and don't belong there.

Cleanups/docs

Missed this file.

Remove the requirement to fetch the current value to check validity.
Do that automatically if current is not specified
A few cleanups here and there

First pass at graphing.
device_ and wireless_ graphs added.

Add RouterOS support

Singleton OS instance isn't required right now.
Remove that to allow some memory to be freed.

Add wireless to the device list metrics.
Make all metrics clickable

Tweak graphs a bit

Implement limit configuration page.
Use sensors page as common code instead of duplicating.
Clean up some javascript interactions:  Allow enter on values to save. Cancel if update is not needed. Enable the clear custom button after setting a custom value.
Add some wireless alert rules to the library.

Add documentation.

Add unifi client counts by ssid in addition to radio.
Optimize Sensor polling a bit.

Add HP MSM clients support (for full controller)
Fix function accessibility

Formalize the discovery and poller interfaces.

Add Xirrus clients and noise floor
move module interfaces to a more appropriate place.
push caching code up to os, unsure about this do to the limitations

No point in selectively enabling wireless discovery.  We only discover if the device supports something.

Add RSSI, Power, and Rate.
Add these sensors for Ubnt Airos.
Clean up some copyrights.

Reduce the amount of files need to add new types.
Leave graph files for consistency and to allow customization.

Remove the old wifi clients graph completely.
ciscowlc should have improved counts (total and per-ssid)

Schema didn't get added.

Impelement the rest of the AirOS sensors
Reformat and re-organize the Airos.php class.

Add several UBNT AirFiber sensors

A few fixes add links to the section headers

Add HP MSM mibs.
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 28, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

Add wireless menu to view sensors across all devices.
Icons in the menu need help :/
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 28, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 28, 2017

Member

#6327 possibly has data you can use.

Member

laf commented Apr 28, 2017

#6327 possibly has data you can use.

Add HeliOS, Mimosa, and Siklu support
Sensors added SNR + Noise
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 29, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 30, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented Apr 30, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented May 1, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant May 1, 2017

Member

I'm almost ready to push this, but I would like to get the power/signal issue figured out.
https://community.librenms.org/t/improve-wifi-collection-and-graphing-code/453/13?u=murrant

Member

murrant commented May 1, 2017

I'm almost ready to push this, but I would like to get the power/signal issue figured out.
https://community.librenms.org/t/improve-wifi-collection-and-graphing-code/453/13?u=murrant

murrant added some commits May 1, 2017

Fix overview graph frequency display
A few decimal place tweaks
Don't allow switching sensor and wireless-sensor graphs, it doesn't w…
…ork.

Change individual distance graphs to use si units
Go through the OS and make sure I got all the sensors I can (probably…
… missed some still)

Because pollWirelessChannelAsFrequency() is generic and a little complex, so pull it up to OS.
Message to help developers adding supports that don't return an array from discover functions.
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant May 2, 2017

Member

Preview :)

image

Member

murrant commented May 2, 2017

Preview :)

image

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented May 2, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

Remove noise and signal for now at least
A couple more fixes
Add a notification
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented May 2, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented May 2, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 2, 2017

The inspection completed: 10 new issues, 184 updated code elements

scrutinizer-notifier commented May 2, 2017

The inspection completed: 10 new issues, 184 updated code elements

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

LibreNMS-CI commented May 2, 2017

Auto-Deploy finished, Test PR at http://6471.ci.librenms.org or https://6471.ci.librenms.org

@murrant murrant merged commit 4c0412b into librenms:master May 2, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant May 2, 2017

Member

Thanks to @FTBZ, @Rosiak, and robdb on IRC for test data!

Member

murrant commented May 2, 2017

Thanks to @FTBZ, @Rosiak, and robdb on IRC for test data!

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 2, 2017

Member

image

Member

laf commented May 2, 2017

image

@mattie47 mattie47 referenced this pull request May 11, 2018

Merged

device: Allied Telesis Wireless Support #8692

1 of 1 task complete
@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.