Add nut server monitor #3967

johnelliott · 2023-10-30T20:58:07Z

Description

Resolves #3932

Hello and thank you to @CommanderStorm for on-boarding me to the project in #3932

This is my first PR to the project. I read the contributions guide and I think I'm going about the style correctly.

Some questions I had were regarding the database schema,

Otherwise I added a separate file for the monitor as Frank suggested, used the new Knex migrations, and modeled the data after the way MQTT worked since the authentication of NUT isn't http basic auth.

Some shortcomings of this patch are that I wasn't sure how to deal with isImportant and the current patch creates some noise with the toasts popping up for each ping. I'd appreciate any help that can point me in towards solving that.

Type of change

New feature (non-breaking change which adds functionality)

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas
(including JSDoc for methods)

Most of the added code is anonymous functions and doesn't ad library code for other code to use, so I didn't add extra documentation

My changes generate no new warnings

I do see a warning I would like help with:

WARN: The ping is not effective when the status is DOWN

Screenshots

Edit Monitor

CommanderStorm · 2023-10-30T21:38:02Z

db/knex_migrations/2023-10-23-add-nut-monitor.js

@@ -0,0 +1,20 @@
+exports.up = function (knex) {


In the description you mentioned the following, what are your questions?

Some questions I had were regarding the database schema,

I wanted to know if these column names are OK and in keeping with the conventions, I think I did the right thing based on what else I saw.

CommanderStorm · 2023-10-30T21:38:21Z

server/monitor-types/nut.js

+
+        const nut = new Nut(monitor.port, monitor.hostname);
+
+        nut.on("ready", () => {


This might be me not knowing js enough, but I think this function is racable (have not debugged this, just a hunch)

First observation is, that none of the functions which are called are actually blocking (): https://www.npmjs.com/package/node-nut?activeTab=code

if Nut does not immediatly become ready, heartbeat being set might be missed as the event loop has already exited this function

I think this might be why you are seeing

WARN: The ping is not effective when the status is DOWN

I think the following code looks more promissing:
https://github.com/skarcha/node-nut/blob/2729b2f3a6d1bb9d9105c68c0b1cdc62933c0f81/examples/polling.js

CommanderStorm · 2023-10-30T21:50:35Z

server/monitor-types/nut.js

+    /**
+     * @inheritdoc
+     */
+    async check(monitor, heartbeat, _server) {


node-nut does not seem to include a timeout function.
Given that this is doing network calls, adding such an option would be a nice touch.

CommanderStorm · 2023-10-30T21:52:38Z

package.json

@@ -166,6 +166,7 @@
        "favico.js": "~0.3.10",
        "jest": "~29.6.1",
        "marked": "~4.2.5",
+        "node-nut": "^1.0.3",


Unsure if this library is a good idea…
skarcha/node-nut#9 (comment)

server/monitor-types/nut.js

CommanderStorm · 2023-10-30T22:08:32Z

src/pages/EditMonitor.vue

+
+                                <div class="my-3">
+                                    <label for="nutPassword" class="form-label">{{ $t("Password") }}</label>
+                                    <input id="nutPassword" v-model="monitor.nutPassword" type="password" class="form-control">


Could you utilise the HiddenInput component instead for all secret fields?

src/pages/EditMonitor.vue

CommanderStorm · 2023-10-30T22:18:35Z

src/pages/EditMonitor.vue

+                            <!-- For NUT Type -->
+                            <template v-if="monitor.type === 'nut'">
+                                <div class="my-3">
+                                    <label for="upsName" class="form-label">UPS {{ $t("Name") }}</label>


I think adding a description on where to obtain the correct ups name might be worth it to prevent other users spending excessive time on this field

OK I agree, do you think it makes sense to add it on the field description? If it's too long, where would that sort of documentation go?

Screenshot as an example of where it could live:

CommanderStorm · 2023-10-30T22:20:44Z

src/pages/EditMonitor.vue

+                            <!-- For NUT Type -->
+                            <template v-if="monitor.type === 'nut'">
+                                <div class="my-3">
+                                    <label for="upsName" class="form-label">UPS {{ $t("Name") }}</label>


I am unshure if a longer name like name of the uninterruptible power supply might be clearer and more importantly translate better.
In any case, I think it would be better if the word order ups name is correct in all languages (some are rtl) => It might be saver to include this in the translated string
Thoughts?

src/pages/EditMonitor.vue

Use template string suggestion Co-authored-by: Frank Elsinga <frank@elsinga.de>

CommanderStorm · 2023-10-31T08:06:15Z

Another (a bit more permananet) solution might be to store the monior config as a json string.
Thoughts?

Use searchable name Co-authored-by: Frank Elsinga <frank@elsinga.de>

CommanderStorm · 2024-08-24T17:00:49Z

@johnelliott
Thanks for your work.
I am going to close the PR as there has not been any progress in nearly a year (see my review from october ^^). The PR has too many things that would have to be fixed by maintainers otherwise (which I don't have the capacity for).
Most importantly, the work that would be required due to skarcha/node-nut#9 (comment) (forking and maintaining that fork) is not something that I can do.
If you/someone else wants to fix this PR up and most importanlt there is a solution to skarcha/node-nut#9 (comment), we can reopen the PR or discuss this in a new PR.

johnelliott added 13 commits October 30, 2023 13:47

NUT support rough

fb58ec7

Use existing port when editing monitor

f9fd532

Fix form label for nut

f01d6fe

Fix up some of nut monitor

779eb76

Fix up/down error

f62cace

Move name not to split mqtt names

2fb468b

Remove log line

4f605a7

Add support for NUT authentication

4f76058

Remove logging from feature development

ddf0d4c

Remove a done TODO

fecaec6

Remove debug logs

ee464c6

Clarify desire to copy vars

9bba305

Delete note :)

2d98cbe

CommanderStorm requested changes Oct 30, 2023

View reviewed changes

Update server/monitor-types/nut.js

09a6c19

Use template string suggestion Co-authored-by: Frank Elsinga <frank@elsinga.de>

This comment was marked as off-topic.

Sign in to view

Update src/pages/EditMonitor.vue

74a4fe2

Use searchable name Co-authored-by: Frank Elsinga <frank@elsinga.de>

This comment was marked as off-topic.

Sign in to view

louislam added this to the 2.1.0 milestone Nov 1, 2023

This comment was marked as off-topic.

Sign in to view

CommanderStorm mentioned this pull request Nov 21, 2023

Allow bulk upload monitors using a CSV file or JSON file. #4065

Closed

1 task

chakflying added the area:monitor Everything related to monitors label Dec 2, 2023

This comment was marked as spam.

Sign in to view

This comment was marked as resolved.

Sign in to view

CommanderStorm added question Further information is requested pr:needs review this PR needs a review by maintainers or other community members pr:please address review comments this PR needs a bit more work to be mergable labels May 19, 2024

github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label May 19, 2024

github-actions bot added pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024

CommanderStorm closed this Aug 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nut server monitor #3967

Add nut server monitor #3967

johnelliott commented Oct 30, 2023 •

edited by CommanderStorm

Loading

CommanderStorm Oct 30, 2023

johnelliott Oct 30, 2023

CommanderStorm Oct 30, 2023

CommanderStorm Oct 30, 2023

CommanderStorm Oct 30, 2023

CommanderStorm Oct 30, 2023

CommanderStorm Oct 30, 2023

johnelliott Oct 31, 2023

CommanderStorm Oct 30, 2023 •

edited

Loading

This comment was marked as off-topic.

CommanderStorm commented Oct 31, 2023 •

edited

Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as spam.

This comment was marked as resolved.

CommanderStorm commented Aug 24, 2024


		const nut = new Nut(monitor.port, monitor.hostname);

		nut.on("ready", () => {

Add nut server monitor #3967

Add nut server monitor #3967

Conversation

johnelliott commented Oct 30, 2023 • edited by CommanderStorm Loading

Description

Type of change

Checklist

Screenshots

Edit Monitor

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CommanderStorm Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

CommanderStorm commented Oct 31, 2023 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as spam.

This comment was marked as resolved.

CommanderStorm commented Aug 24, 2024

johnelliott commented Oct 30, 2023 •

edited by CommanderStorm

Loading

CommanderStorm Oct 30, 2023 •

edited

Loading

CommanderStorm commented Oct 31, 2023 •

edited

Loading