store: collect statistics for /charm-info and /charm/*

store: collect statistics for /charm-info and /charm/*



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

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

(do not edit description out of merge proposal)

[niemeyer, 2012-04-12 21:16:59]

Please take a look.

[rog, 2012-04-17 08:38:43]

LGTM; a couple of minor queries below.

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

https://codereview.appspot.com/6011054/diff/1/store/server.go#newcode83
store/server.go:83: skey = []string{"charm-missing", curl.Series, curl.Name,
curl.User}
why does it matter if curl.User is blank here? having a key of the same length
even when there's no user seems like it might be ok.

https://codereview.appspot.com/6011054/diff/1/store/server.go#newcode89
store/server.go:89: s.store.IncCounter(skey)
one thought (feel free to ignore): the way these are being used, i wonder if it
might make sense to make the argument to IncCounter ...string rather than
[]string.

then instead of assigning to skey above, you'd just call IncCounter directly
e.g.
if curl.User == "" {
    s.store.IncCounter("charm-info", curl.Series, curl.Name)
} else {
    s.store.IncCounter("charm-info", curl.Series, curl.Name, curl.User)
}

the calls below become simpler too.

SumCounter could perhaps get the same treatment for symmetry, except the prefix
argument would need to move to the start.

[niemeyer, 2012-04-17 12:47:29]

*** Submitted:

store: collect statistics for /charm-info and /charm/*

R=rog
CC=
https://codereview.appspot.com/6011054

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

https://codereview.appspot.com/6011054/diff/1/store/server.go#newcode83
store/server.go:83: skey = []string{"charm-missing", curl.Series, curl.Name,
curl.User}
On 2012/04/17 08:38:43, rog wrote:
> why does it matter if curl.User is blank here? having a key of the same length
> even when there's no user seems like it might be ok.

An empty string is still a token. There's a difference between matching
foo:bar:, foo::, and foo:*.

https://codereview.appspot.com/6011054/diff/1/store/server.go#newcode89
store/server.go:89: s.store.IncCounter(skey)
On 2012/04/17 08:38:43, rog wrote:
> one thought (feel free to ignore): the way these are being used, i wonder if
it
> might make sense to make the argument to IncCounter ...string rather than
> []string.

I considered that as well, but gave up on it due to the similarity between
SumCounter and IncCounter. SumCounter would have to have its prefix reordered to
be matched first, or not follow suit.

That said, it was probably not a great reasoning now that you mention it.
SumCounter isn't used nearly as often as IncCounter within the code. Maybe we
should change it.

I'd rather not do that now, though, if that's ok. We have a good stack of
branches on top of that API.

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

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

https://codereview.appspot.com/6011054/diff/1/store/server.go#newcode83
store/server.go:83: skey = []string{"charm-missing", curl.Series, curl.Name,
curl.User}
On 2012/04/17 12:47:29, niemeyer wrote:
> On 2012/04/17 08:38:43, rog wrote:
> > why does it matter if curl.User is blank here? having a key of the same
length
> > even when there's no user seems like it might be ok.
> 
> An empty string is still a token. There's a difference between matching
> foo:bar:, foo::, and foo:*.

an empty string is still a token, but why do you mind? letting "charm-missing::"
match charms with no user name seems like it might be ok.