Added HTTP endpoints for Correctness counts

This adds http endpoints for correctness counts that are easily consumable by front-end JS. 

One can request the counts of positive/negative/untriaged digests aggregated over tests and also for individual tests. 

A second resource allows to request the counts for a specific test and also get labels for digests in the traces for the test. 

BUG=skia:

Committed: https://skia.googlesource.com/buildbot/+/18fa8608981deb898f382162ab0b8224ba55ebd6

[stephana, 2014-10-17 17:34:42]

stephana@google.com changed reviewers:
+ jcgregorio@google.com

[stephana, 2014-10-17 17:34:43]

stephana@google.com changed required reviewers:
+ jcgregorio@google.com

[jcgregorio, 2014-10-17 18:07:52]

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:90: Counts  map[string][][3]int64 `json:"counts"`
[3]int64 is a little ambiguous as far as figuring out which is untriaged vs pos
vs neg. Why not use a simple struct:

struct { 
  Unk int64
  Pos int64
  Neg int64
}

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:106: Labels [][2]uint64 `json:"labels"`
Why not map[int64]int64 or []struct{ ID int64, Label int64}?

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
File golden/go/skiacorrectness/main.go (right):

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:13: //
"skia.googlesource.com/buildbot.git/golden/go/analysis"
leftover?

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:53: func (rr *RestResources)
GetTileCountsHandler(w http.ResponseWriter, r *http.Request) {
sendResponse looks useful, but I'm not sure about the role RestResources is
playing.

func tileCountsHandler(w http.ResponseWriter, r *http.Request) {
  result, err := analyzer.GetTileCounts()
  if err != nil {
    sendErrorResponse(w, err.Error(), http.StatusInternalServerError)
    return
  }
  sendResponse(w, result, http.StatusOK)
}

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:90: func parseJson(r *http.Request, v
interface{}) error {
Doesn't appear to be used?

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:105: w.Header().Set("Content-Length",
fmt.Sprintf("%d", len(jsonBytes)))
Content-Length isn't necessary.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:127: router.HandleFunc("/rest/tilecounts",
resources.GetTileCountsHandler)
Consider adding autogzip for JSON responses:

 http://godoc.org/github.com/fiorix/go-web/autogzip

[stephana, 2014-10-17 20:01:45]

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:90: Counts  map[string][][3]int64 `json:"counts"`
On 2014/10/17 18:07:52, jcgregorio wrote:
> [3]int64 is a little ambiguous as far as figuring out which is untriaged vs
pos
> vs neg. Why not use a simple struct:
> 
> struct { 
>   Unk int64
>   Pos int64
>   Neg int64
> }

Done.

https://codereview.chromium.org/650253003/diff/20001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:106: Labels [][2]uint64 `json:"labels"`
On 2014/10/17 18:07:52, jcgregorio wrote:
> Why not map[int64]int64 or []struct{ ID int64, Label int64}?

Done.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
File golden/go/skiacorrectness/main.go (right):

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:13: //
"skia.googlesource.com/buildbot.git/golden/go/analysis"
On 2014/10/17 18:07:52, jcgregorio wrote:
> leftover?
Yes. Thank you. Removed.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:53: func (rr *RestResources)
GetTileCountsHandler(w http.ResponseWriter, r *http.Request) {
On 2014/10/17 18:07:52, jcgregorio wrote:
> sendResponse looks useful, but I'm not sure about the role RestResources is
> playing.
> 
> func tileCountsHandler(w http.ResponseWriter, r *http.Request) {
>   result, err := analyzer.GetTileCounts()
>   if err != nil {
>     sendErrorResponse(w, err.Error(), http.StatusInternalServerError)
>     return
>   }
>   sendResponse(w, result, http.StatusOK)
> }
> 
> 

I added RestResources as a container for the variables that are used by the
various handlers. Right now it's only 'analyzer'. 
The alternative was to make it a global variable and set it in main.
I am not a big fan of that pattern, if you have any suggestions please let me
know.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:90: func parseJson(r *http.Request, v
interface{}) error {
On 2014/10/17 18:07:52, jcgregorio wrote:
> Doesn't appear to be used?

Agreed that's a remainder from another version. Removed.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:105: w.Header().Set("Content-Length",
fmt.Sprintf("%d", len(jsonBytes)))
On 2014/10/17 18:07:52, jcgregorio wrote:
> Content-Length isn't necessary.
Removed.

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:127: router.HandleFunc("/rest/tilecounts",
resources.GetTileCountsHandler)
On 2014/10/17 18:07:52, jcgregorio wrote:
> Consider adding autogzip for JSON responses:
> 
>  http://godoc.org/github.com/fiorix/go-web/autogzip

I actually had that and removed it. I think we should not add gzip to the app
itself but defer it to the proxy in front of it.

Added a TODO to add it later. I like the bigger data sizes during development to
keep me honest about response sizes.

[jcgregorio, 2014-10-17 20:12:16]

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
File golden/go/skiacorrectness/main.go (right):

https://codereview.chromium.org/650253003/diff/20001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:53: func (rr *RestResources)
GetTileCountsHandler(w http.ResponseWriter, r *http.Request) {
On 2014/10/17 20:01:45, stephana wrote:
> On 2014/10/17 18:07:52, jcgregorio wrote:
> > sendResponse looks useful, but I'm not sure about the role RestResources is
> > playing.
> > 
> > func tileCountsHandler(w http.ResponseWriter, r *http.Request) {
> >   result, err := analyzer.GetTileCounts()
> >   if err != nil {
> >     sendErrorResponse(w, err.Error(), http.StatusInternalServerError)
> >     return
> >   }
> >   sendResponse(w, result, http.StatusOK)
> > }
> > 
> > 
> 
> I added RestResources as a container for the variables that are used by the
> various handlers. Right now it's only 'analyzer'. 
> The alternative was to make it a global variable and set it in main.
> I am not a big fan of that pattern, if you have any suggestions please let me
> know.

As for global, that's not too much of a concern since it will only be global
within the scope of the 'main' package, but I don't care either way. 

As for naming, I've seen similar pieces wrapped up as a Server or State struct
that contains all the state.

[stephana, 2014-10-17 20:38:36]

I made analyzer a global for now. We cal always change that down the road if
necessary.

[jcgregorio, 2014-10-18 00:09:03]

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:17: // Stores a Trace with labels and digests in
memory. CommitIds, Digests and
Comments for things begin with its name:

// LabeledTrace stores a Trace ...

All structs and funcs in this file.

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:119: C int `json:"c"`
I'm all for brevity, but this might be too much. ID and Label are pretty short.

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
File golden/go/skiacorrectness/main.go (right):

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:32: // Response envelope. All responses follow
this format. Some fields might
Comments begin with the name of the thing being described.

// ResponseEnvelope is...

Here and all the functions below.

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:53: // Process a diff request send via HTTP.
sent

[stephana, 2014-10-20 12:56:08]

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:17: // Stores a Trace with labels and digests in
memory. CommitIds, Digests and
On 2014/10/18 00:09:03, jcgregorio wrote:
> Comments for things begin with its name:
> 
> // LabeledTrace stores a Trace ...
> 
> All structs and funcs in this file.

Done.

https://codereview.chromium.org/650253003/diff/60001/golden/go/analysis/analy...
golden/go/analysis/analysis.go:119: C int `json:"c"`
On 2014/10/18 00:09:03, jcgregorio wrote:
> I'm all for brevity, but this might be too much. ID and Label are pretty
short.

Done.

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
File golden/go/skiacorrectness/main.go (right):

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:32: // Response envelope. All responses follow
this format. Some fields might
On 2014/10/18 00:09:03, jcgregorio wrote:
> Comments begin with the name of the thing being described.
> 
> // ResponseEnvelope is...
> 
> Here and all the functions below.

Done.

https://codereview.chromium.org/650253003/diff/60001/golden/go/skiacorrectnes...
golden/go/skiacorrectness/main.go:53: // Process a diff request send via HTTP.
On 2014/10/18 00:09:03, jcgregorio wrote:
> sent

Done.

[jcgregorio, 2014-10-20 14:03:46]

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
golden/go/analysis/analysis.go:171: return a.currentTestCounts[testName], nil
This looks like it will always succeed, so is the error in the return needed?

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
golden/go/analysis/analysis.go:203: for {
for _ := range time.Tick(timeBetweenPolls) {
}

https://codereview.chromium.org/650253003/diff/100001/golden/go/db/db.go
File golden/go/db/db.go (right):

https://codereview.chromium.org/650253003/diff/100001/golden/go/db/db.go#newc...
golden/go/db/db.go:21: // Returns DatabaseConfig instance for running in
production if a
// GetDatabaseConfig returns...

Also, this is already in the db module, so the name could be shortened to
db.GetConfig()?

[stephana, 2014-10-20 14:36:15]

Incorporated feedback and fixed more comments.

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
File golden/go/analysis/analysis.go (right):

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
golden/go/analysis/analysis.go:171: return a.currentTestCounts[testName], nil
On 2014/10/20 14:03:46, jcgregorio wrote:
> This looks like it will always succeed, so is the error in the return needed?

This should return an error if we are unable to read the tiles. Added TODO.

https://codereview.chromium.org/650253003/diff/100001/golden/go/analysis/anal...
golden/go/analysis/analysis.go:203: for {
On 2014/10/20 14:03:46, jcgregorio wrote:
> for _ := range time.Tick(timeBetweenPolls) {
> }

Done.

https://codereview.chromium.org/650253003/diff/100001/golden/go/db/db.go
File golden/go/db/db.go (right):

https://codereview.chromium.org/650253003/diff/100001/golden/go/db/db.go#newc...
golden/go/db/db.go:21: // Returns DatabaseConfig instance for running in
production if a
On 2014/10/20 14:03:46, jcgregorio wrote:
> // GetDatabaseConfig returns...
> 
> Also, this is already in the db module, so the name could be shortened to
> db.GetConfig()?

Done.

[jcgregorio, 2014-10-20 14:39:23]

LGTM

[stephana, 2014-10-20 14:43:02]

The CQ bit was checked by stephana@google.com

[commit-bot: I haz the power, 2014-10-20 14:43:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650253003/140001

[commit-bot: I haz the power, 2014-10-20 14:43:27]

Committed patchset #8 (id:140001) as 18fa8608981deb898f382162ab0b8224ba55ebd6