Merge lp:~wgrant/launchpad/bprdc into lp:launchpad/db-devel

[William Grant]

Add the model for PPA download stats. Landed by henninge.

This branch adds the model for PPA binary download stats. BinaryPackageReleaseDownloadCount is the class and DB table, with a new Archive.updatePackageDownloadCount to create and update them. See bug #139855 for discussion on the issue -- the only thing with which everybody is agreed about collecting is counts for each binary package release in each archive.

BinaryPackageReleaseDownloadCounts will be managed by my next few branches, with the parse-ppa-apache-logs.py script. Otherwise very closely modeled on LibraryFileDownloadCount, it will store counts with an (archive, bpr) key rather than just (lfa,).

[Stuart Bishop (stub)]

Looks good. DB patch approved by me as patch-2207-36-0.sql.

We might need an extra index or three depending on what reports we generate from this information. I think the reports I think we will generate will be fine with the index from the UNIQUE constraint so no worries for now.

review: Approve (db)

[Björn Tillenius (bjornt)]

On Fri, Mar 12, 2010 at 07:20:53AM -0000, William Grant wrote:
> William Grant has proposed merging lp:~wgrant/launchpad/bprdc into lp:launchpad with lp:~wgrant/launchpad/get-bpr-by-filename as a prerequisite.
>
> Requested reviews:
> Björn Tillenius (bjornt): db
> Stuart Bishop (stub): db
> Canonical Launchpad Engineering (launchpad)
>
>
> This branch adds the model for PPA binary download stats.
> BinaryPackageReleaseDownloadCount is the class and DB table, with a new
> Archive.updatePackageDownloadCount to create and update them. See bug
> #139855 for discussion on the issue -- the only thing with which
> everybody is agreed about collecting is counts for each binary package
> release in each archive.

I think this patch is ok, although I have a few questions (and a comment
on the patch itself further down). I'll be on vacation next week, so I
won't block a landing of the patch, waiting for the replies, as long as
my comments are considered, but I'm marking it as needsinfo so that I
can track the discussion more easily.

    vote needsinfo

Can you explain shortly how BinaryPackageRelease relates to archive and
series? As I understand it, this is only for PPAs, right, since we can't
track the downloads of other archives? Would it make sense to make the
name a bit more specific in that case? At the very least, comments.sql
should explain this. Or will this be used by other archives than PPA
ones?

I couldn't find where everybody agreed about collecting counts for each
binary package release. Can you point me to the right place?

What other things are we likely to want to track in the future, if any?

> === added file 'database/schema/patch-2207-99-0.sql'
> --- database/schema/patch-2207-99-0.sql 1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2207-99-0.sql 2010-03-12 07:20:45 +0000
> @@ -0,0 +1,15 @@
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE BinaryPackageReleaseDownloadCount (
> + id serial PRIMARY KEY,
> + archive integer NOT NULL REFERENCES Archive,
> + binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
> + day date NOT NULL,
> + country integer,
> + count integer NOT NULL
> +);
> +
> +ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binarypackagereleasedownloadcount__archive__binary_package_release__day__country__key
> + UNIQUE (archive, binary_package_release, day, country);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 99, 0);

Shouldn't country be a foreign key to Country?

And looking at the tests, I didn't see any tests for not specifying the
country. That would be good to have.

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Needs Information

[William Grant (wgrant)]

