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.

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

feat(reports): send notification on error with grace #13135

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Feb 15, 2021

SUMMARY

Sends a notification to all recipients when an Alert or Report errors. Also implements grace period on these error notifications.

Simple description of the state flow:

1 - Alert created
2 - Alert runs but with error (invalid SQL, bad db connection, etc)
2.1 - A notification is sent to all alert recipients
( next beat)
3 - Alert runs but with error
3.1 - If the last error notification was sent during the configured grace period, no notification is sent


Flappy alert state flow:

1 - Alert created
( next beat)
2 - Alert runs but with error (invalid SQL, bad db connection, etc)
2.1 - A notification is sent to all alert recipients
( next beat)
3 - Alert runs but with error
3.1 - If the last error notification was sent during the configured grace period, no notification is sent
4. User fixes the error
( next beat)
5. Alert triggers an alert (success state)
( next beat)
6. Alert exists grace period
7. User un-fixes alert
( next beat)
8 - Alert runs but with error (invalid SQL, bad db connection, etc)
8.1 - A notification is sent to all alert recipients

Example error email:

Subject: [Report] Error occurred for Alert: <Alert name>
Error: Alert query returned a non-number value.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Feb 15, 2021

Codecov Report

Merging #13135 (f627483) into master (29d6420) will increase coverage by 2.77%.
The diff coverage is 92.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13135      +/-   ##
==========================================
+ Coverage   77.20%   79.97%   +2.77%     
==========================================
  Files         872      300     -572     
  Lines       45101    24420   -20681     
  Branches     5435        0    -5435     
==========================================
- Hits        34820    19530   -15290     
+ Misses      10158     4890    -5268     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress ?
javascript ?
python 79.97% <92.06%> (-0.87%) ⬇️

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% <ø> (ø)
superset/reports/notifications/slack.py 83.72% <71.42%> (-5.47%) ⬇️
superset/reports/notifications/email.py 97.87% <93.33%> (-2.13%) ⬇️
superset/reports/commands/execute.py 93.22% <100.00%> (+0.67%) ⬆️
superset/reports/dao.py 80.17% <100.00%> (+1.66%) ⬆️
superset/reports/notifications/base.py 95.65% <100.00%> (+0.19%) ⬆️
superset/utils/core.py 88.45% <100.00%> (ø)
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/views/database/views.py 62.69% <0.00%> (-24.88%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
... and 590 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 29d6420...f627483. Read the comment docs.

@willbarrett
Copy link
Member

willbarrett commented Feb 16, 2021

Code looks reasonable - would it be possible to add a link back to the alert in Superset to the error notification email? This would allow the user to quickly jump to the right place in the application to resolve the error.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM. Going forward I think adding an optional error recipient would be a good way of ensuring that alerts/reports with errors don't spam large recipient lists to whom the error may not be relevant.

@willbarrett willbarrett merged commit 0b114fc into apache:master Feb 24, 2021
@willbarrett willbarrett deleted the danielgaspar/ch4451/as-a-superset-user-when-an-alert-errors-i branch February 24, 2021 21:31
henryyeh pushed a commit to preset-io/superset that referenced this pull request Mar 3, 2021
* fix: add config to disable dataset ownership on the old api

* fix CI docker build

* fix logic

* add deprecation comment on the config

* feat: send alerts reports errors to recipients

* update

* feat(reports): send notification on error with grace

* merge and revert config

* fix lint and MySQL test

* fix mysql tests

(cherry picked from commit 0b114fc)
henryyeh pushed a commit to preset-io/superset that referenced this pull request Mar 4, 2021
* fix: add config to disable dataset ownership on the old api

* fix CI docker build

* fix logic

* add deprecation comment on the config

* feat: send alerts reports errors to recipients

* update

* feat(reports): send notification on error with grace

* merge and revert config

* fix lint and MySQL test

* fix mysql tests

(cherry picked from commit 0b114fc)
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* fix: add config to disable dataset ownership on the old api

* fix CI docker build

* fix logic

* add deprecation comment on the config

* feat: send alerts reports errors to recipients

* update

* feat(reports): send notification on error with grace

* merge and revert config

* fix lint and MySQL test

* fix mysql tests
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants