Add dated metric and a source for them.

Add dated metric and a source for them.



https://code.launchpad.net/~benji/charmworld/bundle-deploy-count-document/+merge/193618

(do not edit description out of merge proposal)

[benji, 2013-11-01 15:03:47]

Please take a look.

[bac, 2013-11-01 19:16:37]

Very nice branch Benji.  I've made some suggestions but it is LGTM with those
changes.

Willing to be discuss the whole today() thing more.

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

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1853
charmworld/models.py:1853: last 24 hours" are not supported.  Also, at midnight,
the count for
UTC?

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1856
charmworld/models.py:1856: 
I suggest providing a helper fcn that returns datetime.utcnow().date() anytime
we need a today.

It isn't *just* about testing.  If we don't manage time zone ourselves we are
reliant on what the local system thinks about timezones.  Luckily Ubuntu cloud
images currently always use UTC but it seems bad to rely on that when we don't
need to.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1859
charmworld/models.py:1859: self.start = start or datetime.date.today()
Say you started with today, you'd have
b[0] = today
b[1] = yesterday
...
b[n] = today - n

So calling b[0] 'start' is confusing since it is the latest bucket.  

I'll try to get over it as I go through the review.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1862
charmworld/models.py:1862: # A bucket for all the numbers that proceed the
oldest daily bucket.
precede?

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1869
charmworld/models.py:1869: overflow += buckets[-1]
overflow += buckets.pop() and delete the del

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1884
charmworld/models.py:1884: while start != target_date:
i'd prefer start < target_date though i know it shouldn't matter.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1897
charmworld/models.py:1897: """Increment the count for a paricular day (today if
not given)."""
typo: particular

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1905
charmworld/models.py:1905: if bucket_index is None:
How would this ever be none?  I can see it being > METRIC_DAYS_TO_KEEP but not
none.

On IRC you agreed it is broken and will update the tests and code.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1930
charmworld/models.py:1930: total = sum(day_buckets)
I'd move this below the error check so you don't compute a sum and then throw it
away.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1933
charmworld/models.py:1933: if len(day_buckets) < num_days:
It seems this condition could be determined easier than making a slice first
e.g. if 
bucket_index + num_days > METRIC_DAYS_TO_KEEP

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1965
charmworld/models.py:1965: return cls(db.dated_metrics)
The utility of this method is not obvious to me.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1968
charmworld/models.py:1968: """Save a metric under a given key."""
I'm not sure what a metric and a key are at this point.

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

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2308: metric.get_total()
Don't you care that a fresh metric has a total of 0?

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2339: for x in range(METRIC_DAYS_TO_KEEP):
why not use xrange?

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2397: })
This is nice.

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2516: self.assertEqual(source.collection.name,
'dated_metrics')
Oh, now I see.

[benji, 2013-11-04 17:56:53]

Thanks for the good review.  I have addressed your comments and will be landing
soon.

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

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1856
charmworld/models.py:1856: 
On 2013/11/01 19:16:37, bac wrote:
> I suggest providing a helper fcn that returns datetime.utcnow().date() anytime
> we need a today.
> 
> It isn't *just* about testing.  If we don't manage time zone ourselves we are
> reliant on what the local system thinks about timezones.

I'm fine with using datetime.utcnow().date() instead of
datetime.date.today().  (Note that neither produce an object with any
idea of time zone, dates do not have a time zone associated with them).

One issue we will have either way is that users in far east time zones
(e.g., Japan, China, and Australia) will always see a very low count of
"today's" downloads, because UTC will have recently crossed midnight
while people in the west coast of the US will see abnormally high
numbers because they benefit from an almost full day of downloads being
counted.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1859
charmworld/models.py:1859: self.start = start or datetime.date.today()
On 2013/11/01 19:16:37, bac wrote:
> Say you started with today, you'd have
> b[0] = today
> b[1] = yesterday
> ...
> b[n] = today - n
> 
> So calling b[0] 'start' is confusing since it is the latest bucket.  
> 
> I'll try to get over it as I go through the review.

I thought about changing it to "latest" but it didn't seem like an overall
improvement.  I couldn't think of a better name.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1862
charmworld/models.py:1862: # A bucket for all the numbers that proceed the
oldest daily bucket.
On 2013/11/01 19:16:37, bac wrote:
> precede?

Done.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1869
charmworld/models.py:1869: overflow += buckets[-1]
On 2013/11/01 19:16:37, bac wrote:
> overflow += buckets.pop() and delete the del

Nice.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1884
charmworld/models.py:1884: while start != target_date:
On 2013/11/01 19:16:37, bac wrote:
> i'd prefer start < target_date though i know it shouldn't matter.

I see that point of view.  I'm more of the mindset that if a bug causes us to
"miss" the point where start == target_date then it is better to cause an
infinite loop than to paper over the bug.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1897
charmworld/models.py:1897: """Increment the count for a paricular day (today if
not given)."""
On 2013/11/01 19:16:37, bac wrote:
> typo: particular

Done.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1905
charmworld/models.py:1905: if bucket_index is None:
On 2013/11/01 19:16:37, bac wrote:
> How would this ever be none?  I can see it being > METRIC_DAYS_TO_KEEP but not
> none.

Good catch.  I wrote a test that exposed the bug and then fixed it.  I really
wish we would add a test coverage reporting system so bugs like this won't
depend on a good reviewer noticing them.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1930
charmworld/models.py:1930: total = sum(day_buckets)
On 2013/11/01 19:16:37, bac wrote:
> I'd move this below the error check so you don't compute a sum and then throw
it
> away.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1930
charmworld/models.py:1930: total = sum(day_buckets)
On 2013/11/01 19:16:37, bac wrote:
> I'd move this below the error check so you don't compute a sum and then throw
it
> away.

Done.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1933
charmworld/models.py:1933: if len(day_buckets) < num_days:
On 2013/11/01 19:16:37, bac wrote:
> It seems this condition could be determined easier than making a slice first
> e.g. if 
> bucket_index + num_days > METRIC_DAYS_TO_KEEP

True.  The reason I didn't do that is that I want to avoid depending on the
number of buckets actually matching METRIC_DAYS_TO_KEEP.  The reason is that I
expect the "constant" to change (the only constant is change, right?).  That is
why METRIC_DAYS_TO_KEEP is only referenced in the __init__ and nowhere else.

I've added a comment to the effect of the above to the code.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1965
charmworld/models.py:1965: return cls(db.dated_metrics)
On 2013/11/01 19:16:37, bac wrote:
> The utility of this method is not obvious to me.

I think you realized it after reading the tests.  I've used this pattern several
times in charmworld and it seems to be working well.  The intent is to remove
the need for instances to know about the DB or its structure and instead have
them worry only about collections.  This way there is lower coupling and the
tests are easier to write.

https://codereview.appspot.com/20820043/diff/1/charmworld/models.py#newcode1968
charmworld/models.py:1968: """Save a metric under a given key."""
On 2013/11/01 19:16:37, bac wrote:
> I'm not sure what a metric and a key are at this point.

I have added some explanation to the method's docstring.

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

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2308: metric.get_total()
On 2013/11/01 19:16:37, bac wrote:
> Don't you care that a fresh metric has a total of 0?

For this test I was just showing that the method exists and doesn't throw an
exception (perhaps this test is an unwanted artifact of doing TDD).  I have
added another test showing that the initial total is zero.

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2339: for x in range(METRIC_DAYS_TO_KEEP):
On 2013/11/01 19:16:37, bac wrote:
> why not use xrange?

Because a cosmic ray struck my neocortex causing that letter not to be typed by
my fingers.

Fixed (in several places, darn cosmic rays).

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2397: })
On 2013/11/01 19:16:37, bac wrote:
> This is nice.

I hit upon this pattern while working on the bundle model code.  It seems like a
good match for document stores like MongoDB.

https://codereview.appspot.com/20820043/diff/1/charmworld/tests/test_models.p...
charmworld/tests/test_models.py:2516: self.assertEqual(source.collection.name,
'dated_metrics')
On 2013/11/01 19:16:37, bac wrote:
> Oh, now I see.

:)