From c3f1720456fd7e7d94b986b43b5b057e5236e65a Mon Sep 17 00:00:00 2001 From: Bogdan Date: Wed, 16 Sep 2020 11:09:11 -0700 Subject: [PATCH] fix: superset alerting misc fixes (#10891) * Superset alerting misc fixes * Test 0 threshold Co-authored-by: bogdan kyryliuk --- superset/models/alerts.py | 6 ++- superset/tasks/alerts/validator.py | 14 +++--- superset/tasks/schedules.py | 14 +++--- superset/views/alerts.py | 77 +++++++----------------------- tests/alerts_tests.py | 7 ++- 5 files changed, 40 insertions(+), 78 deletions(-) diff --git a/superset/models/alerts.py b/superset/models/alerts.py index 95e7c0604..065f7cf70 100644 --- a/superset/models/alerts.py +++ b/superset/models/alerts.py @@ -59,6 +59,7 @@ 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)) @@ -66,7 +67,7 @@ class Alert(Model, AuditMixinNullable): 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 @@ class Validator(Model, AuditMixinNullable): 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) diff --git a/superset/tasks/alerts/validator.py b/superset/tasks/alerts/validator.py index 56dfad4dd..68d908290 100644 --- a/superset/tasks/alerts/validator.py +++ b/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( diff --git a/superset/tasks/schedules.py b/superset/tasks/schedules.py index 7c5ad4adb..09a42145e 100644 --- a/superset/tasks/schedules.py +++ b/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( diff --git a/superset/views/alerts.py b/superset/views/alerts.py index c4de48bb8..5d0d1b825 100644 --- a/superset/views/alerts.py +++ b/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 @@ from superset.models.alerts import ( 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,10 +178,6 @@ 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", @@ -191,7 +185,20 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors "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) diff --git a/tests/alerts_tests.py b/tests/alerts_tests.py index 5f6464cc1..81c94b00b 100644 --- a/tests/alerts_tests.py +++ b/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"\n",