Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335833002:
Start loading the BigQuery data and serving it to the UI. (Closed)
|
Created:
6 years, 6 months ago by jcgregorio Modified:
6 years, 6 months ago CC:
skia-review_googlegroups.com, benchen, tfarina Base URL:
https://skia.googlesource.com/buildbot.git@master Visibility:
Public. |
DescriptionStart loading the BigQuery data and serving it to the UI.
Many caveats:
- Only loads traces for two skps, loading all of the skps takes two minutes. Loading and caching all the skps in the background will be done as a follow-on CL.
- Doesn't load annotations from MySQL.
- Only returns the full set of data in JSON, no ability to filter.
- Data is restricted to just the last two days.
All of those caveats will be addressed in follow-on CLs. But this code should be enough to start plumbing in FLOT.
BUG=skia:
Committed: https://skia.googlesource.com/buildbot/+/60f8e827685ece075cdfa589b937f7695f1ff4e7
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 9
Patch Set 7 : y #
Total comments: 33
Patch Set 8 : #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/335833002/diff/80001/perf/server/Makefile File perf/server/Makefile (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/Makefile#new... perf/server/Makefile:1: default: can you go with a ninja build file instead (build.ninja)?
https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go#newc... perf/server/data.go:1: package main don't we put copyright boilerplate in go files as well?
https://codereview.chromium.org/335833002/diff/80001/perf/server/Makefile File perf/server/Makefile (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/Makefile#new... perf/server/Makefile:1: default: The Makefile was just a leftover, you can build the project with $ go build Dropped the Makefile for now. We might need to introduce ninja later to handle some of the Javascript processing. On 2014/06/13 18:59:37, tfarina wrote: > can you go with a ninja build file instead (build.ninja)? > https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go#newc... perf/server/data.go:1: package main On 2014/06/13 19:01:04, tfarina wrote: > don't we put copyright boilerplate in go files as well? Done. Also added in the LICENSE file that the copyright boilerplate refers to.
Thanks for putting these together! Just a few questions from me now. https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go#newc... perf/server/data.go:146: func readCommitsFromGit(dir string) ([]GitHash, error) { So this will be replace with reading the githash database table (my understanding). This way we get git number as well. https://codereview.chromium.org/335833002/diff/80001/perf/server/perf.go File perf/server/perf.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/perf.go#newc... perf/server/perf.go:33: port = flag.String("port", ":8000", "HTTP service address (e.g., ':8000')") I see tabs used in this file - is that fine?
https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/data.go#newc... perf/server/data.go:146: func readCommitsFromGit(dir string) ([]GitHash, error) { Yeah, if we have that info in MySQL it would probably be faster to read it from there. On 2014/06/13 19:51:29, benchen wrote: > So this will be replace with reading the githash database table (my > understanding). This way we get git number as well. https://codereview.chromium.org/335833002/diff/80001/perf/server/perf.go File perf/server/perf.go (right): https://codereview.chromium.org/335833002/diff/80001/perf/server/perf.go#newc... perf/server/perf.go:33: port = flag.String("port", ":8000", "HTTP service address (e.g., ':8000')") Yes, Go is formatted with tabs. On 2014/06/13 19:51:29, benchen wrote: > I see tabs used in this file - is that fine?
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. the new copyright boilerplate does not have (c), so no (c) here. https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. I know this is not the place to discuss this, but shouldn't we say The Skia Authors or Google Authors rather than Chromium?
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. This was cut and paste from another file in this project. If we want to change the format or the copyright holder we can do that in a separate CL that changes all the files at once. On 2014/06/13 20:12:57, tfarina wrote: > the new copyright boilerplate does not have (c), so no (c) here.
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/13 20:22:25, jcgregorio wrote: > This was cut and paste from another file in this project. If we want to change > the format or the copyright holder we can do that in a separate CL that changes > all the files at once. > that seems fine by me.
Very nice Go code! I've got a lot to learn from your style. Left a couple comments, ltgm though https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:123: COUNT(gitHash) AS c So why are you counting them? It seems like you don't use this data later https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:159: func readCommitsFromGit(dir string) ([]GitHash, error) { It looks like you can also get the data from https://skia.googlesource.com/skia/+log/HEAD?format=json as well; not sure if this'd be preferable though
On 2014/06/14 00:57:32, kelvinly wrote: > Very nice Go code! I've got a lot to learn from your style. Left a couple > comments, ltgm though > > https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go > File perf/server/data.go (right): > > https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... > perf/server/data.go:123: COUNT(gitHash) AS c > So why are you counting them? It seems like you don't use this data later > > https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... > perf/server/data.go:159: func readCommitsFromGit(dir string) ([]GitHash, error) > { > It looks like you can also get the data from > https://skia.googlesource.com/skia/+log/HEAD?format=json as well; not sure if > this'd be preferable though lgtm* Sorry, long day
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:159: func readCommitsFromGit(dir string) ([]GitHash, error) { The problem with googlesource is pagination. It gives at most 100 results per page, so you'll need a loop. Or maybe there are options I'm not aware of.. On 2014/06/14 00:57:31, kelvinly wrote: > It looks like you can also get the data from > https://skia.googlesource.com/skia/+log/HEAD?format=json as well; not sure if > this'd be preferable though
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:159: func readCommitsFromGit(dir string) ([]GitHash, error) { On 2014/06/14 20:32:37, benchen wrote: > The problem with googlesource is pagination. It gives at most 100 results per > page, so you'll need a loop. Or maybe there are options I'm not aware of.. > On 2014/06/14 00:57:31, kelvinly wrote: > > It looks like you can also get the data from > > https://skia.googlesource.com/skia/+log/HEAD?format=json as well; not sure if > > this'd be preferable though > On the other hand, it gives you a fairly nice JSON structure to work with, and hands you the blamelist and description as well
https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/100001/perf/server/data.go#new... perf/server/data.go:123: COUNT(gitHash) AS c Leftover code, now removed. On 2014/06/14 00:57:31, kelvinly wrote: > So why are you counting them? It seems like you don't use this data later
https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:26: MISSING_DATA_SENTINEL = 1e100 Cute. Go has +Inf, -Inf, and NaN too I think. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:32: ClientSecret: "J4YCkfMXFJISGyuBuVEiH60T", Um... secret? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:87: ID int `json:"id"` Might help to explain ID and Type here a bit. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:89: Author string `json:"author"` This is author-of-annotation right? Not author-of-commit? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:102: type Choices []string Explain this a bit? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:140: Hash string `bq:"gitHash"` Neat. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:151: // GitHash represents information on a single Git commit. Seems like a subset of Commit above? TimeStamp == CommitTime? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:194: func (r *RowIter) poll() error { Odd that bigquery doesn't provide this sort of thing. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:220: Configuration: &bigquery.JobConfiguration{ Can't you elide the nested type names in this sort of expression? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:411: // TODO(jcgregorio) Keep a cache of the gzipped JSON around and serve that as long as it's fresh. How slow is the JSON encoding? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:444: // TODO(jcgregorio) Actuall do the bit where we start a go routine. So Data is going to keep itself updated asynchronously? How do you plan to coordinate updates and reads? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:499: panic(err) Seems like we'd want something a bit more robust? Isn't this the sort of thing we'd expect to fail sometimes? https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go File perf/server/perf.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go#new... perf/server/perf.go:43: data *Data Having data global smells a little to me. I think I'd have expected it scoped to main, and closed over when creating the JSON handler. Something like this? func jsonHandler(data *Data, w http.ResponseWriter, r *http.Request) { ... } func main() { ... data, err := NewData(...) ... http.HandleFunc("/json", autogzip.HandleFunc(func(w http.ResponseWriter, r *http.Request) { jsonHandler(data, w, r) })) https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go#new... perf/server/perf.go:111: log.Printf("Main Handler: %q\n", r.URL.Path) -> JSON Handler: ?
https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:26: MISSING_DATA_SENTINEL = 1e100 On 2014/06/15 15:12:10, mtklein wrote: > Cute. Go has +Inf, -Inf, and NaN too I think. Tested those, Go doesn't seem to marshal them correctly https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:32: ClientSecret: "J4YCkfMXFJISGyuBuVEiH60T", On 2014/06/15 15:12:12, mtklein wrote: > Um... secret? Time to get new keys! https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:194: func (r *RowIter) poll() error { On 2014/06/15 15:12:10, mtklein wrote: > Odd that bigquery doesn't provide this sort of thing. I think the Go library was autogenerated from something else, so it makes sense that it only includes lower-level ish, not-very-idiomatic functions
https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:26: MISSING_DATA_SENTINEL = 1e100 On 2014/06/15 15:49:26, kelvinly wrote: > On 2014/06/15 15:12:10, mtklein wrote: > > Cute. Go has +Inf, -Inf, and NaN too I think. > > Tested those, Go doesn't seem to marshal them correctly Go may have +-Ing and Nan, but JSON doesn't, thus the use of 1e100.
https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:26: MISSING_DATA_SENTINEL = 1e100 Added a note explaining the requirements for the sentinel. On 2014/06/15 16:25:39, jcgregorio wrote: > On 2014/06/15 15:49:26, kelvinly wrote: > > On 2014/06/15 15:12:10, mtklein wrote: > > > Cute. Go has +Inf, -Inf, and NaN too I think. > > > > Tested those, Go doesn't seem to marshal them correctly > > Go may have +-Ing and Nan, but JSON doesn't, thus the use of 1e100. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:32: ClientSecret: "J4YCkfMXFJISGyuBuVEiH60T", In a future CL I will move this to reading the client_secrets.json file from local disk, which is only needed for running locally. At that time I'll revoke these keys. Added as TODO. On 2014/06/15 15:49:26, kelvinly wrote: > On 2014/06/15 15:12:12, mtklein wrote: > > Um... secret? > > Time to get new keys! https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:87: ID int `json:"id"` Added note about this mapping to the MySQL database and where that schema was documented. On 2014/06/15 15:12:10, mtklein wrote: > Might help to explain ID and Type here a bit. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:89: Author string `json:"author"` Same as above. On 2014/06/15 15:12:10, mtklein wrote: > This is author-of-annotation right? Not author-of-commit? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:102: type Choices []string On 2014/06/15 15:12:10, mtklein wrote: > Explain this a bit? Done. https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:151: // GitHash represents information on a single Git commit. Yeah, but this code may be changing soon as we may have the Git info stored in the MySQL database. On 2014/06/15 15:12:11, mtklein wrote: > Seems like a subset of Commit above? TimeStamp == CommitTime? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:194: func (r *RowIter) poll() error { Yeah, that is true, but I was able to provide pagination support in the Python client library: https://developers.google.com/api-client-library/python/guide/pagination But Apiary never settled on a common idiom for long running processes, so it would be difficult or impossible to add it generically across all APIs. On 2014/06/15 15:49:26, kelvinly wrote: > On 2014/06/15 15:12:10, mtklein wrote: > > Odd that bigquery doesn't provide this sort of thing. > > I think the Go library was autogenerated from something else, so it makes sense > that it only includes lower-level ish, not-very-idiomatic functions https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:220: Configuration: &bigquery.JobConfiguration{ AFAIK only if I depend on the order and supply all the parameters that go before the one I really want to supply. On 2014/06/15 15:12:11, mtklein wrote: > Can't you elide the nested type names in this sort of expression? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:411: // TODO(jcgregorio) Keep a cache of the gzipped JSON around and serve that as long as it's fresh. Not so much the JSON encoding as the gzip. The ungzipped JSON is 500K but zips down to just 50K. That's just for SKPs, I presume that the microbench dataset will be larger. On 2014/06/15 15:12:11, mtklein wrote: > How slow is the JSON encoding? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:444: // TODO(jcgregorio) Actuall do the bit where we start a go routine. Once I add the go routine for updating I will also add a Mutex to Data, which will be used by all accessors, of which there is only AsJSON today. On 2014/06/15 15:12:11, mtklein wrote: > So Data is going to keep itself updated asynchronously? How do you plan to > coordinate updates and reads? https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:499: panic(err) This is only done once and on startup and I don't want to start the server if there's no data to serve. Also, if this fails them monit will try to restart the server within 2 seconds, so that's where the retry occurs. Added comment to explain that. On 2014/06/15 15:12:10, mtklein wrote: > Seems like we'd want something a bit more robust? Isn't this the sort of thing > we'd expect to fail sometimes? https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go File perf/server/perf.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go#new... perf/server/perf.go:43: data *Data Data will be protected with a Mutex once the auto update code is added. Also, it isn't really global as it is scoped to package 'main', the code just hasn't grown to the point of needing more than a single package yet. On 2014/06/15 15:12:12, mtklein wrote: > Having data global smells a little to me. I think I'd have expected it scoped > to main, and closed over when creating the JSON handler. Something like this? > > func jsonHandler(data *Data, w http.ResponseWriter, r *http.Request) { ... } > > func main() { > ... > data, err := NewData(...) > ... > http.HandleFunc("/json", autogzip.HandleFunc(func(w http.ResponseWriter, r > *http.Request) { jsonHandler(data, w, r) })) https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go#new... perf/server/perf.go:111: log.Printf("Main Handler: %q\n", r.URL.Path) On 2014/06/15 15:12:12, mtklein wrote: > -> JSON Handler: ? Done. https://codereview.chromium.org/335833002/diff/120001/perf/server/perf.go#new... perf/server/perf.go:111: log.Printf("Main Handler: %q\n", r.URL.Path) On 2014/06/15 15:12:12, mtklein wrote: > -> JSON Handler: ? Done.
https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go File perf/server/data.go (right): https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... perf/server/data.go:411: // TODO(jcgregorio) Keep a cache of the gzipped JSON around and serve that as long as it's fresh. Sorry wrong numbers, that should be 50MB and 5MB respectively. On 2014/06/16 02:46:24, jcgregorio wrote: > Not so much the JSON encoding as the gzip. The ungzipped JSON is 500K but zips > down to just 50K. That's just for SKPs, I presume that the microbench dataset > will be larger. > > On 2014/06/15 15:12:11, mtklein wrote: > > How slow is the JSON encoding? >
On 2014/06/16 12:59:56, jcgregorio wrote: > https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go > File perf/server/data.go (right): > > https://codereview.chromium.org/335833002/diff/120001/perf/server/data.go#new... > perf/server/data.go:411: // TODO(jcgregorio) Keep a cache of the gzipped JSON > around and serve that as long as it's fresh. > Sorry wrong numbers, that should be 50MB and 5MB respectively. > > On 2014/06/16 02:46:24, jcgregorio wrote: > > Not so much the JSON encoding as the gzip. The ungzipped JSON is 500K but zips > > down to just 50K. That's just for SKPs, I presume that the microbench dataset > > will be larger. > > > > On 2014/06/15 15:12:11, mtklein wrote: > > > How slow is the JSON encoding? > > lgtm
The CQ bit was checked by jcgregorio@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/jcgregorio@google.com/335833002/140001
Message was sent while issue was closed.
Change committed as 60f8e827685ece075cdfa589b937f7695f1ff4e7 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
