From 05a694c3a738404744342139915cbd9c95d830ce Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:08:10 -0400 Subject: [PATCH 01/23] Get BUG_COMMENTER_API_KEY from .env file if it exists. --- docker-compose.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/docker-compose.yml b/docker-compose.yml index 687a249b6b7..81c7bc9a88b 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -29,6 +29,7 @@ services: - PROJECTS_TO_INGEST=${PROJECTS_TO_INGEST:-autoland,try} - BUGZILLA_API_URL=${BUGZILLA_API_URL:-} - BUG_FILER_API_KEY=${BUG_FILER_API_KEY:-} + - BUG_COMMENTER_API_KEY=${BUG_COMMENTER_API_KEY:-} - TLS_CERT_PATH=${TLS_CERT_PATH:-} - TELEMETRY_ENABLE_ALERTS=${TELEMETRY_ENABLE_ALERTS:-} - GCLOUD_PROJECT=${GCLOUD_PROJECT:-} From a0f97cfc7cb2c5b5f14c0e12c3693947df10432d Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:19:49 -0400 Subject: [PATCH 02/23] Update telemetry alert models with additional information. --- ...mancetelemetryalert_bug_number_and_more.py | 41 +++++++++++ ...ncetelemetryalert_bug_modified_and_more.py | 23 ++++++ treeherder/perf/models.py | 70 ++++++++++++++----- 3 files changed, 118 insertions(+), 16 deletions(-) create mode 100644 treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py create mode 100644 treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py diff --git a/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py b/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py new file mode 100644 index 00000000000..9a4f810f596 --- /dev/null +++ b/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py @@ -0,0 +1,41 @@ +# Generated by Django 5.1.11 on 2025-10-06 14:43 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("perf", "0060_alter_performancealert_unique_together_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="performancetelemetryalert", + name="bug_number", + field=models.PositiveIntegerField(null=True), + ), + migrations.AddField( + model_name="performancetelemetryalert", + name="notified", + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name="performancetelemetryalert", + name="status", + field=models.IntegerField( + choices=[ + (0, "NEW"), + (1, "FIXED"), + (2, "INVALID"), + (5, "WONTFIX"), + (3, "INACTIVE"), + (4, "DUPLICATE"), + (6, "WORKSFORME"), + (7, "INCOMPLETE"), + (8, "MOVED"), + ], + default=0, + ), + ), + ] diff --git a/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py b/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py new file mode 100644 index 00000000000..4523a7eb373 --- /dev/null +++ b/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py @@ -0,0 +1,23 @@ +# Generated by Django 5.1.12 on 2025-10-08 01:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("perf", "0061_performancetelemetryalert_bug_number_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="performancetelemetryalert", + name="bug_modified", + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name="performancetelemetryalertsummary", + name="bugs_modified", + field=models.BooleanField(default=True), + ), + ] diff --git a/treeherder/perf/models.py b/treeherder/perf/models.py index c5c79b8c1be..71216ce3eab 100644 --- a/treeherder/perf/models.py +++ b/treeherder/perf/models.py @@ -502,6 +502,10 @@ class PerformanceTelemetryAlertSummary(PerformanceAlertSummaryBase): User, on_delete=models.SET_NULL, null=True, related_name="assigned_telemetry_alerts" ) + # This field tells us if all bugs related to this summary were successfully + # modified for group-based modifications + bugs_modified = models.BooleanField(default=True) + def autodetermine_status(self, alert_model=None): return super().autodetermine_status(alert_model=PerformanceTelemetryAlert) @@ -640,6 +644,22 @@ def initial_culprit_job(self) -> Job | None: return self.__initial_culprit_job + def timestamp_first_triage(self): + # use only on code triggered by + # human interaction + if self.first_triaged is None: + self.first_triaged = django_now() + self.summary.timestamp_first_triage().save() + return self + + class Meta: + abstract = True + + def __str__(self): + return f"{self.summary} {self.series_signature} {self.amount_pct}%" + + +class PerformanceAlert(PerformanceAlertBase): def save(self, *args, **kwargs): # validate that we set a status that makes sense for presence # or absence of a related summary @@ -679,22 +699,6 @@ def save(self, *args, **kwargs): if self.related_summary: self.related_summary.update_status(using=using) - def timestamp_first_triage(self): - # use only on code triggered by - # human interaction - if self.first_triaged is None: - self.first_triaged = django_now() - self.summary.timestamp_first_triage().save() - return self - - class Meta: - abstract = True - - def __str__(self): - return f"{self.summary} {self.series_signature} {self.amount_pct}%" - - -class PerformanceAlert(PerformanceAlertBase): class Meta: db_table = "performance_alert" unique_together = ("summary", "series_signature", "sheriffed") @@ -724,6 +728,40 @@ class PerformanceTelemetryAlert(PerformanceAlertBase): prev_p95 = models.FloatField(help_text="Previous P95 value of series before change") new_p95 = models.FloatField(help_text="New P95 value of series after change") + # Each alerting probe will get 1 bug, but we still want to group + # them together so the bug number field is added to the telemetry alerts as well + NEW = 0 + FIXED = 1 + INVALID = 2 + WONTFIX = 5 + INACTIVE = 3 + DUPLICATE = 4 + WORKSFORME = 6 + INCOMPLETE = 7 + MOVED = 8 + + STATUSES = ( + (NEW, "NEW"), + (FIXED, "FIXED"), + (INVALID, "INVALID"), + (WONTFIX, "WONTFIX"), + (INACTIVE, "INACTIVE"), + (DUPLICATE, "DUPLICATE"), + (WORKSFORME, "WORKSFORME"), + (INCOMPLETE, "INCOMPLETE"), + (MOVED, "MOVED"), + ) + + status = models.IntegerField(choices=STATUSES, default=NEW) + bug_number = models.PositiveIntegerField(null=True) + + # This field tells us if the appropriate owners were already notified + notified = models.BooleanField(default=False) + + # This field tells us if the bug was modified successfully for individual-based + # modifications + bug_modified = models.BooleanField(default=True) + class Meta: db_table = "performance_telemetry_alert" unique_together = ("summary", "series_signature") From ea235b8b7228ebbcaf549744db0d949a7eeea23c Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:25:36 -0400 Subject: [PATCH 03/23] Add pytest fixtures for unit tests for generic classes. --- tests/perf/auto_perf_sheriffing/conftest.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/perf/auto_perf_sheriffing/conftest.py b/tests/perf/auto_perf_sheriffing/conftest.py index 20546772f91..434593864b4 100644 --- a/tests/perf/auto_perf_sheriffing/conftest.py +++ b/tests/perf/auto_perf_sheriffing/conftest.py @@ -261,3 +261,24 @@ def job_from_try(hundred_job_blobs, create_jobs): job.repository.is_try_repo = True job.repository.save() return job + + +@pytest.fixture +def mock_bugfiler_settings(monkeypatch): + """Mock Django settings for Bugfiler API.""" + monkeypatch.setattr( + "treeherder.perf.auto_perf_sheriffing.base_bug_manager.settings.BUGFILER_API_URL", + "https://bugzilla.mozilla.org", + ) + monkeypatch.setattr( + "treeherder.perf.auto_perf_sheriffing.base_bug_manager.settings.BUGFILER_API_KEY", + "test-api-key", + ) + monkeypatch.setattr( + "treeherder.perf.auto_perf_sheriffing.base_bug_manager.settings.COMMENTER_API_KEY", + "test-commenter-key", + ) + monkeypatch.setattr( + "treeherder.perf.auto_perf_sheriffing.base_bug_manager.settings.SITE_HOSTNAME", + "treeherder.mozilla.org", + ) From c85db71e8b6da86aec71637815c5573fdc771660 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:27:02 -0400 Subject: [PATCH 04/23] Add base EmailManager with unit tests. --- .../test_base_email_manager.py | 129 ++++++++++++++++++ .../base_email_manager.py | 14 ++ 2 files changed, 143 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/test_base_email_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/base_email_manager.py diff --git a/tests/perf/auto_perf_sheriffing/test_base_email_manager.py b/tests/perf/auto_perf_sheriffing/test_base_email_manager.py new file mode 100644 index 00000000000..13dbe24a6db --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/test_base_email_manager.py @@ -0,0 +1,129 @@ +from unittest.mock import MagicMock, patch + +import pytest + +from treeherder.perf.auto_perf_sheriffing.base_email_manager import EmailManager + + +@pytest.fixture +def mock_taskcluster_notify(): + """Mock the taskcluster notify client factory.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) as mock_factory: + mock_client = MagicMock() + mock_factory.return_value = mock_client + yield mock_client + + +@pytest.fixture +def email_manager(mock_taskcluster_notify): + """Fixture providing an EmailManager instance.""" + return EmailManager() + + +class TestEmailManager: + """Tests for the EmailManager base class.""" + + def test_email_manager_initialization(self, mock_taskcluster_notify): + """Test EmailManager initialization sets notify_client.""" + manager = EmailManager() + assert manager.notify_client is mock_taskcluster_notify + + def test_email_manager_calls_notify_client_factory(self): + """Test EmailManager calls taskcluster.notify_client_factory during init.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) as mock_factory: + mock_client = MagicMock() + mock_factory.return_value = mock_client + + manager = EmailManager() + + mock_factory.assert_called_once() + assert manager.notify_client == mock_client + + def test_get_email_func_returns_client_email(self, email_manager, mock_taskcluster_notify): + """Test get_email_func returns the notify_client.email method.""" + email_func = email_manager.get_email_func() + assert email_func is mock_taskcluster_notify.email + + def test_get_email_func_is_callable(self, email_manager): + """Test get_email_func returns a callable.""" + email_func = email_manager.get_email_func() + assert callable(email_func) + + def test_email_alert_is_no_op(self, email_manager): + """Test email_alert does nothing by default.""" + # Should not raise any exceptions + result = email_manager.email_alert() + assert result is None + + def test_email_alert_with_args(self, email_manager): + """Test email_alert accepts arbitrary args and kwargs.""" + result = email_manager.email_alert("arg1", "arg2", kwarg1="value1", kwarg2="value2") + assert result is None + + def test_notify_client_is_set_on_initialization(self): + """Test that notify_client is set during initialization.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) as mock_factory: + mock_client = MagicMock() + mock_factory.return_value = mock_client + + manager = EmailManager() + + # Verify notify_client was set + assert hasattr(manager, "notify_client") + assert manager.notify_client is not None + + def test_multiple_email_managers_use_separate_clients(self): + """Test that multiple EmailManager instances get separate notify clients.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) as mock_factory: + mock_client1 = MagicMock() + mock_client2 = MagicMock() + mock_factory.side_effect = [mock_client1, mock_client2] + + manager1 = EmailManager() + manager2 = EmailManager() + + assert manager1.notify_client is mock_client1 + assert manager2.notify_client is mock_client2 + assert manager1.notify_client is not manager2.notify_client + + def test_get_email_func_can_be_called_multiple_times( + self, email_manager, mock_taskcluster_notify + ): + """Test get_email_func can be called multiple times and returns same function.""" + email_func1 = email_manager.get_email_func() + email_func2 = email_manager.get_email_func() + + assert email_func1 is email_func2 + assert email_func1 is mock_taskcluster_notify.email + + def test_email_manager_with_mocked_notify_client(self): + """Test EmailManager with a fully mocked notify client.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) as mock_factory: + mock_client = MagicMock() + mock_email_method = MagicMock(return_value={"sent": True}) + mock_client.email = mock_email_method + mock_factory.return_value = mock_client + + manager = EmailManager() + email_func = manager.get_email_func() + + # Call the email function + result = email_func( + address="test@example.com", subject="Test Subject", content="Test Content" + ) + + # Verify the email method was called correctly + mock_email_method.assert_called_once_with( + address="test@example.com", subject="Test Subject", content="Test Content" + ) + assert result == {"sent": True} diff --git a/treeherder/perf/auto_perf_sheriffing/base_email_manager.py b/treeherder/perf/auto_perf_sheriffing/base_email_manager.py new file mode 100644 index 00000000000..2821e330bad --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/base_email_manager.py @@ -0,0 +1,14 @@ +from treeherder.services import taskcluster + + +class EmailManager: + """Formats and emails alert notifications.""" + + def __init__(self): + self.notify_client = taskcluster.notify_client_factory() + + def get_email_func(self): + return self.notify_client.email + + def email_alert(self, *args, **kwargs): + pass From 86ecc0a98e083e2da7adf0c6b826b3814375f543 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:27:31 -0400 Subject: [PATCH 05/23] Add base BugManager with unit tests. --- .../test_base_bug_manager.py | 207 ++++++++++++++++++ .../auto_perf_sheriffing/base_bug_manager.py | 89 ++++++++ 2 files changed, 296 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/test_base_bug_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/base_bug_manager.py diff --git a/tests/perf/auto_perf_sheriffing/test_base_bug_manager.py b/tests/perf/auto_perf_sheriffing/test_base_bug_manager.py new file mode 100644 index 00000000000..ce75596e84e --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/test_base_bug_manager.py @@ -0,0 +1,207 @@ +import pytest +import requests +import responses + +from treeherder.perf.auto_perf_sheriffing.base_bug_manager import BugManager + + +@pytest.fixture +def bug_manager(mock_bugfiler_settings): + """Fixture providing a BugManager instance.""" + return BugManager() + + +class TestBugManager: + """Tests for the BugManager base class.""" + + def test_bug_manager_initialization(self, bug_manager): + """Test BugManager initialization sets correct URL and headers.""" + assert bug_manager.bz_url == "https://bugzilla.mozilla.org/rest/bug" + assert bug_manager.bz_headers == {"Accept": "application/json"} + + def test_get_default_bug_creation_data(self, bug_manager): + """Test that _get_default_bug_creation_data returns correct structure.""" + default_data = bug_manager._get_default_bug_creation_data() + + assert "summary" in default_data + assert "type" in default_data + assert "product" in default_data + assert "component" in default_data + assert "keywords" in default_data + assert "whiteboard" in default_data + assert "regressed_by" in default_data + assert "see_also" in default_data + assert "version" in default_data + assert "severity" in default_data + assert "priority" in default_data + assert "description" in default_data + + assert default_data["summary"] == "" + assert default_data["type"] == "defect" + assert default_data["product"] == "" + assert default_data["component"] == "" + assert default_data["keywords"] == "" + assert default_data["whiteboard"] is None + assert default_data["regressed_by"] is None + assert default_data["see_also"] is None + assert default_data["version"] is None + assert default_data["severity"] == "" + assert default_data["priority"] == "" + assert default_data["description"] == "" + + def test_get_default_bug_comment_data(self, bug_manager): + """Test that _get_default_bug_comment_data returns correct structure.""" + default_data = bug_manager._get_default_bug_comment_data() + + assert "comment" in default_data + assert "body" in default_data["comment"] + assert default_data["comment"]["body"] == "" + + def test_add_needinfo_adds_flag_to_empty_bug_data(self, bug_manager): + """Test _add_needinfo adds needinfo flag to bug data without flags.""" + bug_data = {} + bug_manager._add_needinfo("user@example.com", bug_data) + + assert "flags" in bug_data + assert len(bug_data["flags"]) == 1 + assert bug_data["flags"][0]["name"] == "needinfo" + assert bug_data["flags"][0]["status"] == "?" + assert bug_data["flags"][0]["requestee"] == "user@example.com" + + def test_add_needinfo_appends_to_existing_flags(self, bug_manager): + """Test _add_needinfo appends to existing flags list.""" + bug_data = {"flags": [{"name": "other_flag", "status": "+"}]} + bug_manager._add_needinfo("user@example.com", bug_data) + + assert len(bug_data["flags"]) == 2 + assert bug_data["flags"][0]["name"] == "other_flag" + assert bug_data["flags"][1]["name"] == "needinfo" + assert bug_data["flags"][1]["requestee"] == "user@example.com" + + @responses.activate + def test_create_bug_success(self, bug_manager): + """Test _create successfully creates a bug.""" + expected_response = {"id": 123456} + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + bug_data = { + "summary": "Test Bug", + "product": "Firefox", + "component": "General", + "description": "Test Description", + } + + result = bug_manager._create(bug_data) + + assert result == expected_response + assert len(responses.calls) == 1 + assert responses.calls[0].request.url == "https://bugzilla.mozilla.org/rest/bug" + assert responses.calls[0].request.headers["x-bugzilla-api-key"] == "test-api-key" + assert responses.calls[0].request.headers["Accept"] == "application/json" + + @responses.activate + def test_create_bug_failure(self, bug_manager): + """Test _create raises exception on failure.""" + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json={"error": True, "message": "Invalid data"}, + status=400, + ) + + bug_data = {"summary": "Invalid Bug"} + + with pytest.raises(requests.exceptions.HTTPError): + bug_manager._create(bug_data) + + @responses.activate + def test_create_bug_sends_correct_headers(self, bug_manager): + """Test _create sends correct headers including API key.""" + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json={"id": 123456}, + status=200, + ) + + bug_data = {"summary": "Test"} + bug_manager._create(bug_data) + + request = responses.calls[0].request + assert request.headers["x-bugzilla-api-key"] == "test-api-key" + assert request.headers["Accept"] == "application/json" + + @responses.activate + def test_modify_bug_success(self, bug_manager): + """Test _modify successfully modifies a bug.""" + expected_response = {"bugs": [{"id": 123456, "changes": {}}]} + responses.add( + responses.PUT, + "https://bugzilla.mozilla.org/rest/bug/123456", + json=expected_response, + status=200, + ) + + changes = {"comment": {"body": "Additional comment"}} + result = bug_manager._modify(123456, changes) + + assert result == expected_response + assert len(responses.calls) == 1 + assert responses.calls[0].request.url == "https://bugzilla.mozilla.org/rest/bug/123456" + assert responses.calls[0].request.headers["x-bugzilla-api-key"] == "test-commenter-key" + assert ( + responses.calls[0].request.headers["User-Agent"] == "treeherder/treeherder.mozilla.org" + ) + + @responses.activate + def test_modify_bug_failure(self, bug_manager): + """Test _modify raises exception on failure.""" + responses.add( + responses.PUT, + "https://bugzilla.mozilla.org/rest/bug/123456", + json={"error": True, "message": "Not authorized"}, + status=401, + ) + + changes = {"comment": {"body": "Comment"}} + + with pytest.raises(requests.exceptions.HTTPError): + bug_manager._modify(123456, changes) + + @responses.activate + def test_modify_bug_sends_correct_headers(self, bug_manager): + """Test _modify sends correct headers including commenter API key.""" + responses.add( + responses.PUT, + "https://bugzilla.mozilla.org/rest/bug/999", + json={"bugs": []}, + status=200, + ) + + changes = {"status": "RESOLVED"} + bug_manager._modify(999, changes) + + request = responses.calls[0].request + assert request.headers["x-bugzilla-api-key"] == "test-commenter-key" + assert request.headers["User-Agent"] == "treeherder/treeherder.mozilla.org" + assert request.headers["Accept"] == "application/json" + + def test_file_bug_not_implemented(self, bug_manager): + """Test that file_bug raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + bug_manager.file_bug() + + def test_modify_bug_not_implemented(self, bug_manager): + """Test that modify_bug raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + bug_manager.modify_bug() + + def test_comment_bug_not_implemented(self, bug_manager): + """Test that comment_bug raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + bug_manager.comment_bug() diff --git a/treeherder/perf/auto_perf_sheriffing/base_bug_manager.py b/treeherder/perf/auto_perf_sheriffing/base_bug_manager.py new file mode 100644 index 00000000000..37b45cee2ff --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/base_bug_manager.py @@ -0,0 +1,89 @@ +import requests +from django.conf import settings + + +class BugManager: + """Files bugs, and comments on them for alerts.""" + + def __init__(self): + self.bz_url = settings.BUGFILER_API_URL + "/rest/bug" + self.bz_headers = {"Accept": "application/json"} + + def _get_default_bug_creation_data(self): + return { + "summary": "", + "type": "defect", + "product": "", + "component": "", + "keywords": "", + "whiteboard": None, + "regressed_by": None, + "see_also": None, + "version": None, + "severity": "", + "priority": "", + "description": "", + } + + def _get_default_bug_comment_data(self): + return {"comment": {"body": ""}} + + def _add_needinfo(self, bugzilla_email, bug_data): + bug_data.setdefault("flags", []).append( + { + "name": "needinfo", + "status": "?", + "requestee": bugzilla_email, + } + ) + + def _create(self, bug_data): + """Create a new bug. + + See `_get_default_bug_creation_data` for an example of what the + `bug_data` should be. + """ + headers = self.bz_headers + headers["x-bugzilla-api-key"] = settings.BUGFILER_API_KEY + + resp = requests.post( + url=self.bz_url, + json=bug_data, + headers=headers, + verify=True, + timeout=30, + ) + resp.raise_for_status() + + return resp.json() + + def _modify(self, bug, changes): + """Add a comment, or modify a bug. + + See `_get_default_bug_comment_data` for what the `bug_data` + should be. + """ + modification_url = self.bz_url + f"/{bug}" + headers = self.bz_headers + headers["x-bugzilla-api-key"] = settings.COMMENTER_API_KEY + headers["User-Agent"] = f"treeherder/{settings.SITE_HOSTNAME}" + + resp = requests.put( + url=modification_url, + json=changes, + headers=headers, + verify=True, + timeout=30, + ) + resp.raise_for_status() + + return resp.json() + + def file_bug(self, *args, **kwargs): + raise NotImplementedError() + + def modify_bug(self, *args, **kwargs): + raise NotImplementedError() + + def comment_bug(self, *args, **kwargs): + raise NotImplementedError() From 58a548f8602e6b0c525eb891a042def9cc33ecce Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:28:26 -0400 Subject: [PATCH 06/23] Add base AlertManager with unit tests. --- .../test_base_alert_manager.py | 353 ++++++++++++++++++ .../base_alert_manager.py | 124 ++++++ 2 files changed, 477 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/test_base_alert_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/base_alert_manager.py diff --git a/tests/perf/auto_perf_sheriffing/test_base_alert_manager.py b/tests/perf/auto_perf_sheriffing/test_base_alert_manager.py new file mode 100644 index 00000000000..0da73ccb16d --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/test_base_alert_manager.py @@ -0,0 +1,353 @@ +import logging + +import pytest + +from treeherder.perf.auto_perf_sheriffing.base_alert_manager import Alert, AlertManager + + +class TestAlert: + """Tests for the Alert base class.""" + + def test_alert_initialization(self): + """Test that Alert initializes with failed=False.""" + alert = Alert() + assert alert.failed is False + + def test_alert_set_failed_true(self): + """Test setting alert.failed to True.""" + alert = Alert() + alert.failed = True + assert alert.failed is True + + def test_alert_set_failed_false(self): + """Test setting alert.failed to False.""" + alert = Alert() + alert.failed = True + alert.failed = False + assert alert.failed is False + + def test_alert_failed_property(self): + """Test that failed property getter and setter work correctly.""" + alert = Alert() + assert alert.failed is False + alert.failed = True + assert alert.failed is True + + +class MockBugManager: + """Mock implementation of BugManager for testing.""" + + def __init__(self): + self.file_bug_called = False + self.modify_bug_called = False + self.comment_bug_called = False + + +class MockEmailManager: + """Mock implementation of EmailManager for testing.""" + + def __init__(self): + self.email_alert_called = False + + +class ConcreteAlertManager(AlertManager): + """Concrete implementation of AlertManager for testing.""" + + def __init__(self, bug_manager, email_manager): + super().__init__(bug_manager, email_manager) + self.update_alerts_called = False + self.comment_alert_bugs_called = False + self.file_alert_bug_called = False + self.modify_alert_bugs_called = False + self.email_alert_called = False + self.house_keeping_called = False + + def update_alerts(self, *args, **kwargs): + self.update_alerts_called = True + + def comment_alert_bugs(self, alerts, *args, **kwargs): + self.comment_alert_bugs_called = True + return [123, 456] + + def _file_alert_bug(self, alert, *args, **kwargs): + self.file_alert_bug_called = True + return 789 + + def modify_alert_bugs(self, alerts, commented_bugs, new_bugs, *args, **kwargs): + self.modify_alert_bugs_called = True + + def _email_alert(self, alert, *args, **kwargs): + self.email_alert_called = True + + def house_keeping(self, alerts, commented_bugs, new_bugs, *args, **kwargs): + self.house_keeping_called = True + + +@pytest.fixture +def mock_bug_manager(): + """Fixture providing a mock bug manager.""" + return MockBugManager() + + +@pytest.fixture +def mock_email_manager(): + """Fixture providing a mock email manager.""" + return MockEmailManager() + + +@pytest.fixture +def base_alert_manager(mock_bug_manager, mock_email_manager): + """Fixture providing a base AlertManager instance.""" + return AlertManager(mock_bug_manager, mock_email_manager) + + +@pytest.fixture +def concrete_alert_manager(mock_bug_manager, mock_email_manager): + """Fixture providing a concrete AlertManager instance.""" + return ConcreteAlertManager(mock_bug_manager, mock_email_manager) + + +class TestAlertManager: + """Tests for the AlertManager base class.""" + + def test_alert_manager_initialization(self, mock_bug_manager, mock_email_manager): + """Test AlertManager initialization with bug and email managers.""" + manager = AlertManager(mock_bug_manager, mock_email_manager) + + assert manager.bug_manager is mock_bug_manager + assert manager.email_manager is mock_email_manager + + def test_manage_alerts_calls_all_methods(self, concrete_alert_manager): + """Test that manage_alerts calls all expected methods in order.""" + alerts = [Alert(), Alert()] + concrete_alert_manager.manage_alerts(alerts) + + assert concrete_alert_manager.update_alerts_called + assert concrete_alert_manager.comment_alert_bugs_called + assert concrete_alert_manager.file_alert_bug_called + assert concrete_alert_manager.modify_alert_bugs_called + assert concrete_alert_manager.email_alert_called + assert concrete_alert_manager.house_keeping_called + + def test_manage_alerts_continues_on_update_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts continues after update_alerts fails.""" + + class FailingUpdateManager(ConcreteAlertManager): + def update_alerts(self, *args, **kwargs): + self.update_alerts_called = True + raise Exception("Update failed") + + manager = FailingUpdateManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.INFO): + manager.manage_alerts(alerts) + + assert manager.update_alerts_called + assert manager.comment_alert_bugs_called + assert "Failed to update alerts" in caplog.text + + def test_manage_alerts_continues_on_comment_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts continues after comment_alert_bugs fails.""" + + class FailingCommentManager(ConcreteAlertManager): + def comment_alert_bugs(self, *args, **kwargs): + self.comment_alert_bugs_called = True + raise Exception("Comment failed") + + manager = FailingCommentManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.WARNING): + manager.manage_alerts(alerts) + + assert manager.comment_alert_bugs_called + assert manager.file_alert_bug_called + assert "Failed to comment on existing bugs" in caplog.text + + def test_manage_alerts_continues_on_file_bug_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts continues after file_alert_bugs fails.""" + + class FailingFileBugManager(ConcreteAlertManager): + def _file_alert_bug(self, *args, **kwargs): + self.file_alert_bug_called = True + raise Exception("File bug failed") + + manager = FailingFileBugManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.WARNING): + manager.manage_alerts(alerts) + + assert manager.file_alert_bug_called + assert manager.modify_alert_bugs_called + assert "Failed to file alert bugs" in caplog.text + + def test_manage_alerts_continues_on_modify_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts continues after modify_alert_bugs fails.""" + + class FailingModifyManager(ConcreteAlertManager): + def modify_alert_bugs(self, *args, **kwargs): + self.modify_alert_bugs_called = True + raise Exception("Modify failed") + + manager = FailingModifyManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.WARNING): + manager.manage_alerts(alerts) + + assert manager.modify_alert_bugs_called + assert manager.email_alert_called + assert "Failed to file alert bugs" in caplog.text + + def test_manage_alerts_continues_on_email_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts continues after email_alerts fails.""" + + class FailingEmailManager(ConcreteAlertManager): + def _email_alert(self, *args, **kwargs): + self.email_alert_called = True + raise Exception("Email failed") + + manager = FailingEmailManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.WARNING): + manager.manage_alerts(alerts) + + assert manager.email_alert_called + assert manager.house_keeping_called + assert "Failed to send email alerts" in caplog.text + + def test_manage_alerts_continues_on_housekeeping_failure( + self, mock_bug_manager, mock_email_manager, caplog + ): + """Test that manage_alerts logs error when house_keeping fails.""" + + class FailingHousekeepingManager(ConcreteAlertManager): + def house_keeping(self, *args, **kwargs): + self.house_keeping_called = True + raise Exception("Housekeeping failed") + + manager = FailingHousekeepingManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert()] + with caplog.at_level(logging.WARNING): + manager.manage_alerts(alerts) + + assert manager.house_keeping_called + assert "Housekeeping failed" in caplog.text + + def test_manage_alerts_filters_failed_alerts_before_modify( + self, mock_bug_manager, mock_email_manager + ): + """Test that failed alerts are excluded before modify_alert_bugs.""" + + class TrackingModifyManager(ConcreteAlertManager): + def __init__(self, bug_manager, email_manager): + super().__init__(bug_manager, email_manager) + self.modify_alerts_count = 0 + + def modify_alert_bugs(self, alerts, commented_bugs, new_bugs, *args, **kwargs): + super().modify_alert_bugs(alerts, commented_bugs, new_bugs, *args, **kwargs) + self.modify_alerts_count = len(alerts) + + manager = TrackingModifyManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert(), Alert(), Alert()] + alerts[0].failed = True + alerts[2].failed = True + + manager.manage_alerts(alerts) + + # Only 1 alert should be passed to modify_alert_bugs + assert manager.modify_alerts_count == 1 + + def test_file_alert_bugs_returns_list_of_bugs(self, concrete_alert_manager): + """Test that file_alert_bugs returns a list of bug numbers.""" + alerts = [Alert(), Alert(), Alert()] + bugs = concrete_alert_manager.file_alert_bugs(alerts) + + assert len(bugs) == 3 + assert all(bug == 789 for bug in bugs) + + def test_file_alert_bugs_handles_none_returns(self, mock_bug_manager, mock_email_manager): + """Test that file_alert_bugs skips None returns from _file_alert_bug.""" + + class NoneReturningManager(ConcreteAlertManager): + def __init__(self, bug_manager, email_manager): + super().__init__(bug_manager, email_manager) + self.call_count = 0 + + def _file_alert_bug(self, alert, *args, **kwargs): + self.call_count += 1 + # Return None for odd calls, bug number for even calls + return None if self.call_count % 2 == 1 else 999 + + manager = NoneReturningManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert(), Alert(), Alert(), Alert()] + bugs = manager.file_alert_bugs(alerts) + + assert len(bugs) == 2 + assert all(bug == 999 for bug in bugs) + + def test_email_alerts_calls_email_alert_for_each_alert( + self, mock_bug_manager, mock_email_manager + ): + """Test that email_alerts calls _email_alert for each alert.""" + + class CountingEmailManager(ConcreteAlertManager): + def __init__(self, bug_manager, email_manager): + super().__init__(bug_manager, email_manager) + self.email_count = 0 + + def _email_alert(self, alert, *args, **kwargs): + self.email_count += 1 + + manager = CountingEmailManager(mock_bug_manager, mock_email_manager) + + alerts = [Alert(), Alert(), Alert()] + manager.email_alerts(alerts) + + assert manager.email_count == 3 + + def test_update_alerts_not_implemented(self, base_alert_manager): + """Test that update_alerts raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager.update_alerts() + + def test_comment_alert_bugs_not_implemented(self, base_alert_manager): + """Test that comment_alert_bugs raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager.comment_alert_bugs([]) + + def test_file_alert_bug_not_implemented(self, base_alert_manager): + """Test that _file_alert_bug raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager._file_alert_bug(Alert()) + + def test_modify_alert_bugs_not_implemented(self, base_alert_manager): + """Test that modify_alert_bugs raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager.modify_alert_bugs([], [], []) + + def test_email_alert_not_implemented(self, base_alert_manager): + """Test that _email_alert raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager._email_alert(Alert()) + + def test_house_keeping_not_implemented(self, base_alert_manager): + """Test that house_keeping raises NotImplementedError.""" + with pytest.raises(NotImplementedError): + base_alert_manager.house_keeping([], [], []) diff --git a/treeherder/perf/auto_perf_sheriffing/base_alert_manager.py b/treeherder/perf/auto_perf_sheriffing/base_alert_manager.py new file mode 100644 index 00000000000..feb0d057405 --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/base_alert_manager.py @@ -0,0 +1,124 @@ +import logging +import traceback + +logger = logging.getLogger(__name__) + + +class Alert: + def __init__(self): + self._failed = False + + @property + def failed(self): + return self._failed + + @failed.setter + def failed(self, value): + self._failed = value + + +class AlertManager: + """Handles the alert management. + + This includes the following: + (1) Filing bugs + (2) Setting status for the alerts when bugs are updated + (3) Adding comments to bugs for addition alerts when they are produced + after the bug was. + """ + + def __init__(self, bug_manager, email_manager): + self.bug_manager = bug_manager + self.email_manager = email_manager + + def manage_alerts(self, alerts, *args, **kwargs): + """Handles everything related to alert notifications. + + None of these depend on each other, so a failure in one doesn't always + mean that a failure in another one will happen. + """ + try: + # Update alerts with bug info. Done before filing new bugs + # to prevent updating from recently filed bugs. + self.update_alerts(alerts, *args, **kwargs) + except Exception: + logger.info(f"Failed to update alerts: {traceback.format_exc()}") + + commented_bugs = [] + try: + # Comment on existing bugs. Done before filing new bugs to + # prevent commenting on recently filed bugs + commented_bugs = self.comment_alert_bugs(alerts, *args, **kwargs) + except Exception: + logger.warning(f"Failed to comment on existing bugs: {traceback.format_exc()}") + + new_bugs = [] + try: + # File new bugs + new_bugs = self.file_alert_bugs(alerts, *args, **kwargs) + except Exception: + logger.warning(f"Failed to file alert bugs: {traceback.format_exc()}") + + try: + # Modify any of the bugs that were commented on, or created. Exclude + # alerts that were marked as failures when creating bugs. + alerts = [alert for alert in alerts if not alert.failed] + self.modify_alert_bugs(alerts, commented_bugs, new_bugs, *args, **kwargs) + except Exception: + logger.warning(f"Failed to file alert bugs: {traceback.format_exc()}") + + try: + # Produce email notifications. Done after filing bugs to include + # bug information if that becomes a feature at some point. + self.email_alerts(alerts, *args, **kwargs) + except Exception: + logger.warning(f"Failed to send email alerts: {traceback.format_exc()}") + + try: + # Final stage to let us do any necessary cleanup/houskeeping/etc. from + # this current run or previous runs. Can include general maintenance of + # alerts and tables. + self.house_keeping(alerts, commented_bugs, new_bugs, *args, **kwargs) + except Exception: + logger.warning(f"Failed to perform house keeping: {traceback.format_exc()}") + + def update_alerts(self, *args, **kwargs): + """Updates all alerts with status changes from the associated bugs.""" + raise NotImplementedError() + + def comment_alert_bugs(self, alerts, *args, **kwargs): + """Comments on bugs to mention additional alerting measurements.""" + raise NotImplementedError() + + def file_alert_bugs(self, alerts, *args, **kwargs): + """Files a bug for each telemetry alert summary.""" + bugs = [] + + for alert in alerts: + bug = self._file_alert_bug(alert, *args, **kwargs) + if bug is None: + continue + bugs.append(bug) + + return bugs + + def _file_alert_bug(self, *args, **kwargs): + """Create a bug for an alert.""" + raise NotImplementedError() + + def modify_alert_bugs(self, alerts, commented_bugs, new_bugs, *args, **kwargs): + """Modify alert bugs.""" + raise NotImplementedError() + + def email_alerts(self, alerts, *args, **kwargs): + """Sends out emails for each new alert.""" + for alert in alerts: + self._email_alert(alert, *args, **kwargs) + + def _email_alert(self, *args, **kwargs): + """Produces an email for a new telemetry alert summary.""" + raise NotImplementedError() + + def house_keeping(self, alerts, commented_bugs, new_bugs, *args, **kwargs): + """Used for any sort of general maintenance/cleanup/etc..""" + raise NotImplementedError() From e570f67c0c36aab4d2eae1fa26e5c852dc0e2691 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:29:00 -0400 Subject: [PATCH 07/23] Add base BugSearcher with unit tests. --- .../auto_perf_sheriffing/test_bug_searcher.py | 416 ++++++++++++++++++ .../perf/auto_perf_sheriffing/bug_searcher.py | 105 +++++ 2 files changed, 521 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/test_bug_searcher.py create mode 100644 treeherder/perf/auto_perf_sheriffing/bug_searcher.py diff --git a/tests/perf/auto_perf_sheriffing/test_bug_searcher.py b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py new file mode 100644 index 00000000000..608b4d9227b --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py @@ -0,0 +1,416 @@ +from datetime import datetime, timezone +from unittest.mock import patch + +import pytest +import requests +import responses + +from treeherder.perf.auto_perf_sheriffing.bug_searcher import BugSearcher + + +@pytest.fixture +def bug_searcher(mock_bugfiler_settings): + """Fixture providing a BugSearcher instance.""" + return BugSearcher() + + +class TestBugSearcherInitialization: + """Tests for BugSearcher initialization.""" + + def test_bug_searcher_initialization(self, bug_searcher): + """Test BugSearcher initializes with correct defaults.""" + assert bug_searcher.bz_url == "https://bugzilla.mozilla.org/rest/bug?" + assert bug_searcher.bz_headers == {} + assert bug_searcher._include_fields == ["id"] + assert bug_searcher._products == [] + assert bug_searcher._query == {} + + +class TestBugSearcherSetters: + """Tests for BugSearcher setter methods.""" + + def test_set_include_fields(self, bug_searcher): + """Test set_include_fields updates include_fields correctly.""" + fields = ["id", "summary", "status", "resolution"] + bug_searcher.set_include_fields(fields) + assert bug_searcher._include_fields == fields + + def test_set_include_fields_with_history(self, bug_searcher): + """Test set_include_fields can include history field.""" + fields = ["id", "history", "status"] + bug_searcher.set_include_fields(fields) + assert bug_searcher._include_fields == fields + assert "history" in bug_searcher._include_fields + + def test_set_products_single(self, bug_searcher): + """Test set_products with single product.""" + products = ["Firefox"] + bug_searcher.set_products(products) + assert bug_searcher._products == products + + def test_set_products_multiple(self, bug_searcher): + """Test set_products with multiple products.""" + products = ["Firefox", "Core", "Toolkit"] + bug_searcher.set_products(products) + assert bug_searcher._products == products + + def test_set_products_empty(self, bug_searcher): + """Test set_products with empty list.""" + bug_searcher.set_products([]) + assert bug_searcher._products == [] + + def test_set_query(self, bug_searcher): + """Test set_query updates query correctly.""" + query = { + "f1": "status", + "o1": "equals", + "v1": "NEW", + "chfieldfrom": "2024-01-01", + } + bug_searcher.set_query(query) + assert bug_searcher._query == query + + def test_set_query_with_time_range(self, bug_searcher): + """Test set_query with time range parameters.""" + query = { + "chfieldfrom": "2024-01-01", + "chfieldto": "2024-12-31", + } + bug_searcher.set_query(query) + assert bug_searcher._query["chfieldfrom"] == "2024-01-01" + assert bug_searcher._query["chfieldto"] == "2024-12-31" + + +class TestBugSearcherHelperMethods: + """Tests for BugSearcher helper methods.""" + + @patch("treeherder.perf.auto_perf_sheriffing.bug_searcher.datetime") + def test_get_today_date(self, mock_datetime, bug_searcher): + """Test get_today_date returns correct format.""" + mock_now = datetime(2024, 3, 15, 10, 30, 0, tzinfo=timezone.utc) + mock_datetime.now.return_value = mock_now + + result = bug_searcher.get_today_date() + + assert result == mock_now.date() + mock_datetime.now.assert_called_once_with(timezone.utc) + + def test_find_last_query_num_no_queries(self, bug_searcher): + """Test _find_last_query_num returns 0 when no query fields exist.""" + bug_searcher.set_query({"status": "NEW"}) + result = bug_searcher._find_last_query_num() + assert result == 0 + + def test_find_last_query_num_single_query(self, bug_searcher): + """Test _find_last_query_num with single query field.""" + bug_searcher.set_query({"f1": "status", "o1": "equals", "v1": "NEW"}) + result = bug_searcher._find_last_query_num() + assert result == 1 + + def test_find_last_query_num_multiple_queries(self, bug_searcher): + """Test _find_last_query_num with multiple query fields.""" + bug_searcher.set_query( + { + "f1": "status", + "o1": "equals", + "v1": "NEW", + "f2": "priority", + "o2": "equals", + "v2": "P1", + "f5": "product", + "o5": "equals", + "v5": "Firefox", + } + ) + result = bug_searcher._find_last_query_num() + assert result == 5 + + +class TestBuildBugzillaParams: + """Tests for _build_bugzilla_params method.""" + + def test_build_params_basic_query(self, bug_searcher): + """Test _build_bugzilla_params with basic query.""" + query = {"status": "NEW"} + bug_searcher.set_query(query) + + params = bug_searcher._build_bugzilla_params() + + assert params["status"] == "NEW" + assert params["include_fields"] == ["id"] + + def test_build_params_with_custom_include_fields(self, bug_searcher): + """Test _build_bugzilla_params respects custom include_fields.""" + query = {"status": "NEW"} + fields = ["id", "summary", "status"] + + bug_searcher.set_query(query) + bug_searcher.set_include_fields(fields) + + params = bug_searcher._build_bugzilla_params() + + assert params["include_fields"] == fields + + def test_build_params_query_with_include_fields(self, bug_searcher): + """Test _build_bugzilla_params when query already has include_fields.""" + query = {"status": "NEW", "include_fields": ["id", "history"]} + bug_searcher.set_query(query) + + params = bug_searcher._build_bugzilla_params() + + # Should keep the query's include_fields + assert params["include_fields"] == ["id", "history"] + + def test_build_params_with_products_no_existing_query(self, bug_searcher): + """Test _build_bugzilla_params adds product filter correctly.""" + query = {"status": "NEW"} + products = ["Firefox", "Core"] + + bug_searcher.set_query(query) + bug_searcher.set_products(products) + + params = bug_searcher._build_bugzilla_params() + + assert params["f0"] == "product" + assert params["o0"] == "anywordssubstr" + assert params["v0"] == "Firefox,Core" + + def test_build_params_with_products_and_existing_queries(self, bug_searcher): + """Test _build_bugzilla_params adds product filter after existing queries.""" + query = { + "f1": "status", + "o1": "equals", + "v1": "NEW", + "f2": "priority", + "o2": "equals", + "v2": "P1", + } + products = ["Firefox"] + + bug_searcher.set_query(query) + bug_searcher.set_products(products) + + params = bug_searcher._build_bugzilla_params() + + # Should add product after query 2 + assert params["f2"] == "product" + assert params["o2"] == "anywordssubstr" + assert params["v2"] == "Firefox" + + def test_build_params_without_products(self, bug_searcher): + """Test _build_bugzilla_params without products filter.""" + query = {"status": "NEW"} + bug_searcher.set_query(query) + + params = bug_searcher._build_bugzilla_params() + + assert "f0" not in params + assert "o0" not in params + assert "v0" not in params + + def test_build_params_preserves_original_query(self, bug_searcher): + """Test _build_bugzilla_params doesn't modify original query.""" + original_query = {"status": "NEW"} + bug_searcher.set_query(original_query) + bug_searcher.set_products(["Firefox"]) + + params = bug_searcher._build_bugzilla_params() + + # Original query should not be modified + assert "f0" not in bug_searcher._query + assert "f0" in params + + +class TestGetBugs: + """Tests for get_bugs method.""" + + @responses.activate + def test_get_bugs_success(self, bug_searcher): + """Test get_bugs successfully retrieves bugs.""" + expected_response = { + "bugs": [ + {"id": 123456}, + {"id": 789012}, + ] + } + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = {"status": "NEW"} + bug_searcher.set_query(query) + + result = bug_searcher.get_bugs() + + assert result == expected_response + assert len(responses.calls) == 1 + + @responses.activate + def test_get_bugs_with_include_fields(self, bug_searcher): + """Test get_bugs sends correct include_fields parameter.""" + expected_response = {"bugs": [{"id": 123456, "summary": "Test Bug", "status": "NEW"}]} + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = {"status": "NEW"} + fields = ["id", "summary", "status"] + + bug_searcher.set_query(query) + bug_searcher.set_include_fields(fields) + + result = bug_searcher.get_bugs() + + assert result == expected_response + # Check that the request included the correct parameters + request_params = responses.calls[0].request.url + assert "include_fields" in request_params + + @responses.activate + def test_get_bugs_with_products(self, bug_searcher): + """Test get_bugs includes product filter in request.""" + expected_response = {"bugs": [{"id": 123456}]} + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = {"status": "NEW"} + products = ["Firefox", "Core"] + + bug_searcher.set_query(query) + bug_searcher.set_products(products) + + result = bug_searcher.get_bugs() + + assert result == expected_response + request_url = responses.calls[0].request.url + # Verify product parameters are in the URL + assert "f0=product" in request_url + assert "o0=anywordssubstr" in request_url + + @responses.activate + def test_get_bugs_sends_user_agent_header(self, bug_searcher): + """Test get_bugs sends correct User-Agent header.""" + expected_response = {"bugs": []} + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = {"status": "NEW"} + bug_searcher.set_query(query) + + bug_searcher.get_bugs() + + request_headers = responses.calls[0].request.headers + assert request_headers["User-Agent"] == "treeherder/treeherder.mozilla.org" + + @responses.activate + def test_get_bugs_http_error(self, bug_searcher): + """Test get_bugs raises HTTPError on failure.""" + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json={"error": True, "message": "Invalid request"}, + status=400, + ) + + query = {"status": "NEW"} + bug_searcher.set_query(query) + + with pytest.raises(requests.exceptions.HTTPError): + bug_searcher.get_bugs() + + def test_get_bugs_without_query_returns_none(self, bug_searcher, caplog): + """Test get_bugs returns None when no query is set.""" + # Don't set a query + result = bug_searcher.get_bugs() + + assert result is None + + @responses.activate + def test_get_bugs_empty_result(self, bug_searcher): + """Test get_bugs handles empty bug list correctly.""" + expected_response = {"bugs": []} + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = {"status": "NEW"} + bug_searcher.set_query(query) + + result = bug_searcher.get_bugs() + + assert result == expected_response + assert result["bugs"] == [] + + @responses.activate + def test_get_bugs_complex_query(self, bug_searcher): + """Test get_bugs with complex query parameters.""" + expected_response = {"bugs": [{"id": 123456}]} + + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + query = { + "f1": "status", + "o1": "equals", + "v1": "NEW", + "f2": "priority", + "o2": "anyexact", + "v2": "P1,P2", + "chfieldfrom": "2024-01-01", + "chfieldto": "2024-12-31", + } + + bug_searcher.set_query(query) + bug_searcher.set_include_fields(["id", "summary", "history"]) + bug_searcher.set_products(["Firefox", "Core"]) + + result = bug_searcher.get_bugs() + + assert result == expected_response + request_url = responses.calls[0].request.url + + # Verify all parameters are present + assert "f1=status" in request_url + assert "v1=NEW" in request_url + assert "chfieldfrom=2024-01-01" in request_url + + @responses.activate + def test_get_bugs_network_timeout(self, bug_searcher): + """Test get_bugs handles network timeout.""" + responses.add( + responses.GET, + "https://bugzilla.mozilla.org/rest/bug", + body=requests.exceptions.Timeout("Connection timeout"), + ) + + query = {"status": "NEW"} + bug_searcher.set_query(query) + + with pytest.raises(requests.exceptions.Timeout): + bug_searcher.get_bugs() diff --git a/treeherder/perf/auto_perf_sheriffing/bug_searcher.py b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py new file mode 100644 index 00000000000..71ca07ed74c --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py @@ -0,0 +1,105 @@ +import logging +from copy import deepcopy +from datetime import datetime, timezone + +import requests +from django.conf import settings + +logger = logging.getLogger(__name__) + + +class BugSearcher: + """Helper class to perform queries on Bugzilla. + + TODO: Replace this with libmozdata Bugzilla class (potentially + replacing BugManager too) + """ + + def __init__(self): + self.bz_url = settings.BUGFILER_API_URL + "/rest/bug?" + self.bz_headers = {} + self._include_fields = ["id"] + self._products = [] + self._query = {} + + def set_include_fields(self, include_fields): + """Set the fields that should be included in the returned result. + + By default, only the bug ID is returned. Some example fields are: + * history + * resolution + * status + + Have a look at bugbot rules for more examples: https://github.com/mozilla/bugbot + """ + self._include_fields = include_fields + + def set_products(self, products): + """Set the products to get bugs from. + + By default, bugs come from all products. + """ + self._products = products + + def set_query(self, query): + """Set the query for the bug search. + + There is no default setting, and this query must be specified. + Have a look at bugbot rules for examples: https://github.com/mozilla/bugbot + + It's good practice to have a time range specified on the query. + """ + self._query = query + + def get_today_date(self): + """Helper method to get today's date in the YYYY-MM-DD format.""" + return datetime.now(timezone.utc).date() + + def _find_last_query_num(self): + """Used to find the last query number used.""" + query_num = 0 + for query_field in self._query: + if len(query_field) == 2 and query_field.startswith("f"): + query_num = max(query_num, int(query_field[1])) + return query_num + + def _build_bugzilla_params(self): + """Builds the params for the bugzilla query.""" + params = deepcopy(self._query) + + if self._products: + query_num = self._find_last_query_num() + params.update( + { + f"f{query_num}": "product", + f"o{query_num}": "anywordssubstr", + f"v{query_num}": ",".join(self._products), + } + ) + + if not params.get("include_fields"): + params["include_fields"] = self._include_fields + + return params + + def get_bugs(self): + """Gets all the bugs using the specified query settings.""" + if not self._query: + logger.warning("Cannot perform bug query as no query was defined.") + return + + headers = deepcopy(self.bz_headers) + headers["User-Agent"] = f"treeherder/{settings.SITE_HOSTNAME}" + + params = self._build_bugzilla_params() + + resp = requests.get( + url=self.bz_url, + params=params, + headers=headers, + verify=True, + timeout=30, + ) + resp.raise_for_status() + + return resp.json() From 6c0d7b58b5891f57c98ae42988cdb1fa4ddf4375 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:34:27 -0400 Subject: [PATCH 08/23] Add init files, and pytest fixtures for concrete telemetry unit tests. --- .../telemetry_alerting/__init__.py | 0 .../telemetry_alerting/conftest.py | 237 ++++++++++++++++++ .../telemetry_alerting/__init__.py | 0 3 files changed, 237 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/conftest.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/conftest.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/conftest.py new file mode 100644 index 00000000000..78cffd20818 --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/conftest.py @@ -0,0 +1,237 @@ +from datetime import datetime +from unittest.mock import Mock + +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import TelemetryAlert +from treeherder.perf.models import ( + PerformanceTelemetryAlert, + PerformanceTelemetryAlertSummary, + PerformanceTelemetrySignature, +) + + +@pytest.fixture +def detection_push(create_push, test_repository): + return create_push( + test_repository, + revision="abcdef123456", + author="test@mozilla.com", + time=datetime(2024, 1, 15, 12, 0, 0), + ) + + +@pytest.fixture +def prev_push(create_push, test_repository): + return create_push( + test_repository, + revision="prev123456", + author="test@mozilla.com", + time=datetime(2024, 1, 14, 12, 0, 0), + ) + + +@pytest.fixture +def next_push(create_push, test_repository): + return create_push( + test_repository, + revision="next123456", + author="test@mozilla.com", + time=datetime(2024, 1, 16, 12, 0, 0), + ) + + +@pytest.fixture +def test_telemetry_signature(db): + return PerformanceTelemetrySignature.objects.create( + channel="Nightly", + platform="Windows", + probe="networking_http_channel_page_open_to_first_sent", + probe_type="Glean", + application="Firefox", + ) + + +@pytest.fixture +def test_telemetry_alert_summary( + test_repository, test_perf_framework, detection_push, prev_push, next_push, test_issue_tracker +): + return PerformanceTelemetryAlertSummary.objects.create( + repository=test_repository, + framework=test_perf_framework, + prev_push=prev_push, + push=next_push, + original_push=detection_push, + manually_created=False, + created=datetime(2024, 1, 16, 13, 0, 0), + issue_tracker=test_issue_tracker, + ) + + +@pytest.fixture +def telemetry_alert_obj( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature +): + return TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + +@pytest.fixture +def mock_probe(): + """Mock probe for testing with default configuration.""" + probe = Mock() + probe.name = "test_probe_metric" + probe.get_notification_emails.return_value = ["test@mozilla.com"] + probe.should_file_bug.return_value = True + probe.should_email.return_value = False + return probe + + +@pytest.fixture +def base_metric_info(): + """Base metric info structure matching real telemetry data.""" + return { + "name": "networking_http_channel_page_open_to_first_sent", + "data": { + "name": "networking.http_channel_page_open_to_first_sent", + "description": "Time in milliseconds from AsyncOpen to first byte of request sent", + "tags": ["Core :: Networking"], + "in_source": True, + "latest_fx_release_version": "143.0", + "extra_keys": None, + "type": "timing_distribution", + "expires": None, + "expiry_text": "never", + "sampled": False, + "sampled_text": "Not sampled", + "is_part_of_info_section": False, + "bugs": ["https://bugzilla.mozilla.org/show_bug.cgi?id=1697480"], + "has_annotation": False, + "origin": "gecko", + }, + "platform": "desktop", + } + + +@pytest.fixture +def metric_info_with_alert(base_metric_info): + """Metric info with alert=True and bugzilla_notification_emails.""" + base_metric_info["data"]["monitor"] = { + "alert": True, + "bugzilla_notification_emails": ["email@fake.fake.com"], + } + return base_metric_info + + +@pytest.fixture +def alert_without_bug(test_telemetry_alert_summary, test_telemetry_signature): + """Create a TelemetryAlert object without a bug number.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlertFactory, + ) + + alert_row = PerformanceTelemetryAlert.objects.create( + summary=test_telemetry_alert_summary, + series_signature=test_telemetry_signature, + is_regression=True, + amount_pct=15.5, + amount_abs=100.0, + prev_value=645.5, + new_value=745.5, + sustained=True, + direction="increase", + confidence=0.95, + prev_median=650.0, + new_median=750.0, + prev_p90=700.0, + new_p90=800.0, + prev_p95=720.0, + new_p95=820.0, + bug_number=None, + notified=False, + ) + return TelemetryAlertFactory.construct_alert(alert_row) + + +@pytest.fixture +def alert_with_bug(test_telemetry_alert_summary, test_telemetry_signature): + """Create a TelemetryAlert object with a bug number.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlertFactory, + ) + + alert_row = PerformanceTelemetryAlert.objects.create( + summary=test_telemetry_alert_summary, + series_signature=test_telemetry_signature, + is_regression=True, + amount_pct=15.5, + amount_abs=100.0, + prev_value=645.5, + new_value=745.5, + sustained=True, + direction="increase", + confidence=0.95, + prev_median=650.0, + new_median=750.0, + prev_p90=700.0, + new_p90=800.0, + prev_p95=720.0, + new_p95=820.0, + bug_number=123456, + notified=False, + ) + return TelemetryAlertFactory.construct_alert(alert_row) + + +@pytest.fixture +def create_telemetry_alert(test_telemetry_alert_summary): + """Factory fixture to create telemetry alerts with custom parameters.""" + + def _create_alert(signature, **kwargs): + defaults = { + "is_regression": True, + "amount_pct": 15.5, + "amount_abs": 100.0, + "prev_value": 645.5, + "new_value": 745.5, + "sustained": True, + "direction": "increase", + "confidence": 0.95, + "prev_median": 650.0, + "new_median": 750.0, + "prev_p90": 700.0, + "new_p90": 800.0, + "prev_p95": 720.0, + "new_p95": 820.0, + "bug_number": None, + "notified": False, + "summary": test_telemetry_alert_summary, + } + defaults.update(kwargs) + return PerformanceTelemetryAlert.objects.create(series_signature=signature, **defaults) + + return _create_alert + + +@pytest.fixture +def create_telemetry_signature(): + """Factory fixture to create telemetry signatures with custom parameters.""" + + def _create_signature(**kwargs): + defaults = { + "channel": "Nightly", + "platform": "Windows", + "probe": "test_probe", + "probe_type": "Glean", + "application": "Firefox", + } + defaults.update(kwargs) + return PerformanceTelemetrySignature.objects.create(**defaults) + + return _create_signature + + +@pytest.fixture +def test_telemetry_alert(create_telemetry_signature, create_telemetry_alert): + return create_telemetry_alert(create_telemetry_signature()) diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/__init__.py new file mode 100644 index 00000000000..e69de29bb2d From 77c4c9a3c2f96a7f986dc871884fb237367a77df Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:30:34 -0400 Subject: [PATCH 09/23] Add file that contains utility methods and constants for alert management. --- .../telemetry_alerting/test_utils.py | 105 ++++++++++++++++++ .../telemetry_alerting/utils.py | 64 +++++++++++ 2 files changed, 169 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_utils.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/utils.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_utils.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_utils.py new file mode 100644 index 00000000000..3e6ba6eb60a --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_utils.py @@ -0,0 +1,105 @@ +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + get_glean_dictionary_link, + get_treeherder_detection_link, + get_treeherder_detection_range_link, +) + + +class TestGetGleanDictionaryLink: + def test_desktop_platform_windows(self, test_telemetry_signature): + """Test Glean dictionary link generation for Windows platform.""" + test_telemetry_signature.platform = "Windows" + test_telemetry_signature.probe = "test_probe" + + link = get_glean_dictionary_link(test_telemetry_signature) + + assert ( + link + == "https://dictionary.telemetry.mozilla.org/apps/firefox_desktop/metrics/test_probe" + ) + + def test_mobile_platform_fenix(self, test_telemetry_signature): + """Test Glean dictionary link generation for mobile (non-desktop) platform.""" + test_telemetry_signature.platform = "Android" + test_telemetry_signature.probe = "mobile_probe" + + link = get_glean_dictionary_link(test_telemetry_signature) + + assert link == "https://dictionary.telemetry.mozilla.org/apps/fenix/metrics/mobile_probe" + + +class TestGetTreeherderDetectionLink: + def test_nightly_channel(self, test_telemetry_signature): + """Test Treeherder detection link for Nightly channel.""" + test_telemetry_signature.channel = "Nightly" + detection_range = {"detection": type("obj", (object,), {"revision": "abcdef123456"})()} + + link = get_treeherder_detection_link(detection_range, test_telemetry_signature) + + assert ( + link == "https://treeherder.mozilla.org/jobs?repo=mozilla-central&revision=abcdef123456" + ) + + def test_unknown_channel_defaults_to_central(self, test_telemetry_signature): + """Test Treeherder detection link defaults to mozilla-central for unknown channel.""" + test_telemetry_signature.channel = "UnknownChannel" + detection_range = {"detection": type("obj", (object,), {"revision": "unknown123456"})()} + + link = get_treeherder_detection_link(detection_range, test_telemetry_signature) + + assert ( + link + == "https://treeherder.mozilla.org/jobs?repo=mozilla-central&revision=unknown123456" + ) + + def test_with_long_revision(self, test_telemetry_signature): + """Test Treeherder detection link with full-length revision hash.""" + test_telemetry_signature.channel = "Nightly" + detection_range = { + "detection": type( + "obj", (object,), {"revision": "abcdef1234567890abcdef1234567890abcdef12"} + )() + } + + link = get_treeherder_detection_link(detection_range, test_telemetry_signature) + + assert ( + link + == "https://treeherder.mozilla.org/jobs?repo=mozilla-central&revision=abcdef1234567890abcdef1234567890abcdef12" + ) + + +class TestGetTreeherderDetectionRangeLink: + def test_release_channel_range(self, test_telemetry_signature): + """Test Treeherder detection range link for Release channel.""" + test_telemetry_signature.channel = "Release" + detection_range = { + "from": type("obj", (object,), {"revision": "releaseFrom123"})(), + "to": type("obj", (object,), {"revision": "releaseTo456"})(), + } + + link = get_treeherder_detection_range_link(detection_range, test_telemetry_signature) + + assert ( + link + == "https://treeherder.mozilla.org/jobs?repo=mozilla-release&fromchange=releaseFrom123&tochange=releaseTo456" + ) + + def test_with_full_length_revisions(self, test_telemetry_signature): + """Test Treeherder detection range link with full-length revision hashes.""" + test_telemetry_signature.channel = "Nightly" + detection_range = { + "from": type( + "obj", (object,), {"revision": "abcdef1234567890abcdef1234567890abcdef12"} + )(), + "to": type( + "obj", (object,), {"revision": "fedcba0987654321fedcba0987654321fedcba98"} + )(), + } + + link = get_treeherder_detection_range_link(detection_range, test_telemetry_signature) + + assert ( + link + == "https://treeherder.mozilla.org/jobs?repo=mozilla-central&fromchange=abcdef1234567890abcdef1234567890abcdef12&tochange=fedcba0987654321fedcba0987654321fedcba98" + ) diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/utils.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/utils.py new file mode 100644 index 00000000000..6d0770187de --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/utils.py @@ -0,0 +1,64 @@ +GLEAN_DICTIONARY = "https://dictionary.telemetry.mozilla.org/apps/{page}/metrics/{probe}" +GLEAN_PROBE_INFO = ( + "https://dictionary.telemetry.mozilla.org/data/firefox_desktop/metrics/data_{probe_name}.json" +) +TREEHERDER_PUSH = "https://treeherder.mozilla.org/jobs?repo={repo}&revision={revision}" +TREEHERDER_DATES = ( + "https://treeherder.mozilla.org/jobs?repo={repo}&fromchange={from_change}&tochange={to_change}" +) +PUSH_LOG = ( + "https://hg-edge.mozilla.org/mozilla-central/pushloghtml?" + "startdate={start_date}&enddate={end_date}" +) +BZ_TELEMETRY_ALERTS = ( + "https://bugzilla.mozilla.org/buglist.cgi?" + "keywords=telemetry-alert&keywords_type=allwords" + "&chfield=[Bug%20creation]&chfieldto={today}&chfieldfrom={prev_day}&" +) +BZ_TELEMETRY_ALERTS_CHANGED = ( + "https://bugzilla.mozilla.org/rest/bug?" + "include_fields=id&include_fields=resolution" + "&f1=keywords&o1=allwords&v1=telemetry-alert" + "&f2=resolution&o2=changedbefore&v2={today}" + "&f3=resolution&o3=changedafter&v2={prev_day}" +) +REVISION_INFO = "https://hg.mozilla.org/mozilla-central/json-log/%s" + +DEFAULT_CHANGE_DETECTION = "cdf_squared" +DESKTOP_PLATFORMS = ( + "Windows", + "Linux", + "Darwin", +) +CHANNEL_TO_REPO_MAPPING = { + "Nightly": "mozilla-central", + "Release": "mozilla-release", + "Beta": "mozilla-beta", +} + +MODIFIABLE_ALERT_FIELDS = ("status",) +DEFAULT_ALERT_EMAIL = "gmierzwinski@mozilla.com" + + +def get_glean_dictionary_link(telemetry_signature): + if telemetry_signature.platform in DESKTOP_PLATFORMS: + dictionary_page = "firefox_desktop" + else: + dictionary_page = "fenix" + return GLEAN_DICTIONARY.format(page=dictionary_page, probe=telemetry_signature.probe) + + +def get_treeherder_detection_link(detection_range, telemetry_signature): + repo = CHANNEL_TO_REPO_MAPPING.get(telemetry_signature.channel, "mozilla-central") + + return TREEHERDER_PUSH.format(repo=repo, revision=detection_range["detection"].revision) + + +def get_treeherder_detection_range_link(detection_range, telemetry_signature): + repo = CHANNEL_TO_REPO_MAPPING.get(telemetry_signature.channel, "mozilla-central") + + return TREEHERDER_DATES.format( + repo=repo, + from_change=detection_range["from"].revision, + to_change=detection_range["to"].revision, + ) From a675659e2da097477e0724303ba45ccc2110eda8 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:32:34 -0400 Subject: [PATCH 10/23] Add a TelemetryProbe data class to reperesent telemetry probes. --- .../telemetry_alerting/test_probe.py | 297 ++++++++++++++++++ .../telemetry_alerting/probe.py | 108 +++++++ 2 files changed, 405 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_probe.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_probe.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_probe.py new file mode 100644 index 00000000000..b6b061d6f67 --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_probe.py @@ -0,0 +1,297 @@ +from unittest.mock import Mock, patch + +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.probe import ( + TelemetryProbe, + TelemetryProbeValidationError, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + DEFAULT_CHANGE_DETECTION, +) + + +@pytest.fixture +def metric_info_with_monitor_only(base_metric_info): + """Metric info with alert=False and bugzilla_notification_emails.""" + base_metric_info["data"]["monitor"] = { + "alert": False, + "bugzilla_notification_emails": ["email@fake.fake.com"], + } + return base_metric_info + + +@pytest.fixture +def metric_info_with_bool_true(base_metric_info): + """Metric info with monitor as boolean True.""" + base_metric_info["data"]["monitor"] = True + return base_metric_info + + +@pytest.fixture +def metric_info_with_bool_false(base_metric_info): + """Metric info with monitor as boolean False.""" + base_metric_info["data"]["monitor"] = False + return base_metric_info + + +@pytest.fixture +def metric_info_with_empty_dict(base_metric_info): + """Metric info with monitor as empty dictionary.""" + base_metric_info["data"]["monitor"] = {} + return base_metric_info + + +@pytest.fixture +def metric_info_with_none(base_metric_info): + """Metric info with monitor as None.""" + base_metric_info["data"]["monitor"] = None + return base_metric_info + + +class TestTelemetryProbeInitialization: + def test_initialization_with_alert_true(self, metric_info_with_alert): + """Test probe initialization with alert=True.""" + probe = TelemetryProbe(metric_info_with_alert) + + assert probe.name == "networking_http_channel_page_open_to_first_sent" + assert probe.metric_info == metric_info_with_alert + assert probe.monitor_info["detect_changes"] is True + assert probe.monitor_info["alert"] is True + assert probe.monitor_info["bugzilla_notification_emails"] == ["email@fake.fake.com"] + + def test_initialization_with_monitor_only(self, metric_info_with_monitor_only): + """Test probe initialization with alert=False.""" + probe = TelemetryProbe(metric_info_with_monitor_only) + + assert probe.name == "networking_http_channel_page_open_to_first_sent" + assert probe.monitor_info["detect_changes"] is True + assert probe.monitor_info["alert"] is False + + def test_initialization_with_bool_true(self, metric_info_with_bool_true): + """Test probe initialization with monitor as boolean True.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + assert probe.monitor_info["detect_changes"] is True + assert "alert" not in probe.monitor_info + + def test_initialization_with_bool_false(self, metric_info_with_bool_false): + """Test probe initialization with monitor as boolean False.""" + probe = TelemetryProbe(metric_info_with_bool_false) + + assert probe.monitor_info["detect_changes"] is False + + def test_initialization_with_empty_dict(self, metric_info_with_empty_dict): + """Test probe initialization with monitor as empty dictionary.""" + probe = TelemetryProbe(metric_info_with_empty_dict) + + assert probe.monitor_info["detect_changes"] is False + + def test_initialization_with_none(self, metric_info_with_none): + """Test probe initialization with monitor as None.""" + probe = TelemetryProbe(metric_info_with_none) + + assert probe.monitor_info["detect_changes"] is False + + +class TestTelemetryProbeMonitorInfoSetter: + def test_monitor_info_dict_with_detect_changes(self, base_metric_info): + """Test that dict monitor info sets detect_changes to True.""" + base_metric_info["data"]["monitor"] = { + "alert": True, + "bugzilla_notification_emails": ["test@test.com"], + } + probe = TelemetryProbe(base_metric_info) + + assert probe.monitor_info["detect_changes"] is True + assert probe.monitor_info["alert"] is True + + def test_monitor_info_invalid_type_raises_error(self, base_metric_info): + """Test that invalid monitor type raises validation error.""" + base_metric_info["data"]["monitor"] = "invalid_string" + + with pytest.raises(TelemetryProbeValidationError) as exc_info: + TelemetryProbe(base_metric_info) + + assert "must by either a boolean or dictionary" in str(exc_info.value) + assert "networking_http_channel_page_open_to_first_sent" in str(exc_info.value) + + +class TestTelemetryProbeChangeDetection: + def test_get_change_detection_technique_default(self, metric_info_with_bool_true): + """Test default change detection technique.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + assert probe.get_change_detection_technique() == DEFAULT_CHANGE_DETECTION + + def test_get_change_detection_technique_custom(self, base_metric_info): + """Test custom change detection technique.""" + base_metric_info["data"]["monitor"] = { + "alert": True, + "bugzilla_notification_emails": ["test@test.com"], + "change-detection-technique": "custom_technique", + } + probe = TelemetryProbe(base_metric_info) + + assert probe.get_change_detection_technique() == "custom_technique" + + def test_should_detect_changes_true(self, metric_info_with_alert): + """Test should_detect_changes returns True when enabled.""" + probe = TelemetryProbe(metric_info_with_alert) + + assert probe.should_detect_changes() is True + + def test_should_detect_changes_false(self, metric_info_with_bool_false): + """Test should_detect_changes returns False when disabled.""" + probe = TelemetryProbe(metric_info_with_bool_false) + + assert probe.should_detect_changes() is False + + +class TestTelemetryProbeBugAndEmailDecisions: + def test_should_file_bug_true(self, metric_info_with_alert): + """Test should_file_bug returns True when alert=True.""" + probe = TelemetryProbe(metric_info_with_alert) + + assert probe.should_file_bug() is True + + def test_should_file_bug_false(self, metric_info_with_monitor_only): + """Test should_file_bug returns False when alert=False.""" + probe = TelemetryProbe(metric_info_with_monitor_only) + + assert probe.should_file_bug() is False + + def test_should_file_bug_false_when_alert_not_present(self, metric_info_with_bool_true): + """Test should_file_bug returns False when alert field not present.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + assert probe.should_file_bug() is False + + def test_should_email_false_when_alert_true(self, metric_info_with_alert): + """Test should_email returns False when alert=True.""" + probe = TelemetryProbe(metric_info_with_alert) + + assert probe.should_email() is False + + def test_should_email_true_when_alert_false(self, metric_info_with_monitor_only): + """Test should_email returns True when alert=False.""" + probe = TelemetryProbe(metric_info_with_monitor_only) + + assert probe.should_email() is True + + def test_should_email_true_when_alert_not_present(self, metric_info_with_bool_true): + """Test should_email returns True when alert field not present.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + assert probe.should_email() is True + + +class TestTelemetryProbeNotificationEmails: + def test_get_notification_emails_from_bugzilla_field(self, metric_info_with_alert): + """Test getting notification emails from bugzilla_notification_emails.""" + probe = TelemetryProbe(metric_info_with_alert) + + emails = probe.get_notification_emails() + assert emails == ["email@fake.fake.com"] + + def test_get_notification_emails_from_metric_info(self, base_metric_info): + """Test getting notification emails from metric_info notification_emails.""" + base_metric_info["notification_emails"] = ["metric@test.com"] + base_metric_info["data"]["monitor"] = {"notification_emails": ["metric@test.com"]} + + probe = TelemetryProbe(base_metric_info) + emails = probe.get_notification_emails() + + assert emails == ["metric@test.com"] + + @pytest.mark.parametrize( + "mock_response_data,expected_emails", + [ + ({"notification_emails": ["api@mozilla.com"]}, ["api@mozilla.com"]), + ( + {"notification_emails": ["user1@mozilla.com", "user2@mozilla.com"]}, + ["user1@mozilla.com", "user2@mozilla.com"], + ), + ], + ) + def test_get_notification_emails_from_network_request( + self, metric_info_with_bool_true, mock_response_data, expected_emails + ): + """Test notification emails retrieved from network request.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.probe.requests.get" + ) as mock_get: + mock_response = Mock() + mock_response.json.return_value = mock_response_data + mock_response.raise_for_status = Mock() + mock_get.return_value = mock_response + + probe = TelemetryProbe(metric_info_with_bool_true) + emails = probe.get_notification_emails() + + # Verify the mock was called + mock_get.assert_called_once() + assert emails == expected_emails + + def test_get_notification_emails_network_request_failure(self, metric_info_with_bool_true): + """Test default notification email when network request fails.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.probe.requests.get" + ) as mock_get: + mock_get.side_effect = Exception("Network error") + + emails = probe.get_notification_emails() + assert emails == ["gmierzwinski@mozilla.com"] + + def test_get_notification_emails_custom_default(self, metric_info_with_bool_true): + """Test custom default notification email when network request fails.""" + probe = TelemetryProbe(metric_info_with_bool_true) + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.probe.requests.get" + ) as mock_get: + mock_get.side_effect = Exception("Network error") + + emails = probe.get_notification_emails(default="custom@default.com") + assert emails == ["custom@default.com"] + + +class TestTelemetryProbeValidation: + def test_verify_probe_definition_with_alert_true_and_emails(self, metric_info_with_alert): + """Test validation passes with alert=True and bugzilla_notification_emails.""" + probe = TelemetryProbe(metric_info_with_alert) + # Should not raise + probe.verify_probe_definition() + + def test_verify_probe_definition_alert_true_missing_emails_raises_error(self, base_metric_info): + """Test validation fails with alert=True but missing bugzilla_notification_emails.""" + base_metric_info["data"]["monitor"] = {"alert": True} + + with pytest.raises(TelemetryProbeValidationError) as exc_info: + TelemetryProbe(base_metric_info) + + assert "bugzilla_notification_emails" in str(exc_info.value) + assert "must be set to valid Bugzilla account emails" in str(exc_info.value) + + def test_verify_probe_definition_monitor_only(self, metric_info_with_monitor_only): + """Test validation passes with alert=False.""" + probe = TelemetryProbe(metric_info_with_monitor_only) + # Should not raise + probe.verify_probe_definition() + + def test_verify_probe_definition_monitor_without_bugzilla_emails(self, base_metric_info): + """Test validation for monitor-only probe without bugzilla_notification_emails.""" + base_metric_info["data"]["monitor"] = {"alert": False} + # _verify_monitor_probe returns early, so this should not raise + probe = TelemetryProbe(base_metric_info) + probe.verify_probe_definition() + + +class TestTelemetryProbeValidationError: + def test_validation_error_message_format(self): + """Test validation error message formatting.""" + error = TelemetryProbeValidationError("test_probe", "Something went wrong") + + assert "Probe test_probe: Something went wrong" == str(error) diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py new file mode 100644 index 00000000000..08ef1facd40 --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py @@ -0,0 +1,108 @@ +import logging +import traceback + +import requests + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + DEFAULT_ALERT_EMAIL, + DEFAULT_CHANGE_DETECTION, + GLEAN_PROBE_INFO, +) + +logger = logging.getLogger(__name__) + + +class TelemetryProbeValidationError(Exception): + """Raised when a probes information is incorrect, or missing.""" + + def __init__(self, name, message): + super().__init__(f"Probe {name}: {message}") + + +class TelemetryProbe: + def __init__(self, metric_info): + self.metric_info = metric_info + self.name = self.metric_info["name"] + self._should_file_bug = None + self._should_email = None + + self.monitor_info = self.metric_info["data"].get("monitor") + if self.monitor_info["detect_changes"]: + self.verify_probe_definition() + self.should_file_bug() + self.should_email() + + @property + def monitor_info(self): + return self._monitor_info + + @monitor_info.setter + def monitor_info(self, monitor_info): + self._monitor_info = {} + if isinstance(monitor_info, bool): + self._monitor_info["detect_changes"] = monitor_info + elif isinstance(monitor_info, dict) and monitor_info: + self._monitor_info["detect_changes"] = True + self._monitor_info.update(monitor_info) + elif monitor_info is None or (isinstance(monitor_info, dict) and not monitor_info): + self._monitor_info["detect_changes"] = False + else: + raise TelemetryProbeValidationError( + self.name, + f"`monitor` field must by either a boolean or dictionary. " + f"Found: {type(monitor_info)}", + ) + + def get_change_detection_technique(self): + return self.monitor_info.get("change-detection-technique", DEFAULT_CHANGE_DETECTION) + + def should_file_bug(self): + # Only file bugs when alert is set to True + return self.monitor_info.get("alert", False) + + def should_email(self): + # Only produce emails when alert is undefined or set to False + return not self.monitor_info.get("alert", False) + + def should_detect_changes(self): + return self.monitor_info.get("detect_changes", False) + + def get_notification_emails(self, default=DEFAULT_ALERT_EMAIL): + self.setup_notification_emails(default=default) + return self.monitor_info.get( + "bugzilla_notification_emails", self.monitor_info.get("notification_emails", [default]) + ) + + def setup_notification_emails(self, default=DEFAULT_ALERT_EMAIL): + # These emails are only obtained when we have an alert that we need to + # produce an email for. They are also only obtained if notification emails + # aren't already found in some fields. + if self.monitor_info.get("bugzilla_notification_emails", None) or self.monitor_info.get( + "notification_emails", None + ): + return + + try: + url = GLEAN_PROBE_INFO.format(probe_name=self.name) + response = requests.get(url) + response.raise_for_status() + data = response.json() + self.monitor_info["notification_emails"] = data["notification_emails"] + except Exception: + logger.warning( + f"Failed to obtain notification emails for probe {self.name}: " + f"{traceback.format_exc()}" + ) + self.monitor_info["notification_emails"] = [default] + + def verify_probe_definition(self): + if self.monitor_info.get("alert", False) and not self.monitor_info.get( + "bugzilla_notification_emails" + ): + # This probe will produce bugs, so it needs to have the + # bugzilla_notification_emails set + raise TelemetryProbeValidationError( + self.name, + "`bugzilla_notification_emails` must be set to valid Bugzilla account " + "emails when a probe is set to produce alerts.", + ) From 3615424876a568931e8740791d6731df174863f0 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:54:11 -0400 Subject: [PATCH 11/23] Add a concrete TelemetryAlert class to represent telemetry alerts. --- .../telemetry_alerting/test_alert.py | 149 ++++++++++++++++++ .../telemetry_alerting/alert.py | 86 ++++++++++ 2 files changed, 235 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert.py new file mode 100644 index 00000000000..c9a9f2b075e --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert.py @@ -0,0 +1,149 @@ +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + TelemetryAlertBuildError, + TelemetryAlertFactory, +) + + +class TestTelemetryAlert: + def test_initialization( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ): + """Test TelemetryAlert object initialization.""" + alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + assert alert.telemetry_alert == test_telemetry_alert + assert alert.telemetry_alert_summary == test_telemetry_alert_summary + assert alert.telemetry_signature == test_telemetry_signature + assert alert.related_telemetry_alerts is None + assert alert.detection_range is None + assert alert.failed is False + + def test_get_related_alerts_empty(self, telemetry_alert_obj): + """Test getting related alerts when none exist.""" + related_alerts = telemetry_alert_obj.get_related_alerts() + + assert related_alerts.count() == 0 + # Verify it's cached + assert telemetry_alert_obj.related_telemetry_alerts is not None + + def test_get_related_alerts_with_multiple_alerts( + self, telemetry_alert_obj, create_telemetry_signature, create_telemetry_alert + ): + """Test getting related alerts when multiple alerts exist in the same summary.""" + # Create additional alerts in the same summary + sig2 = create_telemetry_signature(probe="memory_ghost_windows") + alert2 = create_telemetry_alert(sig2) + + sig3 = create_telemetry_signature(probe="cycle_collector_time") + alert3 = create_telemetry_alert(sig3) + + related_alerts = telemetry_alert_obj.get_related_alerts() + + assert related_alerts.count() == 2 + alert_ids = [alert.id for alert in related_alerts] + assert alert2.id in alert_ids + assert alert3.id in alert_ids + assert telemetry_alert_obj.telemetry_alert.id not in alert_ids + + def test_get_related_alerts_uses_cache(self, telemetry_alert_obj): + """Test that get_related_alerts uses cached value on subsequent calls.""" + first_call = telemetry_alert_obj.get_related_alerts() + second_call = telemetry_alert_obj.get_related_alerts() + + # QuerySets are not the same object, but the cached attribute should be set + assert telemetry_alert_obj.related_telemetry_alerts is not None + assert list(first_call) == list(second_call) + + def test_get_detection_range(self, telemetry_alert_obj, prev_push, next_push, detection_push): + """Test getting the detection range.""" + detection_range = telemetry_alert_obj.get_detection_range() + + assert detection_range["from"] == prev_push + assert detection_range["to"] == next_push + assert detection_range["detection"] == detection_push + + def test_get_detection_range_uses_cache(self, telemetry_alert_obj): + """Test that get_detection_range uses cached value on subsequent calls.""" + first_call = telemetry_alert_obj.get_detection_range() + second_call = telemetry_alert_obj.get_detection_range() + + assert first_call is second_call + + def test_str_representation(self, telemetry_alert_obj, test_telemetry_alert): + """Test string representation of TelemetryAlert.""" + str_repr = str(telemetry_alert_obj) + + assert "TelemetryAlert" in str_repr + assert f"alertID={test_telemetry_alert.id}" in str_repr + assert f"alertSummaryID={test_telemetry_alert.summary.id}" in str_repr + assert "probe=networking_http_channel_page_open_to_first_sent" in str_repr + assert "platform=Windows" in str_repr + + +class TestTelemetryAlertFactory: + def test_construct_alert_with_all_parameters( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ): + """Test constructing alert with all parameters provided.""" + alert = TelemetryAlertFactory.construct_alert( + telemetry_alert=test_telemetry_alert, + telemetry_alert_summary=test_telemetry_alert_summary, + telemetry_signature=test_telemetry_signature, + ) + + assert isinstance(alert, TelemetryAlert) + assert alert.telemetry_alert == test_telemetry_alert + assert alert.telemetry_alert_summary == test_telemetry_alert_summary + assert alert.telemetry_signature == test_telemetry_signature + + def test_construct_alert_from_row(self, test_telemetry_alert): + """Test constructing alert from a database row only.""" + alert = TelemetryAlertFactory.construct_alert(telemetry_alert=test_telemetry_alert) + + assert isinstance(alert, TelemetryAlert) + assert alert.telemetry_alert == test_telemetry_alert + assert alert.telemetry_alert_summary == test_telemetry_alert.summary + assert alert.telemetry_signature == test_telemetry_alert.series_signature + + def test_construct_alert_with_no_parameters_raises_error(self): + """Test that constructing alert with no parameters raises error.""" + with pytest.raises(TelemetryAlertBuildError) as exc_info: + TelemetryAlertFactory.construct_alert() + + assert "Could not construct TelemetryAlerts from given arguments" in str(exc_info.value) + + def test_construct_alert_with_only_summary_raises_error(self, test_telemetry_alert_summary): + """Test that constructing alert with only summary raises error.""" + with pytest.raises(TelemetryAlertBuildError) as exc_info: + TelemetryAlertFactory.construct_alert( + telemetry_alert_summary=test_telemetry_alert_summary, + ) + + assert "Could not construct TelemetryAlerts from given arguments" in str(exc_info.value) + + def test_build_alert_private_method( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ): + """Test the private _build_alert method.""" + alert = TelemetryAlertFactory._build_alert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + assert isinstance(alert, TelemetryAlert) + assert alert.telemetry_alert == test_telemetry_alert + assert alert.telemetry_alert_summary == test_telemetry_alert_summary + assert alert.telemetry_signature == test_telemetry_signature + + def test_build_alert_from_row_private_method(self, test_telemetry_alert): + """Test the private _build_alert_from_row method.""" + alert = TelemetryAlertFactory._build_alert_from_row(test_telemetry_alert) + + assert isinstance(alert, TelemetryAlert) + assert alert.telemetry_alert == test_telemetry_alert + assert alert.telemetry_alert_summary == test_telemetry_alert.summary + assert alert.telemetry_signature == test_telemetry_alert.series_signature diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert.py new file mode 100644 index 00000000000..cc761c67881 --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert.py @@ -0,0 +1,86 @@ +from treeherder.perf.auto_perf_sheriffing.base_alert_manager import Alert +from treeherder.perf.models import PerformanceTelemetryAlert + + +class TelemetryAlertBuildError(Exception): + """Generating when the TelemetryFactory cannot build the alerts requested.""" + + pass + + +class TelemetryAlert(Alert): + def __init__(self, telemetry_alert, telemetry_alert_summary, telemetry_signature): + super().__init__() + self.telemetry_alert = telemetry_alert + self.telemetry_alert_summary = telemetry_alert_summary + self.telemetry_signature = telemetry_signature + self.related_telemetry_alerts = None + self.detection_range = None + + def get_related_alerts(self): + if self.related_telemetry_alerts: + return self.related_telemetry_alerts + + self.related_telemetry_alerts = PerformanceTelemetryAlert.objects.filter( + summary_id=self.telemetry_alert_summary.id + ).exclude(id=self.telemetry_alert.id) + + return self.related_telemetry_alerts + + def get_detection_range(self): + if self.detection_range: + return self.detection_range + + self.detection_range = { + "from": self.telemetry_alert_summary.prev_push, + "to": self.telemetry_alert_summary.push, + "detection": self.telemetry_alert_summary.original_push, + } + + return self.detection_range + + def __str__(self): + return ( + f"TelemetryAlert" + ) + + +class TelemetryAlertFactory: + """Provides a single interface for building TelemetryAlerts. + + Currently only builds a single alert at a time, but this could be changed + in the future to build a large number of alerts given some input. + """ + + @staticmethod + def _build_alert(telemetry_alert, telemetry_alert_summary, telemetry_signature): + return TelemetryAlert(telemetry_alert, telemetry_alert_summary, telemetry_signature) + + @staticmethod + def _build_alert_from_row(telemetry_alert): + telemetry_signature = telemetry_alert.series_signature + telemetry_alert_summary = telemetry_alert.summary + + return TelemetryAlert(telemetry_alert, telemetry_alert_summary, telemetry_signature) + + @staticmethod + def construct_alert( + telemetry_alert=None, + telemetry_alert_summary=None, + telemetry_signature=None, + ): + if ( + telemetry_alert is not None + and telemetry_alert_summary is not None + and telemetry_signature is not None + ): + return TelemetryAlertFactory._build_alert( + telemetry_alert, telemetry_alert_summary, telemetry_signature + ) + elif telemetry_alert is not None: + return TelemetryAlertFactory._build_alert_from_row(telemetry_alert) + + raise TelemetryAlertBuildError("Could not construct TelemetryAlerts from given arguments") From 4b32afe5c32e46efcfa2db915f62c11acff90d72 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:55:24 -0400 Subject: [PATCH 12/23] Add TelemetryEmailManager for handling telemetry alert emails. --- .../telemetry_alerting/test_email_manager.py | 490 ++++++++++++++++++ .../telemetry_alerting/email_manager.py | 134 +++++ 2 files changed, 624 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py new file mode 100644 index 00000000000..5799d96c2cc --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py @@ -0,0 +1,490 @@ +from unittest.mock import Mock, call, patch + +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.email_manager import ( + TelemetryEmail, + TelemetryEmailContent, + TelemetryEmailManager, + TelemetryEmailWriter, +) + + +class TestTelemetryEmailManagerIntegration: + """Integration tests with Taskcluster mocking.""" + + @patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) + def test_email_manager_initialization_with_taskcluster(self, mock_notify_factory): + """Test that EmailManager initializes with Taskcluster notify client.""" + mock_client = Mock() + mock_notify_factory.return_value = mock_client + + email_manager = TelemetryEmailManager() + + # Verify taskcluster.notify_client_factory was called + mock_notify_factory.assert_called_once() + assert email_manager.notify_client == mock_client + + @patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) + def test_full_email_flow_with_taskcluster_mock( + self, mock_notify_factory, telemetry_alert_obj, mock_probe + ): + """Test the full email flow from manager to Taskcluster notify client.""" + # Setup mock Taskcluster notify client + mock_client = Mock() + mock_email_func = Mock() + mock_client.email = mock_email_func + mock_notify_factory.return_value = mock_client + + # Create email manager + email_manager = TelemetryEmailManager() + + # Execute email_alert + email_manager.email_alert(mock_probe, telemetry_alert_obj) + + # Verify the Taskcluster email function was called + assert mock_email_func.call_count == 1 + + # Verify the email payload + email_payload = mock_email_func.call_args[0][0] + assert email_payload["address"] == "test@mozilla.com" + assert email_payload["subject"] == "Telemetry Alert for Probe test_probe_metric" + assert "MozDetect has detected a telemetry change" in email_payload["content"] + + @patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) + def test_multiple_emails_sent_to_taskcluster( + self, mock_notify_factory, mock_probe, telemetry_alert_obj + ): + """Test that multiple emails are sent to different recipients via Taskcluster.""" + # Setup mock Taskcluster notify client + mock_client = Mock() + mock_email_func = Mock() + mock_client.email = mock_email_func + mock_notify_factory.return_value = mock_client + + # Setup probe with multiple notification emails + mock_probe.name = "multi_email_probe" + mock_probe.get_notification_emails.return_value = [ + "user1@mozilla.com", + "user2@mozilla.com", + "user3@mozilla.com", + ] + + # Create email manager and send alerts + email_manager = TelemetryEmailManager() + email_manager.email_alert(mock_probe, telemetry_alert_obj) + + # Verify Taskcluster email was called 3 times + assert mock_email_func.call_count == 3 + + # Verify each email has the correct recipient + calls = mock_email_func.call_args_list + recipients = [call[0][0]["address"] for call in calls] + assert "user1@mozilla.com" in recipients + assert "user2@mozilla.com" in recipients + assert "user3@mozilla.com" in recipients + + @patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) + def test_taskcluster_notify_client_not_called_during_initialization(self, mock_notify_factory): + """Test that Taskcluster notify client is created during initialization.""" + mock_client = Mock() + mock_notify_factory.return_value = mock_client + + # Create email manager + TelemetryEmailManager() + + # Verify factory was called + mock_notify_factory.assert_called_once() + + @patch( + "treeherder.perf.auto_perf_sheriffing.base_email_manager.taskcluster.notify_client_factory" + ) + def test_email_payload_structure(self, mock_notify_factory, telemetry_alert_obj, mock_probe): + """Test that the email payload has the correct structure for Taskcluster.""" + mock_client = Mock() + mock_email_func = Mock() + mock_client.email = mock_email_func + mock_notify_factory.return_value = mock_client + + email_manager = TelemetryEmailManager() + email_manager.email_alert(mock_probe, telemetry_alert_obj) + + # Get the payload + email_payload = mock_email_func.call_args[0][0] + + # Verify it's a dict with required keys + assert isinstance(email_payload, dict) + assert "address" in email_payload + assert "subject" in email_payload + assert "content" in email_payload + + # Verify the values are strings + assert isinstance(email_payload["address"], str) + assert isinstance(email_payload["subject"], str) + assert isinstance(email_payload["content"], str) + + +class TestTelemetryEmailManager: + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.email_manager.TelemetryEmail") + def test_email_alert_sends_to_all_notification_emails( + self, mock_telemetry_email_class, mock_probe, telemetry_alert_obj + ): + """Test that email_alert sends emails to all notification emails from probe.""" + # Setup mock probe with notification emails + mock_probe.get_notification_emails.return_value = [ + "user1@mozilla.com", + "user2@mozilla.com", + "user3@mozilla.com", + ] + + # Setup mock email instance + mock_email_instance = Mock() + mock_telemetry_email_class.return_value = mock_email_instance + + # Setup email manager + email_manager = TelemetryEmailManager() + mock_email_func = Mock() + email_manager.get_email_func = Mock(return_value=mock_email_func) + + # Execute + email_manager.email_alert(mock_probe, telemetry_alert_obj) + + # Verify TelemetryEmail was initialized with email function + mock_telemetry_email_class.assert_called_once_with(mock_email_func) + + # Verify email was called for each notification address + assert mock_email_instance.email.call_count == 3 + expected_calls = [ + call("user1@mozilla.com", mock_probe, telemetry_alert_obj), + call("user2@mozilla.com", mock_probe, telemetry_alert_obj), + call("user3@mozilla.com", mock_probe, telemetry_alert_obj), + ] + mock_email_instance.email.assert_has_calls(expected_calls) + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.email_manager.TelemetryEmail") + def test_email_alert_with_single_notification_email( + self, mock_telemetry_email_class, mock_probe, telemetry_alert_obj + ): + """Test email_alert with a single notification email.""" + mock_probe.get_notification_emails.return_value = ["single@mozilla.com"] + + mock_email_instance = Mock() + mock_telemetry_email_class.return_value = mock_email_instance + + email_manager = TelemetryEmailManager() + mock_email_func = Mock() + email_manager.get_email_func = Mock(return_value=mock_email_func) + + email_manager.email_alert(mock_probe, telemetry_alert_obj) + + assert mock_email_instance.email.call_count == 1 + mock_email_instance.email.assert_called_once_with( + "single@mozilla.com", mock_probe, telemetry_alert_obj + ) + + def test_get_email_func_returns_notify_client_email(self): + """Test that get_email_func returns the notify_client.email method.""" + email_manager = TelemetryEmailManager() + mock_email_method = Mock() + email_manager.notify_client = Mock() + email_manager.notify_client.email = mock_email_method + + result = email_manager.get_email_func() + + assert result == mock_email_method + + +class TestTelemetryEmail: + def test_initialization(self): + """Test TelemetryEmail initialization.""" + mock_email_func = Mock() + + telemetry_email = TelemetryEmail(mock_email_func) + + assert telemetry_email.email_func == mock_email_func + assert isinstance(telemetry_email.email_writer, TelemetryEmailWriter) + assert telemetry_email.email_client is None + + def test_email_calls_email_func_with_prepared_email(self, telemetry_alert_obj, mock_probe): + """Test that email method calls email_func with prepared email payload.""" + mock_email_func = Mock() + mock_probe.name = "test_probe" + + telemetry_email = TelemetryEmail(mock_email_func) + + # Execute + telemetry_email.email("test@mozilla.com", mock_probe, telemetry_alert_obj) + + # Verify email_func was called once + assert mock_email_func.call_count == 1 + + # Verify the email payload was passed + email_payload = mock_email_func.call_args[0][0] + assert email_payload["address"] == "test@mozilla.com" + assert email_payload["subject"] == "Telemetry Alert for Probe test_probe" + assert "MozDetect has detected a telemetry change" in email_payload["content"] + + def test_set_email_method(self): + """Test _set_email_method sets the email function.""" + mock_func1 = Mock() + mock_func2 = Mock() + + telemetry_email = TelemetryEmail(mock_func1) + assert telemetry_email.email_func == mock_func1 + + telemetry_email._set_email_method(mock_func2) + assert telemetry_email.email_func == mock_func2 + + def test_prepare_email_returns_email_payload(self, telemetry_alert_obj, mock_probe): + """Test _prepare_email returns properly formatted email payload.""" + mock_email_func = Mock() + mock_probe.name = "test_probe" + + telemetry_email = TelemetryEmail(mock_email_func) + + result = telemetry_email._prepare_email("test@mozilla.com", mock_probe, telemetry_alert_obj) + + assert result["address"] == "test@mozilla.com" + assert result["subject"] == "Telemetry Alert for Probe test_probe" + assert result["content"] is not None + + +class TestTelemetryEmailWriter: + def test_prepare_email_creates_complete_email(self, telemetry_alert_obj, mock_probe): + """Test prepare_email creates a complete email with all required fields.""" + mock_probe.name = "test_probe" + + writer = TelemetryEmailWriter() + result = writer.prepare_email("test@mozilla.com", mock_probe, telemetry_alert_obj) + + assert result["address"] == "test@mozilla.com" + assert result["subject"] == "Telemetry Alert for Probe test_probe" + assert "MozDetect has detected a telemetry change" in result["content"] + + def test_write_address_sets_email_address(self): + """Test _write_address sets the email address.""" + writer = TelemetryEmailWriter() + writer._write_address("test@mozilla.com") + + assert writer._email.address == "test@mozilla.com" + + def test_write_subject_includes_probe_name(self, mock_probe): + """Test _write_subject includes the probe name in the subject.""" + mock_probe.name = "memory_total" + + writer = TelemetryEmailWriter() + writer._write_subject(mock_probe) + + assert writer._email.subject == "Telemetry Alert for Probe memory_total" + + def test_write_content_sets_email_content(self, telemetry_alert_obj, mock_probe): + """Test _write_content sets the email content.""" + mock_probe.name = "test_probe" + + writer = TelemetryEmailWriter() + writer._write_content(mock_probe, telemetry_alert_obj) + + assert writer._email.content is not None + assert "MozDetect has detected a telemetry change" in writer._email.content + + +class TestTelemetryEmailContent: + def test_initialization(self): + """Test TelemetryEmailContent initialization.""" + content = TelemetryEmailContent() + + assert content._raw_content is None + + def test_write_email_initializes_content(self, telemetry_alert_obj, mock_probe): + """Test write_email initializes the content.""" + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + + assert content._raw_content is not None + assert TelemetryEmailContent.DESCRIPTION in content._raw_content + assert TelemetryEmailContent.TABLE_HEADERS in content._raw_content + + def test_write_email_includes_probe_information(self, telemetry_alert_obj, mock_probe): + """Test write_email includes probe information in the table.""" + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + + result = str(content) + # Should include channel + assert "Windows" in result # platform from signature + assert "Nightly" in result # channel from signature + + def test_write_email_without_related_alerts(self, telemetry_alert_obj, mock_probe): + """Test write_email without related alerts.""" + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + + result = str(content) + assert "MozDetect has detected a telemetry change" in result + assert TelemetryEmailContent.ADDITIONAL_PROBES not in result + + def test_write_email_with_related_alerts( + self, telemetry_alert_obj, mock_probe, create_telemetry_signature, create_telemetry_alert + ): + """Test write_email with related alerts.""" + # Create a related alert + sig2 = create_telemetry_signature(probe="related_probe") + create_telemetry_alert(sig2) + + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + + result = str(content) + assert "MozDetect has detected a telemetry change" in result + assert TelemetryEmailContent.ADDITIONAL_PROBES in result + assert "related_probe" in result + + def test_initialize_report_intro_only_once(self, telemetry_alert_obj, mock_probe): + """Test _initialize_report_intro only initializes once.""" + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + first_content = content._raw_content + + content._initialize_report_intro() + assert content._raw_content == first_content + + def test_include_probe_adds_table_row(self, telemetry_alert_obj): + """Test _include_probe adds a table row.""" + content = TelemetryEmailContent() + content._initialize_report_intro() + + initial_content = content._raw_content + + detection_range = telemetry_alert_obj.get_detection_range() + content._include_probe( + detection_range, + telemetry_alert_obj.telemetry_signature, + telemetry_alert_obj.telemetry_alert_summary, + ) + + assert len(content._raw_content) > len(initial_content) + assert "networking_http_channel_page_open_to_first_sent" in content._raw_content + + def test_build_table_row_format(self, telemetry_alert_obj): + """Test _build_table_row creates properly formatted markdown table row.""" + content = TelemetryEmailContent() + detection_range = telemetry_alert_obj.get_detection_range() + + row = content._build_table_row( + detection_range, + telemetry_alert_obj.telemetry_signature, + telemetry_alert_obj.telemetry_alert_summary, + ) + + # Should be a markdown table row with pipes + assert row.startswith("|") + assert row.endswith("|") + assert row.count("|") == 6 # 5 columns means 6 pipes + + # Should include key information + assert "Nightly" in row # channel + assert "Windows" in row # platform + assert "networking_http_channel_page_open_to_first_sent" in row # probe name + + # Should include markdown links + assert "[" in row and "]" in row and "(" in row and ")" in row + + def test_build_table_row_includes_dates(self, telemetry_alert_obj): + """Test _build_table_row includes formatted dates.""" + content = TelemetryEmailContent() + detection_range = telemetry_alert_obj.get_detection_range() + + row = content._build_table_row( + detection_range, + telemetry_alert_obj.telemetry_signature, + telemetry_alert_obj.telemetry_alert_summary, + ) + + # Should include dates in YYYY-MM-DD format + assert "2024-01-14" in row # from date + assert "2024-01-16" in row # to date + + def test_build_table_row_includes_links(self, telemetry_alert_obj): + """Test _build_table_row includes proper links.""" + content = TelemetryEmailContent() + detection_range = telemetry_alert_obj.get_detection_range() + + row = content._build_table_row( + detection_range, + telemetry_alert_obj.telemetry_signature, + telemetry_alert_obj.telemetry_alert_summary, + ) + + # Should include Glean Dictionary link + assert "dictionary.telemetry.mozilla.org" in row + + # Should include Treeherder links + assert "treeherder.mozilla.org" in row + + # Should have markdown link format + assert "[Detection Push]" in row + + def test_str_returns_raw_content(self, telemetry_alert_obj, mock_probe): + """Test __str__ returns the raw content.""" + content = TelemetryEmailContent() + + content.write_email(mock_probe, telemetry_alert_obj) + + result = str(content) + assert result == content._raw_content + assert "MozDetect has detected a telemetry change" in result + + def test_str_raises_error_when_no_content(self): + """Test __str__ raises ValueError when no content is set.""" + content = TelemetryEmailContent() + + with pytest.raises(ValueError) as exc_info: + str(content) + + assert "No content set" in str(exc_info.value) + + def test_include_additional_probes_adds_section( + self, telemetry_alert_obj, create_telemetry_signature, create_telemetry_alert + ): + """Test _include_additional_probes adds the additional probes section.""" + # Create related alerts + sig2 = create_telemetry_signature(probe="related_probe_1") + create_telemetry_alert(sig2) + + content = TelemetryEmailContent() + content._initialize_report_intro() + + content._include_additional_probes(telemetry_alert_obj) + + result = str(content) + assert TelemetryEmailContent.ADDITIONAL_PROBES in result + assert "related_probe_1" in result + + def test_content_description_constant(self): + """Test that DESCRIPTION constant is properly defined.""" + assert "MozDetect has detected a telemetry change" in TelemetryEmailContent.DESCRIPTION + assert "---" in TelemetryEmailContent.DESCRIPTION + + def test_table_headers_constant(self): + """Test that TABLE_HEADERS constant is properly defined.""" + headers = TelemetryEmailContent.TABLE_HEADERS + assert "| Channel | Probe | Platform | Date Range | Detection Push |" in headers + assert ":---:" in headers # Markdown center alignment + + def test_additional_probes_constant(self): + """Test that ADDITIONAL_PROBES constant is properly defined.""" + assert "additional probes" in TelemetryEmailContent.ADDITIONAL_PROBES + assert "---" in TelemetryEmailContent.ADDITIONAL_PROBES diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py new file mode 100644 index 00000000000..37accef67ff --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py @@ -0,0 +1,134 @@ +from treeherder.perf.auto_perf_sheriffing.base_email_manager import EmailManager +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + get_glean_dictionary_link, + get_treeherder_detection_link, + get_treeherder_detection_range_link, +) +from treeherder.perf.email import EmailWriter + + +class TelemetryEmailManager(EmailManager): + """Formats and emails alert notifications.""" + + def email_alert(self, probe, alert): + telemetry_email = TelemetryEmail(self.get_email_func()) + + for email in probe.get_notification_emails(): + telemetry_email.email(email, probe, alert) + + +class TelemetryEmail: + """Adapter for the email producers.""" + + def __init__(self, email_func): + self.email_writer = TelemetryEmailWriter() + self.email_client = None + self._set_email_method(email_func) + + def email(self, *args, **kwargs): + # Email through Taskcluster client + self.email_func(self._prepare_email(*args, **kwargs)) + + def _set_email_method(self, func): + self.email_func = func + + def _prepare_email(self, *args, **kwargs): + return self.email_writer.prepare_email(*args, **kwargs) + + +class TelemetryEmailWriter(EmailWriter): + def prepare_email(self, email, probe, alert, **kwargs): + self._write_address(email) + self._write_subject(probe) + self._write_content(probe, alert) + + return self.email + + def _write_address(self, email): + self._email.address = email + + def _write_subject(self, probe): + self._email.subject = f"Telemetry Alert for Probe {probe.name}" + + def _write_content(self, probe, alert): + content = TelemetryEmailContent() + content.write_email(probe, alert) + self._email.content = str(content) + + +class TelemetryEmailContent: + DESCRIPTION = ( + "MozDetect has detected a telemetry change in a probe you are subscribed to:\n---\n" + ) + + TABLE_HEADERS = ( + "| Channel | Probe | Platform | Date Range | Detection Push |\n" + "| :---: | :---: | :---: | :---: | :---: |\n" + ) + + ADDITIONAL_PROBES = ( + "See below for additional probes that alerted at the same time (or near " + "the same push):" + "\n---\n" + ) + + def __init__(self): + self._raw_content = None + + def write_email(self, probe, alert): + self._initialize_report_intro() + self._include_probe( + alert.get_detection_range(), alert.telemetry_signature, alert.telemetry_alert_summary + ) + + if alert.get_related_alerts(): + self._include_additional_probes(alert) + + def _initialize_report_intro(self): + if self._raw_content is None: + self._raw_content = self.DESCRIPTION + self.TABLE_HEADERS + + def _include_probe(self, detection_range, telemetry_signature, telemetry_alert_summary): + new_table_row = self._build_table_row( + detection_range, telemetry_signature, telemetry_alert_summary + ) + self._raw_content += f"{new_table_row}\n" + + def _include_additional_probes(self, alert): + self._raw_content += f"\n{self.ADDITIONAL_PROBES}{self.TABLE_HEADERS}" + for related_alert in alert.get_related_alerts(): + self._include_probe( + alert.get_detection_range(), + related_alert.series_signature, + alert.telemetry_alert_summary, + ) + + def _build_table_row( + self, detection_range, telemetry_signature, telemetry_alert_summary + ) -> str: + # TODO: Have change-detection-technique/mozdetect provide a method for building + # a row. That way we can decouple the information provided to bugzilla + # users from the alerting system. + return ( + "| {channel} | [{probe}]({glean_dictionary_link}) | {platform} " + "| [{date_from} - {date_to}]({treeherder_date_link})" + "| [Detection Push]({treeherder_push_link}) |" + ).format( + channel=telemetry_signature.channel, + probe=telemetry_signature.probe, + platform=telemetry_signature.platform, + date_from=detection_range["from"].time.strftime("%Y-%m-%d"), + date_to=detection_range["to"].time.strftime("%Y-%m-%d"), + treeherder_date_link=get_treeherder_detection_range_link( + detection_range, telemetry_signature + ), + treeherder_push_link=get_treeherder_detection_link( + detection_range, telemetry_signature + ), + glean_dictionary_link=get_glean_dictionary_link(telemetry_signature), + ) + + def __str__(self): + if self._raw_content is None: + raise ValueError("No content set") + return self._raw_content From 1069d9d6a5dbd9222e5bba25c89f8a928d6857b5 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:57:02 -0400 Subject: [PATCH 13/23] Add TelemetryBugManager for handling filing, commenting, and modifying telemetry bugs. --- .../telemetry_alerting/test_bug_manager.py | 380 ++++++++++++++++++ .../telemetry_alerting/bug_manager.py | 162 ++++++++ 2 files changed, 542 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_manager.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_manager.py new file mode 100644 index 00000000000..69293b428ae --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_manager.py @@ -0,0 +1,380 @@ +from datetime import datetime +from unittest.mock import patch + +import pytest +import responses + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager import ( + TelemetryBugContent, + TelemetryBugManager, +) + + +@pytest.fixture +def bug_manager(mock_bugfiler_settings): + """Fixture providing a TelemetryBugManager instance.""" + return TelemetryBugManager() + + +class TestTelemetryBugManager: + """Tests for the TelemetryBugManager class.""" + + def test_initialization(self, bug_manager): + """Test TelemetryBugManager initialization.""" + assert bug_manager.bz_url == "https://bugzilla.mozilla.org/rest/bug" + assert bug_manager.bz_headers == {"Accept": "application/json"} + + @responses.activate + def test_file_bug_success(self, bug_manager, mock_probe, telemetry_alert_obj): + """Test successfully filing a bug.""" + expected_response = {"id": 123456} + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + result = bug_manager.file_bug(mock_probe, telemetry_alert_obj) + + assert result == expected_response + assert len(responses.calls) == 1 + + # Verify the request data + request = responses.calls[0].request + assert request.headers["x-bugzilla-api-key"] == "test-api-key" + + @responses.activate + def test_file_bug_sets_correct_fields(self, bug_manager, mock_probe, telemetry_alert_obj): + """Test that file_bug sets all required fields correctly.""" + expected_response = {"id": 123456} + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + bug_manager.file_bug(mock_probe, telemetry_alert_obj) + + # Verify bug was filed with correct structure + import json + + request_body = json.loads(responses.calls[0].request.body) + + # Check basic fields + assert "summary" in request_body + assert "description" in request_body + assert request_body["product"] == "Testing" + assert request_body["component"] == "Performance" + assert request_body["severity"] == "S4" + assert request_body["priority"] == "P5" + assert request_body["keywords"] == "telemetry-alert,regression" + + # Check needinfo flag + assert "flags" in request_body + assert len(request_body["flags"]) == 1 + assert request_body["flags"][0]["name"] == "needinfo" + assert request_body["flags"][0]["status"] == "?" + assert request_body["flags"][0]["requestee"] == "test@mozilla.com" + + @responses.activate + def test_file_bug_includes_bug_content(self, bug_manager, mock_probe, telemetry_alert_obj): + """Test that file_bug includes bug content from TelemetryBugContent.""" + expected_response = {"id": 123456} + responses.add( + responses.POST, + "https://bugzilla.mozilla.org/rest/bug", + json=expected_response, + status=200, + ) + + with patch.object(TelemetryBugContent, "build_bug_content") as mock_build: + mock_build.return_value = { + "title": "Test Alert Title", + "description": "Test alert description", + } + + bug_manager.file_bug(mock_probe, telemetry_alert_obj) + + # Verify build_bug_content was called + mock_build.assert_called_once_with(telemetry_alert_obj) + + @responses.activate + def test_modify_bug_success(self, bug_manager): + """Test successfully modifying a bug.""" + expected_response = {"bugs": [{"id": 123456, "changes": {}}]} + responses.add( + responses.PUT, + "https://bugzilla.mozilla.org/rest/bug/123456", + json=expected_response, + status=200, + ) + + bug_manager.modify_bug(123456, {"comment": {"body": "Test comment"}}) + + assert len(responses.calls) == 1 + request = responses.calls[0].request + assert request.headers["x-bugzilla-api-key"] == "test-commenter-key" + + @responses.activate + def test_modify_bug_sends_correct_changes(self, bug_manager): + """Test that modify_bug sends the correct change data.""" + expected_response = {"bugs": [{"id": 999, "changes": {}}]} + responses.add( + responses.PUT, + "https://bugzilla.mozilla.org/rest/bug/999", + json=expected_response, + status=200, + ) + + changes = {"comment": {"body": "Additional information"}} + bug_manager.modify_bug(999, changes) + + import json + + request_body = json.loads(responses.calls[0].request.body) + assert request_body == changes + + def test_comment_bug_not_implemented(self, bug_manager, telemetry_alert_obj): + """Test that comment_bug is not implemented yet.""" + result = bug_manager.comment_bug(telemetry_alert_obj) + assert result is None + + +class TestTelemetryBugContent: + """Tests for the TelemetryBugContent class.""" + + @pytest.fixture + def bug_content(self): + """Fixture providing a TelemetryBugContent instance.""" + return TelemetryBugContent() + + def test_build_bug_content_returns_title_and_description( + self, bug_content, telemetry_alert_obj + ): + """Test that build_bug_content returns title and description.""" + result = bug_content.build_bug_content(telemetry_alert_obj) + + assert "title" in result + assert "description" in result + assert isinstance(result["title"], str) + assert isinstance(result["description"], str) + + def test_build_bug_content_title_format(self, bug_content, telemetry_alert_obj): + """Test that the bug title is formatted correctly.""" + result = bug_content.build_bug_content(telemetry_alert_obj) + + assert "Telemetry Alert for" in result["title"] + assert "networking_http_channel_page_open_to_first_sent" in result["title"] + assert "2024-01-15" in result["title"] + + def test_build_bug_content_description_includes_required_elements( + self, bug_content, telemetry_alert_obj + ): + """Test that the bug description includes all required elements.""" + result = bug_content.build_bug_content(telemetry_alert_obj) + + description = result["description"] + + # Check for key elements in the description + assert "MozDetect has detected changes" in description + assert "2024-01-15" in description + assert "Treeherder pushes" in description or "detection_push_link" in description + assert "push log" in description or "push_log_link" in description + + def test_build_bug_content_description_includes_change_table( + self, bug_content, telemetry_alert_obj + ): + """Test that the bug description includes the change table.""" + result = bug_content.build_bug_content(telemetry_alert_obj) + + description = result["description"] + + # Check for table elements + assert "Probe" in description + assert "Platform" in description + assert "Magnitude" in description + assert "Previous Values" in description + assert "New Values" in description + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager.datetime") + def test_build_bug_content_uses_current_date_for_bugzilla_query( + self, mock_datetime, bug_content, telemetry_alert_obj + ): + """Test that build_bug_content uses current date for the Bugzilla query link.""" + mock_now = datetime(2024, 3, 15, 10, 30, 0) + mock_datetime.now.return_value = mock_now + + result = bug_content.build_bug_content(telemetry_alert_obj) + + # The description should include a bugzilla query link with today's date + assert "2024-03-15" in result["description"] + + def test_build_bug_content_handles_regression_alerts(self, bug_content, telemetry_alert_obj): + """Test that regression alerts are labeled correctly.""" + telemetry_alert_obj.telemetry_alert.is_regression = True + + result = bug_content.build_bug_content(telemetry_alert_obj) + + description = result["description"] + assert "Regressions" in description or "### Regressions" in description + + def test_build_bug_content_handles_improvement_alerts(self, bug_content, telemetry_alert_obj): + """Test that improvement alerts are labeled correctly.""" + telemetry_alert_obj.telemetry_alert.is_regression = False + + result = bug_content.build_bug_content(telemetry_alert_obj) + + description = result["description"] + # Should show generic title for non-regressions + assert "Changes Detected" in description or "### Changes Detected" in description + + def test_build_comment_content_not_implemented(self, bug_content, telemetry_alert_obj): + """Test that build_comment_content is not yet implemented.""" + result = bug_content.build_comment_content(telemetry_alert_obj) + + # Currently returns None as it's not implemented + assert result is None + + def test_build_change_table_includes_probe_info(self, bug_content, telemetry_alert_obj): + """Test that _build_change_table includes probe information.""" + result = bug_content._build_change_table(telemetry_alert_obj) + + # Should include probe name + assert "networking_http_channel_page_open_to_first_sent" in result + # Should include platform + assert "Windows" in result + + def test_build_change_table_includes_values(self, bug_content, telemetry_alert_obj): + """Test that _build_change_table includes metric values.""" + result = bug_content._build_change_table(telemetry_alert_obj) + + # Should include median values + assert "650.0" in result # prev_median + assert "750.0" in result # new_median + + # Should include P90 values + assert "700.0" in result # prev_p90 + assert "800.0" in result # new_p90 + + # Should include P95 values + assert "720.0" in result # prev_p95 + assert "820.0" in result # new_p95 + + def test_build_probe_alert_row_formats_correctly(self, bug_content, telemetry_alert_obj): + """Test that _build_probe_alert_row formats the row correctly.""" + result = bug_content._build_probe_alert_row(telemetry_alert_obj) + + # Check markdown table format + assert "|" in result + assert "Median:" in result + assert "P90:" in result + assert "P95:" in result + + def test_build_probe_alert_row_includes_glean_dictionary_link( + self, bug_content, telemetry_alert_obj + ): + """Test that _build_probe_alert_row includes Glean Dictionary link.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager.get_glean_dictionary_link" + ) as mock_link: + mock_link.return_value = "https://dictionary.telemetry.mozilla.org/test" + + result = bug_content._build_probe_alert_row(telemetry_alert_obj) + + mock_link.assert_called_once_with(telemetry_alert_obj.telemetry_signature) + assert "https://dictionary.telemetry.mozilla.org/test" in result + + def test_build_probe_alert_row_includes_confidence(self, bug_content, telemetry_alert_obj): + """Test that _build_probe_alert_row includes confidence value.""" + telemetry_alert_obj.telemetry_alert.confidence = 0.95 + + result = bug_content._build_probe_alert_row(telemetry_alert_obj) + + assert "0.95" in result + + def test_build_bug_content_calculates_date_range_correctly( + self, bug_content, telemetry_alert_obj + ): + """Test that build_bug_content calculates the detection date range correctly.""" + result = bug_content.build_bug_content(telemetry_alert_obj) + + description = result["description"] + + # Should include start date (prev_push date) + assert "2024-01-14" in description + + # Should include end date + 1 day (next_push date + 1) + assert "2024-01-17" in description + + @patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager.get_treeherder_detection_link" + ) + @patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager.get_treeherder_detection_range_link" + ) + def test_build_bug_content_calls_link_builders( + self, mock_range_link, mock_detection_link, bug_content, telemetry_alert_obj + ): + """Test that build_bug_content calls the link builder functions.""" + mock_detection_link.return_value = "https://treeherder.mozilla.org/detection" + mock_range_link.return_value = "https://treeherder.mozilla.org/range" + + result = bug_content.build_bug_content(telemetry_alert_obj) + + # Verify the link builders were called + assert mock_detection_link.called + assert mock_range_link.called + + # Verify links are included in description + assert "https://treeherder.mozilla.org/detection" in result["description"] + assert "https://treeherder.mozilla.org/range" in result["description"] + + def test_table_headers_constant(self, bug_content): + """Test that TABLE_HEADERS constant is formatted correctly.""" + assert "Probe" in bug_content.TABLE_HEADERS + assert "Platform" in bug_content.TABLE_HEADERS + assert "Magnitude" in bug_content.TABLE_HEADERS + assert "Previous Values" in bug_content.TABLE_HEADERS + assert "New Values" in bug_content.TABLE_HEADERS + assert "|" in bug_content.TABLE_HEADERS + + def test_bug_title_constant_format(self, bug_content): + """Test that BUG_TITLE constant has correct format placeholders.""" + assert "{probe}" in bug_content.BUG_TITLE + assert "{date}" in bug_content.BUG_TITLE + + def test_bug_description_constant_format(self, bug_content): + """Test that BUG_DESCRIPTION constant has correct format placeholders.""" + assert "{date}" in bug_content.BUG_DESCRIPTION + assert "{detection_push_link}" in bug_content.BUG_DESCRIPTION + assert "{change_table}" in bug_content.BUG_DESCRIPTION + assert "{detection_range_link}" in bug_content.BUG_DESCRIPTION + assert "{push_log_link}" in bug_content.BUG_DESCRIPTION + assert "{bz_telemetry_alerts}" in bug_content.BUG_DESCRIPTION + + def test_bug_comment_constant_format(self, bug_content): + """Test that BUG_COMMENT constant has correct format placeholders.""" + assert "{change_table}" in bug_content.BUG_COMMENT + assert "additional probes" in bug_content.BUG_COMMENT + + def test_build_change_table_uses_correct_title_for_regression( + self, bug_content, telemetry_alert_obj + ): + """Test that _build_change_table uses 'Regressions' title for regressions.""" + telemetry_alert_obj.telemetry_alert.is_regression = True + + result = bug_content._build_change_table(telemetry_alert_obj) + + assert "### Regressions" in result + + def test_build_change_table_uses_generic_title_for_non_regression( + self, bug_content, telemetry_alert_obj + ): + """Test that _build_change_table uses generic title for non-regressions.""" + telemetry_alert_obj.telemetry_alert.is_regression = False + + result = bug_content._build_change_table(telemetry_alert_obj) + + assert "### Changes Detected" in result diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py new file mode 100644 index 00000000000..a9c9e253403 --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py @@ -0,0 +1,162 @@ +import logging +from datetime import datetime, timedelta + +from treeherder.perf.auto_perf_sheriffing.base_bug_manager import BugManager +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + BZ_TELEMETRY_ALERTS, + PUSH_LOG, + get_glean_dictionary_link, + get_treeherder_detection_link, + get_treeherder_detection_range_link, +) + +logger = logging.getLogger(__name__) + + +class TelemetryBugManager(BugManager): + """Files bugs, and comments on them for telemetry alerts.""" + + def __init__(self): + super().__init__() + + def file_bug(self, probe, alert): + logger.info(f"Filing bug for alert on probe {probe.name}") + bug_data = self._get_default_bug_creation_data() + bug_content = TelemetryBugContent().build_bug_content(alert) + + bug_data["summary"] = bug_content["title"] + bug_data["description"] = bug_content["description"] + + # For testing use Testing :: Performance, later switch to + # using tags from metrics_info + bug_data["product"] = "Testing" + bug_data["component"] = "Performance" + + bug_data["severity"] = "S4" + bug_data["priority"] = "P5" + bug_data["keywords"] = "telemetry-alert,regression" + + # Only set a needinfo on the first email in the notification list + needinfo_emails = probe.get_notification_emails() + self._add_needinfo(needinfo_emails[0], bug_data) + + bug_info = self._create(bug_data) + logger.info(f"Filed bug {bug_info['id']}") + + return bug_info + + def modify_bug(self, bug, changes): + logger.info(f"Making modifications to bug {bug}") + resp = self._modify(bug, changes) + print(resp) + logger.info(f"Finished modifying bug {bug}") + + def comment_bug(self, alert): + pass + + +class TelemetryBugContent: + """Formats the content of a bug. + + Used for producing the first comment of a bug (description, or comment #0), + and for producing comments after an initial bug is created. + """ + + BUG_DESCRIPTION = ( + "MozDetect has detected changes in the following telemetry probes on " + "builds from [{date}]({detection_push_link}). As the probe owner of the " + "following probes, we need your help to address this regression and " + "find a culprit." + "\n\n{change_table}\n" + "[See these Treeherder pushes for possible culprits for this detected change]" + "({detection_range_link}).\n\n" + "[A push log can be found here for a quicker overview of the changes that" + "occurred around this change]({push_log_link}).\n\n" + "For more information on how to handle these probe changes, and " + "what the various columns mean [see here]" + "(https://firefox-source-docs.mozilla.org/testing/perfdocs/telemetry-alerting.html).\n\n" + "Note that it’s possible the culprit is from a commit in the day before, " + "or the day after these push logs. It’s also possible that these culprits " + "are not the cause, and the change could be coming from a popular " + "website. Please reach out to the Performance team if you suspect this to be " + "the case in [#perf on Matrix](https://matrix.to/#/#perf:mozilla.org), or " + "[#perf-help on Slack](https://mozilla.enterprise.slack.com/archives/C03U19JCSFQ)." + "\n\n" + "[See this bugzilla query for other telemetry alerts that were " + "produced on this date]({bz_telemetry_alerts})." + ) + + BUG_COMMENT = ( + "MozDetect has detected changes in additional probes on the same date " + "which may be related to the changes the bug was originally filed for." + "\n\n{change_table}" + ) + + TABLE_HEADERS = ( + "| **Probe** | **Platform** | **Magnitude** " + "| **Previous Values** | **New Values** |\n" + "| :---: | :---: | :---: | :---: | :---: |\n" + ) + + REGRESSION_TITLE = "### Regressions\n" + IMPROVMENT_TITLE = "### Improvement\n" + GENERIC_TITLE = "### Changes Detected\n" + + BUG_TITLE = "Telemetry Alert for {probe} on {date}" + + def build_bug_content(self, alert): + bug_content = {"title": "", "description": ""} + + detection_range = alert.get_detection_range() + detection_date = detection_range["detection"].time.strftime("%Y-%m-%d") + + # End date is exclusive so we need to add 1 day to it + start_date = detection_range["from"].time.strftime("%Y-%m-%d") + end_date = (detection_range["to"].time + timedelta(days=1)).strftime("%Y-%m-%d") + + bug_content["title"] = self.BUG_TITLE.format( + probe=alert.telemetry_signature.probe, date=detection_date + ) + + bug_content["description"] = self.BUG_DESCRIPTION.format( + date=detection_date, + detection_push_link=get_treeherder_detection_link( + detection_range, alert.telemetry_signature + ), + change_table=self._build_change_table(alert), + detection_range_link=get_treeherder_detection_range_link( + detection_range, alert.telemetry_signature + ), + push_log_link=PUSH_LOG.format(start_date=start_date, end_date=end_date), + bz_telemetry_alerts=BZ_TELEMETRY_ALERTS.format( + today=datetime.now().strftime("%Y-%m-%d"), + prev_day=(datetime.now() - timedelta(days=1)).strftime("%Y-%m-%d"), + ), + ) + + return bug_content + + def build_comment_content(self, alert): + pass + + def _build_change_table(self, alert): + change_table = self.GENERIC_TITLE + + if alert.telemetry_alert.is_regression: + change_table = self.REGRESSION_TITLE + + change_table += self.TABLE_HEADERS + self._build_probe_alert_row(alert) + + return change_table + + def _build_probe_alert_row(self, alert): + # TODO: Have change-detection-technique/mozdetect provide a method for building + # a row. That way we can decouple the information provided to bugzilla + # users from the alerting system. + values = ( + f"| **Median:** {round(alert.telemetry_alert.prev_median, 2)} | **Median:** {round(alert.telemetry_alert.new_median, 2)} |\n" + f"| | | | **P90:** {round(alert.telemetry_alert.prev_p90, 2)} | **P90:** {round(alert.telemetry_alert.new_p90, 2)} |\n" + f"| | | | **P95:** {round(alert.telemetry_alert.prev_p95, 2)} | **P95:** {round(alert.telemetry_alert.new_p95, 2)} |" + ) + + return f"| [{alert.telemetry_signature.probe}]({get_glean_dictionary_link(alert.telemetry_signature)}) | {alert.telemetry_signature.platform} | {alert.telemetry_alert.confidence} {values} \n" From 9bea0e6dc2a2b37ac8fb8da8a1e608d090f72ce0 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:58:38 -0400 Subject: [PATCH 14/23] Add TelemetryAlertModifier to handle mirroring bug updates into the DB. --- .../telemetry_alerting/test_alert_modifier.py | 348 ++++++++++++++++++ .../telemetry_alerting/alert_modifier.py | 134 +++++++ 2 files changed, 482 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py new file mode 100644 index 00000000000..5fa3fb3d4c7 --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py @@ -0,0 +1,348 @@ +from datetime import datetime, timedelta +from unittest.mock import Mock, patch + +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier import ( + TelemetryAlertModifier, +) +from treeherder.perf.models import PerformanceTelemetryAlert + + +class TestTelemetryAlertModifier: + def test_add_updater(self): + """Test adding an updater to the updaters list.""" + initial_count = len(TelemetryAlertModifier.get_updaters()) + + class DummyUpdater: + pass + + TelemetryAlertModifier.add(DummyUpdater) + + assert len(TelemetryAlertModifier.get_updaters()) == initial_count + 1 + assert DummyUpdater in TelemetryAlertModifier.get_updaters() + + # Clean up + TelemetryAlertModifier.updaters.remove(DummyUpdater) + + def test_get_updaters(self): + """Test getting the list of updaters.""" + updaters = TelemetryAlertModifier.get_updaters() + assert isinstance(updaters, list) + # ResolutionModifier should be registered (check by class name since decorator makes it None) + assert any(u.__name__ == "ResolutionModifier" for u in updaters) + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_get_alert_updates_empty(self, mock_bug_searcher_class): + """Test get_alert_updates when updaters return no updates.""" + # Mock BugSearcher to return no bugs + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": []} + mock_bug_searcher_class.return_value = mock_searcher + + alerts = [] + updates = TelemetryAlertModifier.get_alert_updates(alerts) + + assert updates == {} + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_get_alert_updates_with_data(self, mock_bug_searcher_class, test_telemetry_alert): + """Test get_alert_updates with actual updates from updaters.""" + # Mock BugSearcher to return bugs with resolutions + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": [{"id": 12345, "resolution": "FIXED"}]} + mock_bug_searcher_class.return_value = mock_searcher + + # Update the alert with a bug number + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + alerts = [test_telemetry_alert] + updates = TelemetryAlertModifier.get_alert_updates(alerts) + + assert str(test_telemetry_alert.id) in updates + assert "status" in updates[str(test_telemetry_alert.id)] + assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED + + @patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.MODIFIABLE_ALERT_FIELDS", + ("status", "test_field"), + ) + def test_get_alert_updates_with_two_modifiers_different_fields(self, test_telemetry_alert): + """Test get_alert_updates with two modifiers updating different fields for the same alert.""" + + class StatusModifier: + @staticmethod + def update_alerts(**kwargs): + return {str(test_telemetry_alert.id): {"status": 1}} + + class TestFieldModifier: + @staticmethod + def update_alerts(**kwargs): + return {str(test_telemetry_alert.id): {"test_field": "test_value"}} + + # Save original updaters + original_updaters = TelemetryAlertModifier.updaters.copy() + + # Replace with our test updaters + TelemetryAlertModifier.updaters = [StatusModifier, TestFieldModifier] + + try: + updates = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) + + # Both modifiers should contribute their updates + assert str(test_telemetry_alert.id) in updates + assert updates[str(test_telemetry_alert.id)]["status"] == 1 + assert updates[str(test_telemetry_alert.id)]["test_field"] == "test_value" + finally: + # Restore original updaters + TelemetryAlertModifier.updaters = original_updaters + + def test_get_alert_updates_with_two_modifiers_same_field(self, test_telemetry_alert, caplog): + """Test get_alert_updates with two modifiers trying to update the same field.""" + + class FirstStatusModifier: + @staticmethod + def update_alerts(**kwargs): + return {str(test_telemetry_alert.id): {"status": 1}} + + class SecondStatusModifier: + @staticmethod + def update_alerts(**kwargs): + return {str(test_telemetry_alert.id): {"status": 2}} + + # Save original updaters + original_updaters = TelemetryAlertModifier.updaters.copy() + + # Replace with our test updaters + TelemetryAlertModifier.updaters = [FirstStatusModifier, SecondStatusModifier] + + try: + updates = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) + + # First modifier's value should win, second should be logged as warning + assert str(test_telemetry_alert.id) in updates + assert updates[str(test_telemetry_alert.id)]["status"] == 1 + assert ( + f"Multiple modifications found for alert ID {test_telemetry_alert.id}" + in caplog.text + ) + assert "status" in caplog.text + finally: + # Restore original updaters + TelemetryAlertModifier.updaters = original_updaters + + def test_merge_updates_single_updater(self): + """Test merging updates from a single updater.""" + all_updates = {"1": [{"status": 1}], "2": [{"status": 2}]} + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + assert merged == {"1": {"status": 1}, "2": {"status": 2}} + + def test_merge_updates_multiple_updaters_no_conflict(self): + """Test merging updates from multiple updaters with no conflicts.""" + all_updates = { + "1": [{"status": 1}], + "2": [{"status": 2}], + } + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + assert merged == {"1": {"status": 1}, "2": {"status": 2}} + + def test_merge_updates_with_field_conflict(self, caplog): + """Test merging updates when multiple updaters try to modify the same field.""" + all_updates = {"1": [{"status": 1}, {"status": 2}]} + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + # First update should be kept, second should be ignored with warning + assert merged == {"1": {"status": 1}} + assert "Multiple modifications found for alert ID 1" in caplog.text + + def test_merge_updates_with_invalid_field(self, caplog): + """Test merging updates with a non-modifiable field.""" + all_updates = {"1": [{"invalid_field": "value"}]} + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + # Update should be ignored with warning + assert merged == {"1": {}} + assert "Model field invalid_field is not set as a modifiable field" in caplog.text + + def test_merge_updates_mixed_valid_invalid_fields(self, caplog): + """Test merging updates with both valid and invalid fields.""" + all_updates = {"1": [{"status": 1, "invalid_field": "value"}]} + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + # Only valid field should be kept + assert merged == {"1": {"status": 1}} + assert "Model field invalid_field is not set as a modifiable field" in caplog.text + + def test_merge_updates_empty(self): + """Test merging with no updates.""" + all_updates = {} + + merged = TelemetryAlertModifier._merge_updates(all_updates) + + assert merged == {} + + +class TestResolutionModifier: + @pytest.fixture + def resolution_modifier_class(self): + """Get the ResolutionModifier class from the updaters list.""" + # Find ResolutionModifier in the updaters list since the decorator makes it None + for updater in TelemetryAlertModifier.get_updaters(): + if updater.__name__ == "ResolutionModifier": + return updater + pytest.fail("ResolutionModifier not found in updaters list") + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_no_bugs(self, mock_bug_searcher_class, resolution_modifier_class): + """Test update_alerts when no bugs are found.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": []} + mock_bug_searcher_class.return_value = mock_searcher + + updates = resolution_modifier_class.update_alerts() + + assert updates == {} + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_with_fixed_bug( + self, mock_bug_searcher_class, resolution_modifier_class, test_telemetry_alert + ): + """Test update_alerts when bugs have FIXED resolution.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": [{"id": 12345, "resolution": "FIXED"}]} + mock_bug_searcher_class.return_value = mock_searcher + + # Set bug number on alert + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + updates = resolution_modifier_class.update_alerts() + + assert str(test_telemetry_alert.id) in updates + assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_with_multiple_bugs( + self, + mock_bug_searcher_class, + resolution_modifier_class, + test_telemetry_alert, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test update_alerts with multiple bugs and alerts.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = { + "bugs": [ + {"id": 12345, "resolution": "FIXED"}, + {"id": 67890, "resolution": "INVALID"}, + ] + } + mock_bug_searcher_class.return_value = mock_searcher + + # Create another alert with a different bug + sig2 = create_telemetry_signature(probe="test_probe2") + alert2 = create_telemetry_alert(sig2, bug_number=67890) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + updates = resolution_modifier_class.update_alerts() + + assert str(test_telemetry_alert.id) in updates + assert str(alert2.id) in updates + assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED + assert updates[str(alert2.id)]["status"] == PerformanceTelemetryAlert.INVALID + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_bug_not_matching_alert( + self, mock_bug_searcher_class, resolution_modifier_class, test_telemetry_alert + ): + """Test update_alerts when bugs don't match any alerts.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": [{"id": 99999, "resolution": "FIXED"}]} + mock_bug_searcher_class.return_value = mock_searcher + + # Alert has a different bug number + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + updates = resolution_modifier_class.update_alerts() + + assert updates == {} + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_unknown_resolution( + self, mock_bug_searcher_class, resolution_modifier_class, test_telemetry_alert + ): + """Test update_alerts when bug has an unknown resolution.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.return_value = {"bugs": [{"id": 12345, "resolution": "UNKNOWN"}]} + mock_bug_searcher_class.return_value = mock_searcher + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + updates = resolution_modifier_class.update_alerts() + + # Should not update if resolution is not recognized + assert updates == {} + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_exception_handling( + self, mock_bug_searcher_class, resolution_modifier_class, caplog + ): + """Test update_alerts handles exceptions gracefully.""" + mock_searcher = Mock() + mock_searcher.get_today_date.return_value = datetime.now().date() + mock_searcher.get_bugs.side_effect = Exception("API Error") + mock_bug_searcher_class.return_value = mock_searcher + + updates = resolution_modifier_class.update_alerts() + + assert updates is None + assert "Failed to get bugs for alert resolution updates" in caplog.text + assert "API Error" in caplog.text + + @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") + def test_update_alerts_sets_correct_query( + self, mock_bug_searcher_class, resolution_modifier_class + ): + """Test that update_alerts sets the correct bugzilla query.""" + mock_searcher = Mock() + today = datetime.now().date() + mock_searcher.get_today_date.return_value = today + mock_searcher.get_bugs.return_value = {"bugs": []} + mock_bug_searcher_class.return_value = mock_searcher + + resolution_modifier_class.update_alerts() + + # Verify set_include_fields was called + mock_searcher.set_include_fields.assert_called_once_with(["id", "resolution"]) + + # Verify set_query was called with correct parameters + expected_start_date = today - timedelta(7) + call_args = mock_searcher.set_query.call_args[0][0] + + assert call_args["f1"] == "keywords" + assert call_args["o1"] == "anywords" + assert call_args["v1"] == "telemetry-alert" + assert call_args["f2"] == "resolution" + assert call_args["f4"] == "resolution" + assert call_args["o4"] == "changedafter" + assert call_args["v4"] == expected_start_date diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py new file mode 100644 index 00000000000..7826e7f0ecc --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py @@ -0,0 +1,134 @@ +import logging +from datetime import timedelta + +from treeherder.perf.auto_perf_sheriffing.bug_searcher import BugSearcher +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + MODIFIABLE_ALERT_FIELDS, +) +from treeherder.perf.models import PerformanceTelemetryAlert + +logger = logging.getLogger(__name__) + + +class TelemetryAlertModifier: + updaters = [] + + @staticmethod + def add(updater_class): + TelemetryAlertModifier.updaters.append(updater_class) + + @staticmethod + def get_updaters(): + return TelemetryAlertModifier.updaters + + @staticmethod + def get_alert_updates(alerts, *args, **kwargs): + """Runs all the updaters to get alert updates. + + No guarantees on the ordering of the updaters running are made. They + should each be responsible for updating only 1 specific thing, e.g. + one updater for the resolution, etc.. + + Alert updates are gathered and a batch update is performed to update all + the alerts in the DB at the same time. + + Alert updaters are expected to produce a dictionary in the following format: + { + "alert-id": { + "change-field": , + "change-field2": , + ... + }, + "alert-id2": { + "change-field": , + ... + }, + ... + } + + Warnings will be raised when an updater is found to be modify a field that + was already modified. The merge will continue but it will ignore the additional + change to ensure that other alert updates can still be made. + """ + all_updates = {} + for updater in TelemetryAlertModifier.get_updaters(): + updates = updater.update_alerts(**kwargs) + if not updates: + continue + for alert, alert_updates in updates.items(): + all_updates.setdefault(str(alert), []).append(alert_updates) + return TelemetryAlertModifier._merge_updates(all_updates) + + @staticmethod + def _merge_updates(all_updates): + """Merges all the updates that will be made by updaters. + + No field modifications can be combined so warnings will be raised on any alerts + that have the same field being modified. + """ + merged_updates = {} + for alert, updates in all_updates.items(): + alert_updates = merged_updates.setdefault(alert, {}) + for update in updates: + for field, value in update.items(): + if field in alert_updates: + logger.warning( + f"Multiple modifications found for alert ID {alert} field " + f"{field}. Values found: {value}, and {alert_updates[field]}" + ) + continue + if field not in MODIFIABLE_ALERT_FIELDS: + logger.warning(f"Model field {field} is not set as a modifiable field.") + continue + alert_updates[field] = value + return merged_updates + + +@TelemetryAlertModifier.add +class ResolutionModifier: + @staticmethod + def update_alerts(**kwargs): + """Get resolution updates for existing alerts in the DB.""" + bug_searcher = BugSearcher() + + start_date = bug_searcher.get_today_date() - timedelta(7) + + bug_searcher.set_include_fields(["id", "resolution"]) + bug_searcher.set_query( + { + "f1": "keywords", + "o1": "anywords", + "v1": "telemetry-alert", + "f2": "resolution", + "f4": "resolution", + "o4": "changedafter", + "v4": start_date, + } + ) + + try: + bugs = bug_searcher.get_bugs() + except Exception as e: + logger.warning(f"Failed to get bugs for alert resolution updates: {str(e)}") + return + + alerts_to_update = PerformanceTelemetryAlert.objects.filter( + bug_number__in=[bug_info["id"] for bug_info in bugs["bugs"]] + ) + + def __get_alert_status(bug_number): + for bug in bugs.get("bugs", []): + if bug["id"] != bug_number: + continue + status = bug["resolution"] + for status_index, status_choice in PerformanceTelemetryAlert.STATUSES: + if status == status_choice: + return status_index + + updates = {} + for alert in alerts_to_update: + bug_status = __get_alert_status(alert.bug_number) + if bug_status: + updates[str(alert.id)] = {"status": bug_status} + + return updates From ea574db74cdb1b3ac62e4a7ca75f2de77c7ef979 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 22:59:25 -0400 Subject: [PATCH 15/23] Add TelemetryBugModifier to handle telemetry bug modifications. --- .../telemetry_alerting/test_bug_modifier.py | 515 ++++++++++++++++++ .../telemetry_alerting/bug_modifier.py | 127 +++++ 2 files changed, 642 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_modifier.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_modifier.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_modifier.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_modifier.py new file mode 100644 index 00000000000..f62ed6944d0 --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_bug_modifier.py @@ -0,0 +1,515 @@ +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_modifier import ( + TelemetryBugModifier, +) + + +class TestTelemetryBugModifier: + def test_add_modifier(self): + """Test adding a modifier to the modifiers list.""" + initial_count = len(TelemetryBugModifier.get_modifiers()) + + class DummyModifier: + pass + + TelemetryBugModifier.add(DummyModifier) + + assert len(TelemetryBugModifier.get_modifiers()) == initial_count + 1 + assert DummyModifier in TelemetryBugModifier.get_modifiers() + + # Clean up + TelemetryBugModifier.modifiers.remove(DummyModifier) + + def test_get_modifiers(self): + """Test getting the list of modifiers.""" + modifiers = TelemetryBugModifier.get_modifiers() + assert isinstance(modifiers, list) + # SeeAlsoModifier should be registered + assert any(m.__name__ == "SeeAlsoModifier" for m in modifiers) + + def test_get_bug_modifications_empty(self): + """Test get_bug_modifications when modifiers return no changes.""" + alerts = [] + commented_bugs = [] + new_bugs = [] + + modifications = TelemetryBugModifier.get_bug_modifications(alerts, commented_bugs, new_bugs) + + assert modifications == {} + + def test_get_bug_modifications_with_data( + self, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test get_bug_modifications with actual modifications from modifiers.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + # Create another alert with a bug number to trigger see_also modification + sig2 = create_telemetry_signature(probe="test_probe2") + create_telemetry_alert( + sig2, + bug_number=67890, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create TelemetryAlert objects + telemetry_alert1 = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + alerts = [telemetry_alert1] + modifications = TelemetryBugModifier.get_bug_modifications(alerts, [], []) + + # Should have modifications for the see_also field + assert 12345 in modifications + assert "see_also" in modifications[12345] + + def test_get_bug_modifications_with_two_modifiers_different_fields( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ): + """Test get_bug_modifications with two modifiers updating different fields for the same bug.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + class PriorityModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"priority": "P1"}} + + class KeywordModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"keywords": {"add": ["perf-regression"]}}} + + # Save original modifiers + original_modifiers = TelemetryBugModifier.modifiers.copy() + + # Replace with our test modifiers + TelemetryBugModifier.modifiers = [PriorityModifier, KeywordModifier] + + try: + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + modifications = TelemetryBugModifier.get_bug_modifications([telemetry_alert], [], []) + + # Both modifiers should contribute their changes + assert 12345 in modifications + assert modifications[12345]["priority"] == "P1" + assert modifications[12345]["keywords"] == {"add": ["perf-regression"]} + finally: + # Restore original modifiers + TelemetryBugModifier.modifiers = original_modifiers + + def test_get_bug_modifications_with_two_modifiers_same_priority_field( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature, caplog + ): + """Test get_bug_modifications with two modifiers trying to update the priority field.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + class FirstPriorityModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"priority": "P1"}} + + class SecondPriorityModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"priority": "P2"}} + + # Save original modifiers + original_modifiers = TelemetryBugModifier.modifiers.copy() + + # Replace with our test modifiers + TelemetryBugModifier.modifiers = [FirstPriorityModifier, SecondPriorityModifier] + + try: + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + modifications = TelemetryBugModifier.get_bug_modifications([telemetry_alert], [], []) + + # First modifier's value should win, second should be logged as warning + assert 12345 in modifications + assert modifications[12345]["priority"] == "P1" + assert "Bug modification in `priority` from multiple Modifiers" in caplog.text + finally: + # Restore original modifiers + TelemetryBugModifier.modifiers = original_modifiers + + def test_get_bug_modifications_with_two_modifiers_same_see_also_field( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ): + """Test get_bug_modifications with two modifiers both updating the see_also field.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + class FirstSeeAlsoModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"see_also": {"add": [11111]}}} + + class SecondSeeAlsoModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"see_also": {"add": [22222]}}} + + # Save original modifiers + original_modifiers = TelemetryBugModifier.modifiers.copy() + + # Replace with our test modifiers + TelemetryBugModifier.modifiers = [FirstSeeAlsoModifier, SecondSeeAlsoModifier] + + try: + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + modifications = TelemetryBugModifier.get_bug_modifications([telemetry_alert], [], []) + + # Both modifiers should contribute their see_also changes + assert 12345 in modifications + assert "see_also" in modifications[12345] + # Both values should be merged into the add list + assert 11111 in modifications[12345]["see_also"]["add"] + assert 22222 in modifications[12345]["see_also"]["add"] + finally: + # Restore original modifiers + TelemetryBugModifier.modifiers = original_modifiers + + def test_get_bug_modifications_with_comment_conflict( + self, test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature, caplog + ): + """Test get_bug_modifications with two modifiers trying to add comments.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + class FirstCommentModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"comment": "First comment"}} + + class SecondCommentModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + return {12345: {"comment": "Second comment"}} + + # Save original modifiers + original_modifiers = TelemetryBugModifier.modifiers.copy() + + # Replace with our test modifiers + TelemetryBugModifier.modifiers = [FirstCommentModifier, SecondCommentModifier] + + try: + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + modifications = TelemetryBugModifier.get_bug_modifications([telemetry_alert], [], []) + + # Should log a warning about multiple comments, but first comment is kept + assert "Cannot post multiple comments to a bug" in caplog.text + assert 12345 in modifications + assert modifications[12345]["comment"] == "First comment" + finally: + # Restore original modifiers + TelemetryBugModifier.modifiers = original_modifiers + + def test_merge_changes_single_modifier(self): + """Test merging changes from a single modifier.""" + all_changes = {12345: [{"priority": "P1"}], 67890: [{"priority": "P2"}]} + + merged = TelemetryBugModifier._merge_changes(all_changes) + + assert merged == {12345: {"priority": "P1"}, 67890: {"priority": "P2"}} + + def test_merge_changes_multiple_modifiers_no_conflict(self): + """Test merging changes from multiple modifiers with no conflicts.""" + all_changes = { + 12345: [{"priority": "P1"}, {"keywords": {"add": ["perf"]}}], + 67890: [{"priority": "P2"}], + } + + merged = TelemetryBugModifier._merge_changes(all_changes) + + assert merged == { + 12345: {"priority": "P1", "keywords": {"add": ["perf"]}}, + 67890: {"priority": "P2"}, + } + + def test_merge_changes_with_priority_conflict(self, caplog): + """Test merging changes when multiple modifiers try to modify the priority field.""" + all_changes = {12345: [{"priority": "P1"}, {"priority": "P2"}]} + + merged = TelemetryBugModifier._merge_changes(all_changes) + + # First update should be kept (P1), second triggers warning but keeps first value + assert merged == {12345: {"priority": "P1"}} + # Should log warning about different priority values + assert ( + "Bug modification in `priority` from multiple Modifiers" in caplog.text + or "Values found: P2, and P1" in caplog.text + ) + + def test_merge_changes_with_unknown_field(self, caplog): + """Test merging changes with an unknown field.""" + all_changes = {12345: [{"unknown_field": "value1"}, {"unknown_field": "value2"}]} + + merged = TelemetryBugModifier._merge_changes(all_changes) + + # First value should be kept, second triggers warning + assert merged == {12345: {"unknown_field": "value1"}} + # Should log a warning about unknown field + assert "Unable to consolidate field `unknown_field`" in caplog.text + + def test_merge_changes_empty(self): + """Test merging with no changes.""" + all_changes = {} + + merged = TelemetryBugModifier._merge_changes(all_changes) + + assert merged == {} + + +class TestSeeAlsoModifier: + @pytest.fixture + def see_also_modifier_class(self): + """Get the SeeAlsoModifier class from the modifiers list.""" + # Find SeeAlsoModifier in the modifiers list + for modifier in TelemetryBugModifier.get_modifiers(): + if modifier.__name__ == "SeeAlsoModifier": + return modifier + pytest.fail("SeeAlsoModifier not found in modifiers list") + + def test_modify_no_alerts(self, see_also_modifier_class): + """Test modify when no alerts are provided.""" + changes = see_also_modifier_class.modify([], [], []) + + assert changes == {} + + def test_modify_alert_with_no_bug_number( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + ): + """Test modify when alert has no bug number.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + changes = see_also_modifier_class.modify([telemetry_alert], [], []) + + assert changes == {} + + def test_modify_alert_with_no_related_bugs( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test modify when alert has a bug but no related alerts have bugs.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create another alert in same summary but without bug number + sig2 = create_telemetry_signature(probe="test_probe2") + create_telemetry_alert(sig2) + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + changes = see_also_modifier_class.modify([telemetry_alert], [], []) + + # No related bugs, so no see_also changes + assert changes == {} + + def test_modify_alert_with_one_related_bug( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test modify when alert has one related alert with a bug.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create another alert in same summary with a different bug number + sig2 = create_telemetry_signature(probe="test_probe2") + create_telemetry_alert(sig2, bug_number=67890) + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + changes = see_also_modifier_class.modify([telemetry_alert], [], []) + + # Should add related bug to see_also + assert 12345 in changes + assert "see_also" in changes[12345] + assert changes[12345]["see_also"]["add"] == [67890] + + def test_modify_alert_with_multiple_related_bugs( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test modify when alert has multiple related alerts with bugs.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create two more alerts in same summary with different bug numbers + sig2 = create_telemetry_signature(probe="test_probe2") + create_telemetry_alert(sig2, bug_number=67890) + + sig3 = create_telemetry_signature(probe="test_probe3") + create_telemetry_alert(sig3, bug_number=11111) + + telemetry_alert = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + + changes = see_also_modifier_class.modify([telemetry_alert], [], []) + + # Should add all related bugs to see_also + assert 12345 in changes + assert "see_also" in changes[12345] + assert sorted(changes[12345]["see_also"]["add"]) == sorted([67890, 11111]) + + def test_modify_multiple_alerts_with_related_bugs( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test modify when multiple alerts are passed, each with related bugs.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create another alert in same summary with a different bug number + sig2 = create_telemetry_signature(probe="test_probe2") + alert2 = create_telemetry_alert(sig2, bug_number=67890) + + telemetry_alert1 = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + telemetry_alert2 = TelemetryAlert(alert2, test_telemetry_alert_summary, sig2) + + changes = see_also_modifier_class.modify([telemetry_alert1, telemetry_alert2], [], []) + + # Due to the logic that excludes bugs already in changes, only the first alert + # gets the second bug in see_also. The second alert doesn't get the first bug + # because 12345 is already in the changes dict when processing alert2 + assert 12345 in changes + assert changes[12345]["see_also"]["add"] == [67890] + # Bug 67890 doesn't get an entry because 12345 was already processed + assert 67890 not in changes + + def test_modify_excludes_already_processed_bugs( + self, + see_also_modifier_class, + test_telemetry_alert, + test_telemetry_alert_summary, + test_telemetry_signature, + create_telemetry_signature, + create_telemetry_alert, + ): + """Test that modify excludes bugs that have already been processed.""" + from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlert, + ) + + test_telemetry_alert.bug_number = 12345 + test_telemetry_alert.save() + + # Create two more alerts + sig2 = create_telemetry_signature(probe="test_probe2") + alert2 = create_telemetry_alert(sig2, bug_number=67890) + + sig3 = create_telemetry_signature(probe="test_probe3") + alert3 = create_telemetry_alert(sig3, bug_number=11111) + + telemetry_alert1 = TelemetryAlert( + test_telemetry_alert, test_telemetry_alert_summary, test_telemetry_signature + ) + telemetry_alert2 = TelemetryAlert(alert2, test_telemetry_alert_summary, sig2) + telemetry_alert3 = TelemetryAlert(alert3, test_telemetry_alert_summary, sig3) + + # Process alerts in order - each should only see bugs from later alerts + changes = see_also_modifier_class.modify( + [telemetry_alert1, telemetry_alert2, telemetry_alert3], [], [] + ) + + # Alert 1 should see alerts 2 and 3 + assert 12345 in changes + assert sorted(changes[12345]["see_also"]["add"]) == sorted([67890, 11111]) + + # Alert 2 should see alerts 1 and 3, but 1 was already processed + assert 67890 in changes + assert changes[67890]["see_also"]["add"] == [11111] + + # Alert 3 should see alerts 1 and 2, but both were already processed + assert 11111 not in changes diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_modifier.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_modifier.py new file mode 100644 index 00000000000..f4aa1521f15 --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_modifier.py @@ -0,0 +1,127 @@ +import logging + +logger = logging.getLogger(__name__) + + +class TelemetryBugModifier: + modifiers = [] + + @staticmethod + def add(modifier_class): + TelemetryBugModifier.modifiers.append(modifier_class) + + @staticmethod + def get_modifiers(): + return TelemetryBugModifier.modifiers + + @staticmethod + def get_bug_modifications(alerts, commented_bugs, new_bugs, **kwargs): + """Get all the bug modifications that need to be made. + + No guarantees on the ordering of the modifiers running are made. They + can modify the same fields, but certain fields are not allowed to have multiple + modifications. The modifiers should not depend on each other. + + All bug changes from the modifiers are gathered together so that we only + need to do a single call to Bugzilla to modify the bug. These + modifications are then made through the bug manager. + + Bug updates are expected to take the following form: + { + "bug-id": { + "see-also": "value-to-add", + "keyword": "value-to-add2", + ... + }, + "bug-id2": { + ... + }, + ... + } + + Warnings will be raised when a bug field that can only take a single value + is modified multiple times, or when an unknown field is modified by + multiple modifiers. The merge will continue but it will ignore the additional + change to ensure that other bug modifications can still be made. + """ + all_changes = {} + for modifier in TelemetryBugModifier.get_modifiers(): + changes = modifier.modify(alerts, commented_bugs, new_bugs, **kwargs) + if not changes: + continue + for bug, bug_changes in changes.items(): + all_changes.setdefault(bug, []).append(bug_changes) + return TelemetryBugModifier._merge_changes(all_changes) + + @staticmethod + def _merge_changes(all_changes): + """Merges all the changes that will be made by modifiers. + + Some fields can be combined, however, not some can only contain + a single value so they will not be allowed to be modified by + multiple modifiers (e.g. priority, serverity). + """ + + def __merge_change(field, value, existing_value): + if field in ("priority", "severity"): + if value != existing_value: + logger.warning( + f"Bug modification in `{field}` from multiple Modifiers is not" + f"allowed. Values found: {value}, and {existing_value}" + ) + return existing_value + elif field in ("comment",): + logger.warning("Cannot post multiple comments to a bug.") + return existing_value + elif field in ( + "see_also", + "keywords", + "whiteboard", + ): + existing_value["add"].extend(value["add"]) + return existing_value + logger.warning(f"Unable to consolidate field `{field}` with multiple values.") + + merged_changes = {} + + for bug, changes in all_changes.items(): + bug_changes = merged_changes.setdefault(bug, {}) + for change in changes: + for field, value in change.items(): + if field not in bug_changes: + bug_changes[field] = value + continue + merged_value = __merge_change(field, value, bug_changes[field]) + if merged_value is not None: + bug_changes[field] = merged_value + + return merged_changes + + +@TelemetryBugModifier.add +class SeeAlsoModifier: + @staticmethod + def modify(alerts, commented_bugs, new_bugs, **kwargs): + """Get modifications to make to the See Also field in the various bugs. + + :return dict: Returns a dict containing a mapping of bug to a list of + changes for that bug. + """ + changes = {} + + # Get all bugs that are associated with this alert summary (excluding new bugs) + for alert in alerts: + alert_bug = alert.telemetry_alert.bug_number + + # Get all the related bugs, excluding those we've already done since we don't + # want to duplicate the see_also changes + related_bugs = [ + related_alert.bug_number + for related_alert in alert.get_related_alerts() + if related_alert.bug_number is not None and related_alert.bug_number not in changes + ] + + if related_bugs: + changes[alert_bug] = {"see_also": {"add": [bug for bug in related_bugs]}} + + return changes From f8e312a5998d14b3f9a94b781b43925e88c6682d Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 23:00:40 -0400 Subject: [PATCH 16/23] Add TelemetryAlertManager for the overall management of alerts, emails, and bugs. --- .../telemetry_alerting/test_alert_manager.py | 690 ++++++++++++++++++ .../telemetry_alerting/alert_manager.py | 233 ++++++ 2 files changed, 923 insertions(+) create mode 100644 tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py create mode 100644 treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py new file mode 100644 index 00000000000..6f6c581faf1 --- /dev/null +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py @@ -0,0 +1,690 @@ +import logging +from unittest.mock import Mock, patch + +import pytest + +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlertFactory, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager import ( + TelemetryAlertManager, +) +from treeherder.perf.models import PerformanceTelemetryAlert + + +@pytest.fixture +def mock_probes_dict(mock_probe): + """Mock probes dictionary.""" + probes = {"test_probe": mock_probe} + + # Add indexed probes for tests that need them + for i in range(10): + probe = Mock() + probe.name = f"test_probe_{i}" + probe.should_file_bug.return_value = True + probe.should_email.return_value = False + probes[f"test_probe_{i}"] = probe + + return probes + + +@pytest.fixture +def mock_bug_manager(): + """Mock TelemetryBugManager.""" + manager = Mock() + manager.file_bug.return_value = {"id": 123456} + manager.modify_bug.return_value = None + return manager + + +@pytest.fixture +def mock_email_manager(): + """Mock TelemetryEmailManager.""" + manager = Mock() + manager.email_alert.return_value = None + return manager + + +@pytest.fixture +def telemetry_alert_manager(mock_probes_dict): + """TelemetryAlertManager instance with mocked dependencies.""" + with ( + patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryBugManager" + ) as mock_bug_mgr, + patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryEmailManager" + ) as mock_email_mgr, + ): + mock_bug_mgr.return_value = Mock() + mock_email_mgr.return_value = Mock() + manager = TelemetryAlertManager(mock_probes_dict) + return manager + + +@pytest.fixture +def telemetry_alert_with_probe(telemetry_alert_obj, test_telemetry_signature): + """TelemetryAlert object with a probe set.""" + return telemetry_alert_obj + + +class TestTelemetryAlertManager: + def test_initialization(self, mock_probes_dict): + """Test TelemetryAlertManager initialization.""" + with ( + patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryBugManager" + ) as mock_bug_mgr, + patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryEmailManager" + ) as mock_email_mgr, + ): + mock_bug_mgr.return_value = Mock() + mock_email_mgr.return_value = Mock() + + manager = TelemetryAlertManager(mock_probes_dict) + + assert manager.probes == mock_probes_dict + assert manager.bug_manager is not None + assert manager.email_manager is not None + + def test_get_probe_info_success(self, telemetry_alert_manager, mock_probe): + """Test getting probe info for a known probe.""" + probe = telemetry_alert_manager._get_probe_info("test_probe") + assert probe == mock_probe + + def test_get_probe_info_unknown_probe(self, telemetry_alert_manager): + """Test getting probe info for an unknown probe raises exception.""" + with pytest.raises(Exception) as exc_info: + telemetry_alert_manager._get_probe_info("unknown_probe") + + assert "Unknown probe alerted" in str(exc_info.value) + assert "unknown_probe" in str(exc_info.value) + + def test_comment_alert_bugs_does_nothing(self, telemetry_alert_manager, alert_without_bug): + """Test comment_alert_bugs does nothing (pass statement).""" + result = telemetry_alert_manager.comment_alert_bugs([alert_without_bug]) + assert result is None + + def test_update_alerts_no_updates(self, telemetry_alert_manager, alert_without_bug): + """Test update_alerts when there are no updates.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + telemetry_alert_manager.update_alerts([alert_without_bug]) + + mock_modifier.get_alert_updates.assert_called_once() + + def test_update_alerts_with_updates(self, telemetry_alert_manager, alert_without_bug, caplog): + """Test update_alerts when there are updates to apply.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = { + str(alert_without_bug.telemetry_alert.id): {"status": 1} + } + + with caplog.at_level(logging.INFO): + telemetry_alert_manager.update_alerts([alert_without_bug]) + + # Verify the alert was updated + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.status == 1 + + # Verify logging + assert "Updating the following alert IDs" in caplog.text + assert str(alert_without_bug.telemetry_alert.id) in caplog.text + assert "alerts updated with changes" in caplog.text + + def test_update_alerts_ignores_non_modifiable_fields( + self, telemetry_alert_manager, alert_without_bug, caplog + ): + """Test update_alerts ignores fields not in MODIFIABLE_ALERT_FIELDS.""" + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = { + str(alert_without_bug.telemetry_alert.id): { + "status": 1, + "bug_number": 999999, # Not in MODIFIABLE_ALERT_FIELDS + } + } + + telemetry_alert_manager.update_alerts([alert_without_bug]) + + # Only status should be updated + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.status == 1 + assert alert_without_bug.telemetry_alert.bug_number is None + + def test_modify_alert_bugs_disabled(self, telemetry_alert_manager, alert_without_bug): + """Test modify_alert_bugs returns early (currently disabled).""" + result = telemetry_alert_manager.modify_alert_bugs([alert_without_bug], [], []) + assert result is None + + def test_should_file_bug_with_probe_that_should_file( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_file_bug returns True when conditions are met.""" + mock_probe.should_file_bug.return_value = True + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_file_bug( + mock_probe, alert_without_bug + ) + + assert result is True + + def test_should_file_bug_with_existing_bug( + self, telemetry_alert_manager, alert_with_bug, mock_probe + ): + """Test __should_file_bug returns False when bug already exists.""" + mock_probe.should_file_bug.return_value = True + alert_with_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_file_bug( + mock_probe, alert_with_bug + ) + + assert result is False + + def test_should_file_bug_with_failed_alert( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_file_bug returns False when alert has failed.""" + mock_probe.should_file_bug.return_value = True + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = True + + result = telemetry_alert_manager._TelemetryAlertManager__should_file_bug( + mock_probe, alert_without_bug + ) + + assert result is False + + def test_should_file_bug_probe_should_not_file( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_file_bug returns False when probe should not file bug.""" + mock_probe.should_file_bug.return_value = False + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_file_bug( + mock_probe, alert_without_bug + ) + + assert result is False + + def test_file_alert_bug_success(self, telemetry_alert_manager, alert_without_bug, mock_probe): + """Test _file_alert_bug successfully files a bug.""" + mock_probe.should_file_bug.return_value = True + telemetry_alert_manager.bug_manager.file_bug.return_value = {"id": 123456} + alert_without_bug.telemetry_signature.probe = "test_probe" + + bug_id = telemetry_alert_manager._file_alert_bug(alert_without_bug) + + assert bug_id == 123456 + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.bug_number == 123456 + telemetry_alert_manager.bug_manager.file_bug.assert_called_once() + + def test_file_alert_bug_when_should_not_file( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test _file_alert_bug returns None when should not file bug.""" + mock_probe.should_file_bug.return_value = False + alert_without_bug.telemetry_signature.probe = "test_probe" + + bug_id = telemetry_alert_manager._file_alert_bug(alert_without_bug) + + assert bug_id is None + telemetry_alert_manager.bug_manager.file_bug.assert_not_called() + + def test_file_alert_bug_failure_deletes_alert( + self, telemetry_alert_manager, alert_without_bug, mock_probe, caplog + ): + """Test _file_alert_bug deletes alert on failure.""" + mock_probe.should_file_bug.return_value = True + telemetry_alert_manager.bug_manager.file_bug.side_effect = Exception("Bugzilla API error") + alert_without_bug.telemetry_signature.probe = "test_probe" + alert_id = alert_without_bug.telemetry_alert.id + + with caplog.at_level(logging.WARNING): + bug_id = telemetry_alert_manager._file_alert_bug(alert_without_bug) + + assert bug_id is None + assert alert_without_bug.failed is True + assert "Failed to create alert bug" in caplog.text + + # Verify the alert was deleted + assert not PerformanceTelemetryAlert.objects.filter(id=alert_id).exists() + + def test_should_notify_with_probe_that_should_email( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_notify returns True when conditions are met.""" + mock_probe.should_email.return_value = True + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_notify( + mock_probe, alert_without_bug + ) + + assert result is True + + def test_should_notify_with_existing_bug( + self, telemetry_alert_manager, alert_with_bug, mock_probe + ): + """Test __should_notify returns False when bug already exists.""" + mock_probe.should_email.return_value = True + alert_with_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_notify( + mock_probe, alert_with_bug + ) + + assert result is False + + def test_should_notify_with_failed_alert( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_notify returns False when alert has failed.""" + mock_probe.should_email.return_value = True + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = True + + result = telemetry_alert_manager._TelemetryAlertManager__should_notify( + mock_probe, alert_without_bug + ) + + assert result is False + + def test_should_notify_probe_should_not_email( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test __should_notify returns False when probe should not email.""" + mock_probe.should_email.return_value = False + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.failed = False + + result = telemetry_alert_manager._TelemetryAlertManager__should_notify( + mock_probe, alert_without_bug + ) + + assert result is False + + def test_email_alert_success(self, telemetry_alert_manager, alert_without_bug, mock_probe): + """Test _email_alert successfully sends an email.""" + mock_probe.should_email.return_value = True + alert_without_bug.telemetry_signature.probe = "test_probe" + + telemetry_alert_manager._email_alert(alert_without_bug) + + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.notified is True + telemetry_alert_manager.email_manager.email_alert.assert_called_once() + + def test_email_alert_when_should_not_notify( + self, telemetry_alert_manager, alert_without_bug, mock_probe + ): + """Test _email_alert returns early when should not notify.""" + mock_probe.should_email.return_value = False + alert_without_bug.telemetry_signature.probe = "test_probe" + + telemetry_alert_manager._email_alert(alert_without_bug) + + telemetry_alert_manager.email_manager.email_alert.assert_not_called() + + def test_email_alert_failure_sets_notified_false( + self, telemetry_alert_manager, alert_without_bug, mock_probe, caplog + ): + """Test _email_alert sets notified=False on failure.""" + mock_probe.should_email.return_value = True + telemetry_alert_manager.email_manager.email_alert.side_effect = Exception( + "Email sending error" + ) + alert_without_bug.telemetry_signature.probe = "test_probe" + + with caplog.at_level(logging.WARNING): + telemetry_alert_manager._email_alert(alert_without_bug) + + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.notified is False + assert "Failed to create alert email" in caplog.text + + def test_redo_email_alerts( + self, test_telemetry_alert, telemetry_alert_manager, mock_probe, caplog + ): + """Test _redo_email_alerts retries failed email alerts.""" + mock_probe.should_email.return_value = True + + with caplog.at_level(logging.INFO): + telemetry_alert_manager._redo_email_alerts() + + assert "House keeping: retrying emails for alerts" in caplog.text + # The email_manager.email_alert should be called for the retry + telemetry_alert_manager.email_manager.email_alert.assert_called() + + def test_redo_email_alerts_skips_with_bug_number(self, telemetry_alert_manager, alert_with_bug): + """Test _redo_email_alerts skips alerts with bug numbers.""" + alert_with_bug.telemetry_alert.notified = False + alert_with_bug.telemetry_alert.save() + + telemetry_alert_manager._redo_email_alerts() + + # Should not be called because alert has a bug number + telemetry_alert_manager.email_manager.email_alert.assert_not_called() + + def test_redo_email_alerts_skips_already_notified( + self, telemetry_alert_manager, alert_without_bug + ): + """Test _redo_email_alerts skips alerts already notified.""" + alert_without_bug.telemetry_alert.notified = True + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.telemetry_alert.save() + + telemetry_alert_manager._redo_email_alerts() + + # Should not be called because alert is already notified + telemetry_alert_manager.email_manager.email_alert.assert_not_called() + + def test_redo_bug_modifications( + self, telemetry_alert_manager, test_telemetry_alert_summary, caplog + ): + """Test _redo_bug_modifications retries failed bug modifications.""" + test_telemetry_alert_summary.bugs_modified = False + test_telemetry_alert_summary.save() + + with caplog.at_level(logging.INFO): + telemetry_alert_manager._redo_bug_modifications() + + assert "House keeping: retrying bug modifications" in caplog.text + + def test_redo_bug_modifications_no_unmodified_summaries( + self, telemetry_alert_manager, test_telemetry_alert_summary, caplog + ): + """Test _redo_bug_modifications when all summaries are modified.""" + test_telemetry_alert_summary.bugs_modified = True + test_telemetry_alert_summary.save() + + with caplog.at_level(logging.INFO): + telemetry_alert_manager._redo_bug_modifications() + + assert "House keeping: retrying bug modifications" in caplog.text + + def test_redo_bug_modifications_with_alerts( + self, telemetry_alert_manager, test_telemetry_alert_summary, alert_without_bug, caplog + ): + """Test _redo_bug_modifications reconstructs alerts from unmodified summaries.""" + test_telemetry_alert_summary.bugs_modified = False + test_telemetry_alert_summary.save() + + with caplog.at_level(logging.INFO): + telemetry_alert_manager._redo_bug_modifications() + + assert "House keeping: retrying bug modifications" in caplog.text + # This test ensures line 244 is covered (alert construction in loop) + + def test_house_keeping_calls_all_methods( + self, telemetry_alert_manager, alert_without_bug, caplog + ): + """Test house_keeping calls both _redo_email_alerts and _redo_bug_modifications.""" + alert_without_bug.telemetry_alert.notified = False + alert_without_bug.telemetry_alert.bug_number = None + alert_without_bug.telemetry_alert.save() + + with caplog.at_level(logging.INFO): + telemetry_alert_manager.house_keeping([alert_without_bug], [], []) + + assert "Performing house keeping" in caplog.text + assert "House keeping: retrying emails for alerts" in caplog.text + assert "House keeping: retrying bug modifications" in caplog.text + + def test_multiple_alerts_bulk_update( + self, + create_telemetry_signature, + create_telemetry_alert, + telemetry_alert_manager, + test_telemetry_alert_summary, + ): + """Test update_alerts performs bulk updates for multiple alerts.""" + + # Create multiple alerts with different signatures to avoid unique constraint + alerts = [] + for i in range(3): + signature = create_telemetry_signature(probe=f"test_probe_{i}") + alert_row = create_telemetry_alert(signature) + alerts.append(TelemetryAlertFactory.construct_alert(alert_row)) + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = { + str(alerts[0].telemetry_alert.id): {"status": 1}, + str(alerts[1].telemetry_alert.id): {"status": 2}, + str(alerts[2].telemetry_alert.id): {"status": 1}, + } + + telemetry_alert_manager.update_alerts(alerts) + + # Verify all alerts were updated + for i, alert in enumerate(alerts): + alert.telemetry_alert.refresh_from_db() + expected_status = 1 if i in [0, 2] else 2 + assert alert.telemetry_alert.status == expected_status + + def test_manage_alerts_full_workflow( + self, test_telemetry_alert, telemetry_alert_manager, mock_probe + ): + """Test manage_alerts runs the full workflow successfully.""" + + # Create test alerts + alert = TelemetryAlertFactory.construct_alert(test_telemetry_alert) + + # Configure mocks + mock_probe.should_file_bug.return_value = True + mock_probe.should_email.return_value = False + telemetry_alert_manager.bug_manager.file_bug.return_value = {"id": 999888} + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + # Run the full manage_alerts workflow + telemetry_alert_manager.manage_alerts([alert]) + + # Verify all steps were executed + mock_modifier.get_alert_updates.assert_called_once() + telemetry_alert_manager.bug_manager.file_bug.assert_called_once() + + # Verify the bug was filed + test_telemetry_alert.refresh_from_db() + assert test_telemetry_alert.bug_number == 999888 + + def test_manage_alerts_continues_after_update_failure( + self, telemetry_alert_manager, alert_without_bug, mock_probe, caplog + ): + """Test manage_alerts continues after update_alerts fails.""" + mock_probe.should_file_bug.return_value = True + alert_without_bug.telemetry_signature.probe = "test_probe" + + telemetry_alert_manager.bug_manager.file_bug.return_value = {"id": 111222} + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.side_effect = Exception("Update error") + + with caplog.at_level(logging.INFO): + telemetry_alert_manager.manage_alerts([alert_without_bug]) + + # Verify update failed but filing still happened + assert "Failed to update alerts" in caplog.text + telemetry_alert_manager.bug_manager.file_bug.assert_called_once() + + # Bug should still be filed + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.bug_number == 111222 + + def test_manage_alerts_continues_after_file_bug_failure( + self, telemetry_alert_manager, alert_without_bug, mock_probe, caplog + ): + """Test manage_alerts continues after _file_alert_bug fails.""" + mock_probe.should_file_bug.return_value = True + mock_probe.should_email.return_value = False + alert_without_bug.telemetry_signature.probe = "test_probe" + + # Make file_bug fail + telemetry_alert_manager.bug_manager.file_bug.side_effect = Exception("Bugzilla error") + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + with caplog.at_level(logging.INFO): + telemetry_alert_manager.manage_alerts([alert_without_bug]) + + # Verify bug filing failed but housekeeping still ran + assert "Failed to create alert bug" in caplog.text + assert "Performing house keeping" in caplog.text + + def test_manage_alerts_continues_after_email_failure( + self, telemetry_alert_manager, alert_without_bug, mock_probe, caplog + ): + """Test manage_alerts continues after _email_alert fails.""" + mock_probe.should_file_bug.return_value = False + mock_probe.should_email.return_value = True + alert_without_bug.telemetry_signature.probe = "test_probe" + + # Make email_alert fail + telemetry_alert_manager.email_manager.email_alert.side_effect = Exception("Email error") + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + with caplog.at_level(logging.INFO): # Changed to INFO to capture house keeping log + telemetry_alert_manager.manage_alerts([alert_without_bug]) + + # Verify email failed but housekeeping still ran + assert "Failed to create alert email" in caplog.text + assert "Performing house keeping" in caplog.text + + # Verify notified was set to False + alert_without_bug.telemetry_alert.refresh_from_db() + assert alert_without_bug.telemetry_alert.notified is False + + def test_manage_alerts_filters_failed_alerts_before_modify( + self, create_telemetry_signature, create_telemetry_alert, telemetry_alert_manager + ): + """Test manage_alerts filters out failed alerts before modify_alert_bugs.""" + + # Create alerts (probes already exist in telemetry_alert_manager via fixture) + alerts = [] + for i in range(3): + signature = create_telemetry_signature(probe=f"test_probe_{i}") + alert_row = create_telemetry_alert(signature) + alerts.append(TelemetryAlertFactory.construct_alert(alert_row)) + + # Mark some alerts as failed during bug filing + def file_bug_side_effect(probe, alert): + if alert == alerts[0] or alert == alerts[2]: + raise Exception("Bug filing failed") + return {"id": 999777} + + telemetry_alert_manager.bug_manager.file_bug.side_effect = file_bug_side_effect + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + # Create a spy on modify_alert_bugs to verify which alerts are passed + original_modify = telemetry_alert_manager.modify_alert_bugs + modify_calls = [] + + def modify_spy(alerts_arg, commented_bugs, new_bugs): + modify_calls.append(alerts_arg) + return original_modify(alerts_arg, commented_bugs, new_bugs) + + telemetry_alert_manager.modify_alert_bugs = modify_spy + + telemetry_alert_manager.manage_alerts(alerts) + + # Verify modify_alert_bugs was called (could be multiple times due to house_keeping) + # Find the call with non-empty alerts + non_empty_calls = [call for call in modify_calls if len(call) > 0] + assert len(non_empty_calls) >= 1, ( + "modify_alert_bugs should be called with at least one non-empty alert list" + ) + + # Check the first non-empty call (the main manage_alerts call) + passed_alerts = non_empty_calls[0] + # Only the middle alert (index 1) should be passed (others failed) + assert len(passed_alerts) == 1, f"Expected 1 alert, got {len(passed_alerts)}" + assert passed_alerts[0] == alerts[1] + + def test_manage_alerts_with_mixed_bug_and_email_alerts( + self, + create_telemetry_signature, + create_telemetry_alert, + telemetry_alert_manager, + test_telemetry_alert_summary, + ): + """Test manage_alerts with some alerts filing bugs and others sending emails.""" + + # Create two probes with different configurations + probe_with_bug = Mock() + probe_with_bug.name = "probe_bug" + probe_with_bug.should_file_bug.return_value = True + probe_with_bug.should_email.return_value = False + + probe_with_email = Mock() + probe_with_email.name = "probe_email" + probe_with_email.should_file_bug.return_value = False + probe_with_email.should_email.return_value = True + + # Update the manager's probes dictionary + telemetry_alert_manager.probes = { + "probe_bug": probe_with_bug, + "probe_email": probe_with_email, + } + + # Create alerts + sig1 = create_telemetry_signature(probe="probe_bug") + alert_row1 = create_telemetry_alert(sig1) + alert1 = TelemetryAlertFactory.construct_alert(alert_row1) + + sig2 = create_telemetry_signature(probe="probe_email") + alert_row2 = create_telemetry_alert(sig2) + alert2 = TelemetryAlertFactory.construct_alert(alert_row2) + + telemetry_alert_manager.bug_manager.file_bug.return_value = {"id": 333444} + + with patch( + "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" + ) as mock_modifier: + mock_modifier.get_alert_updates.return_value = {} + + telemetry_alert_manager.manage_alerts([alert1, alert2]) + + # Verify bug was filed for alert1 + alert_row1.refresh_from_db() + assert alert_row1.bug_number == 333444 + + # Verify email was sent for alert2 + alert_row2.refresh_from_db() + assert alert_row2.notified is True + assert alert_row2.bug_number is None + + # Verify the correct methods were called + telemetry_alert_manager.bug_manager.file_bug.assert_called_once() + telemetry_alert_manager.email_manager.email_alert.assert_called_once() diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py new file mode 100644 index 00000000000..2bece65d04b --- /dev/null +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py @@ -0,0 +1,233 @@ +import logging +import traceback + +from treeherder.perf.auto_perf_sheriffing.base_alert_manager import AlertManager +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlertFactory, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier import ( + TelemetryAlertModifier, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_manager import ( + TelemetryBugManager, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.bug_modifier import ( + TelemetryBugModifier, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.email_manager import ( + TelemetryEmailManager, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.utils import ( + MODIFIABLE_ALERT_FIELDS, +) +from treeherder.perf.models import ( + PerformanceTelemetryAlert, + PerformanceTelemetryAlertSummary, +) + +logger = logging.getLogger(__name__) + + +class TelemetryAlertManager(AlertManager): + """Alert Management for Telemetry Alerts.""" + + def __init__(self, probes): + super().__init__(TelemetryBugManager(), TelemetryEmailManager()) + + # Convert it to a dictionary for quicker access + self.probes = probes + + def _get_probe_info(self, probe_name): + probe = self.probes.get(probe_name) + if not probe: + raise Exception(f"Unknown probe alerted. No information known about it: {probe_name}") + return probe + + def comment_alert_bugs(self, alerts): + """Comments on bugs to mention additional alerting probes. + + Telemetry alerting doesn't currently have any commenting behaviour. + Associations between related alerts will be done through the modify_alerts + method, and the "See Also" Bugzilla field. + """ + pass + + def update_alerts(self, alerts): + """Updates all alerts with status changes from the associated bugs. + + Queries bugzilla for all telemetry-alert bugs, then goes through the + PerformanceTelemetryAlertSummary objects to update those that changed. + + An alternative is to go through every telemetry alert in the DB, however + that results in many more network requests. However, this approach involves + more DB queries. + """ + alert_updates = TelemetryAlertModifier.get_alert_updates(alerts) + if not alert_updates: + return + + alerts_to_update = set() + fields_to_update = set() + for alert in alerts: + alert_id = str(alert.telemetry_alert.id) + if alert_id not in alert_updates: + continue + + updates = alert_updates[alert_id] + for updated_field, updated_value in updates.items(): + if updated_field not in MODIFIABLE_ALERT_FIELDS: + continue + alerts_to_update.add(alert.telemetry_alert) + fields_to_update.add(updated_field) + + setattr(alert.telemetry_alert, updated_field, updated_value) + + logger.info( + f"Updating the following alert IDs for changes in Bugzilla fields `" + f"{', '.join(fields_to_update)}`: " + f"{', '.join([str(alert.id) for alert in alerts_to_update])}" + ) + num_updated = PerformanceTelemetryAlert.objects.bulk_update( + list(alerts_to_update), list(fields_to_update) + ) + logger.info(f"{num_updated} alerts updated with changes") + + def modify_alert_bugs(self, alerts, commented_bugs, new_bugs): + """Modifies the alert bugs. + + Modifies existing telemetry alert bugs with new bugs that alerted on + the same day. It adds these bugs to the "See Also" field for the other bugs. + """ + for bug, changes in TelemetryBugModifier.get_bug_modifications( + alerts, commented_bugs, new_bugs + ).items(): + # Fields like "see_also" are treated as group modifiers that affect + # groupings of bugs that are related to the PerformanceTelemetryAlertSummary + + bug_alert = None + for alert in alerts: + if str(alert.telemetry_alert.bug_number) == str(bug): + bug_alert = alert + + try: + self.bug_manager.modify_bug(bug, changes) + + bug_alert.telemetry_alert_summary.bugs_modified = True + bug_alert.telemetry_alert.bug_modified = True + except Exception: + logger.warning(f"Failed to modify bug {bug}: {traceback.format_exc()}") + + bug_alert.failed = True + if "see_also" in changes: + bug_alert.telemetry_alert_summary.bugs_modified = False + else: + bug_alert.telemetry_alert.bug_modified = False + finally: + bug_alert.telemetry_alert_summary.save() + bug_alert.telemetry_alert.save() + + def __should_file_bug(self, probe, alert): + """Ensure that the alert should have a bug filed. + + Current criteria for bug filling are: + (1) Monitor field must be defined (already checked by this point). + (2) Monitor field is set to an object, AND the alert field is + set to True. + (3) Alert does not already have a bug filed for it. + """ + return probe.should_file_bug() and not alert.telemetry_alert.bug_number and not alert.failed + + def _file_alert_bug(self, alert): + """Files a bug for each telemetry alert summary. + + Only produced for telemetry alert summaries without a bug number. Those + with a bug number are handled by comment_alert_bugs. + """ + try: + probe = self._get_probe_info(alert.telemetry_signature.probe) + if not self.__should_file_bug(probe, alert): + return + + # File a bug + bug_info = self.bug_manager.file_bug(probe, alert) + + # Associate it with the alert + alert.telemetry_alert.bug_number = int(bug_info["id"]) + alert.telemetry_alert.save() + + return bug_info["id"] + except Exception: + # If we fail to create a bug, output the warning and delete the alert + # so that we can regenerate it and try again another time + logger.warning(f"Failed to create alert bug for {alert}: {traceback.format_exc()}") + alert.telemetry_alert.delete() + alert.failed = True + + def __should_notify(self, probe, alert): + """Ensure that the alert should produce notifications. + + Current criteria for notifications are: + (1) Monitor field must be defined (already checked by this point). + (2) Monitor field is set to True OR monitor field is set to + an object with alert set to False. + (3) Bug has not been filed for the alert. + """ + return probe.should_email() and not alert.telemetry_alert.bug_number and not alert.failed + + def _email_alert(self, alert): + """Sends out emails for each new alert. + + Each probe that alerted will have an email sent out to all alert notification + emails. This means that if a telemetry alert summary (a grouping of alerts) + contains multiple probes that alert, each of those probes will have an email + sent out. + """ + try: + probe = self._get_probe_info(alert.telemetry_signature.probe) + if not self.__should_notify(probe, alert): + return + + # Send notification emails for the alert + self.email_manager.email_alert(probe, alert) + + # Set the alert to notified + alert.telemetry_alert.notified = True + except Exception: + logger.warning(f"Failed to create alert email for {alert}: {traceback.format_exc()}") + alert.telemetry_alert.notified = False + finally: + alert.telemetry_alert.save() + + def _redo_email_alerts(self): + """Handles re-running emails for alerts that don't have any.""" + logger.info("House keeping: retrying emails for alerts") + alerts_no_emails = PerformanceTelemetryAlert.objects.filter( + notified=False, bug_number__isnull=True + ) + + for alert_row in alerts_no_emails: + alert_no_email = TelemetryAlertFactory.construct_alert(alert_row) + self._email_alert(alert_no_email) + + def _redo_bug_modifications(self): + """Handles retrying any bug modifications that are missing because of failures.""" + logger.info("House keeping: retrying bug modifications") + alert_summaries_not_modified = PerformanceTelemetryAlertSummary.objects.filter( + bugs_modified=False + ) + alerts_not_modified = PerformanceTelemetryAlert.objects.filter( + summary_id__in=[summary.id for summary in alert_summaries_not_modified] + ) + + alerts = [] + for alert_row in alerts_not_modified: + alerts.append(TelemetryAlertFactory.construct_alert(alert_row)) + + self.modify_alert_bugs(alerts, [], []) + + def house_keeping(self, alerts, commented_bugs, new_bugs): + """General maintenance of the telemetry alert tables.""" + logger.info("Performing house keeping") + self._redo_email_alerts() + self._redo_bug_modifications() + logger.info("Completed house keeping") From 00ce3337b26a80d6c1be830ccc562916dc4d5014 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 8 Oct 2025 23:01:32 -0400 Subject: [PATCH 17/23] Integrate alert management into telemetry change detection. --- .../perf/auto_perf_sheriffing/sherlock.py | 113 ++++++++++++------ 1 file changed, 77 insertions(+), 36 deletions(-) diff --git a/treeherder/perf/auto_perf_sheriffing/sherlock.py b/treeherder/perf/auto_perf_sheriffing/sherlock.py index b9990df1e82..3697ff5bf5b 100644 --- a/treeherder/perf/auto_perf_sheriffing/sherlock.py +++ b/treeherder/perf/auto_perf_sheriffing/sherlock.py @@ -15,6 +15,16 @@ ) from treeherder.perf.auto_perf_sheriffing.backfill_tool import BackfillTool from treeherder.perf.auto_perf_sheriffing.secretary import Secretary +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert import ( + TelemetryAlertFactory, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager import ( + TelemetryAlertManager, +) +from treeherder.perf.auto_perf_sheriffing.telemetry_alerting.probe import ( + TelemetryProbe, + TelemetryProbeValidationError, +) from treeherder.perf.exceptions import CannotBackfillError, MaxRuntimeExceededError from treeherder.perf.models import ( BackfillNotificationRecord, @@ -34,7 +44,6 @@ ACCESS_TOKEN = settings.PERF_SHERIFF_BOT_ACCESS_TOKEN BUILDID_MAPPING = "https://hg.mozilla.org/mozilla-central/json-firefoxreleases" -REVISION_INFO = "https://hg.mozilla.org/mozilla-central/json-log/%s" INITIAL_PROBES = ( "memory_ghost_windows", @@ -268,17 +277,28 @@ def telemetry_alert(self): metric_definitions = self._get_metric_definitions() + probes = {} + alerts = [] repository = Repository.objects.get(name="mozilla-central") framework = PerformanceFramework.objects.get(name="telemetry") for metric_info in metric_definitions: if metric_info["name"] not in INITIAL_PROBES: continue - logger.info(f"Running detection for {metric_info['name']}") - cdf_ts_detector = ts_detectors[ - metric_info["data"] - .get("monitor", {}) - .get("change-detection-technique", "cdf_squared") - ] + try: + probe = TelemetryProbe(metric_info) + except TelemetryProbeValidationError as e: + logger.warning(f"Failed probe validation: {str(e)}") + continue + + if not probe.should_detect_changes(): + # We should not currently be skipping probes since we're + # only detecting changes on the allowlisted ones. Later, this + # should be a continue + probe.monitor_info["detect_changes"] = True + probes.setdefault(probe.name, probe) + + logger.info(f"Running detection for {probe.name}") + cdf_ts_detector = ts_detectors[probe.get_change_detection_technique()] for platform in ("Windows", "Darwin", "Linux"): if metric_info["platform"] == "mobile" and platform != "Mobile": @@ -288,7 +308,7 @@ def telemetry_alert(self): logger.info(f"On Platform {platform}") try: data = get_metric_table( - metric_info["name"], + probe.name, platform, android=(platform == "Mobile"), use_fog=True, @@ -307,12 +327,18 @@ def telemetry_alert(self): # Only get buildids if there might be a detection if not self._buildid_mappings: self._make_buildid_to_date_mapping() - self._create_detection_alert( - detection, metric_info, platform, repository, framework + alert = self._create_detection_alert( + detection, probe, platform, repository, framework ) + if alert: + alerts.append(alert) except Exception: logger.info(f"Failed: {traceback.format_exc()}") + if alerts: + alert_manager = TelemetryAlertManager(probes) + alert_manager.manage_alerts(alerts) + def _is_prod(self): return settings.SITE_HOSTNAME == "treeherder.mozilla.org" @@ -322,7 +348,7 @@ def _can_run_telemetry(self): def _create_detection_alert( self, detection: object, - probe_info: dict, + probe: TelemetryProbe, platform: str, repository: Repository, framework: PerformanceFramework, @@ -332,7 +358,7 @@ def _create_detection_alert( probe_signature, _ = PerformanceTelemetrySignature.objects.update_or_create( channel="Nightly", platform=platform, - probe=probe_info["name"], + probe=probe.name, probe_type="Glean", application="Firefox", ) @@ -375,36 +401,51 @@ def _create_detection_alert( prev_push=prev_push, push=next_push, original_push=detection_push, + sheriffed=False, defaults={ "manually_created": False, "created": datetime.now(timezone.utc), }, ) - detection_alert, _ = PerformanceTelemetryAlert.objects.update_or_create( - summary_id=detection_summary.id, - series_signature=probe_signature, - defaults={ - "is_regression": True, - "amount_pct": round( - (100.0 * abs(detection.new_value - detection.previous_value)) - / float(detection.previous_value), - 2, - ), - "amount_abs": abs(detection.new_value - detection.previous_value), - "sustained": True, - "direction": detection.direction, - "confidence": detection.confidence, - "prev_value": detection.previous_value, - "new_value": detection.new_value, - "prev_median": detection.optional_detection_info["Interpolated Median"][0], - "new_median": detection.optional_detection_info["Interpolated Median"][1], - "prev_p90": detection.optional_detection_info["Interpolated p05"][0], - "new_p90": detection.optional_detection_info["Interpolated p05"][1], - "prev_p95": detection.optional_detection_info["Interpolated p95"][0], - "new_p95": detection.optional_detection_info["Interpolated p95"][1], - }, - ) + try: + detection_alert = PerformanceTelemetryAlert.objects.get( + summary_id=detection_summary.id, series_signature_id=probe_signature.id + ) + except PerformanceTelemetryAlert.DoesNotExist: + detection_alert = None + + if not detection_alert: + detection_alert, _ = PerformanceTelemetryAlert.objects.update_or_create( + summary_id=detection_summary.id, + series_signature=probe_signature, + defaults={ + "is_regression": True, + "amount_pct": round( + (100.0 * abs(detection.new_value - detection.previous_value)) + / float(detection.previous_value), + 2, + ), + "amount_abs": abs(detection.new_value - detection.previous_value), + "sustained": True, + "direction": detection.direction, + "confidence": detection.confidence, + "prev_value": detection.previous_value, + "new_value": detection.new_value, + "prev_median": detection.optional_detection_info["Interpolated Median"][0], + "new_median": detection.optional_detection_info["Interpolated Median"][1], + "prev_p90": detection.optional_detection_info["Interpolated p05"][0], + "new_p90": detection.optional_detection_info["Interpolated p05"][1], + "prev_p95": detection.optional_detection_info["Interpolated p95"][0], + "new_p95": detection.optional_detection_info["Interpolated p95"][1], + }, + ) + + return TelemetryAlertFactory.construct_alert( + telemetry_alert=detection_alert, + telemetry_alert_summary=detection_summary, + telemetry_signature=probe_signature, + ) def _get_metric_definitions(self) -> list[dict]: metric_definition_urls = [ From a5cd28c413ab11c93760d45f1e79bd400a04709f Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Tue, 21 Oct 2025 19:48:45 -0400 Subject: [PATCH 18/23] Don't send alert emails for non-alerting probes. --- .../auto_perf_sheriffing/telemetry_alerting/probe.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py index 08ef1facd40..52cc7ff84d6 100644 --- a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/probe.py @@ -11,6 +11,11 @@ logger = logging.getLogger(__name__) +ALERTING_PROBES = ( + "networking_http_channel_page_open_to_first_sent", + "perf_largest_contentful_paint", +) + class TelemetryProbeValidationError(Exception): """Raised when a probes information is incorrect, or missing.""" @@ -82,6 +87,11 @@ def setup_notification_emails(self, default=DEFAULT_ALERT_EMAIL): ): return + # XXX: Remove once prototyping is complete + if self.name not in ALERTING_PROBES: + self.monitor_info["notification_emails"] = [default] + return + try: url = GLEAN_PROBE_INFO.format(probe_name=self.name) response = requests.get(url) From 739307ef95844786a4cd76f2415cc227860483b5 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Fri, 24 Oct 2025 11:22:08 -0400 Subject: [PATCH 19/23] Rename get_email_func to get_notify_func. --- .../telemetry_alerting/test_email_manager.py | 10 +++--- .../test_base_email_manager.py | 32 +++++++++---------- .../base_email_manager.py | 2 +- .../telemetry_alerting/email_manager.py | 4 +-- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py index 5799d96c2cc..d2340f78c47 100644 --- a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_email_manager.py @@ -152,7 +152,7 @@ def test_email_alert_sends_to_all_notification_emails( # Setup email manager email_manager = TelemetryEmailManager() mock_email_func = Mock() - email_manager.get_email_func = Mock(return_value=mock_email_func) + email_manager.get_notify_func = Mock(return_value=mock_email_func) # Execute email_manager.email_alert(mock_probe, telemetry_alert_obj) @@ -181,7 +181,7 @@ def test_email_alert_with_single_notification_email( email_manager = TelemetryEmailManager() mock_email_func = Mock() - email_manager.get_email_func = Mock(return_value=mock_email_func) + email_manager.get_notify_func = Mock(return_value=mock_email_func) email_manager.email_alert(mock_probe, telemetry_alert_obj) @@ -190,14 +190,14 @@ def test_email_alert_with_single_notification_email( "single@mozilla.com", mock_probe, telemetry_alert_obj ) - def test_get_email_func_returns_notify_client_email(self): - """Test that get_email_func returns the notify_client.email method.""" + def test_get_notify_func_returns_notify_client_email(self): + """Test that get_notify_func returns the notify_client.email method.""" email_manager = TelemetryEmailManager() mock_email_method = Mock() email_manager.notify_client = Mock() email_manager.notify_client.email = mock_email_method - result = email_manager.get_email_func() + result = email_manager.get_notify_func() assert result == mock_email_method diff --git a/tests/perf/auto_perf_sheriffing/test_base_email_manager.py b/tests/perf/auto_perf_sheriffing/test_base_email_manager.py index 13dbe24a6db..f627d8ab135 100644 --- a/tests/perf/auto_perf_sheriffing/test_base_email_manager.py +++ b/tests/perf/auto_perf_sheriffing/test_base_email_manager.py @@ -43,15 +43,15 @@ def test_email_manager_calls_notify_client_factory(self): mock_factory.assert_called_once() assert manager.notify_client == mock_client - def test_get_email_func_returns_client_email(self, email_manager, mock_taskcluster_notify): - """Test get_email_func returns the notify_client.email method.""" - email_func = email_manager.get_email_func() - assert email_func is mock_taskcluster_notify.email + def test_get_notify_func_returns_client_email(self, email_manager, mock_taskcluster_notify): + """Test get_notify_func returns the notify_client.email method.""" + notify_func = email_manager.get_notify_func() + assert notify_func is mock_taskcluster_notify.email - def test_get_email_func_is_callable(self, email_manager): - """Test get_email_func returns a callable.""" - email_func = email_manager.get_email_func() - assert callable(email_func) + def test_get_notify_func_is_callable(self, email_manager): + """Test get_notify_func returns a callable.""" + notify_func = email_manager.get_notify_func() + assert callable(notify_func) def test_email_alert_is_no_op(self, email_manager): """Test email_alert does nothing by default.""" @@ -94,15 +94,15 @@ def test_multiple_email_managers_use_separate_clients(self): assert manager2.notify_client is mock_client2 assert manager1.notify_client is not manager2.notify_client - def test_get_email_func_can_be_called_multiple_times( + def test_get_notify_func_can_be_called_multiple_times( self, email_manager, mock_taskcluster_notify ): - """Test get_email_func can be called multiple times and returns same function.""" - email_func1 = email_manager.get_email_func() - email_func2 = email_manager.get_email_func() + """Test get_notify_func can be called multiple times and returns same function.""" + notify_func1 = email_manager.get_notify_func() + notify_func2 = email_manager.get_notify_func() - assert email_func1 is email_func2 - assert email_func1 is mock_taskcluster_notify.email + assert notify_func1 is notify_func2 + assert notify_func1 is mock_taskcluster_notify.email def test_email_manager_with_mocked_notify_client(self): """Test EmailManager with a fully mocked notify client.""" @@ -115,10 +115,10 @@ def test_email_manager_with_mocked_notify_client(self): mock_factory.return_value = mock_client manager = EmailManager() - email_func = manager.get_email_func() + notify_func = manager.get_notify_func() # Call the email function - result = email_func( + result = notify_func( address="test@example.com", subject="Test Subject", content="Test Content" ) diff --git a/treeherder/perf/auto_perf_sheriffing/base_email_manager.py b/treeherder/perf/auto_perf_sheriffing/base_email_manager.py index 2821e330bad..9b57e8fe1c5 100644 --- a/treeherder/perf/auto_perf_sheriffing/base_email_manager.py +++ b/treeherder/perf/auto_perf_sheriffing/base_email_manager.py @@ -7,7 +7,7 @@ class EmailManager: def __init__(self): self.notify_client = taskcluster.notify_client_factory() - def get_email_func(self): + def get_notify_func(self): return self.notify_client.email def email_alert(self, *args, **kwargs): diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py index 37accef67ff..668f9c02114 100644 --- a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/email_manager.py @@ -11,7 +11,7 @@ class TelemetryEmailManager(EmailManager): """Formats and emails alert notifications.""" def email_alert(self, probe, alert): - telemetry_email = TelemetryEmail(self.get_email_func()) + telemetry_email = TelemetryEmail(self.get_notify_func()) for email in probe.get_notification_emails(): telemetry_email.email(email, probe, alert) @@ -116,6 +116,7 @@ def _build_table_row( ).format( channel=telemetry_signature.channel, probe=telemetry_signature.probe, + glean_dictionary_link=get_glean_dictionary_link(telemetry_signature), platform=telemetry_signature.platform, date_from=detection_range["from"].time.strftime("%Y-%m-%d"), date_to=detection_range["to"].time.strftime("%Y-%m-%d"), @@ -125,7 +126,6 @@ def _build_table_row( treeherder_push_link=get_treeherder_detection_link( detection_range, telemetry_signature ), - glean_dictionary_link=get_glean_dictionary_link(telemetry_signature), ) def __str__(self): From a9b9aa757df8820ee3e8fcb62dc9599a2da4bfb5 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Fri, 24 Oct 2025 11:26:28 -0400 Subject: [PATCH 20/23] Include alerts to modify as output from modifiers. --- .../telemetry_alerting/test_alert_manager.py | 44 ++++++++++++------- .../telemetry_alerting/test_alert_modifier.py | 43 +++++++++++------- .../telemetry_alerting/alert_manager.py | 14 ++---- .../telemetry_alerting/alert_modifier.py | 21 +++++++-- 4 files changed, 77 insertions(+), 45 deletions(-) diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py index 6f6c581faf1..5c67154a29b 100644 --- a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_manager.py @@ -111,7 +111,7 @@ def test_update_alerts_no_updates(self, telemetry_alert_manager, alert_without_b with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) telemetry_alert_manager.update_alerts([alert_without_bug]) @@ -122,9 +122,11 @@ def test_update_alerts_with_updates(self, telemetry_alert_manager, alert_without with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = { - str(alert_without_bug.telemetry_alert.id): {"status": 1} - } + alert_id = str(alert_without_bug.telemetry_alert.id) + mock_modifier.get_alert_updates.return_value = ( + {alert_id: {"status": 1}}, + {alert_id: alert_without_bug.telemetry_alert}, + ) with caplog.at_level(logging.INFO): telemetry_alert_manager.update_alerts([alert_without_bug]) @@ -145,12 +147,16 @@ def test_update_alerts_ignores_non_modifiable_fields( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = { - str(alert_without_bug.telemetry_alert.id): { - "status": 1, - "bug_number": 999999, # Not in MODIFIABLE_ALERT_FIELDS - } - } + alert_id = str(alert_without_bug.telemetry_alert.id) + mock_modifier.get_alert_updates.return_value = ( + { + alert_id: { + "status": 1, + "bug_number": 999999, # Not in MODIFIABLE_ALERT_FIELDS + } + }, + {alert_id: alert_without_bug.telemetry_alert}, + ) telemetry_alert_manager.update_alerts([alert_without_bug]) @@ -464,11 +470,17 @@ def test_multiple_alerts_bulk_update( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = { + alert_updates = { str(alerts[0].telemetry_alert.id): {"status": 1}, str(alerts[1].telemetry_alert.id): {"status": 2}, str(alerts[2].telemetry_alert.id): {"status": 1}, } + alerts_with_updates = { + str(alerts[0].telemetry_alert.id): alerts[0].telemetry_alert, + str(alerts[1].telemetry_alert.id): alerts[1].telemetry_alert, + str(alerts[2].telemetry_alert.id): alerts[2].telemetry_alert, + } + mock_modifier.get_alert_updates.return_value = (alert_updates, alerts_with_updates) telemetry_alert_manager.update_alerts(alerts) @@ -494,7 +506,7 @@ def test_manage_alerts_full_workflow( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) # Run the full manage_alerts workflow telemetry_alert_manager.manage_alerts([alert]) @@ -546,7 +558,7 @@ def test_manage_alerts_continues_after_file_bug_failure( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) with caplog.at_level(logging.INFO): telemetry_alert_manager.manage_alerts([alert_without_bug]) @@ -569,7 +581,7 @@ def test_manage_alerts_continues_after_email_failure( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) with caplog.at_level(logging.INFO): # Changed to INFO to capture house keeping log telemetry_alert_manager.manage_alerts([alert_without_bug]) @@ -605,7 +617,7 @@ def file_bug_side_effect(probe, alert): with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) # Create a spy on modify_alert_bugs to verify which alerts are passed original_modify = telemetry_alert_manager.modify_alert_bugs @@ -672,7 +684,7 @@ def test_manage_alerts_with_mixed_bug_and_email_alerts( with patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_manager.TelemetryAlertModifier" ) as mock_modifier: - mock_modifier.get_alert_updates.return_value = {} + mock_modifier.get_alert_updates.return_value = ({}, {}) telemetry_alert_manager.manage_alerts([alert1, alert2]) diff --git a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py index 5fa3fb3d4c7..6d4c62d81cc 100644 --- a/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py +++ b/tests/perf/auto_perf_sheriffing/telemetry_alerting/test_alert_modifier.py @@ -42,9 +42,10 @@ def test_get_alert_updates_empty(self, mock_bug_searcher_class): mock_bug_searcher_class.return_value = mock_searcher alerts = [] - updates = TelemetryAlertModifier.get_alert_updates(alerts) + updates, alerts_dict = TelemetryAlertModifier.get_alert_updates(alerts) assert updates == {} + assert alerts_dict == {} @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_get_alert_updates_with_data(self, mock_bug_searcher_class, test_telemetry_alert): @@ -60,11 +61,12 @@ def test_get_alert_updates_with_data(self, mock_bug_searcher_class, test_telemet test_telemetry_alert.save() alerts = [test_telemetry_alert] - updates = TelemetryAlertModifier.get_alert_updates(alerts) + updates, alerts_dict = TelemetryAlertModifier.get_alert_updates(alerts) assert str(test_telemetry_alert.id) in updates assert "status" in updates[str(test_telemetry_alert.id)] assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED + assert str(test_telemetry_alert.id) in alerts_dict @patch( "treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.MODIFIABLE_ALERT_FIELDS", @@ -76,12 +78,14 @@ def test_get_alert_updates_with_two_modifiers_different_fields(self, test_teleme class StatusModifier: @staticmethod def update_alerts(**kwargs): - return {str(test_telemetry_alert.id): {"status": 1}} + alert_id = str(test_telemetry_alert.id) + return ({alert_id: {"status": 1}}, {alert_id: test_telemetry_alert}) class TestFieldModifier: @staticmethod def update_alerts(**kwargs): - return {str(test_telemetry_alert.id): {"test_field": "test_value"}} + alert_id = str(test_telemetry_alert.id) + return ({alert_id: {"test_field": "test_value"}}, {alert_id: test_telemetry_alert}) # Save original updaters original_updaters = TelemetryAlertModifier.updaters.copy() @@ -90,7 +94,7 @@ def update_alerts(**kwargs): TelemetryAlertModifier.updaters = [StatusModifier, TestFieldModifier] try: - updates = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) + updates, alerts_dict = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) # Both modifiers should contribute their updates assert str(test_telemetry_alert.id) in updates @@ -106,12 +110,14 @@ def test_get_alert_updates_with_two_modifiers_same_field(self, test_telemetry_al class FirstStatusModifier: @staticmethod def update_alerts(**kwargs): - return {str(test_telemetry_alert.id): {"status": 1}} + alert_id = str(test_telemetry_alert.id) + return ({alert_id: {"status": 1}}, {alert_id: test_telemetry_alert}) class SecondStatusModifier: @staticmethod def update_alerts(**kwargs): - return {str(test_telemetry_alert.id): {"status": 2}} + alert_id = str(test_telemetry_alert.id) + return ({alert_id: {"status": 2}}, {alert_id: test_telemetry_alert}) # Save original updaters original_updaters = TelemetryAlertModifier.updaters.copy() @@ -120,7 +126,7 @@ def update_alerts(**kwargs): TelemetryAlertModifier.updaters = [FirstStatusModifier, SecondStatusModifier] try: - updates = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) + updates, alerts_dict = TelemetryAlertModifier.get_alert_updates([test_telemetry_alert]) # First modifier's value should win, second should be logged as warning assert str(test_telemetry_alert.id) in updates @@ -210,9 +216,10 @@ def test_update_alerts_no_bugs(self, mock_bug_searcher_class, resolution_modifie mock_searcher.get_bugs.return_value = {"bugs": []} mock_bug_searcher_class.return_value = mock_searcher - updates = resolution_modifier_class.update_alerts() + updates, alerts = resolution_modifier_class.update_alerts() assert updates == {} + assert alerts == {} @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_update_alerts_with_fixed_bug( @@ -228,10 +235,11 @@ def test_update_alerts_with_fixed_bug( test_telemetry_alert.bug_number = 12345 test_telemetry_alert.save() - updates = resolution_modifier_class.update_alerts() + updates, alerts = resolution_modifier_class.update_alerts() assert str(test_telemetry_alert.id) in updates assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED + assert str(test_telemetry_alert.id) in alerts @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_update_alerts_with_multiple_bugs( @@ -260,12 +268,14 @@ def test_update_alerts_with_multiple_bugs( test_telemetry_alert.bug_number = 12345 test_telemetry_alert.save() - updates = resolution_modifier_class.update_alerts() + updates, alerts_dict = resolution_modifier_class.update_alerts() assert str(test_telemetry_alert.id) in updates assert str(alert2.id) in updates assert updates[str(test_telemetry_alert.id)]["status"] == PerformanceTelemetryAlert.FIXED assert updates[str(alert2.id)]["status"] == PerformanceTelemetryAlert.INVALID + assert str(test_telemetry_alert.id) in alerts_dict + assert str(alert2.id) in alerts_dict @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_update_alerts_bug_not_matching_alert( @@ -281,9 +291,10 @@ def test_update_alerts_bug_not_matching_alert( test_telemetry_alert.bug_number = 12345 test_telemetry_alert.save() - updates = resolution_modifier_class.update_alerts() + updates, alerts = resolution_modifier_class.update_alerts() assert updates == {} + assert alerts == {} @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_update_alerts_unknown_resolution( @@ -298,10 +309,11 @@ def test_update_alerts_unknown_resolution( test_telemetry_alert.bug_number = 12345 test_telemetry_alert.save() - updates = resolution_modifier_class.update_alerts() + updates, alerts = resolution_modifier_class.update_alerts() # Should not update if resolution is not recognized assert updates == {} + assert alerts == {} @patch("treeherder.perf.auto_perf_sheriffing.telemetry_alerting.alert_modifier.BugSearcher") def test_update_alerts_exception_handling( @@ -313,9 +325,10 @@ def test_update_alerts_exception_handling( mock_searcher.get_bugs.side_effect = Exception("API Error") mock_bug_searcher_class.return_value = mock_searcher - updates = resolution_modifier_class.update_alerts() + updates, alerts = resolution_modifier_class.update_alerts() - assert updates is None + assert updates == {} + assert alerts == {} assert "Failed to get bugs for alert resolution updates" in caplog.text assert "API Error" in caplog.text diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py index 2bece65d04b..dd3dcc2fdcc 100644 --- a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_manager.py @@ -33,8 +33,6 @@ class TelemetryAlertManager(AlertManager): def __init__(self, probes): super().__init__(TelemetryBugManager(), TelemetryEmailManager()) - - # Convert it to a dictionary for quicker access self.probes = probes def _get_probe_info(self, probe_name): @@ -62,25 +60,21 @@ def update_alerts(self, alerts): that results in many more network requests. However, this approach involves more DB queries. """ - alert_updates = TelemetryAlertModifier.get_alert_updates(alerts) + alert_updates, alerts_with_updates = TelemetryAlertModifier.get_alert_updates(alerts) if not alert_updates: return alerts_to_update = set() fields_to_update = set() - for alert in alerts: - alert_id = str(alert.telemetry_alert.id) - if alert_id not in alert_updates: - continue - + for alert_id, alert in alerts_with_updates.items(): updates = alert_updates[alert_id] for updated_field, updated_value in updates.items(): if updated_field not in MODIFIABLE_ALERT_FIELDS: continue - alerts_to_update.add(alert.telemetry_alert) + alerts_to_update.add(alert) fields_to_update.add(updated_field) - setattr(alert.telemetry_alert, updated_field, updated_value) + setattr(alert, updated_field, updated_value) logger.info( f"Updating the following alert IDs for changes in Bugzilla fields `" diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py index 7826e7f0ecc..5e114fedd28 100644 --- a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/alert_modifier.py @@ -46,18 +46,29 @@ def get_alert_updates(alerts, *args, **kwargs): ... } + They are also expected to produce a dictionary of alerts that will be modified + so that we don't need to perform additional DB queries to find them. They + should follow this format: + { + "alert-id": PerformanceTelemetryAlertObject1, + "alert-id2": PerformanceTelemetryAlertObject2, + ... + } + Warnings will be raised when an updater is found to be modify a field that was already modified. The merge will continue but it will ignore the additional change to ensure that other alert updates can still be made. """ all_updates = {} + all_alerts_to_update = {} for updater in TelemetryAlertModifier.get_updaters(): - updates = updater.update_alerts(**kwargs) + updates, alerts_to_update = updater.update_alerts(**kwargs) if not updates: continue for alert, alert_updates in updates.items(): all_updates.setdefault(str(alert), []).append(alert_updates) - return TelemetryAlertModifier._merge_updates(all_updates) + all_alerts_to_update.update(alerts_to_update) + return TelemetryAlertModifier._merge_updates(all_updates), alerts_to_update @staticmethod def _merge_updates(all_updates): @@ -110,7 +121,7 @@ def update_alerts(**kwargs): bugs = bug_searcher.get_bugs() except Exception as e: logger.warning(f"Failed to get bugs for alert resolution updates: {str(e)}") - return + return ({}, {}) alerts_to_update = PerformanceTelemetryAlert.objects.filter( bug_number__in=[bug_info["id"] for bug_info in bugs["bugs"]] @@ -126,9 +137,11 @@ def __get_alert_status(bug_number): return status_index updates = {} + alerts = {} for alert in alerts_to_update: bug_status = __get_alert_status(alert.bug_number) if bug_status: updates[str(alert.id)] = {"status": bug_status} + alerts[str(alert.id)] = alert - return updates + return updates, alerts From 7a994d003df5b81b91795013931e157300186544 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Fri, 24 Oct 2025 11:29:02 -0400 Subject: [PATCH 21/23] Remove question-mark from query URL, and reduce line lengths. --- .../perf/auto_perf_sheriffing/test_bug_searcher.py | 2 +- .../perf/auto_perf_sheriffing/bug_searcher.py | 2 +- .../telemetry_alerting/bug_manager.py | 14 ++++++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/perf/auto_perf_sheriffing/test_bug_searcher.py b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py index 608b4d9227b..fc1dc599b6d 100644 --- a/tests/perf/auto_perf_sheriffing/test_bug_searcher.py +++ b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py @@ -19,7 +19,7 @@ class TestBugSearcherInitialization: def test_bug_searcher_initialization(self, bug_searcher): """Test BugSearcher initializes with correct defaults.""" - assert bug_searcher.bz_url == "https://bugzilla.mozilla.org/rest/bug?" + assert bug_searcher.bz_url == "https://bugzilla.mozilla.org/rest/bug" assert bug_searcher.bz_headers == {} assert bug_searcher._include_fields == ["id"] assert bug_searcher._products == [] diff --git a/treeherder/perf/auto_perf_sheriffing/bug_searcher.py b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py index 71ca07ed74c..5096a9b69a2 100644 --- a/treeherder/perf/auto_perf_sheriffing/bug_searcher.py +++ b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py @@ -16,7 +16,7 @@ class BugSearcher: """ def __init__(self): - self.bz_url = settings.BUGFILER_API_URL + "/rest/bug?" + self.bz_url = settings.BUGFILER_API_URL + "/rest/bug" self.bz_headers = {} self._include_fields = ["id"] self._products = [] diff --git a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py index a9c9e253403..0bbd45a9c44 100644 --- a/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py +++ b/treeherder/perf/auto_perf_sheriffing/telemetry_alerting/bug_manager.py @@ -154,9 +154,15 @@ def _build_probe_alert_row(self, alert): # a row. That way we can decouple the information provided to bugzilla # users from the alerting system. values = ( - f"| **Median:** {round(alert.telemetry_alert.prev_median, 2)} | **Median:** {round(alert.telemetry_alert.new_median, 2)} |\n" - f"| | | | **P90:** {round(alert.telemetry_alert.prev_p90, 2)} | **P90:** {round(alert.telemetry_alert.new_p90, 2)} |\n" - f"| | | | **P95:** {round(alert.telemetry_alert.prev_p95, 2)} | **P95:** {round(alert.telemetry_alert.new_p95, 2)} |" + f"| **Median:** {round(alert.telemetry_alert.prev_median, 2)} " + f"| **Median:** {round(alert.telemetry_alert.new_median, 2)} |\n" + f"| | | | **P90:** {round(alert.telemetry_alert.prev_p90, 2)} " + f"| **P90:** {round(alert.telemetry_alert.new_p90, 2)} |\n" + f"| | | | **P95:** {round(alert.telemetry_alert.prev_p95, 2)} " + f"| **P95:** {round(alert.telemetry_alert.new_p95, 2)} |" ) - return f"| [{alert.telemetry_signature.probe}]({get_glean_dictionary_link(alert.telemetry_signature)}) | {alert.telemetry_signature.platform} | {alert.telemetry_alert.confidence} {values} \n" + return ( + f"| [{alert.telemetry_signature.probe}]({get_glean_dictionary_link(alert.telemetry_signature)}) " + f"| {alert.telemetry_signature.platform} | {alert.telemetry_alert.confidence} {values} \n" + ) From dd9cc6d72f154ec0e7b0d12367b83478c68ee2c3 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Fri, 24 Oct 2025 11:36:36 -0400 Subject: [PATCH 22/23] Replace query usage with filter in BugSearcher. --- .../auto_perf_sheriffing/test_bug_searcher.py | 18 +++++++-------- .../perf/auto_perf_sheriffing/bug_searcher.py | 22 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/perf/auto_perf_sheriffing/test_bug_searcher.py b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py index fc1dc599b6d..84e8d89935b 100644 --- a/tests/perf/auto_perf_sheriffing/test_bug_searcher.py +++ b/tests/perf/auto_perf_sheriffing/test_bug_searcher.py @@ -95,20 +95,20 @@ def test_get_today_date(self, mock_datetime, bug_searcher): assert result == mock_now.date() mock_datetime.now.assert_called_once_with(timezone.utc) - def test_find_last_query_num_no_queries(self, bug_searcher): - """Test _find_last_query_num returns 0 when no query fields exist.""" + def test_find_last_filter_num_no_queries(self, bug_searcher): + """Test _find_last_filter_num returns 0 when no filter fields exist.""" bug_searcher.set_query({"status": "NEW"}) - result = bug_searcher._find_last_query_num() + result = bug_searcher._find_last_filter_num() assert result == 0 - def test_find_last_query_num_single_query(self, bug_searcher): - """Test _find_last_query_num with single query field.""" + def test_find_last_filter_num_single_query(self, bug_searcher): + """Test _find_last_filter_num with single filter field.""" bug_searcher.set_query({"f1": "status", "o1": "equals", "v1": "NEW"}) - result = bug_searcher._find_last_query_num() + result = bug_searcher._find_last_filter_num() assert result == 1 - def test_find_last_query_num_multiple_queries(self, bug_searcher): - """Test _find_last_query_num with multiple query fields.""" + def test_find_last_filter_num_multiple_queries(self, bug_searcher): + """Test _find_last_filter_num with multiple filter fields.""" bug_searcher.set_query( { "f1": "status", @@ -122,7 +122,7 @@ def test_find_last_query_num_multiple_queries(self, bug_searcher): "v5": "Firefox", } ) - result = bug_searcher._find_last_query_num() + result = bug_searcher._find_last_filter_num() assert result == 5 diff --git a/treeherder/perf/auto_perf_sheriffing/bug_searcher.py b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py index 5096a9b69a2..e880c31b6e2 100644 --- a/treeherder/perf/auto_perf_sheriffing/bug_searcher.py +++ b/treeherder/perf/auto_perf_sheriffing/bug_searcher.py @@ -55,25 +55,25 @@ def get_today_date(self): """Helper method to get today's date in the YYYY-MM-DD format.""" return datetime.now(timezone.utc).date() - def _find_last_query_num(self): - """Used to find the last query number used.""" - query_num = 0 - for query_field in self._query: - if len(query_field) == 2 and query_field.startswith("f"): - query_num = max(query_num, int(query_field[1])) - return query_num + def _find_last_filter_num(self): + """Used to find the last filter number used.""" + filter_num = 0 + for filter_field in self._query: + if len(filter_field) == 2 and filter_field.startswith("f"): + filter_num = max(filter_num, int(filter_field[1])) + return filter_num def _build_bugzilla_params(self): """Builds the params for the bugzilla query.""" params = deepcopy(self._query) if self._products: - query_num = self._find_last_query_num() + filter_num = self._find_last_filter_num() params.update( { - f"f{query_num}": "product", - f"o{query_num}": "anywordssubstr", - f"v{query_num}": ",".join(self._products), + f"f{filter_num}": "product", + f"o{filter_num}": "anywordssubstr", + f"v{filter_num}": ",".join(self._products), } ) From 6f2e463d6dd383661e60e81004d8ddd562706ff8 Mon Sep 17 00:00:00 2001 From: Gregory Mierzwinski Date: Wed, 29 Oct 2025 10:48:57 -0400 Subject: [PATCH 23/23] Rework DB migrations into a single one. --- ...mancetelemetryalert_bug_number_and_more.py | 41 ------------------- ...ncetelemetryalert_bug_modified_and_more.py | 32 ++++++++++++++- 2 files changed, 30 insertions(+), 43 deletions(-) delete mode 100644 treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py diff --git a/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py b/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py deleted file mode 100644 index 9a4f810f596..00000000000 --- a/treeherder/perf/migrations/0061_performancetelemetryalert_bug_number_and_more.py +++ /dev/null @@ -1,41 +0,0 @@ -# Generated by Django 5.1.11 on 2025-10-06 14:43 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ("perf", "0060_alter_performancealert_unique_together_and_more"), - ] - - operations = [ - migrations.AddField( - model_name="performancetelemetryalert", - name="bug_number", - field=models.PositiveIntegerField(null=True), - ), - migrations.AddField( - model_name="performancetelemetryalert", - name="notified", - field=models.BooleanField(default=False), - ), - migrations.AlterField( - model_name="performancetelemetryalert", - name="status", - field=models.IntegerField( - choices=[ - (0, "NEW"), - (1, "FIXED"), - (2, "INVALID"), - (5, "WONTFIX"), - (3, "INACTIVE"), - (4, "DUPLICATE"), - (6, "WORKSFORME"), - (7, "INCOMPLETE"), - (8, "MOVED"), - ], - default=0, - ), - ), - ] diff --git a/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py b/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py index 4523a7eb373..9f111da18e3 100644 --- a/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py +++ b/treeherder/perf/migrations/0062_performancetelemetryalert_bug_modified_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 5.1.12 on 2025-10-08 01:00 +# Generated by Django 5.1.12 on 2025-10-29 14:47 from django.db import migrations, models @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("perf", "0061_performancetelemetryalert_bug_number_and_more"), + ("perf", "0061_performancedatum_os_name_and_more"), ] operations = [ @@ -15,9 +15,37 @@ class Migration(migrations.Migration): name="bug_modified", field=models.BooleanField(default=True), ), + migrations.AddField( + model_name="performancetelemetryalert", + name="bug_number", + field=models.PositiveIntegerField(null=True), + ), + migrations.AddField( + model_name="performancetelemetryalert", + name="notified", + field=models.BooleanField(default=False), + ), migrations.AddField( model_name="performancetelemetryalertsummary", name="bugs_modified", field=models.BooleanField(default=True), ), + migrations.AlterField( + model_name="performancetelemetryalert", + name="status", + field=models.IntegerField( + choices=[ + (0, "NEW"), + (1, "FIXED"), + (2, "INVALID"), + (5, "WONTFIX"), + (3, "INACTIVE"), + (4, "DUPLICATE"), + (6, "WORKSFORME"), + (7, "INCOMPLETE"), + (8, "MOVED"), + ], + default=0, + ), + ), ]