From bede7c13907fe09fb66268b0c4ce9975995cc825 Mon Sep 17 00:00:00 2001 From: William Lachance Date: Tue, 31 May 2016 19:54:49 -0400 Subject: [PATCH] Bug 1260791 - Add API's for creating performance alerts + alert summaries --- tests/conftest.py | 20 +++-- tests/perfalert/test_alert_modification.py | 3 +- .../webapp/api/test_performance_alerts_api.py | 81 ++++++++++++++++++- .../api/test_performance_alertsummary_api.py | 50 ++++++++++++ treeherder/perf/alerts.py | 40 +++++---- .../0016_manually_created_alerts.py | 29 +++++++ treeherder/perf/models.py | 6 +- treeherder/webapp/api/performance_data.py | 66 +++++++++++++++ .../webapp/api/performance_serializers.py | 5 +- 9 files changed, 274 insertions(+), 26 deletions(-) create mode 100644 treeherder/perf/migrations/0016_manually_created_alerts.py diff --git a/tests/conftest.py b/tests/conftest.py index ffb9ce0e5..9ae63d9cc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -472,15 +472,19 @@ def client_credentials(request, test_user): @pytest.fixture -def test_perf_signature(test_repository): +def test_perf_framework(): + from treeherder.perf.models import PerformanceFramework + return PerformanceFramework.objects.create( + name='test_talos') + + +@pytest.fixture +def test_perf_signature(test_repository, test_perf_framework): from treeherder.model.models import (MachinePlatform, Option, OptionCollection) - from treeherder.perf.models import (PerformanceFramework, - PerformanceSignature) + from treeherder.perf.models import PerformanceSignature - framework = PerformanceFramework.objects.create( - name='test_talos') option = Option.objects.create(name='opt') option_collection = OptionCollection.objects.create( option_collection_hash='my_option_hash', @@ -494,7 +498,7 @@ def test_perf_signature(test_repository): signature = PerformanceSignature.objects.create( repository=test_repository, signature_hash=(40*'t'), - framework=framework, + framework=test_perf_framework, platform=platform, option_collection=option_collection, suite='mysuite', @@ -582,12 +586,14 @@ def text_summary_lines(jm, failure_lines, test_repository, artifacts): @pytest.fixture -def test_perf_alert_summary(test_repository, test_perf_signature): +def test_perf_alert_summary(test_repository, test_perf_framework): from treeherder.perf.models import PerformanceAlertSummary return PerformanceAlertSummary.objects.create( repository=test_repository, + framework=test_perf_framework, prev_result_set_id=0, result_set_id=1, + manually_created=False, last_updated=datetime.datetime.now()) diff --git a/tests/perfalert/test_alert_modification.py b/tests/perfalert/test_alert_modification.py index d62121339..281b5062b 100644 --- a/tests/perfalert/test_alert_modification.py +++ b/tests/perfalert/test_alert_modification.py @@ -36,7 +36,8 @@ def test_alert_modification(test_repository, test_perf_signature, repository=test_repository, prev_result_set_id=1, result_set_id=2, - last_updated=datetime.datetime.now()) + last_updated=datetime.datetime.now(), + manually_created=False) assert p.related_summary is None assert p.status == PerformanceAlert.UNTRIAGED diff --git a/tests/webapp/api/test_performance_alerts_api.py b/tests/webapp/api/test_performance_alerts_api.py index d9493eeb4..6e5324806 100644 --- a/tests/webapp/api/test_performance_alerts_api.py +++ b/tests/webapp/api/test_performance_alerts_api.py @@ -1,10 +1,13 @@ +import copy import datetime +import pytest from django.core.urlresolvers import reverse from rest_framework.test import APIClient from treeherder.perf.models import (PerformanceAlert, - PerformanceAlertSummary) + PerformanceAlertSummary, + PerformanceDatum) def test_alerts_get(webapp, test_repository, test_perf_alert): @@ -20,6 +23,7 @@ def test_alerts_get(webapp, test_repository, test_perf_alert): 'amount_abs', 'id', 'is_regression', + 'manually_created', 'new_value', 'prev_value', 'related_summary_id', @@ -40,7 +44,8 @@ def test_alerts_put(webapp, test_repository, test_perf_alert, test_user, repository=test_repository, prev_result_set_id=1, result_set_id=2, - last_updated=datetime.datetime.now()) + last_updated=datetime.datetime.now(), + manually_created=False) resp = webapp.get(reverse('performance-alerts-list')) assert resp.status_int == 200 @@ -80,3 +85,75 @@ def test_alerts_put(webapp, test_repository, test_perf_alert, test_user, }, format='json') assert resp.status_code == 200 assert PerformanceAlert.objects.get(id=1).related_summary_id is None + + +@pytest.fixture +def alert_create_post_blob(test_perf_alert_summary, test_perf_signature): + # this blob should be sufficient to create a new alert (assuming + # the user of this API is authorized to do so!) + return { + 'summary_id': test_perf_alert_summary.id, + 'signature_id': test_perf_signature.id + } + + +def test_alerts_post(webapp, test_repository, test_perf_signature, + test_perf_alert_summary, alert_create_post_blob, + test_user, test_sheriff): + + # generate enough data for a proper alert to be generated + for (result_set_id, value) in zip([0]*15 + [1]*15, [1]*15 + [2]*15): + PerformanceDatum.objects.create(repository=test_repository, + job_id=0, + result_set_id=result_set_id, + signature=test_perf_signature, + value=value, + push_timestamp=datetime.datetime.now()) + + # verify that we fail if not authenticated + webapp.post_json(reverse('performance-alerts-list'), + alert_create_post_blob, status=403) + + # verify that we fail if authenticated, but not staff + client = APIClient() + client.force_authenticate(user=test_user) + resp = client.post(reverse('performance-alerts-list'), + alert_create_post_blob) + assert resp.status_code == 403 + assert PerformanceAlert.objects.count() == 0 + + # verify that we succeed if staff + authenticated + client = APIClient() + client.force_authenticate(user=test_sheriff) + resp = client.post(reverse('performance-alerts-list'), + alert_create_post_blob) + assert resp.status_code == 200 + assert PerformanceAlert.objects.count() == 1 + + alert = PerformanceAlert.objects.all()[0] + assert alert.status == PerformanceAlert.UNTRIAGED + assert alert.manually_created + assert alert.amount_pct == 100 + assert alert.amount_abs == 1 + assert alert.prev_value == 1 + assert alert.new_value == 2 + assert alert.is_regression + assert alert.summary.id == 1 + + +def test_alerts_post_insufficient_data(test_repository, + test_perf_alert_summary, + test_perf_signature, test_sheriff, + alert_create_post_blob): + # we should not succeed if insufficient data is passed through + client = APIClient() + client.force_authenticate(user=test_sheriff) + + for removed_key in ['summary_id', 'signature_id']: + new_post_blob = copy.copy(alert_create_post_blob) + del new_post_blob[removed_key] + + resp = client.post(reverse('performance-alerts-list'), + new_post_blob) + assert resp.status_code == 400 + assert PerformanceAlert.objects.count() == 0 diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index 03f51c74f..19057f69f 100644 --- a/tests/webapp/api/test_performance_alertsummary_api.py +++ b/tests/webapp/api/test_performance_alertsummary_api.py @@ -33,6 +33,7 @@ def test_alert_summaries_get(webapp, test_perf_alert_summary, 'status', 'series_signature', 'is_regression', + 'manually_created', 'prev_value', 'new_value', 't_value', @@ -69,3 +70,52 @@ def test_alert_summaries_put(webapp, test_repository, test_perf_signature, }, format='json') assert resp.status_code == 200 assert PerformanceAlertSummary.objects.get(id=1).status == 1 + + +def test_alert_summary_post(webapp, test_repository, test_perf_signature): + # this blob should be sufficient to create a new alert summary (assuming + # the user of this API is authorized to do so!) + post_blob = { + 'repository_id': test_repository.id, + 'framework_id': test_perf_signature.framework.id, + 'prev_result_set_id': 1, + 'result_set_id': 2 + } + + # verify that we fail if not authenticated + webapp.post_json(reverse('performance-alert-summaries-list'), post_blob, + status=403) + assert PerformanceAlertSummary.objects.count() == 0 + + # verify that we fail if authenticated, but not staff + client = APIClient() + user = User.objects.create(username="testuser1", + email='foo1@bar.com', + is_staff=False) + client.force_authenticate(user=user) + resp = client.post(reverse('performance-alert-summaries-list'), post_blob) + assert resp.status_code == 403 + assert PerformanceAlertSummary.objects.count() == 0 + + # verify that we succeed if authenticated + staff + client = APIClient() + user = User.objects.create(username="testuser2", + email='foo2@bar.com', + is_staff=True) + client.force_authenticate(user=user) + resp = client.post(reverse('performance-alert-summaries-list'), post_blob) + assert resp.status_code == 200 + + assert PerformanceAlertSummary.objects.count() == 1 + alert_summary = PerformanceAlertSummary.objects.all()[0] + assert alert_summary.repository == test_repository + assert alert_summary.framework == test_perf_signature.framework + assert alert_summary.prev_result_set_id == post_blob['prev_result_set_id'] + assert alert_summary.result_set_id == post_blob['result_set_id'] + assert resp.data['alert_summary_id'] == alert_summary.id + + # verify that we don't create a new performance alert summary if one + # already exists (but also don't throw an error) + resp = client.post(reverse('performance-alert-summaries-list'), post_blob) + assert resp.status_code == 200 + assert PerformanceAlertSummary.objects.count() == 1 diff --git a/treeherder/perf/alerts.py b/treeherder/perf/alerts.py index 44b624965..6f43e77b2 100644 --- a/treeherder/perf/alerts.py +++ b/treeherder/perf/alerts.py @@ -1,5 +1,6 @@ import datetime import time +from collections import namedtuple from django.conf import settings from django.db import transaction @@ -10,6 +11,24 @@ from treeherder.perf.models import (PerformanceAlert, from treeherder.perfalert import Analyzer +def get_alert_properties(prev_value, new_value, lower_is_better): + AlertProperties = namedtuple('AlertProperties', + 'pct_change delta is_regression') + if prev_value != 0: + pct_change = (100.0 * abs(new_value - + prev_value) / + float(prev_value)) + else: + pct_change = 0.0 + + delta = (new_value - prev_value) + + is_regression = ((delta > 0 and lower_is_better) or + (delta < 0 and not lower_is_better)) + + return AlertProperties(pct_change, delta, is_regression) + + def generate_new_alerts_in_series(signature): # get series data starting from either: # (1) the last alert, if there is one @@ -62,18 +81,10 @@ def generate_new_alerts_in_series(signature): if cur.state == 'regression': prev_value = cur.historical_stats['avg'] new_value = cur.forward_stats['avg'] - if prev_value != 0: - pct_change = (100.0 * abs(new_value - - prev_value) / - float(prev_value)) - else: - pct_change = 0.0 - delta = (new_value - prev_value) + alert_properties = get_alert_properties( + prev_value, new_value, signature.lower_is_better) - is_regression = ((delta > 0 and signature.lower_is_better) or - (delta < 0 and not signature.lower_is_better)) - - if pct_change < alert_threshold: + if alert_properties.pct_change < alert_threshold: # ignore regressions below the configured regression # threshold continue @@ -84,6 +95,7 @@ def generate_new_alerts_in_series(signature): result_set_id=cur.testrun_id, prev_result_set_id=prev_testrun_id, defaults={ + 'manually_created': False, 'last_updated': datetime.datetime.fromtimestamp( cur.push_timestamp) }) @@ -97,9 +109,9 @@ def generate_new_alerts_in_series(signature): a = PerformanceAlert.objects.create( summary=summary, series_signature=signature, - is_regression=is_regression, - amount_pct=pct_change, - amount_abs=delta, + is_regression=alert_properties.is_regression, + amount_pct=alert_properties.pct_change, + amount_abs=alert_properties.delta, prev_value=prev_value, new_value=new_value, t_value=t_value) diff --git a/treeherder/perf/migrations/0016_manually_created_alerts.py b/treeherder/perf/migrations/0016_manually_created_alerts.py new file mode 100644 index 000000000..7fcda53cf --- /dev/null +++ b/treeherder/perf/migrations/0016_manually_created_alerts.py @@ -0,0 +1,29 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('perf', '0015_make_alerting_configurable_per_series'), + ] + + operations = [ + migrations.AddField( + model_name='performancealert', + name='manually_created', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='performancealertsummary', + name='manually_created', + field=models.BooleanField(default=False), + ), + migrations.AlterField( + model_name='performancealert', + name='t_value', + field=models.FloatField(help_text=b"t value out of analysis indicating confidence that change is 'real'", null=True), + ), + ] diff --git a/treeherder/perf/models.py b/treeherder/perf/models.py index 0d82d9be8..230f4d2d2 100644 --- a/treeherder/perf/models.py +++ b/treeherder/perf/models.py @@ -122,6 +122,8 @@ class PerformanceAlertSummary(models.Model): prev_result_set_id = models.PositiveIntegerField(null=True) result_set_id = models.PositiveIntegerField() + manually_created = models.BooleanField(default=False) + last_updated = models.DateTimeField(db_index=True) UNTRIAGED = 0 @@ -259,7 +261,9 @@ class PerformanceAlert(models.Model): help_text="New value of series after change") t_value = models.FloatField( help_text="t value out of analysis indicating confidence " - "that change is 'real'") + "that change is 'real'", null=True) + + manually_created = models.BooleanField(default=False) def save(self, *args, **kwargs): # validate that we set a status that makes sense for presence diff --git a/treeherder/webapp/api/performance_data.py b/treeherder/webapp/api/performance_data.py index 9586fcc60..69e4adbc0 100644 --- a/treeherder/webapp/api/performance_data.py +++ b/treeherder/webapp/api/performance_data.py @@ -3,6 +3,7 @@ import json import time from collections import defaultdict +from django.conf import settings from rest_framework import (exceptions, filters, pagination, @@ -10,6 +11,7 @@ from rest_framework import (exceptions, from rest_framework.response import Response from treeherder.model import models +from treeherder.perf.alerts import get_alert_properties from treeherder.perf.models import (PerformanceAlert, PerformanceAlertSummary, PerformanceDatum, @@ -212,6 +214,20 @@ class PerformanceAlertSummaryViewSet(viewsets.ModelViewSet): ordering = ('-last_updated', '-id') pagination_class = AlertSummaryPagination + def create(self, request, *args, **kwargs): + data = request.data + alert_summary, _ = PerformanceAlertSummary.objects.get_or_create( + repository_id=data['repository_id'], + framework=PerformanceFramework.objects.get(id=data['framework_id']), + result_set_id=data['result_set_id'], + prev_result_set_id=data['prev_result_set_id'], + defaults={ + 'manually_created': True, + 'last_updated': datetime.datetime.now() + }) + + return Response({"alert_summary_id": alert_summary.id}) + class PerformanceAlertViewSet(viewsets.ModelViewSet): queryset = PerformanceAlert.objects.all() @@ -227,3 +243,53 @@ class PerformanceAlertViewSet(viewsets.ModelViewSet): page_size = 10 pagination_class = AlertPagination + + def create(self, request, *args, **kwargs): + data = request.data + if 'summary_id' not in data or 'signature_id' not in data: + return Response({"message": "Summary and signature ids necessary " + "to create alert"}, status=400) + + summary = PerformanceAlertSummary.objects.get( + id=data['summary_id']) + signature = PerformanceSignature.objects.get( + id=data['signature_id']) + + prev_range = signature.max_back_window + if not prev_range: + prev_range = settings.PERFHERDER_ALERTS_MAX_BACK_WINDOW + new_range = signature.fore_window + if not new_range: + new_range = settings.PERFHERDER_ALERTS_FORE_WINDOW + + prev_data = PerformanceDatum.objects.filter( + signature=signature, + result_set_id__lte=summary.prev_result_set_id).order_by( + 'push_timestamp').values_list('value', flat=True)[:prev_range] + new_data = PerformanceDatum.objects.filter( + signature=signature, + result_set_id__gt=summary.prev_result_set_id).order_by( + '-push_timestamp').values_list('value', flat=True)[:new_range] + if not prev_data or not new_data: + return Response({"message": "Insufficient data to create an " + "alert"}, status=400) + + prev_value = sum(prev_data)/len(prev_data) + new_value = sum(new_data)/len(new_data) + + alert_properties = get_alert_properties(prev_value, new_value, + signature.lower_is_better) + + alert, _ = PerformanceAlert.objects.get_or_create( + summary=summary, + series_signature=signature, + defaults={ + 'is_regression': alert_properties.is_regression, + 'manually_created': True, + 'amount_pct': alert_properties.pct_change, + 'amount_abs': alert_properties.delta, + 'prev_value': prev_value, + 'new_value': new_value, + 't_value': 1000 + }) + return Response({"alert_id": alert.id}) diff --git a/treeherder/webapp/api/performance_serializers.py b/treeherder/webapp/api/performance_serializers.py index f7374616b..18307d4e4 100644 --- a/treeherder/webapp/api/performance_serializers.py +++ b/treeherder/webapp/api/performance_serializers.py @@ -82,7 +82,8 @@ class PerformanceAlertSerializer(serializers.ModelSerializer): model = PerformanceAlert fields = ['id', 'status', 'series_signature', 'is_regression', 'prev_value', 'new_value', 't_value', 'amount_abs', - 'amount_pct', 'summary_id', 'related_summary_id'] + 'amount_pct', 'summary_id', 'related_summary_id', + 'manually_created'] class PerformanceAlertSummarySerializer(serializers.ModelSerializer): @@ -94,6 +95,8 @@ class PerformanceAlertSummarySerializer(serializers.ModelSerializer): slug_field='id') # marking these fields as readonly, the user should not be modifying them + # (after the item is first created, where we don't use this serializer + # class) prev_result_set_id = serializers.ReadOnlyField() result_set_id = serializers.ReadOnlyField() last_updated = serializers.ReadOnlyField()