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.

Sign up for GitHub

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: global context for logging #25920

Closed
wants to merge 2 commits into from
Closed

feat: global context for logging #25920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd1c7d2
move execution_id to global context
eschutho Nov 7, 2023
6ae7967
move notification header to global context
eschutho Nov 7, 2023
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
92 changes: 55 additions & 37 deletions superset/reports/commands/execute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,16 @@
ReportRecipientType,
ReportSchedule,
ReportScheduleType,
ReportSourceFormat,
ReportState,
)
from superset.reports.notifications import create_notification
from superset.reports.notifications.base import NotificationContent
from superset.reports.notifications.exceptions import NotificationError
from superset.tasks.utils import get_executor
from superset.utils.celery import session_scope
from superset.utils.core import HeaderDataType, override_user
from superset.utils.core import override_user
from superset.utils.csv import get_chart_csv_data, get_chart_dataframe
from superset.utils.decorators import context
from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot
from superset.utils.urls import get_url_path

Expand All @@ -83,6 +83,7 @@ class BaseReportState:
current_states: list[ReportState] = []
initial: bool = False

@context()
def __init__(
self,
session: Session,
Expand Down Expand Up @@ -234,7 +235,12 @@ def _get_screenshots(self) -> list[bytes]:
try:
image = screenshot.get_screenshot(user=user)
except SoftTimeLimitExceeded as ex:
logger.warning("A timeout occurred while taking a screenshot.")
logger.warning(
"A timeout occurred while taking a screenshot.",
extra={
"execution_id": self._execution_id,
},
)
raise ReportScheduleScreenshotTimeout() from ex
except Exception as ex:
raise ReportScheduleScreenshotFailedError(
Expand All @@ -254,11 +260,23 @@ def _get_csv_data(self) -> bytes:
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)

if self._report_schedule.chart.query_context is None:
logger.warning("No query context found, taking a screenshot to generate it")
logger.warning(
"No query context found, taking a screenshot to generate it",
extra={
"execution_id": self._execution_id,
},
)
self._update_query_context()

try:
logger.info("Getting chart from %s as user %s", url, user.username)
logger.info(
"Getting chart from %s as user %s",
url,
user.username,
extra={
"execution_id": self._execution_id,
},
)
csv_data = get_chart_csv_data(chart_url=url, auth_cookies=auth_cookies)
except SoftTimeLimitExceeded as ex:
raise ReportScheduleCsvTimeout() from ex
Expand All @@ -283,11 +301,23 @@ def _get_embedded_data(self) -> pd.DataFrame:
auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user)

if self._report_schedule.chart.query_context is None:
logger.warning("No query context found, taking a screenshot to generate it")
logger.warning(
"No query context found, taking a screenshot to generate it",
extra={
"execution_id": self._execution_id,
},
)
self._update_query_context()

try:
logger.info("Getting chart from %s as user %s", url, user.username)
logger.info(
"Getting chart from %s as user %s",
url,
user.username,
extra={
"execution_id": self._execution_id,
},
)
dataframe = get_chart_dataframe(url, auth_cookies)
except SoftTimeLimitExceeded as ex:
raise ReportScheduleDataFrameTimeout() from ex
Expand Down Expand Up @@ -320,27 +350,6 @@ def _update_query_context(self) -> None:
"Please try loading the chart and saving it again."
) from ex

def _get_log_data(self) -> HeaderDataType:
chart_id = None
dashboard_id = None
report_source = None
if self._report_schedule.chart:
report_source = ReportSourceFormat.CHART
chart_id = self._report_schedule.chart_id
else:
report_source = ReportSourceFormat.DASHBOARD
dashboard_id = self._report_schedule.dashboard_id

log_data: HeaderDataType = {
"notification_type": self._report_schedule.type,
"notification_source": report_source,
"notification_format": self._report_schedule.report_format,
"chart_id": chart_id,
"dashboard_id": dashboard_id,
"owners": self._report_schedule.owners,
}
return log_data

def _get_notification_content(self) -> NotificationContent:
"""
Gets a notification content, this is composed by a title and a screenshot
Expand All @@ -351,7 +360,6 @@ def _get_notification_content(self) -> NotificationContent:
embedded_data = None
error_text = None
screenshot_data = []
header_data = self._get_log_data()
url = self._get_url(user_friendly=True)
if (
feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
Expand All @@ -372,7 +380,6 @@ def _get_notification_content(self) -> NotificationContent:
return NotificationContent(
name=self._report_schedule.name,
text=error_text,
header_data=header_data,
)

if (
Expand All @@ -399,7 +406,6 @@ def _get_notification_content(self) -> NotificationContent:
description=self._report_schedule.description,
csv=csv_data,
embedded_data=embedded_data,
header_data=header_data,
)

def _send(
Expand Down Expand Up @@ -440,7 +446,12 @@ def _send(
if notification_errors:
# log all errors but raise based on the most severe
for error in notification_errors:
logger.warning(str(error))
logger.warning(
str(error),
extra={
"execution_id": self._execution_id,
},
)

if any(error.level == ErrorLevel.ERROR for error in notification_errors):
raise ReportScheduleSystemErrorsException(errors=notification_errors)
Expand All @@ -462,14 +473,15 @@ def send_error(self, name: str, message: str) -> None:

:raises: CommandException
"""
header_data = self._get_log_data()
logger.info(
"header_data in notifications for alerts and reports %s, taskid, %s",
header_data,
self._execution_id,
"An error for a notification occurred, sending error notification",
extra={
"execution_id": self._execution_id,
},
)
notification_content = NotificationContent(
name=name, text=message, header_data=header_data
name=name,
text=message,
)

# filter recipients to recipients who are also owners
Expand Down Expand Up @@ -725,6 +737,9 @@ def run(self) -> None:
"Running report schedule %s as user %s",
self._execution_id,
username,
extra={
"execution_id": self._execution_id,
},
)
ReportScheduleStateMachine(
session, self._execution_id, self._model, self._scheduled_dttm
Expand All @@ -740,6 +755,9 @@ def validate(self, session: Session = None) -> None:
"session is validated: id %s, executionid: %s",
self._model_id,
self._execution_id,
extra={
"execution_id": self._execution_id,
},
)
self._model = (
session.query(ReportSchedule).filter_by(id=self._model_id).one_or_none()
Expand Down
2 changes: 0 additions & 2 deletions superset/reports/notifications/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@
import pandas as pd

from superset.reports.models import ReportRecipients, ReportRecipientType
from superset.utils.core import HeaderDataType


@dataclass
class NotificationContent:
name: str
header_data: HeaderDataType # this is optional to account for error states
csv: Optional[bytes] = None # bytes for csv file
screenshots: Optional[list[bytes]] = None # bytes for a list of screenshots
text: Optional[str] = None
Expand Down
14 changes: 9 additions & 5 deletions superset/reports/notifications/email.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,15 @@
from typing import Any, Optional

import nh3
from flask import g
from flask_babel import gettext as __

from superset import app
from superset.exceptions import SupersetErrorsException
from superset.reports.models import ReportRecipientType
from superset.reports.notifications.base import BaseNotification
from superset.reports.notifications.exceptions import NotificationError
from superset.utils.core import HeaderDataType, send_email_smtp
from superset.reports.notifications.utils import send_email_smtp
from superset.utils.decorators import statsd_gauge

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -67,7 +68,6 @@
@dataclass
class EmailContent:
body: str
header_data: Optional[HeaderDataType] = None
data: Optional[dict[str, Any]] = None
images: Optional[dict[str, bytes]] = None

Expand Down Expand Up @@ -172,7 +172,6 @@ def _get_content(self) -> EmailContent:
body=body,
images=images,
data=csv_data,
header_data=self._content.header_data,
)

def _get_subject(self) -> str:
Expand All @@ -190,6 +189,7 @@ def send(self) -> None:
subject = self._get_subject()
content = self._get_content()
to = self._get_to()
global_context = getattr(g, "context", {}) or {}
try:
send_email_smtp(
to,
Expand All @@ -202,10 +202,14 @@ def send(self) -> None:
bcc="",
mime_subtype="related",
dryrun=False,
header_data=content.header_data,
)
logger.info(
"Report sent to email, notification content is %s", content.header_data
"Report sent to email",
extra={
"execution_id": global_context.get("execution_id"),
"dashboard_id": global_context.get("dashboard_id"),
"chart_id": global_context.get("chart_id"),
},
)
except SupersetErrorsException as ex:
raise NotificationError(
Expand Down
10 changes: 9 additions & 1 deletion superset/reports/notifications/slack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import backoff
import pandas as pd
from flask import g
from flask_babel import gettext as __
from slack_sdk import WebClient
from slack_sdk.errors import (
Expand Down Expand Up @@ -166,6 +167,8 @@ def send(self) -> None:
channel = self._get_channel()
body = self._get_body()
file_type = "csv" if self._content.csv else "png"
global_context = getattr(g, "context", {}) or {}

try:
token = app.config["SLACK_API_TOKEN"]
if callable(token):
Expand All @@ -183,7 +186,12 @@ def send(self) -> None:
)
else:
client.chat_postMessage(channel=channel, text=body)
logger.info("Report sent to slack")
logger.info(
"Report sent to slack",
extra={
"execution_id": global_context.get("execution_id"),
},
)
except (
BotUserAccessError,
SlackRequestError,
Expand Down
Loading