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

Sorted ports, loadbalancer vips, pools by ifName and label #9753

Closed
wants to merge 2 commits into from

Conversation

Serazio
Copy link
Contributor

@Serazio Serazio commented Jan 28, 2019

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.

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.

@CLAassistant
Copy link

CLAassistant commented Jan 28, 2019

CLA assistant check
All committers have signed the CLA.

@murrant
Copy link
Member

murrant commented Jan 28, 2019

The only thing is ordering gets wierd because ge1-10 comes before ge1-2.

@Serazio
Copy link
Contributor Author

Serazio commented Jan 28, 2019

I'll check later if "ORDER BY LENGTH(ifName) , ifName" will work better here.

@PipoCanaja
Copy link
Contributor

On some devices, sorting by ifIndex will probably provide the expected result, and the computing cost would be the best (sorting integers is always better than strings) but we'll not find a solution that fits all devices ...

@Serazio
Copy link
Contributor Author

Serazio commented Feb 1, 2019

I don't think that the performance aspect should matter here.
I checked the query time with an juniper device with over 2900 interfaces and the times are not that higher.

If this sorting would happen while polling, i totally agree with you, as this will generate a lot of overhead. But in this case it's just the overview and the bills

$ time mysql -hxxx -ulibrenms -p123 librenms -e "select * from ports where device_id = 1050 ORDER BY LENGTH(ifName), ifName;" > time.test

real 0m0.081s
user 0m0.020s
sys 0m0.013s

$ time mysql -hxxx -ulibrenms -p123 librenms -e "select * from ports where device_id = 1050 ORDER BY ifName;" > time.test

real 0m0.085s
user 0m0.035s
sys 0m0.004s

$ time mysql -hxxx -ulibrenms -p123 librenms -e "select * from ports where device_id = 1050" > time.test

real 0m0.067s
user 0m0.029s
sys 0m0.007s

$ time mysql -hxxx -ulibrenms -p123 librenms -e "select COUNT(*) as count from ports where device_id = 1050"
+-------+
| count |
+-------+
| 2921 |
+-------+

real 0m0.014s
user 0m0.009s
sys 0m0.000s

@murrant
Copy link
Member

murrant commented Feb 7, 2019

@Serazio we weren't debating performance.

We were saying MySQL queries do alphabetical sort, humans expect natural sort (https://en.wikipedia.org/wiki/Natural_sort_order)

LENGTH(ifName), ifName; kludge might be close enough.

@PipoCanaja
Copy link
Contributor

@Serazio You could also try to reorder the result of the SQL query in PHP using natsort(). That may help getting a correct natural display.

@Serazio
Copy link
Contributor Author

Serazio commented Apr 3, 2019

I think i found a suitable way, to do this sorting.
Basicly the following works for the overview:

    $results = dbFetch("SELECT * FROM 'ports' WHERE device_id = ? AND 'deleted' != '1' AND 'disabled' = 0", array($device['device_id']));
    usort($results, function($a, $b) {
        return strnatcasecmp($a['ifName'], $b['ifName']);
    });

    foreach ($results as $data) {

I'll update the pull request in the next hours

@laf laf added the WebUI label Apr 3, 2019
@Serazio Serazio changed the title Sort ports by ifName, instead of time of db insertion in Traffic Bills and Device Overview Sorted ports, loadbalancer vips, pools by ifName and label Apr 4, 2019
@laf
Copy link
Member

laf commented Apr 9, 2019

On some devices, sorting by ifIndex will probably provide the expected result, and the computing cost would be the best (sorting integers is always better than strings) but we'll not find a solution that fits all devices ...

I can't help but agreeing with this but for a slightly different reason. Ordering by ifIndex (for ports overview) means you get them grouped on the type typically, ordering by name now means they ports are alphabetic and all over the show.

This is one of the things that a per user setting would be ideal.

@murrant
Copy link
Member

murrant commented Jun 5, 2019

Yeah, I think ifIndex would be best here for device oriented port lists.

@murrant murrant added the User-Pending Currently waiting for user response label Jun 5, 2019
@laf
Copy link
Member

laf commented Aug 20, 2019

@Serazio Any update on this? You will also need to rebase.

@Jellyfrog
Copy link
Member

Closing due to inactivity. Feel free to reopen!

@Jellyfrog Jellyfrog closed this Dec 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
User-Pending Currently waiting for user response WebUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants