Feature/monitor checks #518

This PR adds more options for checking the response of http requests.

You can perform these checks:

This changes the structure of the http requests a bit. It combines the http and http keyword into one option.

For the http option you can then define certain checks:

The PR also automatically converts existing checks so that the 'accepted_statuscodes' and 'keyword' are converted to a monitor_check row:

accepted_statuscodes => {monitor_id: 1, type: 'HTTP_STATUS_CODE_SHOULD_EQUAL', value: '["200-299"]'}
keyword => {monitor_id: 1, type: 'RESPONSE_SHOULD_CONTAIN_TEXT', value: 'my keyword'}

This conversion also happens for imports from a json backup.

Thanks for the big update!

I recognize that https://github.com/louislam/uptime-kuma/pull/511/'s change rollbacked in this pr, is it a merging problem?

I updated the sqlite to 3.36.0, it supports drop column now. Try to avoid re-creating a table.

ALTER TABLE DROP COLUMN

https://www.sqlite.org/lang_altertable.html#altertabdropcol

It is recommended, as re-creating a table usually make mistakes in my experience. In this case, the monitor.push_token is dropped by this pr unexpectedly.

And it looks like the monitor_checks table created 2 times.

Try to avoid re-creating a table.

fixed

And it looks like the monitor_checks table created 2 times.

This was necessary since there is a foreign key that has a cascade on delete on it between the monitor_checks and monitor tables. But since I now no longer delete the monitor table, it works without having the modify the monitor_id foreign key.

fixed

Start to test this feature, it looks like it breaks @chakflying's feature #173.

And since accepted_statuscodes and keyword is being dropped, it could be a breaking change, for example, breaking the webhook.

Start to test this feature, it looks like it breaks @chakflying's feature #173.

fixed:

And since accepted_statuscodes and keyword is being dropped, it could be a breaking change, for example, breaking the webhook.

Could be, i'm not sure how the webhook feature works currently. Can you provide some details?

Since the webhook notification sends the entire monitor object, anyone (not that I think there are many, if at all) depending on those fields will be affected.

Honestly I'm for deprecating those fields since this structure does look better. But we could keep them in monitor.toJSON() for backward-compatibility.

But we could keep them in monitor.toJSON() for backward-compatibility.

fixed

            // Deprecated: Use the values in the checks list instead
            accepted_statuscodes: JSON.parse(this.accepted_statuscodes_json),
            keyword: this.keyword,

Doesn't work, toJSON() throws exception since those fields don't exist anymore. You'll have to get those values from your checks.

Also, I noticed that the default of 200-299 accepted is gone. If the user doesn't set any checks, the monitor would be down even with 200 response. I don't think this is a good behavior?

Another unexpected side-effect:

previously, since status code was validated in axios, a down-ed monitor would not return a ping value (I think), but now they do for some reason, causing the chart to have both overlapping. We could replicate the old behavior, or if you think it's better for down-ed monitors to return ping anyway, we can think about adjusting the chart implementation.

Fix Typo

For the select-option, I feel that all of them look very similar. The words "response" and "response selector" look ambiguous. The word "should" maybe it is not the best description in the context.

What do you think if the list renamed like this?

Accepted Status Code
=== MATCH === (Grey out)
HTML - Match Keyword
HTML - Regex
JSON - Match Keyword
JSON - Regex
=== NOT === (Grey out)
HTML - Match Keyword (Not)
HTML - Regex (Not)
JSON - Match Keyword (Not)
JSON - Regex (Not)

What do you think if the list renamed like this?

Yeah that is good. I would use TEXT instead of HTML though. Since you can use it for text, html or json

Another option could be to have a checkbox next to the select: invert check. Eg: responde does not contain keyword.
Then you can eliminate 4 options from the drop down and you gain the ability to have: does not match status code

Doesn't work, toJSON() throws exception since those fields don't exist anymore. You'll have to get those values from your checks.

Fixed

Also, I noticed that the default of 200-299 accepted is gone. If the user doesn't set any checks, the monitor would be down even with 200 response. I don't think this is a good behavior?

Fixed

but now they do for some reason, causing the chart to have both overlapping

Fixed

What do you think if the list renamed like this?

I tried it, but it doesn't look all that good.

So i changed it with an extra dropdown to select the "NOT" state:

now the options are:

Start to review the change.

(Bug) Change Monitor Type clear all checks unexpectedly
What is the expected result of this checks? (I got UP)
URL: https://louislam.net/404.txt
What is the expected result of this checks? (I got UP)
URL: https://louislam.net/404.txt
(Bug) After above two cases, I delete the 404 rule, and save.
Actual Result: still "UP"
Expected Result: "DOWN", since the 404 rule has been removed.
If no rule, it is always "DOWN".
(Edit) It is no harm to keep the unused language key. It can be used in the future and it is the work of other contributors. I suggest that it should be kept.

As this PR is very big, there are actually so many test cases. I will write full tests for this PR. Postpone to 1.9.

(Bug) Change Monitor Type clear all checks unexpectedly

I'm not sure what you expect to happen in this case? Checks only make sense for the http type. The accepted statuscodes entry field was also removed when you chose another monitor type on the master branch?

2. What is the expected result of this checks? (I got UP)

Fixed
When one check fails i mark the monitor as down now

3. What is the expected result of this checks? (I got UP)

Fixed

4. Expected Result: "DOWN", since the 404 rule has been removed.

Fixed, now it's marked as DOWN

5. If no rule, it is always "DOWN".

Fixed
Monitors without rules are now marked as UP

6. (Edit) It is no harm to keep the unused language key. It can be used in the future and it is the work of other contributors. I suggest that it should be kept.

I disagree, you should try to keep your code as clean and lean as possible. You can always go fetch old translations from the git history.
But you're the boss, so i reverted the deletions.

@louislam

As this PR is very big, there are actually so many test cases.

Do you write all test cases yourself for all PR's? Are they cypress tests? I'd be willing to write them for you if you give me the cases you want to test for.

@louislam

As this PR is very big, there are actually so many test cases.

Do you write all test cases yourself for all PR's? Are they cypress tests? I'd be willing to write them for you if you give me the cases you want to test for.

I integrated jest to the project.

end-to-end testing (jest + puppeteer) https://github.com/louislam/uptime-kuma/blob/master/test/e2e.spec.js
frontend code unit testing (jest) https://github.com/louislam/uptime-kuma/blob/master/test/frontend.spec.js
server code unit testing - WIP, should be jest too

I know you have already been discussing this for a while and I'm late to the party. I would nevertheless like to make one addition that would render this feature even more usable and extensible.

Instead of having Should > equal (JSON selector) it could be easier to use and to configure/code/validate to split this into three parts instead of two and have JSON selector > should > equal. This would enable a lot of things in the future. For example display only the checks that can be performed on the given element (Response Body > should > contain, JSON selector > should > be lower than, Response Header > should > not exist, and so on).

Instead of having Should > equal (JSON selector) it could be easier to use and to configure/code/validate to split this into three parts instead of two and have JSON selector > should > equal. This would enable a lot of things in the future. For example display only the checks that can be performed on the given element (Response Body > should > contain, JSON selector > should > be lower than, Response Header > should > not exist, and so on).

Yes that sounds like a good improvement.
This PR is already getting big though. We can add it in a separate PR once this one is merged. We can then also add more checks, since it doesn't make much sense to split the options if there are only 5 options.

@louislam Do you need anything from me to get this merged? Since keeping the merge conflicts fixed is taking some work each time.

For example display only the checks that can be performed on the given element (Response Body > should > contain, JSON selector > should > be lower than, Response Header > should > not exist, and so on).

@moay I also prefer that approach but I'm pretty sure you meant Response Header > should not > exist in your last example, right?

@bertyhell Thanks for this great PR! I love it!
In regards to the change suggested by @moay:
You said

We can add it in a separate PR once this one is merged.

Would it be possible to change it after this PR is merged & released and people already created monitors with that feature? Or would it be a breaking change so that users had to recreate their monitors that are using this feature?

@louislam I'm also here to help to get this feature merged. If you need any help just let us know. And thanks so much for your time and effort so far. Uptime Kuma is awesome! 🎉

I'm pretty sure you meant Response Header > should not > exist in your last example, right?

Actually, no. :) I thought about it and ended up thinking that it might be easier to not introduce a new level of logic. If there was only should, the conditions in the background could be leaved untouched and its actually only about the display.

But having should and should not would be an easy addition and would for sure render the feature more useful.

@bertyhell

I manually tested previously, this pr unfortunately is still breaking some existing behaviors. So I think writing unit test for existing behaviors is the best way to tell you the issues.

Unfortunately, writing unit testing requires a lot of time, I don't have recently. I will come back to this later.

But having should and should not would be an easy addition and would for sure render the feature more useful.

@moay This was already added here: https://github.com/louislam/uptime-kuma/pull/518#issuecomment-937000609
Or are you referring to something else?

No, probably not. Sorry, didn't see that it was already done, I was focusing more on splitting the UI part of the feature in three parts instead of two in order to make it easier to limit the possible checks by target. This would simplify adding things like a threshold check for a json value.

Feature/monitor checks #518

Reviewers