fix: superset alerting misc fixes (apache#10891)

* Superset alerting misc fixes

* Test 0 threshold

Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>

superset/models/alerts.py

@@ -59,14 +59,15 @@ class Alert(Model, AuditMixinNullable):
     id = Column(Integer, primary_key=True)
     label = Column(String(150), nullable=False)
     active = Column(Boolean, default=True, index=True)
+    # TODO(bkyryliuk): enforce minimal supported frequency
     crontab = Column(String(50), nullable=False)
 
     alert_type = Column(String(50))
     owners = relationship(security_manager.user_model, secondary=alert_owner)
     recipients = Column(Text)
     slack_channel = Column(Text)
 
-    # TODO: implement log_retention
+    # TODO(bkyryliuk): implement log_retention
     log_retention = Column(Integer, default=90)
     grace_period = Column(Integer, default=60 * 60 * 24)
 
@@ -203,7 +204,8 @@ def alert(self) -> RelationshipProperty:
             backref=backref("validators", cascade="all, delete-orphan"),
         )
 
-    def pretty_print(self) -> str:
+    @property
+    def pretty_config(self) -> str:
         """ String representing the comparison that will trigger a validator """
         config = json.loads(self.config)
 

superset/tasks/alerts/validator.py

@@ -46,7 +46,7 @@ def check_validator(validator_type: str, config: str) -> None:
 
     if validator_type == AlertValidatorType.operator.value:
 
-        if not (config_dict.get("op") and config_dict.get("threshold")):
+        if not (config_dict.get("op") and config_dict.get("threshold") is not None):
             raise SupersetException(
                 "Error: Operator Validator needs specified operator and threshold "
                 'values. Add "op" and "threshold" to config.'
@@ -87,15 +87,13 @@ def operator_validator(observer: SQLObserver, validator_config: str) -> bool:
     Returns True if a SQLObserver's recent observation is greater than or equal to
     the value given in the validator config
     """
-
     observation = observer.get_last_observation()
-    if observation and observation.value not in (None, np.nan):
-        operator = json.loads(validator_config)["op"]
-        threshold = json.loads(validator_config)["threshold"]
-        if OPERATOR_FUNCTIONS[operator](observation.value, threshold):
-            return True
+    if not observation or observation.value in (None, np.nan):
+        return False
 
-    return False
+    operator = json.loads(validator_config)["op"]
+    threshold = json.loads(validator_config)["threshold"]
+    return OPERATOR_FUNCTIONS[operator](observation.value, threshold)
 
 
 def get_validator_function(

superset/tasks/schedules.py

@@ -580,7 +580,7 @@ def deliver_alert(
     recipients = recipients or alert.recipients
     slack_channel = slack_channel or alert.slack_channel
     validation_error_message = (
-        str(alert.observations[-1].value) + " " + alert.validators[0].pretty_print()
+        str(alert.observations[-1].value) + " " + alert.validators[0].pretty_config
         if alert.validators
         else ""
     )
@@ -731,15 +731,13 @@ def validate_observations(alert_id: int, label: str, session: Session) -> bool:
     """
 
     logger.info("Validating observations for alert <%s:%s>", alert_id, label)
-
     alert = session.query(Alert).get(alert_id)
-    if alert.validators:
-        validator = alert.validators[0]
-        validate = get_validator_function(validator.validator_type)
-        if validate and validate(alert.sql_observer[0], validator.config):
-            return True
+    if not alert.validators:
+        return False
 
-    return False
+    validator = alert.validators[0]
+    validate = get_validator_function(validator.validator_type)
+    return bool(validate and validate(alert.sql_observer[0], validator.config))
 
 
 def next_schedules(

superset/views/alerts.py

@@ -14,13 +14,10 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-from typing import Dict, Optional, Union
-
 from croniter import croniter
 from flask_appbuilder import CompactCRUDMixin
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_babel import lazy_gettext as _
-from wtforms import BooleanField, Form, StringField
 
 from superset.constants import RouteMethod
 from superset.models.alerts import (
@@ -30,9 +27,7 @@
     SQLObserver,
     Validator,
 )
-from superset.models.schedules import ScheduleType
 from superset.tasks.alerts.validator import check_validator
-from superset.tasks.schedules import schedule_alert_query
 from superset.utils import core as utils
 from superset.utils.core import get_email_address_str, markdown
 
@@ -47,6 +42,7 @@ class AlertLogModelView(
 ):  # pylint: disable=too-many-ancestors
     datamodel = SQLAInterface(AlertLog)
     include_route_methods = {RouteMethod.LIST} | {"show"}
+    base_order = ("dttm_start", "desc")
     list_columns = (
         "scheduled_dttm",
         "dttm_start",
@@ -60,6 +56,7 @@ class AlertObservationModelView(
 ):  # pylint: disable=too-many-ancestors
     datamodel = SQLAInterface(SQLObservation)
     include_route_methods = {RouteMethod.LIST} | {"show"}
+    base_order = ("dttm", "desc")
     list_title = _("List Observations")
     show_title = _("Show Observation")
     list_columns = (
@@ -130,8 +127,9 @@ class ValidatorInlineView(  # pylint: disable=too-many-ancestors
     add_columns = edit_columns
 
     list_columns = [
-        "validator_type",
         "alert.label",
+        "validator_type",
+        "pretty_config",
     ]
 
     label_columns = {
@@ -180,18 +178,27 @@ class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
     datamodel = SQLAInterface(Alert)
     route_base = "/alert"
     include_route_methods = RouteMethod.CRUD_SET
-    _extra_data: Dict[str, Union[bool, Optional[str]]] = {
-        "test_alert": False,
-        "test_email_recipients": None,
-    }
 
     list_columns = (
         "label",
         "crontab",
         "last_eval_dttm",
         "last_state",
         "active",
-        "creator",
+        "owners",
+    )
+    show_columns = (
+        "label",
+        "active",
+        "crontab",
+        "owners",
+        "slice",
+        "recipients",
+        "slack_channel",
+        "log_retention",
+        "grace_period",
+        "last_eval_dttm",
+        "last_state",
     )
     order_columns = ["label", "last_eval_dttm", "last_state", "active"]
     add_columns = (
@@ -208,9 +215,6 @@ class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
         # "dashboard",
         "log_retention",
         "grace_period",
-        "test_alert",
-        "test_email_recipients",
-        "test_slack_channel",
     )
     label_columns = {
         "log_retention": _("Log Retentions (days)"),
@@ -230,28 +234,6 @@ class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
         ),
     }
 
-    add_form_extra_fields = {
-        "test_alert": BooleanField(
-            "Send Test Alert",
-            default=False,
-            description="If enabled, a test alert will be sent on the creation / update"
-            " of an active alert. All alerts after will be sent only if the SQL "
-            "statement defined above returns True.",
-        ),
-        "test_email_recipients": StringField(
-            "Test Email Recipients",
-            default=None,
-            description="List of recipients to send test email to. "
-            "If empty, an email will be sent to the original recipients.",
-        ),
-        "test_slack_channel": StringField(
-            "Test Slack Channel",
-            default=None,
-            description="A slack channel to send a test message to. "
-            "If empty, an alert will be sent to the original channel.",
-        ),
-    }
-    edit_form_extra_fields = add_form_extra_fields
     edit_columns = add_columns
     related_views = [
         AlertObservationModelView,
@@ -260,34 +242,11 @@ class AlertModelView(SupersetModelView):  # pylint: disable=too-many-ancestors
         SQLObserverInlineView,
     ]
 
-    def process_form(self, form: Form, is_created: bool) -> None:
-        email_recipients = None
-        if form.test_email_recipients.data:
-            email_recipients = get_email_address_str(form.test_email_recipients.data)
-
-        test_slack_channel = (
-            form.test_slack_channel.data.strip()
-            if form.test_slack_channel.data
-            else None
-        )
-
-        self._extra_data["test_alert"] = form.test_alert.data
-        self._extra_data["test_email_recipients"] = email_recipients
-        self._extra_data["test_slack_channel"] = test_slack_channel
-
     def pre_add(self, item: "AlertModelView") -> None:
         item.recipients = get_email_address_str(item.recipients)
 
         if not croniter.is_valid(item.crontab):
             raise SupersetException("Invalid crontab format")
 
-    def post_add(self, item: "AlertModelView") -> None:
-        if self._extra_data["test_alert"]:
-            recipients = self._extra_data["test_email_recipients"] or item.recipients
-            slack_channel = self._extra_data["test_slack_channel"] or item.slack_channel
-            args = (ScheduleType.alert, item.id)
-            kwargs = dict(recipients=recipients, slack_channel=slack_channel)
-            schedule_alert_query.apply_async(args=args, kwargs=kwargs)
-
     def post_update(self, item: "AlertModelView") -> None:
         self.post_add(item)

tests/alerts_tests.py

@@ -238,6 +238,11 @@ def test_operator_validator(setup_database):
         operator_validator(alert1.sql_observer[0], '{"op": ">=", "threshold": 60}')
         is False
     )
+    # ensure that 0 threshold works
+    assert (
+        operator_validator(alert1.sql_observer[0], '{"op": ">=", "threshold": 0}')
+        is False
+    )
 
     # Test passing SQLObserver with result that doesn't pass a greater than threshold
     alert2 = create_alert(dbsession, "SELECT 55")
@@ -330,7 +335,7 @@ def test_deliver_alert_screenshot(
         "initial_comment": f"\n*Triggered Alert: {alert.label} :redalert:*\n"
         f"*Query*:```{alert.sql_observer[0].sql}```\n"
         f"*Result*: {alert.observations[-1].value}\n"
-        f"*Reason*: {alert.observations[-1].value} {alert.validators[0].pretty_print()}\n"
+        f"*Reason*: {alert.observations[-1].value} {alert.validators[0].pretty_config}\n"
         f"<http://0.0.0.0:8080/alert/show/{alert.id}"
         f"|View Alert Details>\n<http://0.0.0.0:8080/superset/slice/{alert.slice_id}/"
         "|*Explore in Superset*>",