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

feature: Added new alert rule builder UI #8293

Merged
merged 72 commits into from Mar 14, 2018

Conversation

Projects
None yet
7 participants
@laf
Member

laf commented Feb 24, 2018

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 8293

NOTE to testers This will break your alert mapping if you go back to master before this PR is merged.

This is ready for widespread testing!

It provides an update to the ui for creating alert rules. It's had limited testing so far so be warned.

Some things to note:

  • A console error will show when editing a new rule. This is expected.
  • Rules created the old way should continue to work. If you edit an old rule, it will be updated.
  • You can still create a new rule using the old system.
  • Interval no longer seems to be honoured so I need to look in to that.

To do:

  • Automatically import old rules when they are edited. Manually allow the importing of old rules.
  • Allow users to paste in an SQL query that will be parsed.
  • Fix sql gen when adding default rules
  • Collection fully converted to query_builder format
  • Allow add from collection to use query_builder format
  • (<,>,<=,>=) missing for integer type
  • List showing SQL instead of rule/query_builder
  • Convert to new format button in old edit dialog or simply - auto convert on edit
  • The webui layout / button will change. This is purely for testing so far.
  • Query Builder select too wide (layout issue not select box)
  • Remove old modal for adding / editing rules.
  • List all resolvable tables + fields
  • Make IN + NOT IN work.

Please expect something to break so don't attempt to test this unless you are prepared.

image

@laf

This comment has been minimized.

Member

laf commented Feb 24, 2018

Just an FYI that the interval value no longer seems to be honoured so I need to look into that.

@murrant

This comment has been minimized.

Member

murrant commented Feb 25, 2018

I definitely love this.

A couple of thoughts suggestions:

  • Should we store db_schema.yaml as json? json_decode() is a lot faster.
  • Put macros before the normal fields instead of after, or sort them all alphabetically.
  • Pass id to mysql_type_to_php_type(), that way we can add some specific customizations/helpers.
  • For example, I changed all macros to Yes/No radio options in v2 instead of an text box

