feat: new reports scheduler #11711

dpgaspar · 2020-11-16T12:09:18Z

SUMMARY

Implements the new alerts and reports processing on the celery workers. PR number 3, previous related PR's #11606 #11550

This is a rework heavily based on the existing alerts implementation, maintains the current logic with the following differences:

Celery beat scheduling, was simplified and does not try to schedule possible lost events from the past, the schedule window is framed present to future.
Only one beat config is needed for alerts and reports, should be configured with the lowest time grain possible for cron, aka 1 minute.
Implements a log prune task, this should be configured on a separate celery beat (e.g. 1 day).
Notifications currently support Email and Slack but it's easy to extend to other platforms.

ADDITIONAL INFORMATION

# Conflicts: # superset/reports/dao.py

codecov-io · 2020-11-16T12:40:07Z

Codecov Report

Merging #11711 (d0e3770) into master (ec8ccd4) will decrease coverage by 2.34%.
The diff coverage is 78.04%.

@@            Coverage Diff             @@
##           master   #11711      +/-   ##
==========================================
- Coverage   62.86%   60.51%   -2.35%     
==========================================
  Files         889      866      -23     
  Lines       43054    42309     -745     
  Branches     4016     3725     -291     
==========================================
- Hits        27064    25604    -1460     
- Misses      15811    16705     +894     
+ Partials      179        0     -179

Flag	Coverage Δ
cypress	`55.03% <ø> (?)`
javascript	`?`
python	`63.38% <78.04%> (+0.49%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/reports/commands/log_prune.py	`0.00% <0.00%> (ø)`
superset/tasks/celery_app.py	`0.00% <0.00%> (ø)`
superset/tasks/scheduler.py	`0.00% <0.00%> (ø)`
superset/reports/dao.py	`76.13% <50.00%> (-8.94%)`	⬇️
superset/reports/notifications/__init__.py	`88.88% <88.88%> (ø)`
superset/reports/notifications/slack.py	`89.18% <89.18%> (ø)`
superset/reports/commands/alert.py	`92.00% <92.00%> (ø)`
superset/reports/notifications/base.py	`95.45% <95.45%> (ø)`
superset/reports/commands/execute.py	`96.29% <96.29%> (ø)`
superset/dao/base.py	`96.49% <100.00%> (+0.12%)`	⬆️
... and 447 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec8ccd4...d0e3770. Read the comment docs.

willbarrett

Good start! A few comments, looking forward to seeing the test suite!!!

willbarrett · 2020-11-17T04:32:00Z

superset/reports/commands/alert.py

+        if len(rows) > 1:
+            raise AlertQueryMultipleRowsError()
+        if len(rows[0]) > 2:
+            raise AlertQueryMultipleColumnsError()


I think we only want to raise here if the validator is something other than "NOT NULL" - if a row is returned, then it's not null :)

ups! Right you are

nit: it may be cleaner to have separate validation functions depending on the type of the validator

superset/reports/commands/alert.py

willbarrett · 2020-11-17T04:33:04Z

superset/reports/commands/alert.py

+        self._report_schedule.last_value = self._result
+
+        if self._report_schedule.validator_type == ReportScheduleValidatorType.NOT_NULL:
+            return self._result not in (0, None, np.nan)


I think this is correct if the result is a float, but if it's a row or set of rows this should behave differently.

superset/reports/commands/exceptions.py

willbarrett · 2020-11-17T04:44:40Z

superset/reports/notifications/__init__.py

+from superset.reports.notifications.slack import SlackNotification
+
+
+def create_notification(


This is the rare instance where I support a non-empty __init__.py. Revel, Daniel, in your success.

willbarrett · 2020-11-17T04:47:27Z

superset/reports/notifications/email.py

+        content = self._get_content()
+        to = self._get_to()
+        send_email_smtp(
+            to,


This is an interesting question - should we treat this as a single SMTP send with all of the recipients in the to address field, or split recipients by , and send to each one individually? My preference would be the latter, but open to discussion. Eventually we should provide configuration to allow MTA batching.

Right now it's somewhat simplistic, each recipients target will trigger an email, if the target contains multiple SMTP addresses then only one email is sent. I would vote for optimising on the next phase

willbarrett · 2020-11-17T04:52:15Z

superset/reports/commands/execute.py

+logger = logging.getLogger(__name__)
+
+
+class AsyncExecuteReportScheduleCommand(BaseCommand):


I read this class a few times looking for suggestions on simplification or breaking it up. I didn't come up with much, but I figured I'd surface that I felt the need to make the effort.

Did a slight refactor, main run is pretty simple now

bkyryliuk

left couple nits, but overall looks really good
definitely needs unit tests, your call if they should be added in this PR or later

Thanks a lot for revamping alerts!

bkyryliuk · 2020-11-20T04:04:13Z

superset/reports/commands/alert.py

+        if len(rows) > 1:
+            raise AlertQueryMultipleRowsError()
+        if len(rows[0]) > 2:
+            raise AlertQueryMultipleColumnsError()


nit: it may be cleaner to have separate validation functions depending on the type of the validator

bkyryliuk · 2020-11-20T04:05:05Z

superset/reports/commands/alert.py

+            self._result = rows[0][1]
+            return
+        # check if query return more then one row
+        if len(rows) > 1:


it will be useful to add result to the exception & surface it to the user in the error msg

What do you think about surfacing the number of returned rows? Would be nice to try to wrap all the rows in a string, but need to be careful and truncate it to a certain point, think about a user mistake with millions of rows?

yep, number of rows is a good start, we can tune it later, your right that it needs smart truncation

ok, done that

bkyryliuk · 2020-11-20T04:10:31Z

superset/reports/commands/execute.py

+            dashboard_id_or_slug=report_schedule.dashboard_id,
+        )
+
+    def _get_screenshot(self, report_schedule: ReportSchedule) -> ScreenshotData:


nit: it would be nice to pass only the attributes needed to calculate the screenshot vs a whole class
it makes it easier to unit tests the functions when less objects needs to be constructed, and read the functions / code as well as functions definition explains what is passed to it.

this is just a personal preference and optional suggestion

Good point, yet commands internally are model centric, we assume that validate validates and populates the model by model_id. My take on unit tests is to test public methods, and assume that the screenshot itself is being tested also

bkyryliuk · 2020-11-20T04:15:39Z

superset/reports/commands/execute.py

+                self.set_state_and_log(
+                    session, start_dttm, ReportLogState.NOOP, error_message=str(ex)
+                )
+            except ReportSchedulePreviousWorkingError as ex:


should this one be retried ?

I think not, since I'm proposing a 1min beat, the beat cycle itself is the retry, yet the cron for the alert can be set to 1h or 10min etc. Undecided..., I propose not to throw too much logic here, unless we find it's something we really should add

sounds good

bkyryliuk · 2020-11-20T04:17:25Z

superset/reports/commands/execute.py

+        if self._model.type == ReportScheduleType.ALERT:
+            last_success = ReportScheduleDAO.find_last_success_log(session)
+            if (
+                last_success


would be nice to move it to a separate function & have unit test

bkyryliuk · 2020-11-20T04:17:43Z

superset/reports/commands/log_prune.py

+logger = logging.getLogger(__name__)
+
+
+class AsyncPruneReportScheduleLogCommand(BaseCommand):


bkyryliuk · 2020-11-20T04:19:05Z

superset/reports/notifications/base.py

+    screenshot: ScreenshotData
+
+
+class BaseNotification:  # pylint: disable=too-few-public-methods


dpgaspar · 2020-11-20T09:09:12Z

left couple nits, but overall looks really good
definitely needs unit tests, your call if they should be added in this PR or later

Thanks a lot for revamping alerts!

Thanks a lot for the review! note that this PR already has unit tests

bkyryliuk · 2020-11-20T20:56:07Z

left couple nits, but overall looks really good
definitely needs unit tests, your call if they should be added in this PR or later
Thanks a lot for revamping alerts!

Thanks a lot for the review! note that this PR already has unit tests

damn, that's my bad - need to keep attention to the collapsed github files :)

dpgaspar added 3 commits November 13, 2020 09:22

feat(reports): scheduler and delivery system

bb7dea9

Merge branch 'master' into feat/reports-models-scheduler

136d4ae

# Conflicts: # superset/reports/dao.py

working version

b352280

superset-github-bot bot added the preset-io label Nov 16, 2020

pull-request-size bot added the size/XL label Nov 16, 2020

dpgaspar changed the title ~~Feat/reports models scheduler~~ feat: new reports scheduler Nov 16, 2020

add missing license

d2f0d05

dpgaspar added 3 commits November 16, 2020 14:45

lint

cfa92eb

improvements and fix grace_period

2ff6de2

lint

0e1f081

willbarrett reviewed Nov 17, 2020

View reviewed changes

add tests and fix bugs

198cd75

pull-request-size bot added size/XXL and removed size/XL labels Nov 19, 2020

dpgaspar added 7 commits November 19, 2020 09:51

test

91751aa

test

260838b

more tests

32e40e3

fix report API test

f78dd9e

test MySQL test fail

44923b4

delete-orphan

89abae3

fix MySQL tests

094a3c7

dpgaspar marked this pull request as ready for review November 19, 2020 15:07

dpgaspar requested a review from bkyryliuk November 19, 2020 15:09

bkyryliuk approved these changes Nov 20, 2020

View reviewed changes

dpgaspar added 2 commits November 23, 2020 16:48

address comments

c5ccd12

lint

d0e3770

dpgaspar requested a review from willbarrett November 24, 2020 09:07

willbarrett approved these changes Nov 24, 2020

View reviewed changes

dpgaspar merged commit f27ebc4 into apache:master Nov 25, 2020

dpgaspar deleted the feat/reports-models-scheduler branch November 25, 2020 08:50

robdiciuccio mentioned this pull request Jan 20, 2021

fix: Stabilize and deprecate legacy alerts module #12627

Merged

6 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new reports scheduler #11711

feat: new reports scheduler #11711

dpgaspar commented Nov 16, 2020 •

edited

Loading

codecov-io commented Nov 16, 2020 •

edited

Loading

willbarrett left a comment

willbarrett Nov 17, 2020

dpgaspar Nov 19, 2020

bkyryliuk Nov 20, 2020

dpgaspar Nov 23, 2020

willbarrett Nov 17, 2020

willbarrett Nov 17, 2020

willbarrett Nov 17, 2020

dpgaspar Nov 24, 2020

willbarrett Nov 17, 2020

dpgaspar Nov 19, 2020

bkyryliuk left a comment

bkyryliuk Nov 20, 2020

bkyryliuk Nov 20, 2020

dpgaspar Nov 20, 2020

bkyryliuk Nov 20, 2020

dpgaspar Nov 23, 2020

bkyryliuk Nov 20, 2020

dpgaspar Nov 24, 2020

bkyryliuk Nov 20, 2020

dpgaspar Nov 20, 2020

bkyryliuk Nov 20, 2020

bkyryliuk Nov 20, 2020

bkyryliuk Nov 20, 2020

bkyryliuk Nov 20, 2020

dpgaspar commented Nov 20, 2020

bkyryliuk commented Nov 20, 2020

		from superset.reports.notifications.slack import SlackNotification


		def create_notification(

		logger = logging.getLogger(__name__)


		class AsyncExecuteReportScheduleCommand(BaseCommand):

		logger = logging.getLogger(__name__)


		class AsyncPruneReportScheduleLogCommand(BaseCommand):

		screenshot: ScreenshotData


		class BaseNotification: # pylint: disable=too-few-public-methods

feat: new reports scheduler #11711

feat: new reports scheduler #11711

Conversation

dpgaspar commented Nov 16, 2020 • edited Loading

SUMMARY

ADDITIONAL INFORMATION

codecov-io commented Nov 16, 2020 • edited Loading

Codecov Report

willbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkyryliuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar commented Nov 20, 2020

bkyryliuk commented Nov 20, 2020

dpgaspar commented Nov 16, 2020 •

edited

Loading

codecov-io commented Nov 16, 2020 •

edited

Loading