store: add statistics listing

store: add statistics listing



https://code.launchpad.net/~niemeyer/juju-core/store-stats-listing/+merge/150868

(do not edit description out of merge proposal)

[niemeyer, 2013-02-27 17:49:42]

Please take a look.

[rog, 2013-02-27 18:40:12]

LGTM with the sort fix and the error return fix applied.
feel free to ignore the other remarks to taste.

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

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode199
store/server.go:199: w.WriteHeader(http.StatusInternalServerError)
return
?

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode224
store/server.go:224: result = append(result, resultItem{string(buf),
entry.Count})
why turn the key into a string when you're about to turn it back into a []byte?

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode230
store/server.go:230: for i := 0; i < len(spaces); i++ {
for i := range spaces {
?

or perhaps just add the spaces inside the loop below
and avoid the creation of the spaces temporary.

for j := len(item.key); j < maxKeyLength+2; j++ {
    buf = append(buf, ' ')
}

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

https://codereview.appspot.com/7387054/diff/1/store/store.go#newcode301
store/store.go:301: func (s *Store) ListCounters(keyPrefix []string) ([]Counter,
error) {
nice

https://codereview.appspot.com/7387054/diff/1/store/store.go#newcode322
store/store.go:322: "    var k = this.k;"+
a little thought:
if you do
"var n = %d;"
here, and use n instead of %d below, you only need one len(skey) argument to the
Sprintf and the javascript is perhaps *slightly* more maintainable.
*shrug*

https://codereview.appspot.com/7387054/diff/1/store/store.go#newcode394
store/store.go:394: }
this needs a fix when len(kj) > len(ki) as discussed online.

[niemeyer, 2013-02-27 18:48:03]

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

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode199
store/server.go:199: w.WriteHeader(http.StatusInternalServerError)
On 2013/02/27 18:40:12, rog wrote:
> return
> ?

Good catch, thanks.

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode224
store/server.go:224: result = append(result, resultItem{string(buf),
entry.Count})
On 2013/02/27 18:40:12, rog wrote:
> why turn the key into a string when you're about to turn it back into a
[]byte?

Why does it matter? It must be copied either way, because buf is reused.

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode230
store/server.go:230: for i := 0; i < len(spaces); i++ {
On 2013/02/27 18:40:12, rog wrote:
> for i := range spaces {
> ?
> 
> or perhaps just add the spaces inside the loop below
> and avoid the creation of the spaces temporary.
> 
> for j := len(item.key); j < maxKeyLength+2; j++ {
>     buf = append(buf, ' ')
> }

Is it cheaper to call append repeatedly, or to call it once with more content?
I'm betting the latter, opting for the nice-looking code, and saving my time not
benchmarking it. Can't possibly be wrong.

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

https://codereview.appspot.com/7387054/diff/1/store/store.go#newcode322
store/store.go:322: "    var k = this.k;"+
On 2013/02/27 18:40:12, rog wrote:
> a little thought:
> if you do
> "var n = %d;"
> here, and use n instead of %d below, you only need one len(skey) argument to
the
> Sprintf and the javascript is perhaps *slightly* more maintainable.
> *shrug*

Doesn't feel relevant.

https://codereview.appspot.com/7387054/diff/1/store/store.go#newcode394
store/store.go:394: }
On 2013/02/27 18:40:12, rog wrote:
> this needs a fix when len(kj) > len(ki) as discussed online.

Good catch as well, thank you!

[rog, 2013-02-27 18:53:16]

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

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode224
store/server.go:224: result = append(result, resultItem{string(buf),
entry.Count})
On 2013/02/27 18:48:03, niemeyer wrote:
> On 2013/02/27 18:40:12, rog wrote:
> > why turn the key into a string when you're about to turn it back into a
> []byte?
> 
> Why does it matter? It must be copied either way, because buf is reused.

good point. ignore me.

https://codereview.appspot.com/7387054/diff/1/store/server.go#newcode230
store/server.go:230: for i := 0; i < len(spaces); i++ {
On 2013/02/27 18:48:03, niemeyer wrote:
> On 2013/02/27 18:40:12, rog wrote:
> > for i := range spaces {
> > ?
> > 
> > or perhaps just add the spaces inside the loop below
> > and avoid the creation of the spaces temporary.
> > 
> > for j := len(item.key); j < maxKeyLength+2; j++ {
> >     buf = append(buf, ' ')
> > }
> 
> Is it cheaper to call append repeatedly, or to call it once with more content?
> I'm betting the latter, opting for the nice-looking code, and saving my time
not
> benchmarking it. Can't possibly be wrong.

that's fine. i still think it should probably be for i := range spaces though.

[dimitern, 2013-02-27 21:00:53]

Very nice! LGTM, just a couple of comments.

https://codereview.appspot.com/7387054/diff/6001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/7387054/diff/6001/store/server.go#newcode167
store/server.go:167: w.WriteHeader(http.StatusForbidden)
Why forbidden? Shouldn't it be something like NotFound or BadRequest?

https://codereview.appspot.com/7387054/diff/6001/store/store.go
File store/store.go (right):

https://codereview.appspot.com/7387054/diff/6001/store/store.go#newcode320
store/store.go:320: mapf := fmt.Sprintf(""+
How about using `` instead of concatenating?
It'll make the JS code easier to edit, if needed.

[niemeyer, 2013-02-27 21:07:51]

https://codereview.appspot.com/7387054/diff/6001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/7387054/diff/6001/store/server.go#newcode167
store/server.go:167: w.WriteHeader(http.StatusForbidden)
On 2013/02/27 21:00:53, dimitern wrote:
> Why forbidden? Shouldn't it be something like NotFound or BadRequest?

Because the request itself is sound. We're just forbidding it for the moment
because it's expensive and not necessary ATM.

Note that this is unrelated to this CL.

https://codereview.appspot.com/7387054/diff/6001/store/store.go
File store/store.go (right):

https://codereview.appspot.com/7387054/diff/6001/store/store.go#newcode320
store/store.go:320: mapf := fmt.Sprintf(""+
On 2013/02/27 21:00:53, dimitern wrote:
> How about using `` instead of concatenating?
> It'll make the JS code easier to edit, if needed.

That's a good idea, thanks. Done.

[niemeyer, 2013-02-27 22:23:05]

*** Submitted:

store: add statistics listing

R=rog, dimitern
CC=
https://codereview.appspot.com/7387054