-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
cb3f913
to
e6e66f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG%nits
superset/models/alerts.py
Outdated
@@ -201,3 +202,15 @@ def alert(self) -> RelationshipProperty: | |||
foreign_keys=[self.alert_id], | |||
backref=backref("validators", cascade="all, delete-orphan"), | |||
) | |||
|
|||
def __str__(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/str/pretty_print
superset/tasks/schedules.py
Outdated
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 = ( |
There was a problem hiding this comment.
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
superset/templates/email/alert.txt
Outdated
@@ -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> |
There was a problem hiding this comment.
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
30c607c
to
9624cf4
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this 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
9624cf4
to
ee9838e
Compare
* added validator info to alerts * adjusted format of messages * added nits Co-authored-by: Jason Davis <@dropbox.com> (cherry picked from commit 54ae3b0)
…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) ...
* added validator info to alerts * adjusted format of messages * added nits Co-authored-by: Jason Davis <@dropbox.com>
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:
Slack:
TEST PLAN
ADDITIONAL INFORMATION