feat: Add SNMP Monitor #4717

mattv8 · 2024-04-27T01:18:36Z

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description

This PR introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol). The following changes have been made:

Added new fields for SNMP configuration, including community string, OID (Object Identifier), SNMP version, control value, and condition.
Updates the database schema to include SNMP-related columns.
Implemented SNMP check logic to determine device status based on SNMP values and configured conditions.
Utilizes net-snmp.

Addresses #1675

Type of change

Please delete any options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

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)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

@CommanderStorm

Thanks for pointing it out @CommanderStorm!

DB must allow nulls otherwise this will break other monitors.

knex requires down function

Updates appropriate values async when editing the SNMP monitor

mattv8 · 2024-04-30T21:51:04Z

@CommanderStorm I'm ready for review. It is not passing ES Lint, although when I run npm run lint:prod --fix there are some changes to server/model/monitor.js that are unrelated to this PR. I'm unsure as how to proceed on the linting side of things.

CommanderStorm

Please have a look at https://github.com/louislam/uptime-kuma/pull/4717/files or https://github.com/louislam/uptime-kuma/actions/runs/8902151438/job/24447507847?pr=4717 and fix the linting mistakes.
I don't know why your local environment is producing different linting results.

I am certain that a review at this time will miss too many things and will thus require another review cycle. Nevertheless, I have attached a few comments.

src/pages/.EditMonitor.vue.swp

src/pages/EditMonitor.vue

server/monitor-types/snmp.js

src/pages/EditMonitor.vue

db/knex_migrations/2024-04-26-0000-snmp-monitor.js

server/monitor-types/snmp.js

CommanderStorm · 2024-04-30T22:29:37Z

server/monitor-types/snmp.js

+                switch (monitor.snmpCondition) {
+                    case '>':
+                        heartbeat.status = numericValue > monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '>=':
+                        heartbeat.status = numericValue >= monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '<':
+                        heartbeat.status = numericValue < monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '<=':
+                        heartbeat.status = numericValue <= monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '==':
+                        if (!isNaN(value) && !isNaN(monitor.snmpControlValue)) {
+                            // Both values are numeric, parse them as numbers
+                            heartbeat.status = parseFloat(value) === parseFloat(monitor.snmpControlValue) ? UP : DOWN;
+                        } else {
+                            // At least one of the values is not numeric, compare them as strings
+                            heartbeat.status = value.toString() === monitor.snmpControlValue.toString() ? UP : DOWN;
+                        }
+                        break;
+                    case 'contains':
+                        heartbeat.status = stringValue.includes(monitor.snmpControlValue) ? UP : DOWN;
+                        break;
+                    default:
+                        heartbeat.status = DOWN;
+                        heartbeat.msg = `Invalid condition: ${monitor.snmpCondition}`;
+                        break;
+                }


About this part @chakflying has commented in #1675 (comment)

Sounds pretty good, but just in case you didn't know, you should take a look at how the json-query monitor works.

Ideally in the long term, we would want all value comparisons to work with the jsonata syntax, and reuse the database fields for better compatibility (see #3919). I don't think it's worth implementing custom value comparison functionality just for this monitor.

=> This needs to be compatible with #4617 and #3919

@chakflying what do you think is the best course forward?

drop this part until Proposal: Make Keyword & Json-Query generic across monitor types #3919 is done or

wait for feat: extend monitor JSON data evaluation operators #4617, then make this compatible and then do Proposal: Make Keyword & Json-Query generic across monitor types #3919? or

do the work here, wait for feat: extend monitor JSON data evaluation operators #4617 to be compatible and then do Proposal: Make Keyword & Json-Query generic across monitor types #3919?

@chakflying & @CommanderStorm I propose waiting until after #4617 is committed and I will re-factor & maintain the SNMP monitor after the fact.

server/monitor-types/snmp.js

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

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

CommanderStorm · 2024-05-23T11:36:09Z

@chakflying (sorry to ping you, hope this is fine ^^)
What do you think about the approach used in this PR?
Is this acceptable or should we "force" the users to use json-query expressions publicly (with a longer helptext how the expressuions above work) as well?

SNMP Initial Commits

d92003e

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

mattv8 mentioned this pull request Apr 27, 2024

SNMP Value Retrieval #1675

Open

1 task

mattv8 changed the title ~~feat: Add SNMP Monitor~~ feat: Add SNMP Monitor (WIP) Apr 27, 2024

mattv8 added 3 commits April 29, 2024 13:58

Use net-snmp instead of snmp-native

a3cdd69

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Updated a comment

ff5890a

Further SNMP monitor development

4a882be

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

This comment has been minimized.

Sign in to view

Wrong variable used

99dc4cf

Thanks for pointing it out @CommanderStorm!

This comment was marked as resolved.

Sign in to view

mattv8 added 6 commits April 30, 2024 15:10

Update db migration: allow nulls

138075a

DB must allow nulls otherwise this will break other monitors.

Update db migration: down function

9c8024c

knex requires down function

Update bean model backend

9d28fcf

Updates appropriate values async when editing the SNMP monitor

Frontend input validation

4593afb

Minor frontend styling

9848ce4

Finalized SNMP monitor

704ffd3

mattv8 changed the title ~~feat: Add SNMP Monitor (WIP)~~ feat: Add SNMP Monitor Apr 30, 2024

mattv8 marked this pull request as ready for review April 30, 2024 21:46

Merge branch 'master' into snmp-monitor

b4bd003

CommanderStorm requested changes Apr 30, 2024

View reviewed changes

mattv8 and others added 6 commits April 30, 2024 17:54

Apply suggestions from code review

ba47aca

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

ES Lint Compliant

7459654

Corrected down function

e944492

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

ES Lint Compliant

97a9094

Remove supurfluous log.debug

9ba0f68

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

Delete .EditMonitor.vue.swp

ba84f01

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

CommanderStorm added area:monitor Everything related to monitors needs:review this PR needs a review by maintainers or other community members labels May 19, 2024

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SNMP Monitor #4717

feat: Add SNMP Monitor #4717

mattv8 commented Apr 27, 2024 •

edited

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 •

edited

CommanderStorm left a comment

CommanderStorm Apr 30, 2024 •

edited

mattv8 May 2, 2024 •

edited

CommanderStorm commented May 23, 2024

feat: Add SNMP Monitor #4717

Are you sure you want to change the base?

feat: Add SNMP Monitor #4717

Conversation

mattv8 commented Apr 27, 2024 • edited

Description

Type of change

Checklist

Screenshots

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 • edited

CommanderStorm left a comment

Choose a reason for hiding this comment

CommanderStorm Apr 30, 2024 • edited

Choose a reason for hiding this comment

mattv8 May 2, 2024 • edited

Choose a reason for hiding this comment

CommanderStorm commented May 23, 2024

mattv8 commented Apr 27, 2024 •

edited

mattv8 commented Apr 30, 2024 •

edited

CommanderStorm Apr 30, 2024 •

edited

mattv8 May 2, 2024 •

edited