-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
chore: additional logging in alerts and reports #21802
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21802 +/- ##
==========================================
+ Coverage 66.85% 66.92% +0.07%
==========================================
Files 1805 1805
Lines 69061 70583 +1522
Branches 7369 7369
==========================================
+ Hits 46169 47238 +1069
- Misses 20995 21448 +453
Partials 1897 1897
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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!
@@ -207,6 +207,8 @@ def send(self) -> None: | |||
dryrun=False, | |||
header_data=content.header_data, | |||
) | |||
logger.info("Report sent to email") | |||
logger.info( | |||
"Report sent to email, notification content is %s", content.header_data |
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.
checked HeaderDataType
is safe to log.
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.
We log it in other places, so I think that it is.
superset/utils/core.py
Outdated
@@ -1020,6 +1021,7 @@ def send_mime_email( | |||
if smtp_user and smtp_password: | |||
smtp.login(smtp_user, smtp_password) | |||
logger.debug("Sent an email to %s", str(e_to)) | |||
logger.info("Sent email notification content is %s", mime_msg.as_string()) |
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.
don't think we should log the mime_msg itself
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.
ok, I think this one we can actually delete. Ty
superset/tasks/scheduler.py
Outdated
@@ -85,10 +85,12 @@ def execute(report_schedule_id: int, scheduled_dttm: str) -> None: | |||
logger.exception( | |||
"An unexpected occurred while executing the report: %s", task_id | |||
) | |||
execute.update_state(state="FAILURE") |
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.
Can you add a test to assert this?
superset/tasks/scheduler.py
Outdated
except CommandException: | ||
logger.exception( | ||
"A downstream exception occurred while generating" " a report: %s", task_id | ||
) | ||
execute.update_state(state="FAILURE") |
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.
can you add a test to assert this?
1c0a622
to
9399632
Compare
9399632
to
eee545e
Compare
eee545e
to
bdd1fdc
Compare
bdd1fdc
to
31d4f8a
Compare
37ee741
to
f33c698
Compare
f33c698
to
5c1f57e
Compare
SUMMARY
This adds more logging for alerts and reports, to better understand downstream exceptions.
Of particular note is that our logs show this messaging often:
![image](https://user-images.githubusercontent.com/48933336/195717778-2718ebf1-1aa4-42d6-89dc-9b8327e2caf7.png)
So we wanted to mark that task as having failed.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION