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

Monitor Conditions #5048

Merged
merged 28 commits into from
Aug 30, 2024
Merged

Monitor Conditions #5048

merged 28 commits into from
Aug 30, 2024

Conversation

simshaun
Copy link
Contributor

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Hello,

We have a need at work for monitoring values of DNS records. I got (really) bored and decided to do something about it. I understand this is a semi-major feature and may need some changes, or maybe you don't like it all. That's fine.

This PR introduces support for setting up conditions that affect UP/DOWN status on monitors. It pertains to #3919 and eventually, I think, could obviate the need for the keyword type, and maybe the JSON Query type.

It is opt-in per type, and I've only opted-in the DNS type for now. So, this doesn't break existing keyword/JSON query types. It's generic but extendable enough in my opinion that it could allow implementing a wide variety of scenarios without needing further UI changes going forward.

Currently missing the ability to drag conditions around, but that shouldn't be too difficult to shoehorn in eventually.

Screenshots below.

Type of change

  • User interface (UI)
  • 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 (if any)

image

Groups:

image

@simshaun

This comment has been minimized.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice. Good job.
The testcases are most appreciated ❤️

I have left a few inline comments below. ^^

server/model/monitor.js Outdated Show resolved Hide resolved
test/backend-test/monitor-conditions/test-evaluator.js Outdated Show resolved Hide resolved
Comment on lines 41 to 42
// eslint-disable-next-line eqeqeq
return variable == value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the following? (also goes for NotEqualsOperator)
Could you add a comment explaining the reasoning ^^

Suggested change
// eslint-disable-next-line eqeqeq
return variable == value;
return variable === value;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially was going to add an mx_priority variable to check MX record priorities. Node's DNS module parses MX records and returns priority as a number type.

In this new conditions UI, comparison values are stored as strings. i.e. My condition would be stored in the monitor as mx_priority equals "10".

I have a set of numeric operators (less than, greater than, etc) that cast the value to Number internally, but the EqualsOperator is currently used for both strings and numbers. It needs loose equality to handle 10 == "10".

An alternate solution could be to rename this operator to StringEquals (make it strict) and add a new NumberEquals operator (also strict) that casts value internally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a comment that this is because of sharing between numbers and strings to handle 10 == "10" is likely fine.

Copy link
Contributor Author

@simshaun simshaun Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm flip-flopping on this.

I can't currently think of a scenario where the loose equality operators wouldn't get the job done, but I think it may be advantageous to start this feature out with strict operators rather than maybe trying to implement them later on and have to worry about backwards compatibility.

So, a monitor type would need to use the default string or number operators, or explicitly use either StringEquals or NumberEquals when declaring its supported condition variables.

server/monitor-types/dns.js Outdated Show resolved Hide resolved
server/monitor-types/dns.js Outdated Show resolved Hide resolved
src/components/EditMonitorCondition.vue Outdated Show resolved Hide resolved
src/components/EditMonitorConditions.vue Outdated Show resolved Hide resolved
src/components/EditMonitorCondition.vue Outdated Show resolved Hide resolved
src/components/EditMonitorConditions.vue Show resolved Hide resolved
db/knex_migrations/2024-08-24-0000-conditions.js Outdated Show resolved Hide resolved
@simshaun

This comment was marked as resolved.

@simshaun

This comment was marked as resolved.

@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable area:monitor Everything related to monitors type:enhance-existing feature wants to enhance existing monitor labels Aug 27, 2024
@simshaun
Copy link
Contributor Author

Since #5056 was merged, I've fixed, added to, and improved the E2E tests for this PR.

I'll go through your remaining comments sometime later today.

@simshaun
Copy link
Contributor Author

I think I've handled all the remaining issues.

@CommanderStorm
Copy link
Collaborator

Thanks for the enhancement! 🎉

Note

This PR is part of the v2.0 merge window => see #4500 for the bugs that need to be addressed before we can ship this release ^^

All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval

@CommanderStorm CommanderStorm merged commit 36f8be0 into louislam:master Aug 30, 2024
18 checks passed
@vishalsabhaya
Copy link
Contributor

vishalsabhaya commented Sep 18, 2024

@simshaun @CommanderStorm

i am working on below improvement.
#5025
i want to do final testing so i taken pull from master branch but i am getting below error.

image

as per my research.
i have putted log for debug

console.log("sdfwfwkjfhwhfkwefjewf");
console.log(this.conditions);
console.log("sdfwfwkjfhwhfkwefjewf");

this.conditions is no value so it is "".
SyntaxError: Unexpected end of JSON input is occurs.

i think blank then we need to pass some default json or null.

@simshaun
Copy link
Contributor Author

@vishalsabhaya

The migration in this database changes sets a default value of [] for the conditions column in the database. I'm curious how yours is ending up empty.

@vishalsabhaya
Copy link
Contributor

vishalsabhaya commented Sep 19, 2024

@simshaun

i am using mariadb. is there any problem with MySQL? have you written migration for MySQL?

@simshaun
Copy link
Contributor Author

@vishalsabhaya The migration should be database agnostic.

https://github.com/louislam/uptime-kuma/pull/5048/files#diff-4111d2d321bfe6baf61dc1b78521a8ee9445bf86f599aac8fa3be00cb85139f1

Can you confirm via DBMS the default value of the conditions column in the monitor table?

@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors pr:please address review comments this PR needs a bit more work to be mergable question Further information is requested type:enhance-existing feature wants to enhance existing monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants