net/vnstat: dashboard widget#5336
Conversation
c73cb32 to
b73a1c0
Compare
b73a1c0 to
bda8564
Compare
|
i am trying to figure how to do that. the sizeToContent seems like its just a suggested size and almost all existing widgets have scrollbars and don't resize automatically to content, with the exception of the interface statistics widget. there is a lot of text in this widget, so if its squashed to 1 column, it looks weird, but its not obvious how to require 2+ columns. i've added an option to show/hide the avg rate column, which should help ppl who want a smaller widget, but still 4 columns don't really render under one column. open to suggestions, but also prefer not over engineer this simple and helpful widget |
bda8564 to
50cc26c
Compare
|
ok i improved the height resizing, but still limited the sizeToContent to 1000px, which is more than all the other widgets currently, which seems to be 650px max. so scrollbar can appear in edges cases. i also added another config option to exclude selected interfaces from the widget, with the defaults of enc0, pflog0, pfsync0. not sure if there is a better way to get the list of interfaces, currently i am getting them from the vnstat database vs the system interfaces. either choose has pros and cons |
50cc26c to
06ae5fd
Compare
|
i know a dashboard widget isn't super critical, but at the same time, its annoying to keep having to rebase, etc. would it be possible to this this merged or provide whatever other feedback? |
|
I can request a review from the plugin maintainer for you. Done. |
06ae5fd to
416a3a1
Compare
29c9413 to
46d0e17
Compare
|
i am really confused by this PR. i am not trying to be disrespectful, but this has been open for more than a month. on the forums, opnsense folks are always saying "PRs welcome" .. yet this doesn't feel that way. maybe i am wrong and this is the average for the time a PR stays open and unreviewed |
46d0e17 to
7a040c7
Compare
|
You are inside the plugin repo with this PR and here are multiple maintainers each maintaining one or multiple plugins. For example, I maintain caddy, ndp-proxy-go, ndproxy (Tier 3) and also contribute to some Tier 2 and 1 plugins (frr, opnwaf). Each maintainer is listed in the Makefile of the individual plugin. Some plugins with higher tiers have priority and are directly supported by opnsense main developers. So things like these just take a while if nobody has time I'm afraid. |
| $interfaces = array_map('trim', explode("\n", $response)); | ||
| $interfaces = array_values(array_filter($interfaces, function ($v) { | ||
| return !empty($v); | ||
| })); |
There was a problem hiding this comment.
just how unreliable is the dbiflist output?
| * retrieve vnstat data as structured JSON for a specific interface | ||
| * @return array | ||
| */ | ||
| public function jsonAction() |
There was a problem hiding this comment.
this action name is too generic
| return array("error" => "Invalid or missing interface name"); | ||
| } | ||
| $backend = new Backend(); | ||
| $response = json_decode($backend->configdRun("vnstat json " . escapeshellarg($iface)), true); |
There was a problem hiding this comment.
| $response = json_decode($backend->configdRun("vnstat json " . escapeshellarg($iface)), true); | |
| $response = json_decode($backend->configdpRun('vnstat json', [$iface]), true); |
should do the same but canonically
| if (empty($iface) || !preg_match('/^[a-zA-Z0-9_.]+$/', $iface)) { | ||
| return array("error" => "Invalid or missing interface name"); | ||
| } |
There was a problem hiding this comment.
this seems spurious.. it's either an interface or not. even better to test the error when asking for garbage input (as an empty string can also be passed safely)
| $backend = new Backend(); | ||
| $response = json_decode($backend->configdRun("vnstat json " . escapeshellarg($iface)), true); | ||
| if ($response === null) { | ||
| return array("error" => "Failed to retrieve vnstat data"); |
There was a problem hiding this comment.
normally we use ['status' => 'failed'] everywhere. the error message is seldomly shown and not even translated
| message:request Vnstat interface list | ||
|
|
||
| [json] | ||
| command:/usr/local/bin/vnstat --json --iface %s |
| PLUGIN_VERSION= 1.3 | ||
| PLUGIN_REVISION= 1 | ||
| PLUGIN_REVISION= 2 |
There was a problem hiding this comment.
would be nicer to set version 1.4 and remove the revision since this is a big enough change
fichtner
left a comment
There was a problem hiding this comment.
The widget code itself feels a bit much for what it tries to achieve but I'm not objecting to it. There needs to be more review in any case.
| this.currentPeriod = 'month'; | ||
| this.currentInterface = null; | ||
| this.configurable = true; | ||
| this.excludedInterfaces = ['enc0', 'pflog0', 'pfsync0']; |
There was a problem hiding this comment.
this looks like something a backend or controller would ideally know about so it can be enforced?

Important notices
Before you submit a pull request, we ask you kindly to acknowledge the following:
If AI was used, please disclose:
Related issue
If this pull request relates to an issue, link it here: closes #5353
Describe the problem
Vnstat dashboard widget
Describe the proposed solution
Adds vnstat dashboard widget. Allows you to select interface, and period (hourly, daily, monthly, yearly)
It remembers the last selected interface/period
Preferences to select the update tick interval, defaulting to 5 minutes