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

fix: add validator information to email/slack alerts #10762

Merged
merged 3 commits into from
Sep 3, 2020

Conversation

JasonD28
Copy link
Contributor

@JasonD28 JasonD28 commented Sep 2, 2020

SUMMARY

This PR updates alert messages by adding the reason why a SQL-based alert was triggered. Information is taken from the alert's validator string.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Email:
Screen Shot 2020-09-02 at 12 19 34 PM
Slack:
Screen Shot 2020-09-02 at 12 20 10 PM

TEST PLAN

  • local
  • unit test

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

Copy link
Member

@bkyryliuk bkyryliuk left a comment

Choose a reason for hiding this comment

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

LG%nits

@@ -201,3 +202,15 @@ def alert(self) -> RelationshipProperty:
foreign_keys=[self.alert_id],
backref=backref("validators", cascade="all, delete-orphan"),
)

def __str__(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

s/str/pretty_print

sql = alert.sql_observer[0].sql if alert.sql_observer else ""
observation_value = (
str(alert.observations[-1].value) if alert.observations else "Value"
validation_str = (
Copy link
Member

Choose a reason for hiding this comment

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

s/validation_str/ validation_error_message

@@ -20,6 +20,7 @@
<p><b>SQL Statement:</b></p>
<code><mark style="background-color: LightGrey; font-size: 1.1em">{{sql}}</mark></code></p>
<p><b>SQL Result</b>: {{observation_value}}</p>
<p><b>Reason For Alert</b>: {{validation_str}}</p>
Copy link
Member

Choose a reason for hiding this comment

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

let's change it here a bit:

Query:
Result:
Reason:

to keep it a bit simpler

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

Merging #10762 into master will increase coverage by 1.88%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10762      +/-   ##
==========================================
+ Coverage   59.23%   61.11%   +1.88%     
==========================================
  Files         768      801      +33     
  Lines       36651    37774    +1123     
  Branches     3302     3555     +253     
==========================================
+ Hits        21710    23086    +1376     
+ Misses      14759    14502     -257     
- Partials      182      186       +4     
Flag Coverage Δ
#cypress ?
#javascript 61.59% <ø> (?)
#python 60.83% <80.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/models/alerts.py 96.80% <75.00%> (-2.03%) ⬇️
superset/tasks/schedules.py 75.74% <100.00%> (ø)
superset-frontend/src/views/App.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/views/menu.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/views/index.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/index.tsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 323 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 1ee87cc...ee9838e. Read the comment docs.

Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

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

LGTM once CI passes

@bkyryliuk bkyryliuk merged commit 54ae3b0 into apache:master Sep 3, 2020
JasonD28 added a commit to JasonD28/incubator-superset that referenced this pull request Sep 3, 2020
* added validator info to alerts

* adjusted format of messages

* added nits

Co-authored-by: Jason Davis <@dropbox.com>
(cherry picked from commit 54ae3b0)
amitmiran137 pushed a commit to ofekisr/incubator-superset that referenced this pull request Sep 7, 2020
…boards_permissions

* upstream/master: (32 commits)
  docs: Add a note to contributing.md on reporting security vulnerabilities (apache#10796)
  Fix: Include RLS filters for cache keys (apache#10805)
  feat: filters for database list view (apache#10772)
  fix: MVC show saved query (apache#10781)
  added creator column and adjusted order columns (apache#10789)
  security: disallow uuid package on jinja2 (apache#10794)
  feat: CRUD REST API for saved queries (apache#10777)
  fix: disable domain sharding on explore view (apache#10787)
  fix: can not type `0.05` in `TextControl` (apache#10778)
  fix: pivot table timestamp grouping (apache#10774)
  fix: add validator information to email/slack alerts (apache#10762)
  More Label touchups (margins) (apache#10722)
  fix: dashboard extra filters (apache#10692)
  fix: re-installing local superset in cache image (apache#10766)
  feat: SIP-34 table list view for databases (apache#10705)
  refactor: convert DatasetList schema filter to use new distinct api (apache#10746)
  chore: removing fsevents dependency (apache#10751)
  Fix precommit hook for docs/installation.rst (apache#10759)
  feat(database): POST, PUT, DELETE API endpoints (apache#10741)
  docs: Update OAuth configuration in installation.rst (apache#10748)
  ...
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* added validator info to alerts

* adjusted format of messages

* added nits

Co-authored-by: Jason Davis <@dropbox.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.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 size/M 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants