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

chore: additional logging in alerts and reports #21802

Merged
merged 4 commits into from
Oct 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion superset/reports/notifications/email.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

Copy link
Member Author

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.

)
except Exception as ex:
raise NotificationError(ex) from ex
7 changes: 5 additions & 2 deletions superset/tasks/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
# under the License.
import logging

from celery import Celery
from celery.exceptions import SoftTimeLimitExceeded
from dateutil import parser

Expand Down Expand Up @@ -70,8 +71,8 @@ def scheduler() -> None:
)


@celery_app.task(name="reports.execute")
def execute(report_schedule_id: int, scheduled_dttm: str) -> None:
@celery_app.task(name="reports.execute", bind=True)
def execute(self: Celery.task, report_schedule_id: int, scheduled_dttm: str) -> None:
task_id = None
try:
task_id = execute.request.id
Expand All @@ -90,10 +91,12 @@ def execute(report_schedule_id: int, scheduled_dttm: str) -> None:
logger.exception(
"An unexpected occurred while executing the report: %s", task_id
)
self.update_state(state="FAILURE")
except CommandException:
logger.exception(
"A downstream exception occurred while generating" " a report: %s", task_id
)
self.update_state(state="FAILURE")


@celery_app.task(name="reports.prune_log")
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/reports/commands_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import json
from contextlib import contextmanager
from datetime import datetime, timedelta, timezone
from typing import Any, Dict, List, Optional
from typing import List, Optional
from unittest.mock import Mock, patch
from uuid import uuid4

Expand Down
28 changes: 25 additions & 3 deletions tests/integration_tests/reports/scheduler_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from typing import List
from random import randint
from unittest.mock import patch

from freezegun import freeze_time
from freezegun.api import FakeDatetime # type: ignore

from superset.extensions import db
from superset.reports.models import ReportScheduleType
from superset.tasks.scheduler import scheduler
from superset.tasks.scheduler import execute, scheduler
from tests.integration_tests.reports.utils import insert_report_schedule
from tests.integration_tests.test_app import app

Expand Down Expand Up @@ -87,7 +87,6 @@ def test_scheduler_celery_timeout_utc(execute_mock):

with freeze_time("2020-01-01T09:00:00Z"):
scheduler()
print(execute_mock.call_args)
assert execute_mock.call_args[1]["soft_time_limit"] == 3601
assert execute_mock.call_args[1]["time_limit"] == 3610
db.session.delete(report_schedule)
Expand Down Expand Up @@ -136,3 +135,26 @@ def test_scheduler_feature_flag_off(execute_mock, is_feature_enabled):
execute_mock.assert_not_called()
db.session.delete(report_schedule)
db.session.commit()


@patch("superset.reports.commands.execute.AsyncExecuteReportScheduleCommand.__init__")
@patch("superset.reports.commands.execute.AsyncExecuteReportScheduleCommand.run")
@patch("superset.tasks.scheduler.execute.update_state")
def test_execute_task(update_state_mock, command_mock, init_mock):
from superset.reports.commands.exceptions import ReportScheduleUnexpectedError

with app.app_context():
report_schedule = insert_report_schedule(
type=ReportScheduleType.ALERT,
name=f"report-{randint(0,1000)}",
crontab="0 4 * * *",
timezone="America/New_York",
)
init_mock.return_value = None
command_mock.side_effect = ReportScheduleUnexpectedError("Unexpected error")
with freeze_time("2020-01-01T09:00:00Z"):
execute(report_schedule.id, "2020-01-01T09:00:00Z")
update_state_mock.assert_called_with(state="FAILURE")

db.session.delete(report_schedule)
db.session.commit()