launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:stats-upload-numbers-with-your-uploads into launchpad:master.

Commit message:
Add stats reporting to various UploadJobs

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/391626
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:stats-upload-numbers-with-your-uploads into launchpad:master.

diff --git a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
index fa44fb9..402aff2 100644
--- a/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/model/buildfarmjobbehaviour.py
@@ -34,6 +34,7 @@ from lp.services.config import config
 from lp.services.helpers import filenameToContentType
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.utils import copy_and_close
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
 from lp.services.utils import sanitise_urls
 from lp.services.webapp import canonical_url
 
@@ -152,6 +153,12 @@ class BuildFarmJobBehaviourBase:
         (status, info) = yield self._slave.build(
             cookie, builder_type, chroot.content.sha1, filename_to_sha1, args)
 
+        # Update stats
+        job_type = getattr(self.build, 'job_type', None)
+        job_type_name = job_type.name if job_type else 'UNKNOWN'
+        getUtility(IStatsdClient).incr(
+            'build.count,job_type={}'.format(job_type_name))
+
         logger.info(
             "Job %s (%s) started on %s: %s %s"
             % (cookie, self.build.title, self._builder.url, status, info))
diff --git a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
index 8cbda44..d769732 100644
--- a/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
+++ b/lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py
@@ -44,6 +44,7 @@ from lp.buildmaster.tests.mock_slaves import (
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
 from lp.services.log.logger import BufferLogger
+from lp.services.statsd.tests import StatsMixin
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.testing import (
     TestCase,
@@ -55,6 +56,7 @@ from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     LaunchpadZopelessLayer,
     ZopelessDatabaseLayer,
+    ZopelessLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
 
@@ -155,8 +157,9 @@ class TestBuildFarmJobBehaviourBase(TestCaseWithFactory):
         self.assertIs(False, behaviour.extraBuildArgs()["fast_cleanup"])
 
 
-class TestDispatchBuildToSlave(TestCase):
+class TestDispatchBuildToSlave(StatsMixin, TestCase):
 
+    layer = ZopelessLayer
     run_tests_with = AsynchronousDeferredRunTest
 
     def makeBehaviour(self, das):
@@ -260,6 +263,20 @@ class TestDispatchBuildToSlave(TestCase):
         self.assertDispatched(
             slave, logger, 'chroot-fooix-bar-y86.tar.gz', 'chroot')
 
+    @defer.inlineCallbacks
+    def test_dispatchBuildToSlave_stats(self):
+        self.setUpStats()
+        behaviour = self.makeBehaviour(FakeDistroArchSeries())
+        builder = MockBuilder()
+        slave = OkSlave()
+        logger = BufferLogger()
+        behaviour.setBuilder(builder, slave)
+        yield behaviour.dispatchBuildToSlave(logger)
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build.count,job_type=UNKNOWN',))
+
 
 class TestGetUploadMethodsMixin:
     """Tests for `IPackageBuild` that need objects from the rest of LP."""
diff --git a/lib/lp/oci/model/ocirecipebuildjob.py b/lib/lp/oci/model/ocirecipebuildjob.py
index 73fa4d6..18cfa2f 100644
--- a/lib/lp/oci/model/ocirecipebuildjob.py
+++ b/lib/lp/oci/model/ocirecipebuildjob.py
@@ -49,6 +49,7 @@ from lp.services.job.model.job import (
     )
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.propertycache import get_property_cache
+from lp.services.statsd import UploadJobStatsMixin
 from lp.services.webapp.snapshot import notify_modified
 
 
@@ -155,7 +156,7 @@ class OCIRecipeBuildJobDerived(
 
 @implementer(IOCIRegistryUploadJob)
 @provider(IOCIRegistryUploadJobSource)
-class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
+class OCIRegistryUploadJob(UploadJobStatsMixin, OCIRecipeBuildJobDerived):
 
     class_job_type = OCIRecipeBuildJobType.REGISTRY_UPLOAD
 
@@ -235,6 +236,7 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
         try:
             try:
                 client.upload(self.build)
+                self.reportStats(True)
             except OCIRegistryError as e:
                 self.error_summary = str(e)
                 self.errors = e.errors
@@ -245,4 +247,5 @@ class OCIRegistryUploadJob(OCIRecipeBuildJobDerived):
                 raise
         except Exception:
             transaction.commit()
+            self.reportStats(False)
             raise
diff --git a/lib/lp/oci/tests/test_ocirecipebuildjob.py b/lib/lp/oci/tests/test_ocirecipebuildjob.py
index fd3e0ff..24aded3 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildjob.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildjob.py
@@ -40,6 +40,7 @@ from lp.oci.model.ocirecipebuildjob import (
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.runner import JobRunner
+from lp.services.statsd.tests import StatsMixin
 from lp.services.webapp import canonical_url
 from lp.services.webhooks.testing import LogsScheduledWebhooks
 from lp.testing import TestCaseWithFactory
@@ -106,7 +107,7 @@ class TestOCIRecipeBuildJob(TestCaseWithFactory):
         self.assertEqual(expected, oops)
 
 
-class TestOCIRegistryUploadJob(TestCaseWithFactory):
+class TestOCIRegistryUploadJob(StatsMixin, TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
@@ -117,6 +118,7 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
             OCI_RECIPE_WEBHOOKS_FEATURE_FLAG: 'on',
             OCI_RECIPE_ALLOW_CREATE: 'on'
             }))
+        self.setUpStats()
 
     def makeOCIRecipeBuild(self, **kwargs):
         ocibuild = self.factory.makeOCIRecipeBuild(
@@ -180,6 +182,11 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         self.assertIsNone(job.errors)
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(ocibuild, ["Pending", "Uploaded"], logger)
+        # check we got stats in statsd
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build_upload.count,job_type=REGISTRY_UPLOAD'',success=True',))
 
     def test_run_failed_registry_error(self):
         # A run that fails with a registry error sets the registry upload
@@ -221,6 +228,11 @@ class TestOCIRegistryUploadJob(TestCaseWithFactory):
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
             ocibuild, ["Pending", "Failed to upload"], logger)
+        # check we got stats in statsd
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build_upload.count,job_type=REGISTRY_UPLOAD'',success=False',))
 
     def test_run_failed_other_error(self):
         # A run that fails for some reason other than a registry error sets
diff --git a/lib/lp/services/statsd/__init__.py b/lib/lp/services/statsd/__init__.py
index e69de29..31636ef 100644
--- a/lib/lp/services/statsd/__init__.py
+++ b/lib/lp/services/statsd/__init__.py
@@ -0,0 +1,17 @@
+from zope.component import getUtility
+
+from lp.services.statsd.interfaces.statsd_client import IStatsdClient
+
+
+class UploadJobStatsMixin:
+    """Upload job stats reporting.
+
+    Upload jobs have no useful common base class, so add the
+    required methods via mixin.
+    """
+
+    def reportStats(self, success):
+        job_type = self.class_job_type.name
+        getUtility(IStatsdClient).incr(
+            'build_upload.count,job_type={},success={}'.format(
+                job_type, success))
diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
index 298c00f..36dd544 100644
--- a/lib/lp/snappy/model/snapbuildjob.py
+++ b/lib/lp/snappy/model/snapbuildjob.py
@@ -49,6 +49,7 @@ from lp.services.job.model.job import (
     )
 from lp.services.job.runner import BaseRunnableJob
 from lp.services.propertycache import get_property_cache
+from lp.services.statsd import UploadJobStatsMixin
 from lp.snappy.interfaces.snapbuildjob import (
     ISnapBuildJob,
     ISnapBuildStoreUploadStatusChangedEvent,
@@ -179,7 +180,7 @@ class SnapBuildStoreUploadStatusChangedEvent(ObjectEvent):
 
 @implementer(ISnapStoreUploadJob)
 @provider(ISnapStoreUploadJobSource)
-class SnapStoreUploadJob(SnapBuildJobDerived):
+class SnapStoreUploadJob(SnapBuildJobDerived, UploadJobStatsMixin):
     """A Job that uploads a snap build to the store."""
 
     class_job_type = SnapBuildJobType.STORE_UPLOAD
@@ -346,6 +347,7 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
                     self.store_url, self.store_revision = (
                         client.checkStatus(self.status_url))
                 self.error_message = None
+                self.reportStats(True)
             except self.retry_error_types:
                 raise
             except Exception as e:
@@ -373,4 +375,5 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
             # we want to commit instead: the only database changes we make
             # are to this job's metadata and should be preserved.
             transaction.commit()
+            self.reportStats(False)
             raise
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index 90bb9db..ed9c096 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -27,6 +27,7 @@ from lp.services.database.interfaces import IStore
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.runner import JobRunner
+from lp.services.statsd.tests import StatsMixin
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webhooks.testing import LogsScheduledWebhooks
 from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
@@ -95,7 +96,7 @@ class TestSnapBuildJob(TestCaseWithFactory):
             ISnapBuildJob)
 
 
-class TestSnapStoreUploadJob(TestCaseWithFactory):
+class TestSnapStoreUploadJob(StatsMixin, TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
@@ -104,6 +105,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
         self.status_url = "http://sca.example/dev/api/snaps/1/builds/1/status";
         self.store_url = "http://sca.example/dev/click-apps/1/rev/1/";
+        self.setUpStats()
 
     def test_provides_interface(self):
         # `SnapStoreUploadJob` objects provide `ISnapStoreUploadJob`.
@@ -183,6 +185,11 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Uploaded"], logger)
+        # check we got stats in statsd
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build_upload.count,job_type=STORE_UPLOAD'',success=True',))
 
     def test_run_failed(self):
         # A failed run sets the store upload status to FAILED.
@@ -204,6 +211,11 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         self.assertEqual([], pop_notifications())
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Failed to upload"], logger)
+        # check we got stats in statsd
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build_upload.count,job_type=STORE_UPLOAD'',success=False',))
 
     def test_run_unauthorized_notifies(self):
         # A run that gets 401 from the store sends mail.
diff --git a/lib/lp/soyuz/model/packagetranslationsuploadjob.py b/lib/lp/soyuz/model/packagetranslationsuploadjob.py
index 0af6707..0574fd1 100644
--- a/lib/lp/soyuz/model/packagetranslationsuploadjob.py
+++ b/lib/lp/soyuz/model/packagetranslationsuploadjob.py
@@ -33,6 +33,7 @@ from lp.services.job.runner import BaseRunnableJob
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.utils import filechunks
 from lp.services.mail.sendmail import format_address_for_person
+from lp.services.statsd import UploadJobStatsMixin
 from lp.soyuz.interfaces.packagetranslationsuploadjob import (
     IPackageTranslationsUploadJob,
     IPackageTranslationsUploadJobSource,
@@ -82,7 +83,7 @@ class PackageTranslationsUploadJobDerived(
     config = config.IPackageTranslationsUploadJobSource
 
     def __init__(self, job):
-        assert job.base_job_type == JobType.UPLOAD_PACKAGE_TRANSLATIONS
+        assert job.base_job_type == self.class_job_type
         self.job = job
         self.context = self
 
@@ -97,7 +98,7 @@ class PackageTranslationsUploadJobDerived(
     def create(cls, distroseries, libraryfilealias, sourcepackagename,
                requester):
         job = Job(
-            base_job_type=JobType.UPLOAD_PACKAGE_TRANSLATIONS,
+            base_job_type=cls.class_job_type,
             requester=requester,
             base_json_data=json.dumps(
                 {'distroseries': distroseries.id,
@@ -112,7 +113,7 @@ class PackageTranslationsUploadJobDerived(
     def iterReady(cls):
         jobs = IStore(Job).find(
             Job, Job.id.is_in(Job.ready_jobs),
-            Job.base_job_type == JobType.UPLOAD_PACKAGE_TRANSLATIONS)
+            Job.base_job_type == cls.class_job_type)
         return (cls(job) for job in jobs)
 
     def getOperationDescription(self):
@@ -151,7 +152,10 @@ class PackageTranslationsUploadJobDerived(
 
 @implementer(IPackageTranslationsUploadJob)
 @provider(IPackageTranslationsUploadJobSource)
-class PackageTranslationsUploadJob(PackageTranslationsUploadJobDerived):
+class PackageTranslationsUploadJob(
+        PackageTranslationsUploadJobDerived, UploadJobStatsMixin):
+
+    class_job_type = JobType.UPLOAD_PACKAGE_TRANSLATIONS
 
     def attachTranslationFiles(self, by_maintainer):
         distroseries = self.distroseries
@@ -181,3 +185,4 @@ class PackageTranslationsUploadJob(PackageTranslationsUploadJobDerived):
 
     def run(self):
         self.attachTranslationFiles(True)
+        self.reportStats(True)
diff --git a/lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py b/lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py
index cef16f6..2ff0f90 100644
--- a/lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py
+++ b/lib/lp/soyuz/tests/test_packagetranslationsuploadjob.py
@@ -14,6 +14,7 @@ from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.tests import block_on_job
 from lp.services.mail.sendmail import format_address_for_person
+from lp.services.statsd.tests import StatsMixin
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
 from lp.soyuz.interfaces.packagetranslationsuploadjob import (
     IPackageTranslationsUploadJob,
@@ -77,7 +78,7 @@ class LocalTestHelper(TestCaseWithFactory):
         return self.factory.makeLibraryFileAlias(content=tarfile_content)
 
 
-class TestPackageTranslationsUploadJob(LocalTestHelper):
+class TestPackageTranslationsUploadJob(LocalTestHelper, StatsMixin):
 
     layer = LaunchpadZopelessLayer
 
@@ -147,6 +148,19 @@ class TestPackageTranslationsUploadJob(LocalTestHelper):
         # Check if the file in tar_content is queued:
         self.assertTrue("po/foobar.pot", entries_in_queue[0].path)
 
+    def test_stats(self):
+        self.setUpStats()
+        _, _, job = self.makeJob()
+        method = FakeMethod()
+        removeSecurityProxy(job).attachTranslationFiles = method
+        transaction.commit()
+        _, job.run()
+        self.assertEqual(1, self.stats_client.incr.call_count)
+        self.assertEqual(
+            self.stats_client.incr.call_args_list[0][0],
+            ('build_upload.count,job_type=UPLOAD_PACKAGE_TRANSLATIONS'
+             ',success=True',))
+
 
 class TestViaCelery(LocalTestHelper):
     """PackageTranslationsUploadJob runs under Celery."""