On Fri, 2010-03-12 at 09:42 +0000, Björn Tillenius wrote:
> Review: Needs Information
> On Fri, Mar 12, 2010 at 07:20:53AM -0000, William Grant wrote:
> > William Grant has proposed merging lp:~wgrant/launchpad/bprdc into lp:launchpad with lp:~wgrant/launchpad/get-bpr-by-filename as a prerequisite.
> >
> > Requested reviews:
> > Björn Tillenius (bjornt): db
> > Stuart Bishop (stub): db
> > Canonical Launchpad Engineering (launchpad)
> >
> >
> > This branch adds the model for PPA binary download stats.
> > BinaryPackageReleaseDownloadCount is the class and DB table, with a new
> > Archive.updatePackageDownloadCount to create and update them. See bug
> > #139855 for discussion on the issue -- the only thing with which
> > everybody is agreed about collecting is counts for each binary package
> > release in each archive.
>
> I think this patch is ok, although I have a few questions (and a comment
> on the patch itself further down). I'll be on vacation next week, so I
> won't block a landing of the patch, waiting for the replies, as long as
> my comments are considered, but I'm marking it as needsinfo so that I
> can track the discussion more easily.

Thanks.

> vote needsinfo
>
> Can you explain shortly how BinaryPackageRelease relates to archive and
> series? As I understand it, this is only for PPAs, right, since we can't
> track the downloads of other archives? Would it make sense to make the
> name a bit more specific in that case? At the very least, comments.sql
> should explain this. Or will this be used by other archives than PPA
> ones?

A BinaryPackageRelease can be published in any number of archives and
distribution series (linked by BinaryPackagePublishingHistory). There
was talk a couple of months ago about a desire to use this for the
partner archive too -- I'm not sure if that's still on the cards, but
this schema works fine for that too, and the subsequent branches would
need only slight extension.

The primarily archive is more impossible, since it is heavily mirrored
across hundreds of hosts.

> I couldn't find where everybody agreed about collecting counts for each
> binary package release. Can you point me to the right place?
>
> What other things are we likely to want to track in the future, if any?

The last few comments all speak of counting binaries and indices, but no
concrete method of usefully counting index downloads has yet been
mentioned, and it's much much harder to get right (it involves lots of
guessing). Binary counting is easy and a good first step.

>
> > === added file 'database/schema/patch-2207-99-0.sql'
> > --- database/schema/patch-2207-99-0.sql 1970-01-01 00:00:00 +0000
> > +++ database/schema/patch-2207-99-0.sql 2010-03-12 07:20:45 +0000
> > @@ -0,0 +1,15 @@
> > +SET client_min_messages=ERROR;
> > +
> > +CREATE TABLE BinaryPackageReleaseDownloadCount (
> > + id serial PRIMARY KEY,
> > + archive integer NOT NULL REFERENCES Archive,
> > + binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
> > + day date NOT NULL,
> > + country integer,
> > + count integer NOT NULL
> > +);
> > +
> > +ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binar...

[Stuart Bishop (stub)]

The FOREIGN KEY reference to Country is correct. Thanks.

review: Approve (db)

[Björn Tillenius (bjornt)]

review: Approve (db)

[Henning Eggers (henninge)]

All is well. Thank you. ;-)

review: Approve (code)

[Julian Edwards (julian-edwards)]

review: Approve

=== added file 'database/schema/patch-2207-36-0.sql'
--- database/schema/patch-2207-36-0.sql	1970-01-01 00:00:00 +0000
+++ database/schema/patch-2207-36-0.sql	2010-03-15 10:41:18 +0000
@@ -0,0 +1,15 @@
+SET client_min_messages=ERROR;
+
+CREATE TABLE BinaryPackageReleaseDownloadCount (
+    id serial PRIMARY KEY,
+    archive integer NOT NULL REFERENCES Archive,
+    binary_package_release integer NOT NULL REFERENCES BinaryPackageRelease,
+    day date NOT NULL,
+    country integer REFERENCES Country,
+    count integer NOT NULL
+);
+
+ALTER TABLE BinaryPackageReleaseDownloadCount ADD CONSTRAINT binarypackagereleasedownloadcount__archive__binary_package_release__day__country__key
+     UNIQUE (archive, binary_package_release, day, country);
+
+INSERT INTO LaunchpadDatabaseRevision VALUES (2207, 36, 0);

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2010-03-15 06:23:18 +0000
+++ database/schema/security.cfg	2010-03-15 10:41:18 +0000
@@ -131,6 +131,7 @@
 public.authtoken                        = SELECT, INSERT, UPDATE, DELETE
 public.binaryandsourcepackagenameview   = SELECT
 public.binarypackagepublishinghistory   = SELECT
+public.binarypackagereleasedownloadcount= SELECT, INSERT, UPDATE
 public.bountysubscription               = SELECT, INSERT, UPDATE, DELETE
 public.branchrevision                   = SELECT, INSERT, UPDATE, DELETE
 public.branch                           = SELECT, INSERT, UPDATE, DELETE

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2010-03-12 06:19:52 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2010-03-15 10:41:18 +0000
@@ -647,6 +647,20 @@
             archive.
         """
 
+    def updatePackageDownloadCount(bpr, day, country, count):
+        """Update the daily download count for a given package.
+
+        :param bpr: The `IBinaryPackageRelease` to update the count for.
+        :param day: The date to update the count for.
+        :param country: The `ICountry` to update the count for.
+        :param count: The new download count.
+
+        If there's no matching `IBinaryPackageReleaseDownloadCount` entry,
+        we create one with the given count.  Otherwise we just increase the
+        count of the existing one by the given amount.
+        """
+
+
 class IArchiveView(IHasBuildRecords):
     """Archive interface for operations restricted by view privilege."""
 

=== modified file 'lib/lp/soyuz/interfaces/binarypackagerelease.py'
--- lib/lp/soyuz/interfaces/binarypackagerelease.py	2009-06-25 04:06:00 +0000
+++ lib/lp/soyuz/interfaces/binarypackagerelease.py	2010-03-15 10:41:18 +0000
@@ -11,17 +11,18 @@
     'BinaryPackageFileType',
     'BinaryPackageFormat',
     'IBinaryPackageRelease',
+    'IBinaryPackageReleaseDownloadCount',
     'IBinaryPackageReleaseSet',
     ]
 
-from zope.schema import Bool, Int, Text, TextLine, Datetime
+from lazr.enum import DBEnumeratedType, DBItem
+from lazr.restful.fields import Reference
+from zope.schema import Bool, Choice, Date, Int, Text, TextLine, Datetime
 from zope.interface import Interface, Attribute
 
 from canonical.launchpad import _
-
 from canonical.launchpad.validators.version import valid_debian_version
-
-from lazr.enum import DBEnumeratedType, DBItem
+from lp.soyuz.interfaces.archive import IArchive
 
 
 class IBinaryPackageRelease(Interface):
@@ -79,6 +80,7 @@
         argument remains untouched.
         """
 
+
 class IBinaryPackageReleaseSet(Interface):
     """A set of binary packages"""
 
@@ -91,6 +93,23 @@
         """Get an BinaryPackageRelease in a DistroSeries by its name"""
 
 
+class IBinaryPackageReleaseDownloadCount(Interface):
+    """Daily download count of a binary package release in an archive."""
+
+    archive = Reference(
+        title=_('The archive'), schema=IArchive, required=True,
+        readonly=True)
+    binary_package_release = Reference(
+        title=_('The binary package release'), schema=IBinaryPackageRelease,
+        required=True, readonly=True)
+    day = Date(
+        title=_('The day of the downloads'), required=True, readonly=True)
+    count = Int(
+        title=_('The number of downloads'), required=True, readonly=False)
+    country = Choice(
+        title=_('Country'), required=False, vocabulary='CountryName')
+
+
 class BinaryPackageFileType(DBEnumeratedType):
     """Binary Package File Type
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-03-12 06:19:52 +0000
+++ lib/lp/soyuz/model/archive.py	2010-03-15 10:41:18 +0000
@@ -39,6 +39,8 @@
 from lp.soyuz.model.archivedependency import ArchiveDependency
 from lp.soyuz.model.archiveauthtoken import ArchiveAuthToken
 from lp.soyuz.model.archivesubscriber import ArchiveSubscriber
+from lp.soyuz.model.binarypackagerelease import (
+    BinaryPackageReleaseDownloadCount)
 from lp.soyuz.model.build import Build
 from lp.soyuz.model.distributionsourcepackagecache import (
     DistributionSourcePackageCache)
@@ -1327,6 +1329,19 @@
         result_set.config(distinct=True).order_by(SourcePackageRelease.id)
         return result_set
 
+    def updatePackageDownloadCount(self, bpr, day, country, count):
+        """See `IArchive`."""
+        store = Store.of(self)
+        entry = store.find(
+            BinaryPackageReleaseDownloadCount, archive=self,
+            binary_package_release=bpr, day=day, country=country).one()
+        if entry is None:
+            entry = BinaryPackageReleaseDownloadCount(
+                archive=self, binary_package_release=bpr, day=day,
+                country=country, count=count)
+        else:
+            entry.count += count
+
     def _setBuildStatuses(self, status):
         """Update the pending Build Jobs' status for this archive."""
 

=== modified file 'lib/lp/soyuz/model/binarypackagerelease.py'
--- lib/lp/soyuz/model/binarypackagerelease.py	2009-06-25 04:06:00 +0000
+++ lib/lp/soyuz/model/binarypackagerelease.py	2010-03-15 10:41:18 +0000
@@ -4,18 +4,23 @@
 # pylint: disable-msg=E0611,W0212
 
 __metaclass__ = type
-__all__ = ['BinaryPackageRelease', 'BinaryPackageReleaseSet']
+__all__ = [
+    'BinaryPackageRelease',
+    'BinaryPackageReleaseDownloadCount',
+    'BinaryPackageReleaseSet'
+    ]
 
 
 from zope.interface import implements
 
 from sqlobject import StringCol, ForeignKey, IntCol, SQLMultipleJoin, BoolCol
+from storm.locals import Date, Int, Reference, Storm
 
 from canonical.database.sqlbase import SQLBase, quote, sqlvalues, quote_like
 
 from lp.soyuz.interfaces.binarypackagerelease import (
     BinaryPackageFileType, BinaryPackageFormat, IBinaryPackageRelease,
-    IBinaryPackageReleaseSet)
+    IBinaryPackageReleaseDownloadCount, IBinaryPackageReleaseSet)
 from lp.soyuz.interfaces.publishing import (
     PackagePublishingPriority, PackagePublishingStatus)
 from canonical.database.enumcol import EnumCol
@@ -199,3 +204,29 @@
 
         return query, clauseTables
 
+
+class BinaryPackageReleaseDownloadCount(Storm):
+    """See `IBinaryPackageReleaseDownloadCount`."""
+
+    implements(IBinaryPackageReleaseDownloadCount)
+    __storm_table__ = 'BinaryPackageReleaseDownloadCount'
+
+    id = Int(primary=True)
+    archive_id = Int(name='archive', allow_none=False)
+    archive = Reference(archive_id, 'Archive.id')
+    binary_package_release_id = Int(
+        name='binary_package_release', allow_none=False)
+    binary_package_release = Reference(
+        binary_package_release_id, 'BinaryPackageRelease.id')
+    day = Date(allow_none=False)
+    country_id = Int(name='country', allow_none=True)
+    country = Reference(country_id, 'Country.id')
+    count = Int(allow_none=False)
+
+    def __init__(self, archive, binary_package_release, day, country, count):
+        super(BinaryPackageReleaseDownloadCount, self).__init__()
+        self.archive = archive
+        self.binary_package_release = binary_package_release
+        self.day = day
+        self.country = country
+        self.count = count

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2010-03-12 06:19:52 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2010-03-15 10:41:18 +0000
@@ -3,7 +3,7 @@
 
 """Test Archive features."""
 
-from datetime import datetime, timedelta
+from datetime import date, datetime, timedelta
 import pytz
 import unittest
 
@@ -19,6 +19,7 @@
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.person import IPersonSet
 from lp.services.job.interfaces.job import JobStatus
+from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.soyuz.interfaces.archive import (
     IArchiveSet, ArchivePurpose, CannotSwitchPrivacy)
 from lp.soyuz.interfaces.archivearch import IArchiveArchSet
@@ -26,6 +27,8 @@
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.soyuz.model.build import Build
+from lp.soyuz.model.binarypackagerelease import (
+    BinaryPackageReleaseDownloadCount)
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import TestCaseWithFactory
 
@@ -562,6 +565,105 @@
         self.assertEqual('0.5.11~ppa1', pubs[0].source_package_version)
 
 
+class TestUpdatePackageDownloadCount(TestCaseWithFactory):
+    """Ensure that updatePackageDownloadCount works as expected."""
+
+    layer = LaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestUpdatePackageDownloadCount, self).setUp()
+        self.publisher = SoyuzTestPublisher()
+        self.publisher.prepareBreezyAutotest()
+
+        self.store = getUtility(IStoreSelector).get(
+            MAIN_STORE, DEFAULT_FLAVOR)
+
+        self.archive = self.factory.makeArchive()
+        self.bpr_1 = self.publisher.getPubBinaries(
+                archive=self.archive)[0].binarypackagerelease
+        self.bpr_2 = self.publisher.getPubBinaries(
+                archive=self.archive)[0].binarypackagerelease
+
+        country_set = getUtility(ICountrySet)
+        self.australia = country_set['AU']
+        self.new_zealand = country_set['NZ']
+
+    def assertCount(self, count, archive, bpr, day, country):
+        self.assertEqual(count, self.store.find(
+            BinaryPackageReleaseDownloadCount,
+            archive=archive, binary_package_release=bpr,
+            day=day, country=country).one().count)
+
+    def test_creates_new_entry(self):
+        # The first update for a particular archive, package, day and
+        # country will create a new BinaryPackageReleaseDownloadCount
+        # entry.
+        day = date(2010, 2, 20)
+        self.assertIs(None, self.store.find(
+            BinaryPackageReleaseDownloadCount,
+            archive=self.archive, binary_package_release=self.bpr_1,
+            day=day, country=self.australia).one())
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
+
+    def test_reuses_existing_entry(self):
+        # A second update will simply add to the count on the existing
+        # BPRDC.
+        day = date(2010, 2, 20)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 3)
+        self.assertCount(13, self.archive, self.bpr_1, day, self.australia)
+
+    def test_differentiates_between_countries(self):
+        # A different country will cause a new entry to be created.
+        day = date(2010, 2, 20)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.new_zealand, 3)
+
+        self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
+        self.assertCount(3, self.archive, self.bpr_1, day, self.new_zealand)
+
+    def test_country_can_be_none(self):
+        # The country can be None, indicating that it is unknown.
+        day = date(2010, 2, 20)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, None, 3)
+
+        self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
+        self.assertCount(3, self.archive, self.bpr_1, day, None)
+
+    def test_differentiates_between_days(self):
+        # A different date will also cause a new entry to be created.
+        day = date(2010, 2, 20)
+        another_day = date(2010, 2, 21)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, another_day, self.australia, 3)
+
+        self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
+        self.assertCount(
+            3, self.archive, self.bpr_1, another_day, self.australia)
+
+    def test_differentiates_between_bprs(self):
+        # And even a different package will create a new entry.
+        day = date(2010, 2, 20)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_1, day, self.australia, 10)
+        self.archive.updatePackageDownloadCount(
+            self.bpr_2, day, self.australia, 3)
+
+        self.assertCount(10, self.archive, self.bpr_1, day, self.australia)
+        self.assertCount(3, self.archive, self.bpr_2, day, self.australia)
+
+
 class TestARMBuildsAllowed(TestCaseWithFactory):
     """Ensure that ARM builds can be allowed and disallowed correctly."""