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
feat: Add SNMP Monitor #4717
base: master
Are you sure you want to change the base?
feat: Add SNMP Monitor #4717
Conversation
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thanks for pointing it out @CommanderStorm!
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
DB must allow nulls otherwise this will break other monitors.
knex requires down function
Updates appropriate values async when editing the SNMP monitor
@CommanderStorm I'm ready for review. It is not passing ES Lint, although when I run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
server/monitor-types/snmp.js
Outdated
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chakflying & @CommanderStorm I propose waiting until after #4617 is committed and I will re-factor & maintain the SNMP monitor after the fact.
Co-authored-by: Frank Elsinga <frank@elsinga.de>
Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>
Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>
Equivalent functionality as before, but we're now building json-query expressions for the user.
@chakflying (sorry to ping you, hope this is fine ^^) |
Tick the checkbox if you understand [x]:
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:
Addresses #1675
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots