feat: refractored SQL-based alerting framework #10605

JasonD28 · 2020-08-14T20:50:39Z

SUMMARY

Currently superset has all of its functionality centralized inside the Alerts table. This PR separates the different functionalities of alerting into separate components in order to increase the functionality of alerting and to allow this feature more flexibility for future upgrades.

Major Changes:

Created new table for SQLObservers which will handle the functionality of making SQL queries for the alert
Created new table for SQLObservations which will hold the information gathered from each SQLObserver query
Created new table for Validators which will handle looking at the values from SQLObservations to determine if an alert should be triggered

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

unit test
local test
dropbox staging
dropbox prod

ADDITIONAL INFORMATION

superset/models/alerts.py

superset/tasks/schedules.py

codecov-commenter · 2020-08-27T01:07:25Z

Codecov Report

Merging #10605 into master will increase coverage by 1.42%.
The diff coverage is 78.51%.

@@            Coverage Diff             @@
##           master   #10605      +/-   ##
==========================================
+ Coverage   58.92%   60.35%   +1.42%     
==========================================
  Files         756      363     -393     
  Lines       36072    23421   -12651     
  Branches     3301        0    -3301     
==========================================
- Hits        21256    14135    -7121     
+ Misses      14633     9286    -5347     
+ Partials      183        0     -183

Flag	Coverage Δ
#cypress	`?`
#python	`60.35% <78.51%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ations/versions/01616b213391_refractor_alerting.py	`0.00% <0.00%> (ø)`
superset/app.py	`80.00% <25.00%> (-0.96%)`	⬇️
superset/views/alerts.py	`73.68% <82.60%> (+7.01%)`	⬆️
superset/tasks/schedules.py	`76.13% <88.88%> (-3.40%)`	⬇️
superset/tasks/alerts/validator.py	`97.61% <97.61%> (ø)`
superset/models/alerts.py	`97.67% <98.03%> (ø)`
superset/tasks/alerts/observer.py	`100.00% <100.00%> (ø)`
superset/models/schedules.py	`86.79% <0.00%> (-7.55%)`	⬇️
superset/db_engines/hive.py	`82.14% <0.00%> (-3.58%)`	⬇️
... and 401 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 234b6bb...7fe216f. Read the comment docs.

bkyryliuk

really good progress here, some small comments.
let's start testing the solution on staging.

bkyryliuk · 2020-08-27T18:35:04Z

superset/models/alerts.py

+            backref=backref("sql_observers", cascade="all, delete-orphan"),
+        )
+
+    # TODO: Abstract observations from the sqlamodls e.g.


s/sqlamodls/sqlalchemy models
add some context, e.g. observation table will be constantly growing and would need to either have some retention policy or be moved to the more scalable db. Same as alert log

bkyryliuk · 2020-08-27T18:37:54Z

superset/models/alerts.py

+        default=textwrap.dedent(
+            """
+            {
+                "example_threshold": 50


let's keep the default empty

bkyryliuk · 2020-08-27T18:39:31Z

superset/models/alerts.py

+        )
+
+
+def not_null_executor(


this should live outside the models, you can create a separate module for them
e.g. tasks/schedules/alerts/validator.py

bkyryliuk · 2020-08-27T18:41:08Z

superset/models/alerts.py

+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]
+    if observation.value:


this one could be tricky
if 0 returns false :)

bkyryliuk · 2020-08-27T18:43:19Z

superset/tasks/schedules.py

@@ -538,16 +543,15 @@ def schedule_alert_query(  # pylint: disable=unused-argument
            logger.info("Ignoring deactivated alert")
            return

+        if schedule.sql_observers:
+            sql = schedule.sql_observers[0].sql


could there be multiple observers?
if not, let's change sql_observers to sql_observer

bkyryliuk · 2020-08-27T18:52:25Z

tests/alerts_tests.py

@@ -41,95 +50,204 @@
 def setup_database():
    with app.app_context():
        slice_id = db.session.query(Slice).all()[0].id
-        database_id = utils.get_example_database().id
+        database = utils.get_example_database()


s/database/example_database

tests/alerts_tests.py

bkyryliuk · 2020-08-27T18:55:50Z

tests/alerts_tests.py

-                alert_type="email",
-                slice_id=slice_id,
-                database_id=database_id,
+            Alert(**common_data, id=1, label="alert_1"),


let's not specify ids here and those are autogenerated, use label to retrieve alerts you care about

I specified id's in order to control with SQLObserver is linked to which alert (alert_id column in SQLObserver), I'm not sure if there is another way to do this

bkyryliuk · 2020-08-27T18:56:31Z

tests/alerts_tests.py

+    # Test error with alert lacking observer
+    alert5 = dbsession.query(Alert).filter_by(id=5).one()
+    check_alert(alert5.id, alert5.label)
+    assert alert5.logs[-1].state == AlertState.ERROR


let's add successful alert tests as well

bkyryliuk · 2020-08-27T18:57:32Z

tests/alerts_tests.py

    dbsession = setup_database
+    null_val1 = Validator(


let's decouple db and validators, validators are just the simple functions.
You can pass observation list and config to them instead.

JasonD28 · 2020-09-01T15:38:14Z

@willbarrett when you have a chance could you take a look?

bkyryliuk

LG%nit

tests/alerts_tests.py

mistercrunch · 2020-10-02T16:51:21Z

@bkyryliuk the observer/validator pattern is really confusing, I heard there's been discussions on reverting this. Personally I feel like we should 100% revert this. What's the plan!?

bkyryliuk · 2020-10-05T20:58:28Z

@bkyryliuk the observer/validator pattern is really confusing, I heard there's been discussions on reverting this. Personally I feel like we should 100% revert this. What's the plan!?

@mistercrunch we have synced with @willbarrett on this topic. I will merge the observer & validator back into alert object within next 2 weeks. Will keep Observation object to support validation across multiple observations.

willbarrett · 2020-10-05T21:16:17Z

Thanks for the response @bkyryliuk - if you're unable to get to it this week please let me know and we'll take on the task on our side. We're under some time pressure to evolve this feature. Thanks!

* added new tables for alerting refractor * reformatted inheritance structure * added workflow for updated framework * added suggested changes * cleaned up changes * added obervations to alert table to enable view * added comments * added requested changes * fix tests * added styling changes * mypy * added requested changes * updated operator logic * requested changes, 1 validator, styling changes * refactored tests * fix test alert workflow * fixed create_alert in test Co-authored-by: Jason Davis <@dropbox.com>

pull-request-size bot added the size/L label Aug 14, 2020

JasonD28 marked this pull request as draft August 14, 2020 20:50

bkyryliuk reviewed Aug 14, 2020

View reviewed changes