Merge lp:~benji/charmworld/bundle-deploy-metric-api into lp:charmworld

[Benji York]

Add an API for bundle metrics

https://codereview.appspot.com/23740043/

R=bac

Add an API for bundle metrics

https://codereview.appspot.com/23740043/

[Benji York (benji)]

Reviewers: mp+194564_code.launchpad.net,

Message:
Please take a look.

Description:
Add an API for bundle metrics

https://code.launchpad.net/~benji/charmworld/bundle-deploy-metric-api/+merge/194564

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/23740043/

Affected files (+173, -22 lines):
   A [revision details]
   M charmworld/models.py
   M charmworld/testing/factory.py
   M charmworld/tests/test_models.py
   M charmworld/views/api/__init__.py
   A charmworld/views/tests/test_metrics_api.py

[Benji York (benji)]

https://codereview.appspot.com/23740043/diff/1/charmworld/models.py
File charmworld/models.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/models.py#newcode1
charmworld/models.py:1: # Copyright 2012, 2013 Marco Ceppi, Canonical
Ltd. This software is
All the changes in this file are actually from my previous branch that
was apparently not merged. They have been reviewed previously.

https://codereview.appspot.com/23740043/diff/1/charmworld/tests/test_models.py
File charmworld/tests/test_models.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/tests/test_models.py#newcode1
charmworld/tests/test_models.py:1: # Copyright 2012, 2013 Marco Ceppi,
Canonical Ltd. This software is
All the changes in this file are actually from my previous branch that
was apparently not merged. They have been reviewed previously.

https://codereview.appspot.com/23740043/

[Brad Crittenden (bac)]

LGTMB

https://codereview.appspot.com/23740043/diff/1/charmworld/testing/factory.py
File charmworld/testing/factory.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/testing/factory.py#newcode365
charmworld/testing/factory.py:365: makeBundle = make_bundle
You sure you'd not like to just fix them?

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py
File charmworld/views/api/__init__.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py#newcode511
charmworld/views/api/__init__.py:511: bundle_metric_types =
('deployments')
I think you meant for this to be a tuple. One of my least favorite
warts.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py#newcode521
charmworld/views/api/__init__.py:521: if metric_name not in
self.bundle_metric_types:
Huh, it works anyway.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py
File charmworld/views/tests/test_metrics_api.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py#newcode2
charmworld/views/tests/test_metrics_api.py:2:
del

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py#newcode20
charmworld/views/tests/test_metrics_api.py:20:
Thank you for not piling onto the other test files.

https://codereview.appspot.com/23740043/

[lp:~benji/charmworld/bundle-deploy-metric-api]

456. By Benji York on 2013-11-08

review fixes

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

[Richard Harding (rharding)]

+1 on brad's point of the tuple.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py
File charmworld/views/api/__init__.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py#newcode521
charmworld/views/api/__init__.py:521: if metric_name not in
self.bundle_metric_types:
On 2013/11/08 19:03:29, bac wrote:
> Huh, it works anyway.

metric types will be the string and you can check if one string is in
another. It just so happens to work because deployments is in the string
deployments.

It needs a comma up above.

https://codereview.appspot.com/23740043/

[Benji York (benji)]

I forgot to publish my draft comments on Friday. I know everyone has
been waiting patiently for them.

https://codereview.appspot.com/23740043/diff/1/charmworld/testing/factory.py
File charmworld/testing/factory.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/testing/factory.py#newcode365
charmworld/testing/factory.py:365: makeBundle = make_bundle
On 2013/11/08 19:03:29, bac wrote:
> You sure you'd not like to just fix them?

I'd like to... but I figured a mechanical branch later would be better.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py
File charmworld/views/api/__init__.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py#newcode511
charmworld/views/api/__init__.py:511: bundle_metric_types =
('deployments')
Good eye. Fixed.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/api/__init__.py#newcode521
charmworld/views/api/__init__.py:521: if metric_name not in
self.bundle_metric_types:
On 2013/11/08 19:03:29, bac wrote:
> Huh, it works anyway.

Well, I learned something new today.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py
File charmworld/views/tests/test_metrics_api.py (right):

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py#newcode2
charmworld/views/tests/test_metrics_api.py:2:
On 2013/11/08 19:03:29, bac wrote:
> del

Done.

https://codereview.appspot.com/23740043/diff/1/charmworld/views/tests/test_metrics_api.py#newcode20
charmworld/views/tests/test_metrics_api.py:20:
On 2013/11/08 19:03:29, bac wrote:
> Thank you for not piling onto the other test files.

Yeah, we need to start breaking those files apart. A few are way over
the weight limit.

https://codereview.appspot.com/23740043/

=== modified file 'charmworld/models.py'
--- charmworld/models.py	2013-11-08 15:20:57 +0000
+++ charmworld/models.py	2013-11-08 19:14:48 +0000
@@ -1863,6 +1863,10 @@
     return hashes
 
 
+def utc_today():
+    return datetime.datetime.utcnow().date()
+
+
 class DatedMetric:
     """A metric which is recorded per day as well as totaled over all time.
 
@@ -1882,21 +1886,24 @@
 
     def __init__(self, start=None, buckets=None, overflow=0):
         # The date of the zeroth daily bucket.
-        self.start = start or datetime.date.today()
-        # A sequence of buckets for many days proceeding the starting day.
+        self.start = start or utc_today()
+
+        # A sequence of buckets for many days preceding the starting day.
         self.buckets = buckets or [0] * METRIC_DAYS_TO_KEEP
-        # A bucket for all the numbers that proceed the oldest daily bucket.
+        # The above reference to METRIC_DAYS_TO_KEEP should be the only place
+        # in the (non-test) code base that this particular constant is
+        # referenced.  This is to protect against the "constant" changing and
+        # causing subtile bugs around already-existing database entries.
+
+        # A bucket for all the numbers that precede the oldest daily bucket.
         self.overflow = overflow
 
     @staticmethod
     def _shift_buckets(buckets, overflow):
         """Make a new bucket at the head of the list, accumulating overflow."""
-        # Put the oldest bucket's count into the overflow.
-        overflow += buckets[-1]
-        # Remove the oldest bucket.
-        del buckets[-1]
-        # Put a new bucket on the head of the list and update the starting
-        # day.
+        # Remove the oldest bucket and put its count into the overflow.
+        overflow += buckets.pop()
+        # Add a new bucket at the head of the list.
         buckets.insert(0, 0)
         return buckets, overflow
 
@@ -1920,21 +1927,22 @@
         return (start - day).days
 
     def increment(self, day=None):
-        """Increment the count for a paricular day (today if not given)."""
-        day = day or datetime.date.today()
+        """Increment the count for a particular day (today if not given)."""
+        day = day or utc_today()
         # If we are being asked to increment a day's counter but that day's
         # bucket does not yet exist, make it.
         if day > self.start:
             self.start, self.buckets, self.overflow = self._fill_in_buckets(
                 day, self.start, self.buckets, self.overflow)
         bucket_index = self._find_day_offset(day, self.start)
-        if bucket_index is None:
+        if bucket_index > len(self.buckets):
             # We couldn't find the day's bucket so the increment must go
             # into the overflow bucket.
             self.overflow += 1
             return
-        # We found the requested day's bucket, now increment its count.
-        self.buckets[bucket_index] += 1
+        else:
+            # We found the requested day's bucket, now increment its count.
+            self.buckets[bucket_index] += 1
 
     def get_total(self):
         """Fetch the total number of increments over all time."""
@@ -1953,15 +1961,14 @@
         bucket_index = cls._find_day_offset(day, start)
         # Select the buckets that are in the requested date range.
         day_buckets = buckets[bucket_index:bucket_index + num_days]
-        total = sum(day_buckets)
         # If there were more days requested than we have data for, raise an
         # error.
         if len(day_buckets) < num_days:
             raise ValueError('data not available')
-        return total
+        return sum(day_buckets)
 
     def get_range_total(self, num_days, day=None):
-        day = day or datetime.date.today()
+        day = day or utc_today()
         return self._get_range_total(
             num_days, day, self.start, self.buckets, self.overflow)
 
@@ -2001,6 +2008,8 @@
     def retrieve(self, key):
         """Fetch a metric by its key."""
         document = self.collection.find_one({'_id': quote_key(key)})
+        if document is None:
+            raise KeyError(key)
         # Since MongoDB doesn't know about dates we have to parse the start
         # out of a string and into a datetime.date instance.
         start = datetime.date(*map(int, document['start'].split('-')))

=== modified file 'charmworld/testing/factory.py'
--- charmworld/testing/factory.py	2013-10-24 17:24:43 +0000
+++ charmworld/testing/factory.py	2013-11-08 19:14:48 +0000
@@ -341,7 +341,7 @@
     return bundle_doc
 
 
-def makeBundle(db, *args, **kwargs):
+def make_bundle(db, *args, **kwargs):
     with_basket = False
     if 'with_basket' in kwargs:
         with_basket = True
@@ -361,6 +361,10 @@
     return bundle, bundle_data
 
 
+# Backwards compatability with incorrectly-named function.
+makeBundle = make_bundle
+
+
 def make_charm_file(db, charm, path, content=None):
     fs = getfs(db)
     charm_file = CharmFileSet.make_charm_file(

=== modified file 'charmworld/tests/test_models.py'
--- charmworld/tests/test_models.py	2013-11-07 21:37:09 +0000
+++ charmworld/tests/test_models.py	2013-11-08 19:14:48 +0000
@@ -626,7 +626,7 @@
 
     def test_changes_since(self):
         changes = [factory.makeChange(created=date(2013, 6, num))
-                   for num in range(1, 31)]
+                   for num in xrange(1, 31)]
         charm = Charm(factory.get_charm_json(changes=changes))
         self.assertEqual([], charm.changes_since(datetime(2013, 7, 1)))
         self.assertEqual(30, len(charm.changes_since(datetime(2013, 5, 1))))
@@ -2338,6 +2338,11 @@
         metric = DatedMetric()
         metric.get_total()
 
+    def test_fresh_metric_has_zero_total(self):
+        # If a metric has never been incremented, it has a total of zero.
+        metric = DatedMetric()
+        self.assertEqual(metric.get_total(), 0)
+
     def test_incrementing(self):
         # A metric can be incremented, which means adding one to a particular
         # day's count.
@@ -2367,7 +2372,7 @@
         # No values have been added, so the starting day hasn't changed.
         assert metric.start == original_starting_day
         # Add enough records to fill all the buckets.
-        for x in range(METRIC_DAYS_TO_KEEP):
+        for x in xrange(METRIC_DAYS_TO_KEEP):
             metric.increment(original_starting_day + timedelta(x))
         # The overflow is still empty because up until this point there have
         # been enough buckets to hold all the days.
@@ -2393,7 +2398,7 @@
         # The total will include any values that have overflowed.
         original_starting_day = date(2000, 1, 1)
         metric = DatedMetric(original_starting_day)
-        for x in range(METRIC_DAYS_TO_KEEP + 10):
+        for x in xrange(METRIC_DAYS_TO_KEEP + 10):
             metric.increment(original_starting_day + timedelta(x))
         self.assertEqual(metric.get_total(), METRIC_DAYS_TO_KEEP + 10)
 
@@ -2405,7 +2410,7 @@
         # No values have been added, so the starting day hasn't changed.
         assert metric.start == original_starting_day
         # Add enough records to fill all the buckets.
-        for x in range(METRIC_DAYS_TO_KEEP):
+        for x in xrange(METRIC_DAYS_TO_KEEP):
             metric.increment(original_starting_day + timedelta(x))
             metric.increment(original_starting_day + timedelta(x))
         # Now that values have been added, the starting day has changed.
@@ -2427,6 +2432,17 @@
                 'overflow': metric.overflow,
             })
 
+    def test_incrementing_a_far_past_date(self):
+        # When incrementing a date far in the past (so long ago there are no
+        # buckets for a date that old), the increment goes into the overflow
+        # bucket.
+        original_starting_day = date(2000, 1, 1)
+        metric = DatedMetric(start=original_starting_day)
+        # The overflow starts empty.
+        assert metric.overflow == 0
+        metric.increment(date(1000, 1, 1))
+        self.assertEqual(metric.overflow, 1)
+
 
 class TestDatedMetricIncidentals(TestCase):
     """Herein lie the tests for the internal machinery of DatedMetrics"""
@@ -2558,6 +2574,12 @@
         retrieved = self.source.retrieve('metric-key')
         self.assertEqual(retrieved, metric)
 
+    def test_retrieving_a_metric_that_does_not_exist(self):
+        # If a key is given that does not have a matching metric, a KeyError
+        # is generated.
+        with self.assertRaises(KeyError):
+            self.source.retrieve('missing-key')
+
     def test_key_quoting(self):
         # Keys are quoted to make them MongoDB-safe.
         metric = DatedMetric(start=date(2000, 1, 1))

=== modified file 'charmworld/views/api/__init__.py'
--- charmworld/views/api/__init__.py	2013-11-05 13:13:01 +0000
+++ charmworld/views/api/__init__.py	2013-11-08 19:14:48 +0000
@@ -27,6 +27,8 @@
     BundledCharmDescription,
     Charm,
     CharmFileSet,
+    DatedMetric,
+    DatedMetricSource,
     FeaturedSource,
     getfs,
     QADataSource,
@@ -506,6 +508,38 @@
             headerlist=headerlist,
             status_code=200)
 
+    bundle_metric_types = ('deployments',)
+
+    def _bundle_metric(self, bundle_id, metric_name, operation):
+        """Handle a request for a bundle metric.
+
+        The metric_name will be something like "deployments" and the operation
+        can be "total" (retrieve the current total) or "increment" (add one
+        to the count).
+        """
+        # The metric names are a controlled vocabulary.
+        if metric_name not in self.bundle_metric_types:
+            raise HTTPNotFound
+        # Define the key that corresponds to the desired metric.
+        metric_key = '/'.join([metric_name, bundle_id])
+        # Create a metric source and look up (or create) the metric.
+        source = DatedMetricSource.from_db(self.request.db)
+        try:
+            metric = source.retrieve(metric_key)
+        except KeyError:
+            metric = DatedMetric()
+
+        # Do the thing we were asked to do.
+        if operation == 'total':
+            return metric.get_total()
+        elif operation == 'increment':
+            metric.increment()
+            # Save the newly updated metric to the database.
+            source.store(metric, metric_key)
+            return
+        else:
+            raise HTTPNotFound('unexpected operation: %r' % operation)
+
     def bundle(self, path=None):
         """Retrieve a bundle based on ID."""
         if path is None:
@@ -529,6 +563,9 @@
             return self._bundle_icon(bundle)
         elif trailing and trailing[0] == 'file':
             return self._bundle_file(bundle, '/'.join(trailing[1:]))
+        elif trailing and trailing[0] == 'metric' and len(trailing) == 3:
+            return json_response(
+                200, self._bundle_metric(bundle_id, *trailing[1:]))
         else:
             raise HTTPNotFound('/'.join(path))
 

=== modified file 'charmworld/views/tests/test_api.py'
--- charmworld/views/tests/test_api.py	2013-11-05 15:00:41 +0000
+++ charmworld/views/tests/test_api.py	2013-11-08 19:14:48 +0000
@@ -1,7 +1,5 @@
-# Copyright 2012, 2013 Canonical
-
-# Ltd.  This software is licensed under the GNU Affero General Public License
-# version 3 (see the file LICENSE).
+# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the GNU
+# Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 

=== added file 'charmworld/views/tests/test_metrics_api.py'
--- charmworld/views/tests/test_metrics_api.py	1970-01-01 00:00:00 +0000
+++ charmworld/views/tests/test_metrics_api.py	2013-11-08 19:14:48 +0000
@@ -0,0 +1,77 @@
+# Copyright 2012, 2013 Canonical
+
+# Ltd.  This software is licensed under the GNU Affero General Public License
+# version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+
+from charmworld.testing import factory
+from charmworld.models import (
+    DatedMetric,
+    DatedMetricSource,
+)
+from charmworld.views.tests.test_api import (
+    APITestBase,
+    API3Mixin,
+)
+from pyramid.httpexceptions import HTTPNotFound
+
+
+class TestAPI3BundleMetrics(API3Mixin, APITestBase):
+    """Test API 3's "charms" endpoint."""
+
+    def test_incrementing_bundle_metric_count(self):
+        # The API provides a way to increment a metric count.
+        bundle = factory.make_bundle(self.db)[0]
+        response = self.get_response(
+            'bundle', bundle.id + '/metric/deployments/increment')
+        self.assertEqual(200, response.status_code)
+
+        # Now look up the metric that was supposed to be incremented...
+        source = DatedMetricSource.from_db(self.db)
+        metric_key = 'deployments/' + bundle.id
+        metric = source.retrieve(metric_key)
+        # ...and check that it has the right value.
+        self.assertEqual(metric.get_total(), 1)
+
+    def test_fetching_bundle_metric_count(self):
+        # The API provides a way to retrieve a metric count.
+        bundle = factory.make_bundle(self.db)[0]
+        metric = DatedMetric()
+        metric.increment()
+        metric.increment()
+        source = DatedMetricSource.from_db(self.db)
+        source.store(metric, 'deployments/' + bundle.id)
+        response = self.get_response(
+            'bundle', bundle.id + '/metric/deployments/total')
+        self.assertEqual(200, response.status_code)
+        # ...and check that it has the right value.
+        self.assertEqual(response.body, '2')
+
+    def test_fetching_unknown_bundle_metric_type_generates_a_404(self):
+        # If a metric type is unknown, requesting it generates a 404.
+        bundle = factory.make_bundle(self.db)[0]
+        with self.assertRaises(HTTPNotFound):
+            self.get_response(
+                'bundle', bundle.id + '/metric/does-not-exist/total')
+
+    def test_requesting_unknown_operation_generates_a_404(self):
+        # If an operation is unknown, requesting it generates a 404.
+        bundle = factory.make_bundle(self.db)[0]
+        with self.assertRaises(HTTPNotFound):
+            self.get_response(
+                'bundle', bundle.id + '/metric/deployments/do-something-else')
+
+    def test_not_including_a_metric_name(self):
+        # If no metric name is included, a 404 is generated.
+        bundle = factory.make_bundle(self.db)[0]
+        with self.assertRaises(HTTPNotFound):
+            self.get_response('bundle', bundle.id + '/metric')
+
+    def test_sending_no_metric_operation_generates_a_404(self):
+        # If a metrics request does not provide an operation, a 404 is
+        # generated.
+        bundle = factory.make_bundle(self.db)[0]
+        with self.assertRaises(HTTPNotFound):
+            self.get_response('bundle', bundle.id + '/metric/deployments')