Skip to content
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

VRP - Fix SSID Client count #12629

Merged
merged 9 commits into from Mar 26, 2021
Merged

VRP - Fix SSID Client count #12629

merged 9 commits into from Mar 26, 2021

Conversation

PipoCanaja
Copy link
Contributor

This PR fixes information that was misplaced:
Back when I added support for VRP wireless, only "per radio and per SSID" count was available. Resulting in a huge amount of sensors. And at that time no aggregation of wireless sensors was possible. So at the end of the day, it was impossible to visualize the amount of clients per SSID at all.

Now, both aspects improved:

  • We now have "per band and per SSID" which mean, today, twice as much data as expected for SSID client Count. So one sensor for 5Ghz and one for 2.4 Ghz.
  • We can aggregate those using wireless-sensors (creating a 3rd "Total" sensor for each SSID)

The number of clients per radio is of course still available in the AccessPoints screen.

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@PipoCanaja PipoCanaja added Device 🖥️ New or added device support Sensor Device sensors labels Mar 18, 2021
@PipoCanaja PipoCanaja self-assigned this Mar 18, 2021
//Clients (STA) for 2.4Ghz and 5Ghz
$sta5gTable = $this->getCacheTable('hwWlanSsid5gStaCnt', 'HUAWEI-WLAN-VAP-MIB', 3);
$sta2gTable = $this->getCacheTable('hwWlanSsid2gStaCnt', 'HUAWEI-WLAN-VAP-MIB', 3);
$staTable = array_merge_recursive($sta5gTable, $sta2gTable);
Copy link
Member

@murrant murrant Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snmpwalk_group() can accept an input array so you don't have to merge them. Also, it doesn't look like this data is used for any other sensor, so no need to cache them, that just wastes ram.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Will fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, snmpwalk_group only works with one OID, not an array. So the merge is still mandatory. But at least caching will be avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, like this (changed to snmpwalk_cache_oid because group does not allow specifying snmp flags):

        $staTable = snmpwalk_cache_oid($this->getDeviceArray(), 'hwWlanSsid', [], 'HUAWEI-WLAN-VAP-MIB', null, '-OQUsb');
        $staTable = snmpwalk_cache_oid($this->getDeviceArray(), 'hwWlanSsid2gStaCnt', $staTable, 'HUAWEI-WLAN-VAP-MIB', null, '-OQUsb');
        $staTable = snmpwalk_cache_oid($this->getDeviceArray(), 'hwWlanSsid2gStaCnt', $staTable, 'HUAWEI-WLAN-VAP-MIB', null, '-OQUsb');

Copy link
Member

@murrant murrant Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See how passing the same array inserts data into it. Much better than a recursive merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All index values must have an oid you can access from. Look at the snmp table, it is a member of the table. Does it return nothing from the captured data or the actual device? Because it would not be in the captured data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not this one. The index itself cannot be walked. And it is a string index.
So the only way to grab the index is to split the OID in the snmpwalk result. But doing so will only give me the index in numerical or in string (depending how I do the walk). And then I'll need to convert it to the other form (either numerical to string or string to OID).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation confirms the behaviour. The hwWlanSsid is the index, and cannot be walked by itself. So no other choice than extracting (and converting) the index from num to str or the opposite.

OID Object Name Syntax Max Access Description Implemented Specifications
1.3.6.1.4.1.2011.6.139.17.1.2.1.1 hwWlanSsid OCTET STRING not-accessible This object indicates the table index. This object is implemented as defined in the corresponding MIB file.
1.3.6.1.4.1.2011.6.139.17.1.2.1.2 hwWlanSsid2gStaCnt Integer32 read-only This object indicates the number of users at 2.4G frequency band. This object is implemented as defined in the corresponding MIB file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly fun :)

@murrant murrant merged commit aac57e6 into librenms:master Mar 26, 2021
@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/21-4-0-changelog/15528/1

@PipoCanaja PipoCanaja deleted the wlc-sta-count branch August 12, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Device 🖥️ New or added device support Sensor Device sensors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants