feat: add header_data into emails #20903

AAfghahi · 2022-07-28T16:15:26Z

SUMMARY

This adds a new config mutator that adds logging data into a notification email should a user desire that.

The mutator works similarly to the SQL mutator that is already in position, accepting kwargs so that the logging data can be customized.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-07-28T16:44:23Z

Codecov Report

Merging #20903 (c3ab6b9) into master (aa53c10) will decrease coverage by 11.52%.
The diff coverage is 59.51%.

❗ Current head c3ab6b9 differs from pull request most recent head 821e662. Consider uploading reports for the commit 821e662 to get more accurate results

@@             Coverage Diff             @@
##           master   #20903       +/-   ##
===========================================
- Coverage   66.38%   54.85%   -11.53%     
===========================================
  Files        1766     1772        +6     
  Lines       67225    67647      +422     
  Branches     7137     7204       +67     
===========================================
- Hits        44625    37111     -7514     
- Misses      20774    28694     +7920     
- Partials     1826     1842       +16

Flag	Coverage Δ
hive	`53.17% <38.66%> (-0.04%)`	⬇️
mysql	`?`
postgres	`?`
presto	`53.07% <38.66%> (-0.03%)`	⬇️
python	`57.81% <44.88%> (-23.76%)`	⬇️
sqlite	`?`
unit	`50.73% <40.88%> (+0.21%)`	⬆️

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

Impacted Files	Coverage Δ
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <ø> (ø)`
...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts	`100.00% <ø> (ø)`
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...rts/src/BigNumber/BigNumberTotal/transformProps.ts	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/BoxPlot/EchartsBoxPlot.tsx	`0.00% <0.00%> (ø)`
.../plugins/plugin-chart-echarts/src/BoxPlot/types.ts	`0.00% <ø> (ø)`
.../plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx	`0.00% <0.00%> (ø)`
...d/plugins/plugin-chart-echarts/src/Funnel/types.ts	`100.00% <ø> (ø)`
...ns/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx	`0.00% <0.00%> (ø)`
... and 401 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eschutho · 2022-08-08T23:28:46Z

superset/reports/commands/execute.py

@@ -342,23 +343,34 @@ def _get_notification_content(self) -> NotificationContent:
        ):
            embedded_data = self._get_embedded_data()

+        notification_source = None
+        notification_source_id = None


nit, but can you declare these two at the beginning of the method with the others? It read at first as if you were reassigning the values rather than declaring and I had to check the top of the method to be sure which one it was.

eschutho · 2022-08-08T23:34:48Z

superset/utils/core.py

@@ -917,6 +918,9 @@ def send_email_smtp(  # pylint: disable=invalid-name,too-many-arguments,too-many
    msg["Subject"] = subject
    msg["From"] = smtp_mail_from
    msg["To"] = ", ".join(smtp_mail_to)
+
+    api_object = {"metadata": log_data}
+    msg["X-MSYS-API"] = json.dumps(api_object)


are you moving these two lines and the _get_log_data method to the config?

eschutho · 2022-08-08T23:37:58Z

superset/reports/models.py

@@ -82,6 +82,11 @@ class ReportCreationMethod(str, enum.Enum):
    ALERTS_REPORTS = "alerts_reports"


+class ReportSourceFormat(str, enum.Enum):
+    CHART = "chart"
+    DASHBOARDS = "dashboard"


nit, plural/singular consistency

eschutho · 2022-08-08T23:40:18Z

superset/reports/commands/execute.py

+            notification_type=self._report_schedule.type,
+            notification_source=notification_source,
+            notification_source_id=notification_source_id,
+            notification_format=self._report_schedule.report_format,


I would suggest grouping all of this new content together into either log_data or header_data since it all serves the same function and doesn't need to be passed in separately. It also makes it clear with that naming what it's going to be used for.

also we can be more flexible about the shape of the data that way, and if it changes, we don't have to update it in many places. The only thing that really relies on the shape is the config method which is optional.

eschutho · 2022-08-10T17:23:23Z

superset/config.py

@@ -1066,6 +1066,12 @@ def SQL_QUERY_MUTATOR(  # pylint: disable=invalid-name,unused-argument
    return sql


+def NOTIFICATION_EMAIL_HEADER_MUTATOR(  # pylint: disable=invalid-name,unused-argument
+    header: Dict[str, Any], **kwargs: Any


I believe this is message, right? Rather than header? And is there a class type for the message?

yes, sorry. Will change but will wait for the rest of your review first

eschutho · 2022-08-10T17:28:07Z

superset/reports/commands/execute.py

+    def _get_log_data(self) -> Dict[str, Any]:
+        chart_id = None
+        dashboard_id = None
+        owners = json.loads(self._report_schedule.owners) or None


will self._report_schedule.owners be a string?

it should be an array

eschutho · 2022-08-10T17:29:16Z

superset/reports/commands/execute.py

+            report_source = ReportSourceFormat.DASHBOARD
+            dashboard_id = self._report_schedule.dashboard_id
+        log_data = {
+            "notification_type": self._report_schedule.type or None,


what is a "notification" in this sense? I see that term used below, but I wonder if it has the same meaning outside of this context.

either an alert or a report.

dpgaspar · 2022-08-16T14:08:09Z

tests/integration_tests/reports/commands/execute_dashboard_report_tests.py

@@ -66,3 +67,47 @@ def test_report_for_dashboard_with_tabs(
        assert digest == dashboard.digest
        assert send_email_smtp_mock.call_count == 1
        assert len(send_email_smtp_mock.call_args.kwargs["images"]) == 1
+
+


consider adding unit tests for send_email_smtp with the actual use of NOTIFICATION_EMAIL_HEADER_MUTATOR

Would that exist in this repo or in one with an actual NOTIFICATION_EMAIL_HEADER_MUTATOR?

You can mock an example NOTIFICATION_EMAIL_HEADER_MUTATOR and test it in this repo

eschutho · 2022-08-17T21:59:30Z

superset/reports/commands/execute.py

@@ -73,6 +74,8 @@
 from superset.utils.urls import get_url_path
 from superset.utils.webdriver import DashboardStandaloneMode

+from ...utils.core import HeaderDataType


can you make this import absolute instead of relative?

yeah, I keep on doing this and the black reformats it. So annoying.

+1 please fix this format

yeah latest commit fixes it.

superset/utils/core.py

eschutho · 2022-08-17T22:08:37Z

superset/utils/core.py

+    notification_type: str
+    notification_source: Optional[str]
+    chart_id: Optional[int]
+    dashboard_id: Optional[int]


all of these are optional, right?

Type and format shouldn't be since they are required in order to make a report. I can make all of them optional however.

eschutho · 2022-08-17T22:09:43Z

superset/reports/notifications/base.py



 @dataclass
 class NotificationContent:
    name: str
+    header_data: Optional[
+        HeaderDataType
+    ] = None  # this is optional to account for error states


can you explain why this is optional?

Yeah, we also create notification_content when there is an error, should we be pushing header data into those cases as well? I assumed we wouldn't be but happy to make it not optional, and create header data in those instances as well.

john-bodley · 2022-09-15T17:45:26Z

superset/reports/notifications/base.py



 @dataclass
 class NotificationContent:
    name: str
+    header_data: HeaderDataType  # this is optional to account for error states


@AAfghahi your comment doesn't seem to reflect the actual type and default, i.e., header_data is required.

@AAfghahi it looks like this value is always being passed into the class. Can we just remove the comment or is there some other intention here?

pull-request-size bot added the size/XS label Jul 28, 2022

test sparkpost

2faa388

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from 9a8d670 to 2faa388 Compare August 3, 2022 16:55

pull-request-size bot added size/M and removed size/XS labels Aug 8, 2022

added logging info

d2dce2b

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from 1130f1d to d2dce2b Compare August 8, 2022 21:19

AAfghahi requested review from betodealmeida, eschutho, hughhhh, lyndsiWilliams and pkdotson August 8, 2022 21:23

eschutho reviewed Aug 8, 2022

View reviewed changes

AAfghahi requested a review from eschutho August 10, 2022 15:39

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch 6 times, most recently from c7b9572 to 2a1aed1 Compare August 10, 2022 16:45

AAfghahi changed the title ~~feat: test sparkpost~~ feat: add header_data into emails Aug 10, 2022

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from 2a1aed1 to 048f72d Compare August 10, 2022 17:08

eschutho reviewed Aug 10, 2022

View reviewed changes

header function implementation

f5c9c09

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from 048f72d to f5c9c09 Compare August 10, 2022 20:47

dpgaspar reviewed Aug 16, 2022

View reviewed changes

daniel revisions

5b29358

AAfghahi requested a review from dpgaspar August 17, 2022 16:32

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch 5 times, most recently from 68d9c72 to c0e1e53 Compare August 17, 2022 19:46

daniel revision

141bd50

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from c0e1e53 to 141bd50 Compare August 17, 2022 19:53

eschutho reviewed Aug 17, 2022

View reviewed changes

superset/utils/core.py Outdated Show resolved Hide resolved

eschutho reviewed Aug 17, 2022

View reviewed changes

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch 2 times, most recently from ac023c8 to 9a4d32e Compare August 17, 2022 22:43

elizabeth review

821e662

AAfghahi force-pushed the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch from 9a4d32e to 821e662 Compare August 17, 2022 23:01

dpgaspar approved these changes Aug 18, 2022

View reviewed changes

AAfghahi merged commit dda1dcf into master Aug 18, 2022

john-bodley reviewed Sep 15, 2022

View reviewed changes

eschutho mentioned this pull request Nov 9, 2023

feat: global context for logging #25920

Closed

9 tasks

eschutho mentioned this pull request Nov 18, 2023

[SIP-109] Global logs context #26019

Closed

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

mistercrunch deleted the arash.afghahi/sc-49091/log-alerts-reports-message-metadata branch March 26, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add header_data into emails #20903

feat: add header_data into emails #20903

AAfghahi commented Jul 28, 2022 •

edited

codecov bot commented Jul 28, 2022 •

edited

eschutho Aug 8, 2022

eschutho Aug 8, 2022

eschutho Aug 8, 2022

eschutho Aug 8, 2022

eschutho Aug 8, 2022

eschutho Aug 10, 2022 •

edited

AAfghahi Aug 10, 2022

eschutho Aug 10, 2022

AAfghahi Aug 10, 2022

eschutho Aug 10, 2022 •

edited

AAfghahi Aug 10, 2022

dpgaspar Aug 16, 2022

AAfghahi Aug 16, 2022

eschutho Sep 16, 2022

eschutho Aug 17, 2022 •

edited

AAfghahi Aug 17, 2022

hughhhh Aug 17, 2022

AAfghahi Aug 17, 2022

eschutho Aug 17, 2022

AAfghahi Aug 17, 2022

eschutho Aug 17, 2022

AAfghahi Aug 17, 2022

john-bodley Sep 15, 2022

eschutho Sep 16, 2022

feat: add header_data into emails #20903

feat: add header_data into emails #20903

Conversation

AAfghahi commented Jul 28, 2022 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 28, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Aug 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi commented Jul 28, 2022 •

edited

codecov bot commented Jul 28, 2022 •

edited

eschutho Aug 10, 2022 •

edited

eschutho Aug 10, 2022 •

edited

eschutho Aug 17, 2022 •

edited