store: serve counters via the API

store: serve counters via the API

Will likely need to protect with a key soon, but
as a start this should be fine.

https://code.launchpad.net/~niemeyer/juju/go-store-stats-api/+merge/101672

Requires: https://code.launchpad.net/~niemeyer/juju/go-store-stats/+merge/101442

(do not edit description out of merge proposal)

[niemeyer, 2012-04-12 01:55:38]

Please take a look.

[fwereade, 2012-04-17 13:11:42]

LGTM

https://codereview.appspot.com/6010048/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode131
store/server.go:131: w.WriteHeader(http.StatusForbidden)
Shouldn't we just return here?

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode146
store/server.go:146: if err == nil {
Might be nice to invert this test and thereby write a more specific err to the
log here...

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode152
store/server.go:152: if err != nil {
...because "can't write content", below, is not *necessarily* correct.

[rog, 2012-04-17 13:16:21]

LGTM

https://codereview.appspot.com/6010048/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode125
store/server.go:125: }
how about:
path := r.URL.Path[len(base):]
then using it below (if path == "", etc).

another advantage of this: the logic won't have to change much when you move to
using a smarter mux.

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode138
store/server.go:138: if len(key) == 0 {
getting the sum of all counters seems like it might potentially be useful enough
to justify deleting the code (for seeing how much the counter logic is being
used, for example).

[niemeyer, 2012-04-17 16:46:41]

https://codereview.appspot.com/6010048/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode125
store/server.go:125: }
On 2012/04/17 13:16:21, rog wrote:
> how about:
> path := r.URL.Path[len(base):]

Sounds good.

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode131
store/server.go:131: w.WriteHeader(http.StatusForbidden)
On 2012/04/17 13:11:42, fwereade wrote:
> Shouldn't we just return here?

That'd mean pretending there's valid content for this page.

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode138
store/server.go:138: if len(key) == 0 {
On 2012/04/17 13:16:21, rog wrote:
> getting the sum of all counters seems like it might potentially be useful
enough
> to justify deleting the code (for seeing how much the counter logic is being
> used, for example).

This would be the most expensive of all counters, for a very artificial reason.
There's no point in counting something undefined.

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode146
store/server.go:146: if err == nil {
On 2012/04/17 13:11:42, fwereade wrote:
> Might be nice to invert this test and thereby write a more specific err to the
> log here...

True, will do.

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode152
store/server.go:152: if err != nil {
On 2012/04/17 13:11:42, fwereade wrote:
> ...because "can't write content", below, is not *necessarily* correct.

Agreed, thanks.

[niemeyer, 2012-04-17 17:04:44]

https://codereview.appspot.com/6010048/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/6010048/diff/1/store/server.go#newcode131
store/server.go:131: w.WriteHeader(http.StatusForbidden)
On 2012/04/17 16:46:41, niemeyer wrote:
> On 2012/04/17 13:11:42, fwereade wrote:
> > Shouldn't we just return here?
> 
> That'd mean pretending there's valid content for this page.
> 

Sorry, I misunderstood what you said. Yes, we definitely should return. Well
spotted.

[niemeyer, 2012-04-17 17:15:09]

*** Submitted:

store: serve counters via the API

Will likely need to protect with a key soon, but
as a start this should be fine.

R=fwereade, rog
CC=
https://codereview.appspot.com/6010048