feat: superset report slack integration #9810

bkyryliuk · 2020-05-14T21:47:14Z

SUMMARY

Implements slack support for the superset scheduled reports. This is a simple extension of the existing email functionality.

It also includes a some refactor that decouple the utility functions from the email schedule object to keep them reusable for other features, e.g. alerting

Form:

Here are couple examples on how it looks like in slack.

TEST PLAN

[x] unit tests
[x] local testing
[x] dropbox staging

ADDITIONAL INFORMATION

mistercrunch · 2020-05-15T04:05:25Z

codecov-io · 2020-05-18T21:52:12Z

Codecov Report

Merging #9810 into master will decrease coverage by 0.03%.
The diff coverage is 65.15%.

@@            Coverage Diff             @@
##           master    #9810      +/-   ##
==========================================
- Coverage   71.01%   70.98%   -0.04%     
==========================================
  Files         586      583       -3     
  Lines       30635    30643       +8     
  Branches     3159     3154       -5     
==========================================
- Hits        21756    21751       -5     
- Misses       8768     8781      +13     
  Partials      111      111

Flag	Coverage Δ
#cypress	`53.55% <ø> (-0.03%)`	⬇️
#javascript	`59.25% <ø> (+0.01%)`	⬆️
#python	`71.22% <65.15%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
superset/views/schedules.py	`59.29% <0.00%> (-1.62%)`	⬇️
superset/tasks/schedules.py	`75.94% <67.79%> (-3.87%)`	⬇️
superset/config.py	`89.66% <100.00%> (+0.08%)`	⬆️
superset/models/schedules.py	`89.79% <100.00%> (+0.21%)`	⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js	`66.81% <0.00%> (ø)`
...rset-frontend/src/components/ListView/ListView.tsx	`98.07% <0.00%> (ø)`
...frontend/src/components/ListView/LegacyFilters.tsx	`75.00% <0.00%> (ø)`
...rontend/src/explore/components/PropertiesModal.tsx	`16.17% <0.00%> (ø)`
...src/SqlLab/components/ExploreCtasResultsButton.jsx	`13.33% <0.00%> (ø)`
...rset-frontend/src/components/OptionDescription.jsx
... and 3 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 7a95c52...4e9d829. Read the comment docs.

codecov-commenter · 2020-05-19T00:57:27Z

Codecov Report

Merging #9810 into master will increase coverage by 0.02%.
The diff coverage is 85.07%.

@@            Coverage Diff             @@
##           master    #9810      +/-   ##
==========================================
+ Coverage   70.41%   70.43%   +0.02%     
==========================================
  Files         585      586       +1     
  Lines       31058    31090      +32     
  Branches     3267     3277      +10     
==========================================
+ Hits        21868    21899      +31     
- Misses       9079     9080       +1     
  Partials      111      111

Flag	Coverage Δ
#cypress	`53.93% <ø> (+0.07%)`	⬆️
#javascript	`59.36% <ø> (ø)`
#python	`70.03% <85.07%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
superset/views/schedules.py	`59.32% <0.00%> (-1.55%)`	⬇️
superset/tasks/schedules.py	`79.91% <86.36%> (+0.47%)`	⬆️
superset/config.py	`89.95% <100.00%> (+0.08%)`	⬆️
superset/models/schedules.py	`89.79% <100.00%> (+0.21%)`	⬆️
superset/tasks/slack_util.py	`100.00% <100.00%> (ø)`
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx	`72.72% <0.00%> (+6.81%)`	⬆️

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 280b4dc...0b6cfdd. Read the comment docs.

bkyryliuk · 2020-05-21T00:43:39Z

@mistercrunch , @mahendra would you be open to review this PR or suggest who else could help with it ?

mistercrunch · 2020-05-22T04:42:31Z

@dpgaspar can you take a look when you have a moment?

bkyryliuk · 2020-05-27T20:11:32Z

@dpgaspar could you please take a look ?
It is in dbx production and has been working fine for a while

villebro · 2020-05-28T05:56:51Z

FYI @bkyryliuk , @dpgaspar is currently on PTO, will be back tomorrow IIRC. I'll ping him when he's back.

dpgaspar

First pass, also it would be really nice to add some documentation around this new feature

dpgaspar · 2020-06-01T12:41:54Z

setup.py

@@ -102,6 +102,7 @@ def get_git_sha():
        "retry>=0.9.2",
        "selenium>=3.141.0",
        "simplejson>=3.15.0",
+        "slackclient>=2.5.0",


Can we bump to the latest 2.6.2, and what do you think about making this requirement optional (extras)

dpgaspar · 2020-06-01T12:42:13Z

requirements.txt

@@ -83,6 +83,7 @@ six==1.14.0               # via bleach, cryptography, flask-jwt-extended, flask-
 sqlalchemy-utils==0.36.4  # via apache-superset (setup.py), flask-appbuilder
 sqlalchemy==1.3.16        # via alembic, apache-superset (setup.py), flask-sqlalchemy, marshmallow-sqlalchemy, sqlalchemy-utils
 sqlparse==0.3.1           # via apache-superset (setup.py)
+slackclient==2.5.0        # via apache-superset (setup.py)


Can we bump to the latest 2.6.2

dpgaspar · 2020-06-01T12:55:19Z

superset/tasks/schedules.py

+            delay = int(ex.response.headers["Retry-After"])
+            logger.info(f"Rate limited. Retrying in {delay} seconds")
+            time.sleep(delay)
+            client.files_upload(


The second retry can fail again, also I wonder how will this behave on high concurrency. May be over optimisation but we could consider a simple loop count or a simple form of exponential backoff

sounds good, will keep it simple

dpgaspar · 2020-06-01T13:06:46Z

superset/views/schedules.py

+        "test_slack_channel": StringField(
+            "Test Slack Channel",
+            default=None,
+            description="A slack channel to send test message to.",


description="A slack channel to send a test message to.",

Also missing translations:

from flask_babel import lazy_gettext as _ "test_slack_channel": StringField( _("Test Slack Channel"), default=None, description=_("A slack channel to send test message to."), ... ....

this file doesn't have translations, should probably be a separate PR

superset/tasks/schedules.py

mistercrunch

mistercrunch · 2020-06-03T05:45:25Z

requirements.txt

@@ -4,92 +4,87 @@
 #
 #    pip-compile --output-file=requirements.txt setup.py
 #
-alembic==1.4.2            # via flask-migrate
+alembic==1.3.2            # via flask-migrate


Looks like there's a bad rebase here, lots of downgrades...

mistercrunch · 2020-06-03T05:49:04Z

superset/tasks/schedules.py

@@ -33,24 +33,25 @@
 from flask import render_template, Response, session, url_for
 from flask_babel import gettext as __
 from flask_login import login_user
-from retry.api import retry_call
+from retry.api import retry, retry_call


schedules.py is getting pretty big, would it make sense to have autils/slack.py?

that's a good suggestion, moved slack to tasks/slack_util.py for now as there is a single use case, once we have new use case like alerting -> will move to the shared util.
Also added TODO to do the same for email functions

bkyryliuk · 2020-06-09T18:31:44Z

sorry for the delay, will address the comments today

bkyryliuk

@mistercrunch , @dpgaspar ready for more feedback

bkyryliuk · 2020-06-09T19:15:44Z

superset/tasks/schedules.py

@@ -33,24 +33,25 @@
 from flask import render_template, Response, session, url_for
 from flask_babel import gettext as __
 from flask_login import login_user
-from retry.api import retry_call
+from retry.api import retry, retry_call


that's a good suggestion, moved slack to tasks/slack_util.py for now as there is a single use case, once we have new use case like alerting -> will move to the shared util.
Also added TODO to do the same for email functions

dpgaspar

LGTM

Fix slack another typo another typo Fix slack Add channels to the form another typo Another set of changes Make code more transparent Fix tests Add logging logging use logger import logging import logging import logging add assert more logging Fix channels Fix channels

* First draft for the slack integration Fix slack another typo another typo Fix slack Add channels to the form another typo Another set of changes Make code more transparent Fix tests Add logging logging use logger import logging import logging import logging add assert more logging Fix channels Fix channels * Address comments * Move slack into a separate module Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com> (cherry picked from commit 29e9f2c)

* First draft for the slack integration Fix slack another typo another typo Fix slack Add channels to the form another typo Another set of changes Make code more transparent Fix tests Add logging logging use logger import logging import logging import logging add assert more logging Fix channels Fix channels * Address comments * Move slack into a separate module Co-authored-by: bogdan kyryliuk <bogdankyryliuk@dropbox.com>

pull-request-size bot added the size/L label May 14, 2020

bkyryliuk marked this pull request as draft May 14, 2020 21:47

bkyryliuk force-pushed the bogdan/slack branch from f97c724 to a3e87c5 Compare May 14, 2020 21:50

bkyryliuk closed this May 18, 2020

bkyryliuk reopened this May 18, 2020

bkyryliuk force-pushed the bogdan/slack branch 6 times, most recently from cda16ef to 4e9d829 Compare May 18, 2020 21:45

bkyryliuk force-pushed the bogdan/slack branch from 4e9d829 to d77ad13 Compare May 19, 2020 00:51

bkyryliuk changed the title ~~feat: [WIP] Superset report slack integration~~ feat: superset report slack integration May 19, 2020

bkyryliuk marked this pull request as ready for review May 19, 2020 17:06

villebro requested a review from dpgaspar May 28, 2020 05:56

dpgaspar reviewed Jun 1, 2020

View reviewed changes

mistercrunch requested changes Jun 3, 2020

View reviewed changes

bkyryliuk force-pushed the bogdan/slack branch 5 times, most recently from a901cfa to 97fa344 Compare June 9, 2020 22:20

bkyryliuk force-pushed the bogdan/slack branch 8 times, most recently from b096d22 to 0b6cfdd Compare June 10, 2020 01:03

bkyryliuk commented Jun 10, 2020

View reviewed changes

dpgaspar approved these changes Jun 17, 2020

View reviewed changes

bogdan-dbx added 3 commits June 17, 2020 10:45

Address comments

0b303d6

Move slack into a separate module

bbacbc5

bkyryliuk force-pushed the bogdan/slack branch from 0b6cfdd to bbacbc5 Compare June 17, 2020 17:46

bkyryliuk merged commit 29e9f2c into apache:master Jun 17, 2020

bkyryliuk deleted the bogdan/slack branch June 17, 2020 18:01

villebro mentioned this pull request Jun 20, 2020

chore: Bump sqlalchemy-utils and cachelib and refresh requirements.txt #10117

Merged

6 tasks

bkyryliuk added changelog-highlight v0.37 labels Jul 2, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: superset report slack integration #9810

feat: superset report slack integration #9810

bkyryliuk commented May 14, 2020 •

edited

Loading

mistercrunch commented May 15, 2020

codecov-io commented May 18, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading

bkyryliuk commented May 21, 2020

mistercrunch commented May 22, 2020

bkyryliuk commented May 27, 2020

villebro commented May 28, 2020

dpgaspar left a comment

dpgaspar Jun 1, 2020

dpgaspar Jun 1, 2020

dpgaspar Jun 1, 2020

bkyryliuk Jun 3, 2020

dpgaspar Jun 1, 2020

dpgaspar Jun 1, 2020

bkyryliuk Jun 3, 2020

mistercrunch left a comment

mistercrunch Jun 3, 2020

mistercrunch Jun 3, 2020

bkyryliuk Jun 9, 2020

bkyryliuk commented Jun 9, 2020

bkyryliuk left a comment

bkyryliuk Jun 9, 2020

dpgaspar left a comment

feat: superset report slack integration #9810

feat: superset report slack integration #9810

Conversation

bkyryliuk commented May 14, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

mistercrunch commented May 15, 2020

codecov-io commented May 18, 2020 • edited Loading

Codecov Report

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

bkyryliuk commented May 21, 2020

mistercrunch commented May 22, 2020

bkyryliuk commented May 27, 2020

villebro commented May 28, 2020

dpgaspar 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

mistercrunch 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

bkyryliuk commented Jun 9, 2020

bkyryliuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgaspar left a comment

Choose a reason for hiding this comment

bkyryliuk commented May 14, 2020 •

edited

Loading

codecov-io commented May 18, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading