This is an automated email from the ASF dual-hosted git repository. maximebeauchemin pushed a commit to branch revert-10891-bogdan/alerting_fixes in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
commit 0df21d8c4d5d2d25823e47c4e7a88d34ed8b3023 Author: Maxime Beauchemin <[email protected]> AuthorDate: Fri Oct 2 09:57:15 2020 -0700 Revert "fix: superset alerting misc fixes (#10891)" This reverts commit c3f1720456fd7e7d94b986b43b5b057e5236e65a. --- 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, 78 insertions(+), 40 deletions(-) diff --git a/superset/models/alerts.py b/superset/models/alerts.py index 065f7cf..95e7c06 100644 --- a/superset/models/alerts.py +++ b/superset/models/alerts.py @@ -59,7 +59,6 @@ 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)) @@ -67,7 +66,7 @@ class Alert(Model, AuditMixinNullable): recipients = Column(Text) slack_channel = Column(Text) - # TODO(bkyryliuk): implement log_retention + # TODO: implement log_retention log_retention = Column(Integer, default=90) grace_period = Column(Integer, default=60 * 60 * 24) @@ -204,8 +203,7 @@ class Validator(Model, AuditMixinNullable): backref=backref("validators", cascade="all, delete-orphan"), ) - @property - def pretty_config(self) -> str: + def pretty_print(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 68d9082..56dfad4 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") is not None): + if not (config_dict.get("op") and config_dict.get("threshold")): raise SupersetException( "Error: Operator Validator needs specified operator and threshold " 'values. Add "op" and "threshold" to config.' @@ -87,13 +87,15 @@ 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 not observation or observation.value in (None, np.nan): - return False + 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 - operator = json.loads(validator_config)["op"] - threshold = json.loads(validator_config)["threshold"] - return OPERATOR_FUNCTIONS[operator](observation.value, threshold) + return False def get_validator_function( diff --git a/superset/tasks/schedules.py b/superset/tasks/schedules.py index 39394c0..ea48543 100644 --- a/superset/tasks/schedules.py +++ b/superset/tasks/schedules.py @@ -591,7 +591,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_config + str(alert.observations[-1].value) + " " + alert.validators[0].pretty_print() if alert.validators else "" ) @@ -742,13 +742,15 @@ 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 not alert.validators: - return False + 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 - validator = alert.validators[0] - validate = get_validator_function(validator.validator_type) - return bool(validate and validate(alert.sql_observer[0], validator.config)) + return False def next_schedules( diff --git a/superset/views/alerts.py b/superset/views/alerts.py index 5d0d1b8..c4de48b 100644 --- a/superset/views/alerts.py +++ b/superset/views/alerts.py @@ -14,10 +14,13 @@ # 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 ( @@ -27,7 +30,9 @@ 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 @@ -42,7 +47,6 @@ 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", @@ -56,7 +60,6 @@ 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 = ( @@ -127,9 +130,8 @@ class ValidatorInlineView( # pylint: disable=too-many-ancestors add_columns = edit_columns list_columns = [ - "alert.label", "validator_type", - "pretty_config", + "alert.label", ] label_columns = { @@ -178,6 +180,10 @@ 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", @@ -185,20 +191,7 @@ class AlertModelView(SupersetModelView): # pylint: disable=too-many-ancestors "last_eval_dttm", "last_state", "active", - "owners", - ) - show_columns = ( - "label", - "active", - "crontab", - "owners", - "slice", - "recipients", - "slack_channel", - "log_retention", - "grace_period", - "last_eval_dttm", - "last_state", + "creator", ) order_columns = ["label", "last_eval_dttm", "last_state", "active"] add_columns = ( @@ -215,6 +208,9 @@ 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)"), @@ -234,6 +230,28 @@ 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, @@ -242,11 +260,34 @@ 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 53245e6..6a453fd 100644 --- a/tests/alerts_tests.py +++ b/tests/alerts_tests.py @@ -259,11 +259,6 @@ 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") @@ -356,7 +351,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_config}\n" + f"*Reason*: {alert.observations[-1].value} {alert.validators[0].pretty_print()}\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*>",