I assume you have some refinement for the webui planned after you get it functioning fully. (Non-modal, doesn't dismiss when saving)

@laf

This comment has been minimized.

Member

laf commented Feb 25, 2018

Should we store db_schema.yaml as json? json_decode() is a lot faster.

No issue from my side, we don't need human readable file for that so json will be fine. I'll class it as out of scope for this PR for now.

Put macros before the normal fields instead of after, or sort them all alphabetically.

Done.

Pass id to mysql_type_to_php_type(), that way we can add some specific customizations/helpers.

What ID?

For example, I changed all macros to Yes/No radio options in v2 instead of an text box

You can't use Yes/No for all macros, some require int such as bandwidth > x.

I assume you have some refinement for the webui planned after you get it functioning fully. (Non-modal, doesn't dismiss when saving)

Once it's tested in enough detail yes. Although I think I'll keep the non-modal. I don't think it will look good in a confined space like that.

@murrant

This comment has been minimized.

Member

murrant commented Feb 25, 2018

Id is table.field

@laf

This comment has been minimized.

Member

laf commented Feb 26, 2018

Id is table.field

Added.

queryBuilder wasn't playing nice within a modal so it's a separate page for now.

Would love proper testing now and we can work out how to make the UI work later.

@murrant

This comment has been minimized.

Member

murrant commented Feb 28, 2018

Fixed a couple issues with the rule collection.
TODO:

Moved to the top.

@laf

This comment has been minimized.

Member

laf commented Feb 28, 2018

Thanks for taking a look :)

Collection fully converted to query_builder format

Any reason why?

@laf

This comment has been minimized.

Member

laf commented Feb 28, 2018

I've tackled a few more on that list. Will get to the auto converting bit soon.

@murrant

This comment has been minimized.

Member

murrant commented Mar 1, 2018

I'd like to convert the saved rules so people can edit them with the new editor after adding.

Perhaps we should add an export button to make it easier to share rules.

@laf

This comment has been minimized.

Member

laf commented Mar 1, 2018

I'd like to convert the saved rules so people can edit them with the new editor after adding.

Unless I'm missing something when you import from the collection you can edit using the new ui as normal - do you mean some other way?

Export sounds good. They can just copy the query shown in the alert list though.

@laf

This comment has been minimized.

Member

laf commented Mar 1, 2018

So I've update the edit rule to deal with the old format. It doesn't auto save but parses the old format ready for use. If the user chooses not to save then we just re-import next time they edit. If they do save, we store the query_builder value and it's then used going forward.

I've moved the double and float types to be strings, the reason for this is that REGEXP can be used on those but doesn't pass validation.

@murrant

This comment has been minimized.

Member

murrant commented Mar 3, 2018

Modal is working mostly. Looks much better with larger size.
Select box works, but width is a little wonky, that needs some tweaking.
There are probably lots of unused things that need to be removed.
I don't understand what a "device limited rule" is so that may be broken.

@laf

This comment has been minimized.

Member

laf commented Mar 3, 2018

Modal is working mostly. Looks much better with larger size.

Ace, thanks for that.

Select box works, but width is a little wonky, that needs some tweaking.

Any reason you're trying to narrow it down?

I don't understand what a "device limited rule" is so that may be broken.

Not sure where you've got device limited rule from but I'm guessing you mean device specific rule? That's where you create a rule in the edit device section specifically so it's automatically bound to the device.

We potentially could update this to drop the device_id = -1 think and just say users need to bind rules to groups. It does mean it's an extra step for them + we can't auto convert those existing rules that are bound to a specific device without creating a unique group for each rule.

@laf

This comment has been minimized.

Member

laf commented Mar 3, 2018

Oh and the table reload itself. We aren't using bootgrid here. A quick attempt to by just doing:

$("#grid-basic").bootgrid(); resulted in the table disappearing. I didn't look much past that but it's easy enough to just update the relevant cells possibly as we do that on delete, just remove the td associated with that rule - or obviously get bootgrid working.

@murrant

This comment has been minimized.

Member

murrant commented Mar 3, 2018

@laf I don't want the select narrow :) It automatically sets width to the first option, which is '-----', I've got a little compromise for that merged now, seems ok.

Ok, I'll check the device create/edit rules later.

Reloading bootgrid won't work because we embed the data in the html, not ajax. That is why I just reload the page for now.

@laf

This comment has been minimized.

Member

laf commented Mar 3, 2018

Reloading bootgrid won't work because we embed the data in the html, not ajax. That is why I just reload the page for now.

It would work with full bootgrid as the data is loaded in via ajax, I just wanted to check if it worked loading bootgrid against an existing table which it didn't and I didn't want to spend much time working that one out.

@murrant

murrant approved these changes Mar 5, 2018

Looks good to me. More testing now that this is basically done would be good.

@murrant

This comment has been minimized.

Member

murrant commented Mar 6, 2018

@laf during my modal refactoring looks like I've broken editing old rules. Any idea where the issue is?

@murrant

I'm wondering should we store the raw querybuilder json?
Yeah, I know this requires some re-writing I would be willing.

@laf

This comment has been minimized.

Member

laf commented Mar 7, 2018

I'm wondering should we store the raw querybuilder json?
Yeah, I know this requires some re-writing I would be willing.

I was doing that at some stage. It's pretty quick to implement that but out of interest why?

It makes parsing much better as it doesn't use the sql plugin but we'd only be able to display the full query in the alert rules list.

@laf

This comment has been minimized.

Member

laf commented Mar 7, 2018

@laf during my modal refactoring looks like I've broken editing old rules. Any idea where the issue is?

Looks like no code for the old way is running anymore. It just pulls in the query_builder value which is empty for old rules.

@laf

This comment has been minimized.

Member

laf commented Mar 7, 2018

diff --git a/html/includes/forms/parse-alert-rule.inc.php b/html/includes/forms/parse-alert-rule.inc.php
index 7913085ea..7e277c809 100644
--- a/html/includes/forms/parse-alert-rule.inc.php
+++ b/html/includes/forms/parse-alert-rule.inc.php
@@ -27,6 +27,15 @@ if (is_numeric($alert_id) && $alert_id > 0) {
     $rule = $tmp_rules[$template_id];
 }
 if (is_array($rule)) {
+    if (empty($rule['query_builder'])) {
+        $sql_query = $rule['rule'];
+        $sql_query = str_replace('&&', 'AND', $sql_query);
+        $sql_query = str_replace('||', 'OR', $sql_query);
+        $sql_query = str_replace('%', '', $sql_query);
+        $sql_query = str_replace('"', "'", $sql_query);
+        $sql_query = str_replace('~', "REGEXP", $sql_query);
+        $rule['query_builder'] = $sql_query;
+    }
     $output             = array(
         'severity'      => $rule['severity'],
         'extra'         => $rule['extra'],

murrant added some commits Mar 9, 2018

Some quick tests for Schema
Simplified output for findRelationshipPath. Always includes start and target in the result.
This simplifies a lot of code in QueryBuilderParser.php
This also always loads the target table data now (which we want)
Fix glue (devices.device_id = ports.port_id) is incorrect :D
Show ALL tables we can resolve relationships for in the query builder filter.
@imcdona

This comment has been minimized.

Contributor

imcdona commented Mar 13, 2018

What are these tables for? I'd like to add comments to the schema updates in this push request.

alert_group_map` `alert_rules` `alert_map` `alert_device_map

@laf

This comment has been minimized.

Member

laf commented Mar 13, 2018

alert_group_map = the mapping of groups to an alert rule.

alert_rules = the actual alert rules.

alert_map = is now alert_device_map

alert_device_map = the mapping of devices to an alert rule.

@murrant

This comment has been minimized.

Member

murrant commented Mar 13, 2018

Note to self remove macros from schema.

Remove all macros from the database
Remove insert statements, leave updates to update user's existing rules.
@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Mar 14, 2018

Still testing this, so far no issues.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 14, 2018

The inspection completed: 9 new issues, 46 updated code elements

@laf laf merged commit 03076c4 into librenms:master Mar 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:feature/new-alert-builder branch Mar 14, 2018

@thomseddon

This comment has been minimized.

Contributor

thomseddon commented Mar 15, 2018

I think this broke our alert mapping as we've just received a few hundred alerts for devices to which the alert isn't mapped too.

Checking in the code, I don't think it's observing groups or mappings correctly,

Previous (prior to this change)

function GetRules($device)
{
    $groups = GetGroupsFromDevice($device);
    $params = array($device,$device);
    $where = "";
    foreach ($groups as $group) {
        $where .= " || alert_map.target = ?";
        $params[] = 'g'.$group;
    }
    return dbFetchRows('SELECT alert_rules.* FROM alert_rules LEFT JOIN alert_map ON alert_rules.id=alert_map.rule WHERE alert_rules.disabled = 0 && ( (alert_rules.device_id = -1 || alert_rules.device_id = ? ) || alert_map.target = ? '.$where.' )', $params);
}

Now we have:

function GetRules($device)
{
    $groups = GetGroupsFromDevice($device);
    $params = array($device, $device);
    $where = "";
    foreach ($groups as $group) {
        $where .= " || group_id = ?";
        $params[] = $group;
    }
    $query = "SELECT DISTINCT a.* FROM alert_rules a LEFT JOIN alert_device_map d ON a.id=d.rule_id LEFT JOIN alert_group_map g ON a.id=d.rule_id WHERE a.disabled = 0 AND ((d.device_id=? OR d.device_id IS NULL) $where)";
    return dbFetchRows($query, $params);
}

Notice how the previous query checked for alert_rules.device_id = -1 whereas the current query checks d.device_id IS NULL. The issue with the new query is that in the case there are no rows in alert_device_map that match the join (a.id=d.rule_id ) then the value of d.device_id will be NULL (i.e. it will match in the absence of a rule, where it shouldn't).

As an example, see the rules matched below where there is nothing in the alert_device_map table:

MariaDB [librenms]> SELECT * FROM alert_device_map;
Empty set (0.01 sec)

MariaDB [librenms]> SELECT DISTINCT a.id, a.name, d.*, g.*  FROM alert_rules a LEFT JOIN alert_device_map d ON a.id=d.rule_id LEFT JOIN alert_group_map g ON a.id=d.rule_id WHERE a.disabled = 0 AND ((d.device_id='402' OR d.device_id IS NULL) );
+----+---------------------------------+------+---------+-----------+------+---------+----------+
| id | name                            | id   | rule_id | device_id | id   | rule_id | group_id |
+----+---------------------------------+------+---------+-----------+------+---------+----------+
|  3 | bgp session down                | NULL |    NULL |      NULL | NULL |    NULL |     NULL |
|  5 | Port status up/down             | NULL |    NULL |      NULL | NULL |    NULL |     NULL |
|  6 | Port utilisation over threshold | NULL |    NULL |      NULL | NULL |    NULL |     NULL |
+----+---------------------------------+------+---------+-----------+------+---------+----------+
3 rows in set (0.00 sec)

Also, device_id was correctly updated to d.device_id but I think group_id should be updated to g.group_id.

@thomseddon

This comment has been minimized.

Contributor

thomseddon commented Mar 15, 2018

Also, I can submit a PR, but as @laf has been working on this so recently I thought he may be best place to understand all implications and check any other gotchas.

I think the schema will need to be changed, happy to build a PR if it would help.

@laf

This comment has been minimized.

Member

laf commented Mar 15, 2018

@thomseddon We fixed that earlier, just re-run daily.sh

@laf

This comment has been minimized.

Member

laf commented Mar 15, 2018

In fact you already have.

@laf

This comment has been minimized.

Member

laf commented Mar 15, 2018

it probably should be:

$query = "SELECT DISTINCT a.* FROM alert_rules a LEFT JOIN alert_device_map d ON a.id=d.rule_id LEFT JOIN alert_group_map g ON a.id=g.rule_id WHERE a.disabled = 0 AND ((d.device_id=? OR d.device_id IS NULL) $where)";

Like you said, can you test and submit a PR @thomseddon

@murrant

This comment has been minimized.

Member

murrant commented Mar 15, 2018

@thomseddon Fixed, update your install
#8389

@thomseddon

This comment has been minimized.

Contributor

thomseddon commented Mar 15, 2018

Yeah that fixed it 👍 thanks

@murrant

This comment has been minimized.

Member

murrant commented Mar 15, 2018

And double fixed #8391

inetAnt added a commit to criteo-forks/librenms that referenced this pull request Mar 19, 2018

feature: Added new alert rule builder UI and rule mapping UI (librenm…
…s#8293)

* feature: Added new alert rule builder UI

* Updated to export sql queries

* More updates

* more changes

* removed debug

* fix scrut

* Updated to include import options + various other fixes

* fix rule

* Populate name from collection rules.

* Fix default rule import
Allow new and old style rules in the collection.
Don't add new yet as I'm not sure GenSQL() is working.

* Fix GenSQL call

* Extract filter building to class so it is nicely contained in one place

* moved schema

* some fixes and tweaks

* travis fixes

* Some more features / updates

* Fix up my mistakes when adding default rules

* Use a modal for new alert (Incomplete)
Larger dialog!!
Remove page loading stuff.

Working:
Loading rules, resetting dialog, importing from collection.

Not working yet:
select width
device limited rule access? don't know what this is...

Lots of unused stuff to delete...

* reload "table" after save

* fixed editing rule

* Auto select2 width

* Reload window on save

* Restore per-device alert. Remove debug.

* Small cleanups. Rule Name first.

* Restore button to button type. Rename schema.

* Fixes: wrong command to reload window, remove extra attributes, rule is never passed

* Fixed old rule editing

* some small updates for old imports

* travis update to use trusty

* maybe travis fix

* Ability to set alert rule mappings on the rule edit screen

* pip installs one line, no quiet for deploy

* update schema def

* Fix style and some copyright headers

* fix docs missing file

* Allow new versions of snmpsim and libraries

* Parser WIP

* Fix default rules insert

* reorganize

* Legacy import first draft done

* Implement saving
Skip translation to sql for now

* Working on glues

* small rule collection fix

* Working on glues

* Working on glues

* Docs updates + small UI changes

* Parser WIP

* reorganize

* Legacy import first draft done

* Implement saving
Skip translation to sql for now

* Working on glues

* Working on glues

* Working on glues

* Add table mapping, should move to it's own class

* WIP

* Glue working!!

* Extract Schema class

* Some final touches.
revert alerts_rules.json for now.

* Finish up initial implementation
Needs more tests

* Fix a few places

* small doc updates

* Fix finding tables in grouped rules.

* remove unused code

* code format fixes

* Some quick tests for Schema
Simplified output for findRelationshipPath. Always includes start and target in the result.
This simplifies a lot of code in QueryBuilderParser.php
This also always loads the target table data now (which we want)

* Make bill_id the PRIMARY index for the bills table

* Load macros from a json file in misc instead of the database.

* Fix whitespace and wrong key for collection.

* Handle IN properly when generating SQL

* Fix glue (devices.device_id = ports.port_id) is incorrect :D
Show ALL tables we can resolve relationships for in the query builder filter.

* Remove all macros from the database
Remove insert statements, leave updates to update user's existing rules.
@BradHooper

This comment has been minimized.

BradHooper commented Mar 22, 2018

Could we please add the operators IN and regex for number based values?

For example previously I could match on multiple BGP ASN in a list now I cannot.

@murrant

This comment has been minimized.

Member

murrant commented Mar 23, 2018

You can, but it means you have to add a lot of rules. I meant here btw #8353

@lock lock bot locked as resolved and limited conversation to collaborators May 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.