bkyryliuk commented on a change in pull request #10605:
URL: 
https://github.com/apache/incubator-superset/pull/10605#discussion_r480232654



##########
File path: superset/models/alerts.py
##########
@@ -51,16 +55,16 @@ class Alert(Model):
     __tablename__ = "alerts"
 
     id = Column(Integer, primary_key=True)
-    label = Column(String(150))
+    label = Column(String(150), nullable=False)

Review comment:
       let's inherit objects that are modifiable by the user from AuditMixin:
   e.g. 
https://github.com/apache/incubator-superset/blob/9fc37ea9f1ff032dea923012613d4e7189ada178/superset/migrations/versions/27ae655e4247_make_creator_owners.py#L65
   
   it will require you to regenerate the migrations

##########
File path: superset/tasks/alerts/observer.py
##########
@@ -0,0 +1,98 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from datetime import datetime
+from typing import Optional
+
+import pandas as pd
+
+from superset import db
+from superset.models.alerts import SQLObservation, SQLObserver
+from superset.sql_parse import ParsedQuery
+
+logger = logging.getLogger("tasks.email_reports")
+
+
+def observe(alert_id: int) -> Optional[str]:
+    """
+    Runs the SQL query in an alert's SQLObserver and then
+    stores the result in a SQLObservation.
+    Returns an error message if the observer value was not valid
+    """
+
+    sql_observer = 
db.session.query(SQLObserver).filter_by(alert_id=alert_id).one()
+
+    value = None
+
+    parsed_query = ParsedQuery(sql_observer.sql)
+    sql = parsed_query.stripped()
+    df = sql_observer.database.get_df(sql)
+
+    error_msg = check_observer_result(df, sql_observer.id, sql_observer.name)
+
+    if not error_msg and df.to_records()[0][1] is not None:
+        value = float(df.to_records()[0][1])

Review comment:
       add a comment that we also support numerical observations in the 
observer UI

##########
File path: superset/models/alerts.py
##########
@@ -100,3 +97,103 @@ class AlertLog(Model):
     @property
     def duration(self) -> int:
         return (self.dttm_end - self.dttm_start).total_seconds()
+
+
+# TODO: Currently SQLObservation table will constantly grow with no limit,
+# add some retention restriction or more to a more scalable db e.g.
+# 
https://github.com/apache/incubator-superset/blob/master/superset/utils/log.py#L32
+class SQLObserver(Model):
+    """Runs SQL-based queries for alerts"""
+
+    __tablename__ = "sql_observers"
+
+    id = Column(Integer, primary_key=True)
+    name = Column(String(150), nullable=False)

Review comment:
       let's get rid of name here, clicked trough the ui, do not see any use 
case for it.
   it can be added later the use case will arise

##########
File path: superset/tasks/alerts/observer.py
##########
@@ -0,0 +1,98 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from datetime import datetime
+from typing import Optional
+
+import pandas as pd
+
+from superset import db
+from superset.models.alerts import SQLObservation, SQLObserver
+from superset.sql_parse import ParsedQuery
+
+logger = logging.getLogger("tasks.email_reports")
+
+
+def observe(alert_id: int) -> Optional[str]:
+    """
+    Runs the SQL query in an alert's SQLObserver and then
+    stores the result in a SQLObservation.
+    Returns an error message if the observer value was not valid
+    """
+
+    sql_observer = 
db.session.query(SQLObserver).filter_by(alert_id=alert_id).one()
+
+    value = None
+
+    parsed_query = ParsedQuery(sql_observer.sql)
+    sql = parsed_query.stripped()
+    df = sql_observer.database.get_df(sql)
+
+    error_msg = check_observer_result(df, sql_observer.id, sql_observer.name)

Review comment:
       s/check_observer_result/validate__observer_result
   after thinking a bit more about it, validate would be more appropriate

