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

[widget] Top Devices #13748

Merged
merged 2 commits into from
Feb 12, 2022
Merged

[widget] Top Devices #13748

merged 2 commits into from
Feb 12, 2022

Conversation

Npeca75
Copy link
Contributor

@Npeca75 Npeca75 commented Feb 3, 2022

calculate bitrate only from ports which are UP

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.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

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
Copy link
Contributor

Hi @Npeca75
Could you describe your use case ?
Cause most of the time, an interface down is not propagating any traffic (so this change is useless and harmless). And in a few particular types of interfaces, even in operdown status, propagation occurs (and you may still want to see it).
Thx

@Npeca75
Copy link
Contributor Author

Npeca75 commented Feb 3, 2022

hi @PipoCanaja
i stumbled in this bug when i shut down one of high bitrate port
so, device was still on top of list, because TopDevices widget does not consider which ports are up or down, simply SUM all ports.
Expected behavior is, if device was on top, and you shut down a port, widget need to reflect these changes

@Npeca75
Copy link
Contributor Author

Npeca75 commented Feb 3, 2022

@PipoCanaja

to be more specific, ports which are DOWN are left "as is" in DB with last data, so they are never zeroed out

@PipoCanaja
Copy link
Contributor

hi @PipoCanaja i stumbled in this bug when i shut down one of high bitrate port so, device was still on top of list, because TopDevices widget does not consider which ports are up or down, simply SUM all ports. Expected behavior is, if device was on top, and you shut down a port, widget need to reflect these changes

Do you use "per port polling" by any chance ?

@Npeca75
Copy link
Contributor Author

Npeca75 commented Feb 3, 2022

"per port polling"

no, nothing like that :)

@ottorei
Copy link
Contributor

ottorei commented Feb 7, 2022

@PipoCanaja After looking at the code it seems that the widget just sums ingress and egress bits from all available interfaces. If there was a port with massive amounts of data transferred and that port went down for any reason, looking at the sum itself would skew the result if other ports would not compensate, right? The device would still be listed as the top device even when it would not pass much traffic at all. I think the calculation should only sum the ports that were up during the last poll or were up during the last n-minutes. This is for the real time view.

@Npeca75
Copy link
Contributor Author

Npeca75 commented Feb 7, 2022

hi @ottorei

we discussed this case with @murrant on discord
there is 2 solution, one to constant fill port counters with zero, for ports which are down (very ugly solution)
or
as @murrant suggested, to skip counting "down" ports in widget
so i made this modification

@PipoCanaja PipoCanaja merged commit c7b63b5 into librenms:master Feb 12, 2022
@PipoCanaja
Copy link
Contributor

All right, merged. Thanx for your new contribution @Npeca75

@Npeca75 Npeca75 deleted the widget-topdevices branch February 13, 2022 03:48
@librenms-bot
Copy link

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

https://community.librenms.org/t/22-2-0-release/18177/1

@librenms-bot
Copy link

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

https://community.librenms.org/t/22-2-0-changelog/18177/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants