Merge lp:~bigkevmcd/offspring/add-queue-wait-time into lp:offspring

[Kevin McDermott]

This refactors the metric calculation logic and adds support for the average queue time.

Still to do...non-scheduled builds (involves adding logic to exclude specific types from the reason).

[lp:~bigkevmcd/offspring/add-queue-wait-time]

119. By Kevin McDermott on 2011-12-14

Merge forward with changes to automated request lookups.

120. By Kevin McDermott on 2011-12-14

Update clarified docstring.

121. By Kevin McDermott on 2011-12-14

Merge forward.

122. By Kevin McDermott on 2011-12-14

Merge forward.

123. By Kevin McDermott on 2011-12-14

Update docstring, and removed some unused code.

124. By Kevin McDermott on 2011-12-14

Merge forward.

125. By Kevin McDermott on 2011-12-14

Fix command test to reflect new properties.

126. By Kevin McDermott on 2011-12-14

Merge forward.

127. By Kevin McDermott on 2011-12-14

Merge forward.

128. By Kevin McDermott on 2011-12-14

Merge forward.

129. By Kevin McDermott on 2011-12-15

Merge forward.

130. By Kevin McDermott on 2011-12-16

Merge trunk.

[Guilherme Salgado (salgado)]

This looks quite neat! I was hoping there'd be some template tag in Django to convert a timedetal into a human readable string but I couldn't find anything. I guess you've probably searched for one as well before rolling your own?

Also, in a couple places you create a build result using the object factory, set the requested_at/dispatched_at attributes and then save the object again. It'd be nicer if you changed the factory method to take requested_at/dispatched_at as optional arguments to avoid duplicating it in even more places in the future.

review: Approve

[lp:~bigkevmcd/offspring/add-queue-wait-time]

131. By Kevin McDermott on 2012-01-11

Merge trunk.

132. By Kevin McDermott on 2012-01-12

Update factory.make_build_result to allow started_at/finished_at etc.

[Kevin McDermott (bigkevmcd)]

> This looks quite neat! I was hoping there'd be some template tag in Django to
> convert a timedetal into a human readable string but I couldn't find anything.
> I guess you've probably searched for one as well before rolling your own?
Yeah...the normal time one doesn't understand timedeltas (I guess it's relatively rare).

> Also, in a couple places you create a build result using the object factory,
> set the requested_at/dispatched_at attributes and then save the object again.
> It'd be nicer if you changed the factory method to take
> requested_at/dispatched_at as optional arguments to avoid duplicating it in
> even more places in the future.

It's a bit icky...but it works...

I added **kwargs because it would just keep growing...if there are specific things we want to check for, then it's good to move them to the list of named args, otherwise...

    def make_build_result(self, project=None, name=None, result=None,
                          builder=None, requestor=None, **kwargs):
        """
        Create a BuildResult with the given project or a newly created one if
        none is given.
        """
        if name is None:
            name = self.get_unique_string()
        if project is None:
            project = self.make_project()
        requested_at = kwargs.pop("requested_at", None)
        build_result = BuildResult.objects.create(
            project=project, name=name, result=result, builder=builder,
            requestor=requestor, **kwargs)
        # requested_at is auto_now_add, so we can't pass it into the object
        # creation function.
        if requested_at:
            build_result.requested_at = requested_at
            build_result.save()
        return build_result

=== modified file 'lib/offspring/web/queuemanager/management/commands/build_metrics.py'
--- lib/offspring/web/queuemanager/management/commands/build_metrics.py	2011-12-14 07:32:32 +0000
+++ lib/offspring/web/queuemanager/management/commands/build_metrics.py	2012-01-12 08:35:33 +0000
@@ -10,5 +10,6 @@
 
     def handle_noargs(self, **options):
         metrics = get_build_result_metrics()
-        for item, value in metrics.iteritems():
+
+        for item, value in metrics:
             self.stdout.write("%s = %s\n" % (item, value))

=== modified file 'lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py'
--- lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py	2011-12-14 08:44:40 +0000
+++ lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py	2012-01-12 08:35:33 +0000
@@ -17,7 +17,7 @@
         """
         build_result_metrics_mock = self.mocker.replace(get_build_result_metrics)
         build_result_metrics_mock()
-        self.mocker.result({"Description of value": 200})
+        self.mocker.result([("Description of value", 200)])
         self.mocker.replay()
 
         command = Command()

=== modified file 'lib/offspring/web/queuemanager/metrics.py'
--- lib/offspring/web/queuemanager/metrics.py	2011-12-14 17:21:33 +0000
+++ lib/offspring/web/queuemanager/metrics.py	2012-01-12 08:35:33 +0000
@@ -4,13 +4,18 @@
 from offspring.enums import ProjectBuildStates
 
 
-def get_average_build_time(project=None, automated=None, period=None):
+def get_average_time(query_params, columns, project=None, automated=None):
+
     """
-    Return the average time taken between BuildResult.started_at and
-    BuildResult.finished_at in seconds for all BuildResults associated with the
-    provided Project.
-
-    If no Project is supplied, average the build times across all projects.
+    Return the average time between the two named columns.
+
+    The columns must be listed as the start_time and finish_time for the
+    metric.
+
+    If no Project is supplied, average the times across all projects.
+
+    query_params: A dictionary of Djanqo query filters.
+    columns: Columns to be queried, must be start_time, end_time.
 
     project: A Project to filter BuildResults with, if none supplied, will use
         all Projects
@@ -23,54 +28,86 @@
     # don't run using PostgreSQL...
     # FIXME: This code should migrate to a manager in order to do the heavy
     # lifting  https://bugs.launchpad.net/offspring/+bug/904334
-    def total_seconds(delta):
+    def delta_to_seconds(delta):
         """
         Return the total amount of time represented by a timedelta.
         """
-        if delta:
-            return ((delta.microseconds + (delta.seconds + delta.days *
-                                           24 * 3600)
-                    * 10 ** 6) / 10 ** 6)
-        else:
-            return 0.0
-
-    query_params = {"finished_at__isnull": False,
-                    "result__exact": ProjectBuildStates.SUCCESS}
+        return ((delta.microseconds + (delta.seconds + delta.days
+                                       * 24 * 3600)
+                * 10 ** 6) / 10 ** 6)
     if project is not None:
         query_params["project"] = project
 
     if automated is not None:
         query_params["requestor__isnull"] = automated
-    if period is not None:
-        query_params["finished_at__range"] = period
-
     build_results = BuildResult.objects.filter(**query_params)
-    time_stamps = build_results.values_list("started_at", "finished_at")
+    time_stamps = build_results.values_list(*columns)
     if not time_stamps:
         return 0.0
-    times = [total_seconds(finished_at - started_at)
-             for (started_at, finished_at) in time_stamps]
+    times = [delta_to_seconds(end_time - start_time)
+             for (start_time, end_time) in time_stamps]
     return float(sum(times)) / len(time_stamps)
 
 
+def get_average_build_time(project=None, automated=None, period=None):
+    """
+    Return the average time taken between BuildResult.started_at and
+    BuildResult.finished_at in seconds for all BuildResults associated with the
+    provided Project.
+
+    period: A tuple of datetime objects (start_time, end_time)
+
+    See get_average_time for the project and automated parameters.
+    """
+    query_params = {"finished_at__isnull": False,
+                    "result__exact": ProjectBuildStates.SUCCESS}
+    if period is not None:
+        query_params["finished_at__range"] = period
+    return get_average_time(query_params, ("started_at", "finished_at"),
+                            project=project, automated=automated)
+
+
+def get_average_queue_time(project=None, automated=None, period=None):
+    """
+    Return the average time taken between BuildResult.requested_at and
+    BuildResult.dispatched_at in seconds for all BuildResults associated with the
+    provided Project.
+
+    period: A tuple of datetime objects (start_time, end_time)
+
+    See get_average_time for the project and automated parameters.
+    """
+    query_params = {}
+    if period is not None:
+        query_params["dispatched_at__range"] = period
+    return get_average_time(query_params, ("requested_at", "dispatched_at"),
+                            project=project, automated=automated)
+
+
 def get_build_result_metrics():
     """
     Return some simple metrics, in a format suitable for iterating over and
     displaying in a command-line tool.
     """
-    results = {}
+    results = []
     start_date = datetime.utcnow()
 
     seven_days = (start_date - timedelta(days=7), start_date)
-    results["Average scheduled build time (7 days)"] = get_average_build_time(
-        period=seven_days, automated=True)
-
     thirty_days = (start_date - timedelta(days=30), start_date)
-    results["Average scheduled build time (30 days)"] = get_average_build_time(
-        period=thirty_days, automated=True)
-
     ninety_days = (start_date - timedelta(days=90), start_date)
-    results["Average scheduled build time (90 days)"] = get_average_build_time(
-        period=ninety_days, automated=True)
+
+
+    results.append(("Average scheduled build time (7 days)",
+        get_average_build_time(period=seven_days, automated=True)))
+    results.append(("Average scheduled build time (30 days)",
+        get_average_build_time(period=thirty_days, automated=True)))
+    results.append(("Average scheduled build time (90 days)",
+        get_average_build_time(period=ninety_days, automated=True)))
+    results.append(("Average scheduled queue time (7 days)",
+        get_average_queue_time(period=seven_days, automated=True)))
+    results.append(("Average scheduled queue time (30 days)",
+        get_average_queue_time(period=thirty_days, automated=True)))
+    results.append(("Average scheduled queue time (90 days)",
+        get_average_queue_time(period=ninety_days, automated=True)))
 
     return results

=== modified file 'lib/offspring/web/queuemanager/models.py'
--- lib/offspring/web/queuemanager/models.py	2011-12-14 14:44:27 +0000
+++ lib/offspring/web/queuemanager/models.py	2012-01-12 08:35:33 +0000
@@ -1,17 +1,8 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-__all__ = ["Project", "ProjectGroup", "Lexbuilder", "BuildRequest",
-           "BuildResult", "DailyBuildOrder", "LaunchpadProject",
-           "LaunchpadProjectMilestone", "ProjectNotificationSubscription",
-           "Release"]
-
-from datetime import (
-    date,
-    datetime,
-    timedelta
-)
 import math
+from datetime import date, datetime, timedelta
 
 from django.contrib.auth.models import AnonymousUser, User
 from django.db import (
@@ -26,6 +17,10 @@
 from offspring.config import get_configuration
 from offspring.enums import ProjectBuildStates
 
+__all__ = ["Project", "ProjectGroup", "Lexbuilder", "BuildRequest",
+           "BuildResult", "DailyBuildOrder", "LaunchpadProject",
+           "LaunchpadProjectMilestone", "ProjectNotificationSubscription",
+           "Release"]
 
 config = get_configuration()
 
@@ -85,7 +80,7 @@
             project_group=self)
 
     @property
-    def builder(self): 
+    def builder(self):
         return None
 
     @property
@@ -389,7 +384,7 @@
     requestor = models.ForeignKey(User, db_column="requestor_id", blank=True, null=True, editable=False)
     reason = models.CharField('request reason', blank=True, null=True, max_length=200, editable=False)
     requested_at = models.DateTimeField('date created', auto_now_add=True, blank=True, null=True, editable=False)
-    dispatched_at = models.DateTimeField('date dispatched', editable=False, null=True, blank=True) 
+    dispatched_at = models.DateTimeField('date dispatched', editable=False, null=True, blank=True)
     notes = models.CharField(max_length=200, null=True, blank=True)
 
     access_relation = 'project'

=== modified file 'lib/offspring/web/queuemanager/tests/factory.py'
--- lib/offspring/web/queuemanager/tests/factory.py	2011-12-16 10:08:55 +0000
+++ lib/offspring/web/queuemanager/tests/factory.py	2012-01-12 08:35:33 +0000
@@ -73,7 +73,7 @@
         return ProjectGroup.objects.create(name=name, title=title)
 
     def make_build_result(self, project=None, name=None, result=None,
-                          builder=None, requestor=None):
+                          builder=None, requestor=None, **kwargs):
         """
         Create a BuildResult with the given project or a newly created one if
         none is given.
@@ -82,9 +82,16 @@
             name = self.get_unique_string()
         if project is None:
             project = self.make_project()
-        return BuildResult.objects.create(
+        requested_at = kwargs.pop("requested_at", None)
+        build_result = BuildResult.objects.create(
             project=project, name=name, result=result, builder=builder,
-            requestor=requestor)
+            requestor=requestor, **kwargs)
+        # requested_at is auto_now_add, so we can't pass it into the object
+        # creation function.
+        if requested_at:
+            build_result.requested_at = requested_at
+            build_result.save()
+        return build_result
 
     def make_release(self, build=None, name=None, creator=None):
         """

=== modified file 'lib/offspring/web/queuemanager/tests/test_metrics.py'
--- lib/offspring/web/queuemanager/tests/test_metrics.py	2011-12-14 17:38:39 +0000
+++ lib/offspring/web/queuemanager/tests/test_metrics.py	2012-01-12 08:35:33 +0000
@@ -5,7 +5,8 @@
 from offspring.enums import ProjectBuildStates
 from offspring.web.queuemanager.models import User
 from offspring.web.queuemanager.metrics import (
-    get_average_build_time, get_build_result_metrics)
+    get_average_build_time, get_average_queue_time,
+    get_build_result_metrics)
 
 from offspring.web.queuemanager.tests.factory import factory
 
@@ -23,10 +24,25 @@
     finished_at = datetime.combine(build_date, time())
     for minutes in durations:
         build_result = factory.make_build_result(
-            project=project, requestor=requestor, result=result)
-        build_result.finished_at = finished_at
-        build_result.started_at = finished_at - timedelta(minutes=minutes)
-        build_result.save()
+            project=project, requestor=requestor, result=result,
+            started_at=finished_at - timedelta(minutes=minutes),
+            finished_at=finished_at)
+
+
+def create_build_results_with_queue_times(project, queue_times, requestor=None,
+                                          build_date=date.today()):
+    """
+    Create BuildResults with queue times specified.
+
+    project: The Project to associate the BuildResults with.
+    queue_times: A sequence of time periods in minutes.
+    """
+    dispatched_at = datetime.combine(build_date, time())
+    for minutes in queue_times:
+        build_result = factory.make_build_result(
+            project=project, requestor=requestor,
+            requested_at=dispatched_at - timedelta(minutes=minutes),
+            dispatched_at=dispatched_at)
 
 
 class BuildResultMetricsTests(TestCase):
@@ -39,13 +55,12 @@
         get_average_build_time should return the average time taken to build a
         project.
         """
+        finished_at = datetime.utcnow()
         build_result = factory.make_build_result(
-            project=self.project, result=ProjectBuildStates.SUCCESS)
-        finished_at = datetime.now()
-        build_result.finished_at = finished_at
-        build_result.started_at = finished_at - timedelta(minutes=10)
-        build_result.save()
-        self.assertEqual(600, get_average_build_time(self.project))
+            project=self.project, result=ProjectBuildStates.SUCCESS,
+            finished_at=finished_at,
+            started_at=finished_at - timedelta(minutes=10))
+        self.assertEqual(10*60, get_average_build_time(self.project))
 
     def test_get_average_build_time_only_includes_completed_builds(self):
         """
@@ -72,9 +87,9 @@
         If a build has started, but not yet finished, this we should get 0.0
         for the average for this entry.
         """
-        build_result = factory.make_build_result(project=self.project)
-        build_result.started_at = datetime.now() - timedelta(minutes=20)
-        build_result.save()
+        started_at = datetime.utcnow() - timedelta(minutes=20)
+        build_result = factory.make_build_result(project=self.project,
+                                                 started_at=started_at)
         self.assertEqual(0.0, get_average_build_time(self.project))
 
     def test_get_average_build_time_multiple_builds(self):
@@ -82,7 +97,7 @@
         The time for each BuildResult should be averaged together.
         """
         create_build_results_with_durations(self.project, [10, 20])
-        self.assertEqual(15 * 60.0, get_average_build_time(self.project))
+        self.assertEqual(15*60, get_average_build_time(self.project))
 
     def test_get_average_build_time_multiple_builds_ignores_unfinished(self):
         """
@@ -91,11 +106,11 @@
         create_build_results_with_durations(self.project, [10, 20])
 
         # Create an additional unfinished build.
-        build_result = factory.make_build_result(project=self.project)
-        build_result.started_at = datetime.now() - timedelta(minutes=20)
-        build_result.save()
+        started_at = datetime.utcnow() - timedelta(minutes=20)
+        build_result = factory.make_build_result(project=self.project,
+                                                 started_at=started_at)
 
-        self.assertEqual(15 * 60.0, get_average_build_time(self.project))
+        self.assertEqual(15*60, get_average_build_time(self.project))
 
     def test_get_average_build_time_ignores_other_projects(self):
         """
@@ -104,8 +119,7 @@
         """
         create_build_results_with_durations(self.project, [10, 20])
         create_build_results_with_durations(factory.make_project(), [50, 100])
-
-        self.assertEqual(15 * 60.0, get_average_build_time(self.project))
+        self.assertEqual(15*60, get_average_build_time(self.project))
 
     def test_get_average_build_time_no_project(self):
         """
@@ -116,7 +130,7 @@
         create_build_results_with_durations(
             factory.make_project(), [50, 100])
         # 10 + 20 + 50 + 100 = 180 / 4 = 45 minutes...
-        self.assertEqual(45 * 60.0, get_average_build_time())
+        self.assertEqual(45*60, get_average_build_time())
 
     def test_get_average_build_time_with_automated_flag(self):
         """
@@ -168,8 +182,7 @@
 
         # From the day after we create our builds 'til today.
         time_period = (build_date + timedelta(days=1), datetime.utcnow())
-        self.assertEqual(75 * 60.0,
-                         get_average_build_time(period=time_period))
+        self.assertEqual(75*60, get_average_build_time(period=time_period))
 
     def test_get_average_build_time_filters_by_date_and_automated(self):
         """
@@ -189,34 +202,95 @@
 
         # From the day after we create our builds 'til today.
         time_period = (build_date + timedelta(days=1), datetime.utcnow())
-        self.assertEqual(40 * 60.0,
+        self.assertEqual(40*60,
                          get_average_build_time(period=time_period,
                                                 automated=True))
 
+
+class QueueTimeMetricsTests(TestCase):
+
+    def setUp(self):
+        self.project = factory.make_project()
+
+    def test_average_queue_time_single_build(self):
+        """
+        get_average_queue_time should return the average time a build waits
+        before being dispatched.
+        """
+        dispatched_at = datetime.utcnow()
+        build_result = factory.make_build_result(
+            project=self.project, dispatched_at=dispatched_at,
+            requested_at=dispatched_at - timedelta(minutes=30))
+        self.assertEqual(30*60, get_average_queue_time(self.project))
+
+    def test_average_queue_time_no_builds(self):
+        """
+        If there are no builds, we should get an average of 0.0.
+        """
+        self.assertEqual(0.0*60, get_average_queue_time(self.project))
+
+    def test_average_queue_time_includes_by_date(self):
+        """
+        It should be possible to restrict the date queried to a specific time
+        frame, specifying a period should only include those BuildResults
+        within that period.
+        """
+        build_date = datetime.utcnow() - timedelta(days=30)
+        create_build_results_with_queue_times(
+            self.project, [10, 20], build_date=build_date)
+
+        create_build_results_with_queue_times(
+            factory.make_project(), [50, 100],
+            build_date=build_date + timedelta(days=2))
+
+        # From the day after we create our builds 'til today.
+        time_period = (build_date + timedelta(days=1), datetime.utcnow())
+        self.assertEqual(75*60,
+                         get_average_queue_time(period=time_period))
+
+
+class GetBuildResultsTests(TestCase):
+
+    def setUp(self):
+        self.project = factory.make_project()
+
     def test_get_build_result_metrics(self):
         """
-        get_build_result_metrics should return a dictionary with the following:
-
-        "Average scheduled build time (7 days)": value,
-        "Average scheduled build time (30 days)": value,
-        "Average scheduled build time (90 days)": value
+        get_build_result_metrics should return a list of tuples with the metric
+        being measured, and the time in minutes.
         """
         start_date = datetime.utcnow()
         # Two this week, average = 10 + 20/2 = 15
         create_build_results_with_durations(
             self.project, [10, 20], build_date=start_date)
 
+        # Two this week, average = 30 + 18/2 = 24.0
+        create_build_results_with_queue_times(
+            self.project, [30, 18], build_date=start_date)
+
         last_week = start_date - timedelta(days=8)
         # Two last week, 10 + 10 + 10 + 20/4 = 12.5
         create_build_results_with_durations(
             self.project, [10, 10], build_date=last_week)
 
+        # Two last week, average = 30 + 18 + 14  11/4 = 73
+        create_build_results_with_queue_times(
+            self.project, [14, 11], build_date=last_week)
+
         last_month = start_date - timedelta(days=35)
         # Two last month, 10 + 10 + 10 + 20 + 30 + 40/6 = 20
         create_build_results_with_durations(
             self.project, [30, 40], build_date=last_month)
 
-        self.assertEqual({"Average scheduled build time (7 days)": 900,
-                          "Average scheduled build time (30 days)": 750,
-                          "Average scheduled build time (90 days)": 1200},
-                         get_build_result_metrics())
+        # Two last month, average = 30 + 18 + 14  11 + 30 + 20/6 = 82
+        create_build_results_with_queue_times(
+            self.project, [30, 20], build_date=last_month)
+
+        self.assertEqual(
+            [("Average scheduled build time (7 days)", 15.0*60),
+             ("Average scheduled build time (30 days)", 12.5*60),
+             ("Average scheduled build time (90 days)", 20.0*60),
+             ("Average scheduled queue time (7 days)", 24.0*60),
+             ("Average scheduled queue time (30 days)", 18.25*60),
+             ("Average scheduled queue time (90 days)", 20.5*60)],
+            get_build_result_metrics())