##########
File path: superset/tasks/alerts/observer.py
##########
@@ -0,0 +1,98 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import logging
+from datetime import datetime
+from typing import Optional
+
+import pandas as pd
+
+from superset import db
+from superset.models.alerts import SQLObservation, SQLObserver
+from superset.sql_parse import ParsedQuery
+
+logger = logging.getLogger("tasks.email_reports")
+
+
+def observe(alert_id: int) -> Optional[str]:
+    """
+    Runs the SQL query in an alert's SQLObserver and then
+    stores the result in a SQLObservation.
+    Returns an error message if the observer value was not valid
+    """
+
+    sql_observer = 
db.session.query(SQLObserver).filter_by(alert_id=alert_id).one()
+
+    value = None
+
+    parsed_query = ParsedQuery(sql_observer.sql)
+    sql = parsed_query.stripped()
+    df = sql_observer.database.get_df(sql)
+
+    error_msg = check_observer_result(df, sql_observer.id, sql_observer.name)
+
+    if not error_msg and df.to_records()[0][1] is not None:
+        value = float(df.to_records()[0][1])
+
+    observation = SQLObservation(
+        observer_id=sql_observer.id,
+        alert_id=alert_id,
+        dttm=datetime.utcnow(),
+        value=value,
+        error_msg=error_msg,
+    )
+
+    db.session.add(observation)
+    db.session.commit()
+
+    return error_msg
+
+
+def check_observer_result(
+    sql_result: pd.DataFrame, observer_id: int, observer_name: str
+) -> Optional[str]:
+    """
+    Verifies if a DataFrame SQL query result to see if
+    it contains a valid value for a SQLObservation.
+    Returns an error message if the result is invalid.
+    """
+    try:
+        assert (
+            not sql_result.empty
+        ), f"Observer <{observer_id}:{observer_name}> returned no rows"

Review comment:
       taking into account that alert -> observer is 1-1, you can add alert 
name to the error message

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '
+                "in the config."
+            )
+
+
+def not_null_validator(
+    observer: SQLObserver, validator_config: str  # pylint: 
disable=unused-argument
+) -> bool:
+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]
+    if observation.error_msg or observation.value in (0, None, np.nan):

Review comment:
       add a todo to deal separately with the malformed observations, it may be 
useful to treat failed & malformed separately

##########
File path: superset/views/alerts.py
##########
@@ -47,6 +55,130 @@ class AlertLogModelView(
     )
 
 
+class AlertObservationModelView(
+    CompactCRUDMixin, SupersetModelView
+):  # pylint: disable=too-many-ancestors
+    datamodel = SQLAInterface(SQLObservation)
+    include_route_methods = {RouteMethod.LIST} | {"show"}
+    list_title = _("List Observations")
+    show_title = _("Show Observation")
+    list_columns = (
+        "dttm",
+        "value",
+        "error_msg",
+    )
+    label_columns = {
+        "error_msg": _("Error Message"),
+    }
+
+
+# TODO: add a button to the form to test if the SQL statment can run with no 
errors
+class SQLObserverInlineView(  # pylint: disable=too-many-ancestors
+    CompactCRUDMixin, SupersetModelView
+):
+    datamodel = SQLAInterface(SQLObserver)
+    include_route_methods = RouteMethod.RELATED_VIEW_SET | RouteMethod.API_SET
+    list_title = _("SQL Observers")
+    show_title = _("Show SQL Observer")
+    add_title = _("Add SQL Observer")
+    edit_title = _("Edit SQL Observer")
+
+    edit_columns = [
+        "name",
+        "alert",
+        "database",
+        "sql",
+    ]
+
+    add_columns = edit_columns
+
+    list_columns = [
+        "name",
+        "alert.label",
+        "database",
+    ]
+
+    label_columns = {
+        "name": _("Name"),
+        "alert": _("Alert"),
+        "database": _("Database"),
+        "sql": _("SQL"),
+    }
+
+    description_columns = {
+        "sql": _(
+            "A SQL statement that defines whether the alert should get 
triggered or "
+            "not. The query is expected to return either NULL or a number 
value."
+        )
+    }
+
+    def pre_add(self, item: "SQLObserverInlineView") -> None:
+        if item.alert.sql_observer and item.alert.sql_observer[0].id != 
item.id:
+            raise SupersetException("Error: An alert should only have one 
observer.")
+
+
+class ValidatorInlineView(  # pylint: disable=too-many-ancestors
+    CompactCRUDMixin, SupersetModelView
+):
+    datamodel = SQLAInterface(Validator)
+    include_route_methods = RouteMethod.RELATED_VIEW_SET | RouteMethod.API_SET
+    list_title = _("Validators")
+    show_title = _("Show Validator")
+    add_title = _("Add Validator")
+    edit_title = _("Edit Validator")
+
+    edit_columns = [
+        "name",
+        "alert",
+        "validator_type",
+        "config",
+    ]
+
+    add_columns = edit_columns
+
+    list_columns = [
+        "name",
+        "validator_type",
+        "alert.label",
+    ]
+
+    label_columns = {
+        "name": _("Name"),
+        "validator_type": _("Validator Type"),
+        "alert": _("Alert"),
+    }
+
+    description_columns = {
+        "validator_type": utils.markdown(
+            "Determines when to trigger alert based off value from SQLObserver 
query. "
+            "Alerts will be triggered with these validator types:"
+            "<ul><li>Not Null - When the return value is Not NULL, Empty, or 
0</li>"
+            "<li>Operator - When `sql_return_value comparison_operator 
threshold`"
+            " is True e.g. `50 <= 75`<br>Supports the comparison operators <, 
<=, "
+            ">, >=, ==, and !=</li></ul>",
+            True,
+        ),
+        "config": utils.markdown(

Review comment:
       show a full config example for each type of the operator

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'

Review comment:
       add a list of supported operators and shot the current value in the 
error msg

##########
File path: superset/models/alerts.py
##########
@@ -100,3 +97,103 @@ class AlertLog(Model):
     @property
     def duration(self) -> int:
         return (self.dttm_end - self.dttm_start).total_seconds()
+
+
+# TODO: Currently SQLObservation table will constantly grow with no limit,
+# add some retention restriction or more to a more scalable db e.g.
+# 
https://github.com/apache/incubator-superset/blob/master/superset/utils/log.py#L32
+class SQLObserver(Model):
+    """Runs SQL-based queries for alerts"""
+
+    __tablename__ = "sql_observers"
+
+    id = Column(Integer, primary_key=True)
+    name = Column(String(150), nullable=False)
+    sql = Column(Text, nullable=False)
+
+    @declared_attr
+    def alert_id(self) -> int:
+        return Column(Integer, ForeignKey("alerts.id"), nullable=False)
+
+    @declared_attr
+    def alert(self) -> RelationshipProperty:
+        return relationship(
+            "Alert",
+            foreign_keys=[self.alert_id],
+            backref=backref("sql_observer", cascade="all, delete-orphan"),
+        )
+
+    @declared_attr
+    def database_id(self) -> int:
+        return Column(Integer, ForeignKey("dbs.id"), nullable=False)
+
+    @declared_attr
+    def database(self) -> RelationshipProperty:
+        return relationship(
+            "Database",
+            foreign_keys=[self.database_id],
+            backref=backref("sql_observers", cascade="all, delete-orphan"),
+        )
+
+    def get_observations(self, observation_num: Optional[int] = 2) -> 
List[Any]:
+        return (
+            db.session.query(SQLObservation)
+            .filter_by(observer_id=self.id)
+            .order_by(SQLObservation.dttm.desc())
+            .limit(observation_num)
+        )
+
+
+class SQLObservation(Model):  # pylint: disable=too-few-public-methods
+    """Keeps track of values retrieved from SQLObservers"""
+
+    __tablename__ = "sql_observations"
+
+    id = Column(Integer, primary_key=True)
+    dttm = Column(DateTime, default=datetime.utcnow, index=True)
+    observer_id = Column(Integer, ForeignKey("sql_observers.id"), 
nullable=False)
+    observer = relationship(
+        "SQLObserver",
+        foreign_keys=[observer_id],
+        backref=backref("observations", cascade="all, delete-orphan"),
+    )
+    alert_id = Column(Integer, ForeignKey("alerts.id"))
+    alert = relationship(
+        "Alert",
+        foreign_keys=[alert_id],
+        backref=backref("observations", cascade="all, delete-orphan"),
+    )
+    value = Column(Float)
+    error_msg = Column(String(500))
+
+
+class Validator(Model):
+    """Used to determine how an alert and its observations should be 
validated"""
+
+    __tablename__ = "alert_validators"
+
+    id = Column(Integer, primary_key=True)
+    name = Column(String(150), nullable=False)

Review comment:
       same as above

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '
+                "in the config."
+            )
+
+
+def not_null_validator(
+    observer: SQLObserver, validator_config: str  # pylint: 
disable=unused-argument
+) -> bool:
+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]

Review comment:
       add a observer method: `get_last_observation`

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]

Review comment:
       make it a const in the file & a set

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '

Review comment:
       same here, show the threshold in the error msg

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '
+                "in the config."
+            )
+
+
+def not_null_validator(
+    observer: SQLObserver, validator_config: str  # pylint: 
disable=unused-argument
+) -> bool:
+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]
+    if observation.error_msg or observation.value in (0, None, np.nan):
+        return False
+    return True
+
+
+def operator_validator(  # pylint: disable=too-many-return-statements
+    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_observations(1)[0]
+    if observation.value is not None:

Review comment:
       I think you need to check for np.nan as well here

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '
+                "in the config."
+            )
+
+
+def not_null_validator(
+    observer: SQLObserver, validator_config: str  # pylint: 
disable=unused-argument
+) -> bool:
+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]
+    if observation.error_msg or observation.value in (0, None, np.nan):
+        return False
+    return True
+
+
+def operator_validator(  # pylint: disable=too-many-return-statements
+    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_observations(1)[0]
+    if observation.value is not None:
+        operator = json.loads(validator_config)["op"]
+        threshold = json.loads(validator_config)["threshold"]
+        if operator == ">=" and observation.value >= threshold:

Review comment:
       we can make it even nicer:
   https://docs.python.org/3/library/operator.html
   
   just create a dictionary lookup from the symbol to the function

##########
File path: superset/tasks/schedules.py
##########
@@ -637,6 +658,8 @@ def deliver_slack_alert(alert_content: AlertContent, 
slack_channel: str) -> None
             "slack/alert_no_screenshot.txt",
             label=alert_content.label,
             sql=alert_content.sql,
+            observation_value=alert_content.observation_value,

Review comment:
       when possible it would be nice to have a custom error message e.g.
   ```
   SQL Result: 2561720.0
   Expected: 2561720.0 < 100000
   ```

##########
File path: superset/tasks/schedules.py
##########
@@ -705,6 +722,21 @@ def run_alert_query(
     )
     db.session.commit()
 
+
+def validate_observations(alert_id: int, label: str) -> Optional[str]:
+    """
+    Runs an alert's validators to check if it should be triggered or not
+    If so, return the name of the validator that returned true
+    """
+
+    logger.info("Validating observations for alert <%s:%s>", alert_id, label)
+
+    alert = db.session.query(Alert).get(alert_id)
+    for validator in alert.alert_validators:

Review comment:
       hm, this is a deviation from design, any specific reason to do that?

##########
File path: superset/tasks/alerts/validator.py
##########
@@ -0,0 +1,112 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import enum
+import json
+from typing import Callable
+
+import numpy as np
+
+from superset.exceptions import SupersetException
+from superset.models.alerts import SQLObserver
+
+
+class AlertValidatorType(enum.Enum):
+    not_null = "not null"
+    operator = "operator"
+
+    @classmethod
+    def valid_type(cls, validator_type: str) -> bool:
+        return any(val_type.value == validator_type for val_type in cls)
+
+
+def check_validator(validator_type: str, config: str) -> None:
+    if not AlertValidatorType.valid_type(validator_type):
+        raise SupersetException(
+            f"Error: {validator_type} is not a valid validator type."
+        )
+
+    config_dict = json.loads(config)
+
+    if validator_type == AlertValidatorType.operator.value:
+        valid_operators = ["<", "<=", ">", ">=", "==", "!="]
+
+        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.'
+            )
+
+        if not config_dict["op"] in valid_operators:
+            raise SupersetException(
+                'Error: Invalid operator type. Recheck "op" value in the 
config.'
+            )
+
+        if not isinstance(config_dict["threshold"], (int, float)):
+            raise SupersetException(
+                'Error: Invalid threshold value. Recheck "threshold" value '
+                "in the config."
+            )
+
+
+def not_null_validator(
+    observer: SQLObserver, validator_config: str  # pylint: 
disable=unused-argument
+) -> bool:
+    """Returns True if a SQLObserver's recent observation is not NULL"""
+
+    observation = observer.get_observations(1)[0]
+    if observation.error_msg or observation.value in (0, None, np.nan):
+        return False
+    return True
+
+
+def operator_validator(  # pylint: disable=too-many-return-statements
+    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_observations(1)[0]
+    if observation.value is not None:
+        operator = json.loads(validator_config)["op"]
+        threshold = json.loads(validator_config)["threshold"]
+        if operator == ">=" and observation.value >= threshold:
+            return True
+        if operator == ">" and observation.value > threshold:
+            return True
+        if operator == "<=" and observation.value <= threshold:
+            return True
+        if operator == "<" and observation.value < threshold:
+            return True
+        if operator == "==" and observation.value == threshold:
+            return True
+        if operator == "!=" and observation.value != threshold:
+            return True
+
+    return False
+
+
+def get_validator_function(validator_type: str) -> Callable[[SQLObserver, 
str], bool]:
+    """Returns a validation function based on validator_type"""
+
+    alert_validators = {
+        AlertValidatorType.not_null.value: not_null_validator,
+        AlertValidatorType.operator.value: operator_validator,
+    }
+
+    return alert_validators[validator_type.lower()]

Review comment:
       should you use .value here to be consistent with code above?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to