diff --git a/schemas/performance-artifact.json b/schemas/performance-artifact.json index 7ceb8bee92e..00fde2f7186 100644 --- a/schemas/performance-artifact.json +++ b/schemas/performance-artifact.json @@ -205,6 +205,21 @@ "type": "integer", "minimum": 1, "maximum": 255 + }, + "monitor": { + "description": "Enable non-sheriffed alerting (ignores shouldAlert settings)", + "title": "Monitor", + "type": "boolean" + }, + "alertNotifyEmails": { + "type": "array", + "title": "Notify these emails for any alerts produced", + "items": { + "type": "string", + "maxLength": 100 + }, + "uniqueItems": true, + "maxItems": 8 } }, "required": ["name", "subtests"], diff --git a/tests/etl/conftest.py b/tests/etl/conftest.py index 764965d7fd7..c198546d724 100644 --- a/tests/etl/conftest.py +++ b/tests/etl/conftest.py @@ -21,3 +21,10 @@ def perf_job(perf_push, failure_classifications, generic_reference_data): return create_generic_job( "myfunguid", perf_push.repository, perf_push.id, generic_reference_data ) + + +@pytest.fixture +def perf_job_nonsheriffed(perf_push, failure_classifications, generic_reference_data): + return create_generic_job( + "myfunguid", perf_push.repository, perf_push.id, generic_reference_data, tier=3 + ) diff --git a/tests/etl/test_perf_data_load.py b/tests/etl/test_perf_data_load.py index 6765185609b..496e5ca5a0e 100644 --- a/tests/etl/test_perf_data_load.py +++ b/tests/etl/test_perf_data_load.py @@ -3,6 +3,7 @@ import json import operator import time +from unittest import mock import pytest from django.core.management import call_command @@ -234,6 +235,52 @@ def test_default_ingest_workflow( _verify_datum(suite["name"], subtest["name"], subtest["value"], perf_push.time) +@mock.patch("treeherder.etl.perf.generate_alerts") +def test_monitored_perf_data( + mocked_generate_alerts, + test_repository, + perf_push, + later_perf_push, + perf_job_nonsheriffed, + generic_reference_data, + sample_perf_artifact, +): + for suite in sample_perf_artifact["blob"]["suites"]: + suite["monitor"] = True + perf_datum, submit_datum = _prepare_test_data(sample_perf_artifact) + + store_performance_artifact(perf_job_nonsheriffed, submit_datum) + + assert DATA_PER_ARTIFACT == PerformanceSignature.objects.all().count() + assert 1 == PerformanceFramework.objects.all().count() + + # Ensure that alert generation gets triggered on a tier-3 test with + # monitor set to True + mocked_generate_alerts.apply_async.assert_called() + + +@mock.patch("treeherder.etl.perf.generate_alerts") +def test_unmonitored_perf_data( + mocked_generate_alerts, + test_repository, + perf_push, + later_perf_push, + perf_job_nonsheriffed, + generic_reference_data, + sample_perf_artifact, +): + perf_datum, submit_datum = _prepare_test_data(sample_perf_artifact) + + store_performance_artifact(perf_job_nonsheriffed, submit_datum) + + assert DATA_PER_ARTIFACT == PerformanceSignature.objects.all().count() + assert 1 == PerformanceFramework.objects.all().count() + + # Ensure that alert generation does not get triggered when monitor is False + # on a tier-3 job + mocked_generate_alerts.apply_async.assert_not_called() + + def test_hash_remains_unchanged_for_default_ingestion_workflow( test_repository, perf_job, sample_perf_artifact ): diff --git a/tests/perfalert/test_alerts.py b/tests/perfalert/test_alerts.py index d8f822cea7c..8576ecc654c 100644 --- a/tests/perfalert/test_alerts.py +++ b/tests/perfalert/test_alerts.py @@ -1,5 +1,6 @@ import datetime import time +from unittest import mock import pytest @@ -341,3 +342,101 @@ def test_alert_change_type_absolute( assert PerformanceAlert.objects.count() == expected_num_alerts assert PerformanceAlertSummary.objects.count() == expected_num_alerts + + +def test_alert_monitor_no_sheriff( + test_repository, + test_issue_tracker, + failure_classifications, + generic_reference_data, + test_perf_signature, +): + # modify the test signature to have it as a monitored signature, but not sheriffed + test_perf_signature.monitor = True + test_perf_signature.should_alert = True + test_perf_signature.save() + + base_time = time.time() # generate it based off current time + interval = 60 + _generate_performance_data( + test_repository, + test_perf_signature, + base_time, + 1, + 0.5, + int(interval / 2), + ) + _generate_performance_data( + test_repository, + test_perf_signature, + base_time, + int(interval / 2) + 1, + 1.0, + int(interval / 2), + ) + + generate_new_alerts_in_series(test_perf_signature) + + assert PerformanceAlert.objects.count() == 1 + assert PerformanceAlertSummary.objects.count() == 1 + + # When monitor is true, then alert should not be sheriffed + # regardless of should_alert settings + assert [not alert.sheriffed for alert in PerformanceAlert.objects.all()] + + +@mock.patch("treeherder.perf.alerts.taskcluster") +def test_alert_emails( + mocked_taskcluster, + test_repository, + test_issue_tracker, + failure_classifications, + generic_reference_data, + test_perf_signature, +): + mocked_email_client = mock.MagicMock() + mocked_taskcluster.notify_client_factory.return_value = mocked_email_client + + emails = "fake@email.com fake2@email.com" + test_perf_signature.alert_notify_emails = emails + test_perf_signature.save() + + base_time = time.time() # generate it based off current time + interval = 60 + _generate_performance_data( + test_repository, + test_perf_signature, + base_time, + 1, + 0.5, + int(interval / 2), + ) + _generate_performance_data( + test_repository, + test_perf_signature, + base_time, + int(interval / 2) + 1, + 1.0, + int(interval / 2), + ) + + generate_new_alerts_in_series(test_perf_signature) + + assert PerformanceAlert.objects.count() == 1 + assert PerformanceAlertSummary.objects.count() == 1 + + # When monitor is False, then the alerts should be sheriffed + assert [alert.sheriffed for alert in PerformanceAlert.objects.all()] + + # Make sure the email service was called correctly for 2 emails + assert mocked_taskcluster.notify_client_factory.call_count == 1 + assert mocked_email_client.email.call_count == 2 + + # Ensure that each email specified has an email sent to it + for email in emails.split(): + assert any( + [ + email in call_arg[0][0]["address"] + for call_arg in mocked_email_client.email.call_args_list + ] + ) diff --git a/treeherder/etl/perf.py b/treeherder/etl/perf.py index 692df68f35c..50c05b1905b 100644 --- a/treeherder/etl/perf.py +++ b/treeherder/etl/perf.py @@ -97,7 +97,7 @@ def _suite_should_alert_based_on( and new_datum_ingested and job.repository.performance_alerts_enabled and job.tier_is_sheriffable - ) + ) or (signature.monitor and job.repository.name not in ("try",)) def _test_should_alert_based_on( @@ -114,7 +114,7 @@ def _test_should_alert_based_on( and new_datum_ingested and job.repository.performance_alerts_enabled and job.tier_is_sheriffable - ) + ) or (signature.monitor and job.repository.name not in ("try",)) def _test_should_gather_replicates_based_on( @@ -205,6 +205,8 @@ def _load_perf_datum(job: Job, perf_datum: dict): # these properties below can be either True, False, or null # (None). Null indicates no preference has been set. "should_alert": suite.get("shouldAlert"), + "monitor": suite.get("monitor"), + "alert_notify_emails": _order_and_concat(suite.get("alertNotifyEmails", [])), "alert_change_type": PerformanceSignature._get_alert_change_type( suite.get("alertChangeType") ), @@ -272,6 +274,8 @@ def _load_perf_datum(job: Job, perf_datum: dict): # null (None). Null indicates no preference has been # set. "should_alert": subtest.get("shouldAlert"), + "monitor": suite.get("monitor"), + "alert_notify_emails": _order_and_concat(suite.get("alertNotifyEmails", [])), "alert_change_type": PerformanceSignature._get_alert_change_type( subtest.get("alertChangeType") ), diff --git a/treeherder/perf/alerts.py b/treeherder/perf/alerts.py index cf4e25e522d..a038867dc02 100644 --- a/treeherder/perf/alerts.py +++ b/treeherder/perf/alerts.py @@ -9,6 +9,7 @@ from django.conf import settings from django.db import transaction +from treeherder.perf.email import AlertNotificationWriter from treeherder.perf.models import ( PerformanceAlert, PerformanceAlertSummary, @@ -16,10 +17,21 @@ PerformanceSignature, ) from treeherder.perfalert.perfalert import RevisionDatum, detect_changes +from treeherder.services import taskcluster logger = logging.getLogger(__name__) +def send_alert_emails(emails, alert, alert_summary): + notify_client = taskcluster.notify_client_factory() + + for email in emails: + logger.info(f"Sending alert email to {email}") + notification_writer = AlertNotificationWriter() + email = notification_writer.prepare_new_email(email, alert, alert_summary) + notify_client.email(email) + + def geomean(iterable): # Returns a geomean of a list of values. a = np.array(iterable) @@ -138,6 +150,7 @@ def generate_new_alerts_in_series(signature): framework=signature.framework, push_id=cur.push_id, prev_push_id=prev.push_id, + sheriffed=not signature.monitor, defaults={ "manually_created": False, "created": datetime.utcfromtimestamp(cur.push_timestamp), @@ -150,9 +163,10 @@ def generate_new_alerts_in_series(signature): if t_value == float("inf"): t_value = 1000 - PerformanceAlert.objects.update_or_create( + alert, _ = PerformanceAlert.objects.update_or_create( summary=summary, series_signature=signature, + sheriffed=not signature.monitor, defaults={ "noise_profile": noise_profile, "is_regression": alert_properties.is_regression, @@ -163,3 +177,6 @@ def generate_new_alerts_in_series(signature): "t_value": t_value, }, ) + + if signature.alert_notify_emails: + send_alert_emails(signature.alert_notify_emails.split(), alert, summary) diff --git a/treeherder/perf/email.py b/treeherder/perf/email.py index c27081bc85b..9eb7a2ac9c4 100644 --- a/treeherder/perf/email.py +++ b/treeherder/perf/email.py @@ -292,3 +292,47 @@ def _write_content(self, must_mention: list[PerformanceSignature]): content.include_signatures(must_mention) self._email.content = str(content) + + +class AlertNotificationContent: + DESCRIPTION = "Perfherder has detected a performance change on the test **{test}** and produced an alert. See {alert_summary} for more information." + ALERT_SUMMARY_LINK = "https://treeherder.mozilla.org/perfherder/alerts?id={alert_summary_id}" + + def __init__(self): + self._raw_content = None + + def include_alert_summary(self, test: str, alert_summary_id: str): + self._raw_content = self.DESCRIPTION.format( + test=test, + alert_summary=( + f"[this alert summary]" + f"({self.ALERT_SUMMARY_LINK.format(alert_summary_id=alert_summary_id)})" + ), + ) + + def __str__(self): + if self._raw_content is None: + raise ValueError("No content set") + return self._raw_content + + +class AlertNotificationWriter(EmailWriter): + def prepare_new_email( + self, email: str, alert: PerformanceAlert, alert_summary: PerformanceAlertSummary + ) -> dict: + self._write_address(email) + self._write_subject(alert.series_signature.test) + self._write_content(alert.series_signature.test, str(alert_summary.id)) + return self.email + + def _write_address(self, email: str): + self._email.address = email + + def _write_subject(self, test: str): + self._email.subject = f"Alert: Test {test} detected a performance change" + + def _write_content(self, test: str, alert_summary_id: str): + content = AlertNotificationContent() + content.include_alert_summary(test, alert_summary_id) + + self._email.content = str(content) diff --git a/treeherder/perf/migrations/0059_alter_performancealert_unique_together_and_more.py b/treeherder/perf/migrations/0059_alter_performancealert_unique_together_and_more.py new file mode 100644 index 00000000000..7e72ef6075e --- /dev/null +++ b/treeherder/perf/migrations/0059_alter_performancealert_unique_together_and_more.py @@ -0,0 +1,72 @@ +# Generated by Django 5.1.8 on 2025-05-02 18:54 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("model", "0041_update_search_vector"), + ("perf", "0058_performancealertsummary_original_prev_push_and_more"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="performancealert", + unique_together=set(), + ), + migrations.AlterUniqueTogether( + name="performancealertsummary", + unique_together=set(), + ), + migrations.AddField( + model_name="performancealert", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancealertsummary", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancealertsummarytesting", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancealerttesting", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancesignature", + name="alert_notify_emails", + field=models.CharField(max_length=422, null=True), + ), + migrations.AddField( + model_name="performancesignature", + name="monitor", + field=models.BooleanField(null=True), + ), + migrations.AddField( + model_name="performancetelemetryalert", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancetelemetryalertsummary", + name="sheriffed", + field=models.BooleanField(default=True), + ), + migrations.AlterUniqueTogether( + name="performancealert", + unique_together={("summary", "series_signature", "sheriffed")}, + ), + migrations.AlterUniqueTogether( + name="performancealertsummary", + unique_together={ + ("repository", "framework", "prev_push", "push", "sheriffed") + }, + ), + ] diff --git a/treeherder/perf/models.py b/treeherder/perf/models.py index f671f8f6a34..8af8bf2f326 100644 --- a/treeherder/perf/models.py +++ b/treeherder/perf/models.py @@ -92,6 +92,8 @@ class PerformanceSignature(models.Model): ALERT_CHANGE_TYPES = ((ALERT_PCT, "percentage"), (ALERT_ABS, "absolute")) should_alert = models.BooleanField(null=True) + monitor = models.BooleanField(null=True) + alert_notify_emails = models.CharField(max_length=422, null=True) alert_change_type = models.IntegerField(choices=ALERT_CHANGE_TYPES, null=True) alert_threshold = models.FloatField(null=True) min_back_window = models.IntegerField(null=True) @@ -324,6 +326,7 @@ class PerformanceAlertSummaryBase(models.Model): assignee = models.ForeignKey( User, on_delete=models.SET_NULL, null=True, related_name="assigned_alerts" ) + sheriffed = models.BooleanField(default=True) created = models.DateTimeField(auto_now_add=True, db_index=True) triage_due_date = models.DateTimeField(null=True, default=None) @@ -488,7 +491,7 @@ def __str__(self): class PerformanceAlertSummary(PerformanceAlertSummaryBase): class Meta: db_table = "performance_alert_summary" - unique_together = ("repository", "framework", "prev_push", "push") + unique_together = ("repository", "framework", "prev_push", "push", "sheriffed") class PerformanceTelemetryAlertSummary(PerformanceAlertSummaryBase): @@ -543,6 +546,7 @@ class PerformanceAlertBase(models.Model): classifier = models.ForeignKey( User, on_delete=models.CASCADE, null=True ) # null if autoclassified + sheriffed = models.BooleanField(default=True) created = models.DateTimeField(auto_now_add=True, null=True) # time when human user 1st interacted with alert @@ -690,7 +694,7 @@ def __str__(self): class PerformanceAlert(PerformanceAlertBase): class Meta: db_table = "performance_alert" - unique_together = ("summary", "series_signature") + unique_together = ("summary", "series_signature", "sheriffed") class PerformanceTelemetryAlert(PerformanceAlertBase): diff --git a/treeherder/webapp/api/performance_data.py b/treeherder/webapp/api/performance_data.py index 21ff2943d62..952a68a0dae 100644 --- a/treeherder/webapp/api/performance_data.py +++ b/treeherder/webapp/api/performance_data.py @@ -474,6 +474,17 @@ class PerformanceAlertSummaryViewSet(viewsets.ModelViewSet): def list(self, request, *args, **kwargs): queryset = self.filter_queryset(self.queryset) + if request.query_params.get("monitored_alerts"): + queryset = queryset.filter( + alerts__series_signature__monitor=True, + ) + else: + queryset = queryset.filter( + alerts__series_signature__monitor=None, + ) | queryset.filter( + alerts__series_signature__monitor=False, + ) + pk = request.query_params.get("id") page = self.paginate_queryset(queryset) if page is not None: diff --git a/treeherder/webapp/api/performance_serializers.py b/treeherder/webapp/api/performance_serializers.py index e9c9a0b8f95..1fd02ad2864 100644 --- a/treeherder/webapp/api/performance_serializers.py +++ b/treeherder/webapp/api/performance_serializers.py @@ -310,6 +310,7 @@ class PerformanceAlertSummarySerializer(serializers.ModelSerializer): first_triaged = serializers.ReadOnlyField() triage_due_date = serializers.ReadOnlyField() bug_due_date = serializers.ReadOnlyField() + monitored_alerts = serializers.BooleanField(required=False) def update(self, instance, validated_data): instance.timestamp_first_triage() @@ -355,6 +356,7 @@ class Meta: "assignee_email", "performance_tags", "duplicated_summaries_ids", + "monitored_alerts", ] diff --git a/ui/perfherder/Navigation.jsx b/ui/perfherder/Navigation.jsx index 4b11265fd08..a83b3ba45ec 100644 --- a/ui/perfherder/Navigation.jsx +++ b/ui/perfherder/Navigation.jsx @@ -29,6 +29,14 @@ const Navigation = ({ user, setUser, notify }) => ( Alerts + + + Monitoring + + Tests diff --git a/ui/perfherder/alerts/AlertsView.jsx b/ui/perfherder/alerts/AlertsView.jsx index eec51b42299..d3229ba8724 100644 --- a/ui/perfherder/alerts/AlertsView.jsx +++ b/ui/perfherder/alerts/AlertsView.jsx @@ -102,6 +102,7 @@ class AlertsView extends React.Component { filterText: this.getDefaultFilterText(validated), hideDownstream: convertParams(validated, 'hideDwnToInv'), hideAssignedToOthers: convertParams(validated, 'hideAssignedToOthers'), + monitoredAlerts: convertParams(validated, 'monitoredAlerts'), }; }; @@ -251,6 +252,7 @@ class AlertsView extends React.Component { filterText, hideDownstream, hideAssignedToOthers, + monitoredAlerts, } = filters; this.setState({ page }); @@ -271,6 +273,9 @@ class AlertsView extends React.Component { if (hideAssignedToOthers) { params.with_assignee = user.username; } + if (monitoredAlerts) { + params.monitored_alerts = monitoredAlerts; + } } const url = getApiUrl( diff --git a/ui/perfherder/alerts/AlertsViewControls.jsx b/ui/perfherder/alerts/AlertsViewControls.jsx index ae739aca0fd..9f7b2a5829d 100644 --- a/ui/perfherder/alerts/AlertsViewControls.jsx +++ b/ui/perfherder/alerts/AlertsViewControls.jsx @@ -132,6 +132,7 @@ export default class AlertsViewControls extends React.Component { filterText, hideDownstream, hideAssignedToOthers, + monitoredAlerts, framework, status, } = filters; @@ -177,6 +178,11 @@ export default class AlertsViewControls extends React.Component { stateName: 'hideDownstream', disable: this.state.disableHideDownstream, }, + { + text: 'Monitored Alerts', + state: monitoredAlerts, + stateName: 'monitoredAlerts', + }, ]; if (user.isLoggedIn && isListMode) { @@ -246,6 +252,7 @@ export default class AlertsViewControls extends React.Component { filterText, hideDownstream, hideAssignedToOthers, + monitoredAlerts, }} alertSummary={alertSummary} fetchAlertSummaries={fetchAlertSummaries} @@ -280,6 +287,7 @@ AlertsViewControls.propTypes = { filterText: PropTypes.string.isRequired, hideDownstream: PropTypes.bool.isRequired, hideAssignedToOthers: PropTypes.bool.isRequired, + monitoredAlerts: PropTypes.bool.isRequired, framework: PropTypes.shape({}).isRequired, status: PropTypes.string.isRequired, }).isRequired,