Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: refactored SQL-based alerts to not pass sqlalchemy objects as args #10506

Merged
merged 4 commits into from
Aug 4, 2020
Merged

fix: refactored SQL-based alerts to not pass sqlalchemy objects as args #10506

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cddfd4a
refractored alerting to not pass sqlalchemy obj as args
Aug 3, 2020
4a7db8f
updated to pass only alert id as arg
Aug 3, 2020
7f74e7c
used object id instead of argument
Aug 3, 2020
9a2a3dc
updated alerts_tests.py to reflect change
Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 40 additions & 20 deletions superset/tasks/schedules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
from superset import app, db, security_manager, thumbnail_cache
from superset.extensions import celery_app
from superset.models.alerts import Alert, AlertLog
from superset.models.core import Database
from superset.models.dashboard import Dashboard
from superset.models.schedules import (
EmailDeliveryType,
Expand Down Expand Up @@ -551,10 +552,17 @@ def schedule_alert_query( # pylint: disable=unused-argument

if report_type == ScheduleType.alert:
if is_test_alert and recipients:
deliver_alert(schedule, recipients)
deliver_alert(schedule.id, schedule.slice.id, schedule.label, recipients)
return

if run_alert_query(schedule):
if run_alert_query(
schedule.id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing in every param needed from schedule, maybe it's cleaner to only pass in the schedule id, then you can get the schedule object within the function and reference all the params there.

schedule.database_id,
schedule.slice.id,
schedule.sql,
schedule.label,
schedule.recipients,
):
# deliver_dashboard OR deliver_slice
return
else:
Expand All @@ -567,21 +575,21 @@ class AlertState:
PASS = "pass"


def deliver_alert(alert: Alert, recipients: Optional[str] = None) -> None:
logging.info("Triggering alert: %s", alert)
def deliver_alert(alert_id: int, slice_id: int, label: str, recipients: str) -> None:
logging.info("Triggering alert: <%s:%s>", alert_id, label)
img_data = None
images = {}
recipients = recipients or alert.recipients
alert_slice = db.session.query(Slice).get(slice_id)

if alert.slice:
if alert_slice:

chart_url = get_url_path(
"Superset.slice", slice_id=alert.slice.id, standalone="true"
"Superset.slice", slice_id=alert_slice.id, standalone="true"
)
screenshot = ChartScreenshot(chart_url, alert.slice.digest)
screenshot = ChartScreenshot(chart_url, alert_slice.digest)
cache_key = screenshot.cache_key()
image_url = get_url_path(
"ChartRestApi.screenshot", pk=alert.slice.id, digest=cache_key
"ChartRestApi.screenshot", pk=alert_slice.id, digest=cache_key
)

user = security_manager.find_user(current_app.config["THUMBNAIL_SELENIUM_USER"])
Expand All @@ -593,7 +601,7 @@ def deliver_alert(alert: Alert, recipients: Optional[str] = None) -> None:
image_url = "https://media.giphy.com/media/dzaUX7CAG0Ihi/giphy.gif"

# generate the email
subject = f"[Superset] Triggered alert: {alert.label}"
subject = f"[Superset] Triggered alert: {label}"
deliver_as_group = False
data = None
if img_data:
Expand All @@ -605,56 +613,68 @@ def deliver_alert(alert: Alert, recipients: Optional[str] = None) -> None:
<img src="cid:screenshot" alt="%(label)s" />
"""
),
label=alert.label,
label=label,
image_url=image_url,
)

_deliver_email(recipients, deliver_as_group, subject, body, data, images)


def run_alert_query(alert: Alert) -> Optional[bool]:
def run_alert_query(
alert_id: int,
database_id: int,
slice_id: int,
sql: str,
label: str,
recipients: str,
) -> Optional[bool]:
"""
Execute alert.sql and return value if any rows are returned
database_id, sql, alert_id, slice_id, label
"""
logger.info("Processing alert ID: %i", alert.id)
database = alert.database
logger.info("Processing alert ID: %i", alert_id)
database = db.session.query(Database).get(database_id)
if not database:
logger.error("Alert database not preset")
return None

if not alert.sql:
if not sql:
logger.error("Alert SQL not preset")
return None

parsed_query = ParsedQuery(alert.sql)
parsed_query = ParsedQuery(sql)
sql = parsed_query.stripped()

state = None
dttm_start = datetime.utcnow()

df = pd.DataFrame()
try:
logger.info("Evaluating SQL for alert %s", alert)
logger.info("Evaluating SQL for alert <%s:%s>", alert_id, label)
df = database.get_df(sql)
except Exception as exc: # pylint: disable=broad-except
state = AlertState.ERROR
logging.exception(exc)
logging.error("Failed at evaluating alert: %s (%s)", alert.label, alert.id)
logging.error("Failed at evaluating alert: %s (%s)", label, alert_id)

dttm_end = datetime.utcnow()

if state != AlertState.ERROR:
alert.last_eval_dttm = datetime.utcnow()
if not df.empty:
# Looking for truthy cells
for row in df.to_records():
if any(row):
state = AlertState.TRIGGER
deliver_alert(alert)
deliver_alert(alert_id, slice_id, label, recipients)
break
if not state:
state = AlertState.PASS

alert = db.session.query(Alert).get(alert_id)

if state != AlertState.ERROR:
alert.last_eval_dttm = datetime.utcnow()

alert.last_state = state
alert.logs.append(
AlertLog(
Expand Down