You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2025/01/14 18:08:41 UTC
[PR] feat: Mutate SQL query executed by alerts [superset]
Vitor-Avila opened a new pull request, #31840:
URL: https://github.com/apache/superset/pull/31840
### SUMMARY
This PR adds a new config `MUTATE_ALERT_QUERY` that determines if SQL queries executed to validate the alert condition should be mutated via the `SQL_QUERY_MUTATOR` config.
It also leverages the ``logs_context`` decorator to pass the `report_schedule_id` and `execution_id` (that can then be retrieved in the mutator function if desired).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
Added tests. For manual testing:
1. Make sure you have a `SQL_QUERY_MUTATOR` method defined in your config.
2. Update this method to add the `report_schedule_id` and `execution_id` as comments in the SQL.
3. Set `MUTATE_ALERT_QUERY` to `True` in the config.
4. Schedule an alert.
5. Validate in the target DB logs that a) the query was mutated and b) both `report_schedule_id` and `execution_id` are added to the query.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Mutate SQL query executed by alerts [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #31840:
URL: https://github.com/apache/superset/pull/31840#discussion_r1915640919
##########
tests/integration_tests/reports/alert_tests.py:
##########
@@ -184,9 +265,42 @@ def _mocked_execute_query() -> None:
execute_query_mock.side_effect = _mocked_execute_query
execute_query_mock.__name__ = "mocked_execute_query"
- command = AlertCommand(report_schedule=mocker.Mock())
+ command = AlertCommand(report_schedule=mocker.Mock(), execution_id=uuid.uuid4())
with suppress(AlertQueryError):
command.validate()
# Should match the value defined in superset_test_config.py
assert execute_query_mock.call_count == 3
+
+
+def test_get_alert_metadata_from_object(
+ mocker: MockerFixture,
+ app_context: AppContext,
+ get_user,
+) -> None:
+ from superset.commands.report.alert import AlertCommand
+ from superset.reports.models import ReportSchedule
+
+ app.config["ALERT_REPORTS_EXECUTE_AS"] = [ExecutorType.OWNER]
Review Comment:
same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Mutate SQL query executed by alerts [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #31840:
URL: https://github.com/apache/superset/pull/31840
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Mutate SQL query executed by alerts [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #31840:
URL: https://github.com/apache/superset/pull/31840#discussion_r1915640439
##########
tests/integration_tests/reports/alert_tests.py:
##########
@@ -98,6 +99,86 @@ def test_execute_query_as_report_executor(
app.config["ALERT_REPORTS_EXECUTE_AS"] = original_config
+def test_execute_query_mutate_query_enabled(
+ mocker: MockerFixture,
+ app_context: AppContext,
+ get_user,
+) -> None:
+ from superset.commands.report.alert import AlertCommand
+ from superset.reports.models import ReportSchedule
+
+ def mock_mutate_query(sql: str, **kwargs: Any) -> str:
+ return "before " + sql + " after"
+
+ app.config["SQL_QUERY_MUTATOR"] = mock_mutate_query
+ app.config["MUTATE_ALERT_QUERY"] = True
+ app.config["ALERT_REPORTS_EXECUTE_AS"] = [ExecutorType.OWNER]
+
+ mocker.patch("superset.commands.report.alert.override_user")
+ mock_df = mocker.MagicMock(spec=pd.DataFrame)
+ mock_df.empty = True
+ mock_database = get_example_database()
+ mock_get_df = mocker.patch.object(mock_database, "get_df", return_value=mock_df)
+ mock_limited_sql = mocker.patch.object(
+ mock_database, "apply_limit_to_sql", return_value="SELECT 1\nLIMIT 2"
+ )
+
+ report_schedule = ReportSchedule(
+ created_by=get_user("admin"),
+ owners=[get_user("admin")],
+ type=ReportScheduleType.ALERT,
+ description="description",
+ crontab="0 9 * * *",
+ creation_method=ReportCreationMethod.ALERTS_REPORTS,
+ sql="SELECT 1",
+ grace_period=14400,
+ working_timeout=3600,
+ database=mock_database,
+ validator_config_json='{"op": "==", "threshold": 1}',
+ )
+
+ AlertCommand(report_schedule=report_schedule, execution_id=uuid.uuid4()).run()
+ mock_get_df.assert_called_once_with(
+ sql=f"before {mock_limited_sql.return_value} after"
+ )
+
+
+def test_execute_query_mutate_query_disabled(
+ mocker: MockerFixture,
+ app_context: AppContext,
+ get_user,
+) -> None:
+ from superset.commands.report.alert import AlertCommand
+ from superset.reports.models import ReportSchedule
+
+ def mock_mutate_query(sql: str, **kwargs: Any) -> str:
+ return "before " + sql + " after"
+
+ app.config["SQL_QUERY_MUTATOR"] = mock_mutate_query
+ app.config["MUTATE_ALERT_QUERY"] = False
+ app.config["ALERT_REPORTS_EXECUTE_AS"] = [ExecutorType.OWNER]
Review Comment:
@Vitor-Avila I think you're going to want to reset these at the end of the test for cleanup, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Mutate SQL query executed by alerts [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #31840:
URL: https://github.com/apache/superset/pull/31840#discussion_r1915875141
##########
tests/integration_tests/reports/alert_tests.py:
##########
@@ -98,6 +99,86 @@ def test_execute_query_as_report_executor(
app.config["ALERT_REPORTS_EXECUTE_AS"] = original_config
+def test_execute_query_mutate_query_enabled(
+ mocker: MockerFixture,
+ app_context: AppContext,
+ get_user,
+) -> None:
+ from superset.commands.report.alert import AlertCommand
+ from superset.reports.models import ReportSchedule
+
+ def mock_mutate_query(sql: str, **kwargs: Any) -> str:
+ return "before " + sql + " after"
+
+ app.config["SQL_QUERY_MUTATOR"] = mock_mutate_query
+ app.config["MUTATE_ALERT_QUERY"] = True
+ app.config["ALERT_REPORTS_EXECUTE_AS"] = [ExecutorType.OWNER]
+
+ mocker.patch("superset.commands.report.alert.override_user")
+ mock_df = mocker.MagicMock(spec=pd.DataFrame)
+ mock_df.empty = True
+ mock_database = get_example_database()
+ mock_get_df = mocker.patch.object(mock_database, "get_df", return_value=mock_df)
+ mock_limited_sql = mocker.patch.object(
+ mock_database, "apply_limit_to_sql", return_value="SELECT 1\nLIMIT 2"
+ )
+
+ report_schedule = ReportSchedule(
+ created_by=get_user("admin"),
+ owners=[get_user("admin")],
+ type=ReportScheduleType.ALERT,
+ description="description",
+ crontab="0 9 * * *",
+ creation_method=ReportCreationMethod.ALERTS_REPORTS,
+ sql="SELECT 1",
+ grace_period=14400,
+ working_timeout=3600,
+ database=mock_database,
+ validator_config_json='{"op": "==", "threshold": 1}',
+ )
+
+ AlertCommand(report_schedule=report_schedule, execution_id=uuid.uuid4()).run()
+ mock_get_df.assert_called_once_with(
+ sql=f"before {mock_limited_sql.return_value} after"
+ )
+
+
+def test_execute_query_mutate_query_disabled(
+ mocker: MockerFixture,
+ app_context: AppContext,
+ get_user,
+) -> None:
+ from superset.commands.report.alert import AlertCommand
+ from superset.reports.models import ReportSchedule
+
+ def mock_mutate_query(sql: str, **kwargs: Any) -> str:
+ return "before " + sql + " after"
+
+ app.config["SQL_QUERY_MUTATOR"] = mock_mutate_query
+ app.config["MUTATE_ALERT_QUERY"] = False
+ app.config["ALERT_REPORTS_EXECUTE_AS"] = [ExecutorType.OWNER]
Review Comment:
good catch! Changed the test logic a bit and re-applied the initial app configs at the end of them 🙌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org