Milo: Add LogDog annotation stream support.

Milo: Add LogDog annotation stream support.

Add LogDog annotation stream loading support. Using the
"/logdog/project/path" URL to a Milo annotation protobuf, this will
render that protobuf as a Milo build. The LogDog host is optionally
specified using the "?host=..." query parameter, and defaults to Chrome
Infra's production LogDog host.

This modifies how LogDog build is generated, which affects Swarming
expectations. This is because previously the Start/Finish time was being
calculated independently; however, now the Start/Finish time is
calculated from the annotation protobuf.

BUG=chromium:624961
TEST=local
  - Ran in `dev_appserver`, loads LogDog streams.

Committed: https://github.com/luci/luci-go/commit/13ee0b897f1429d897f0f5e1b6443b8c406e3419

[dnj, 2016-07-29 16:30:46]

dnj@chromium.org changed reviewers:
+ hinoka@chromium.org, nodir@chromium.org

[dnj, 2016-07-29 16:30:47]

PTAL, I really thought I sent this out already...

[Ryan Tseng, 2016-07-29 18:16:37]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2016-07-29 18:16:38]

looks good! mostly style / naming comments, also an infra failure is getting
marked as "Running" for some reason.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:1: // Copyright 2016 The LUCI Authors. All
rights reserved.
rename this build.go, to match up with the other folder's naming convention
(build.go implements functions for build.html)

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:52: type AnnotationStream struct{}
Move AnnotationStream and it's methods to html.go.  html.go validates inputs and
calls the implementation. The idea in the future is to add a prpc.go (grpc.go?)
as a separate entry point to call the implementations.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:288: step.FailureDetails.Text = "Text
expired"
Task

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:466: type swarmingURLBuilder string
nit: Drop the builder, swarmingURL is good enough (also the term "builder" is
already used in milo elsewhere)

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-hang.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-hang.json:36: "Running
bot_update"
This doesn't seem right (And status on L97)

[nodir, 2016-07-29 18:42:07]

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:40: intermediateCacheLifetime = 10 *
time.Second
this is a bit of regression because with swarming milo always displays fresh
data. can we omit caching non-terminated streams and see how bad it is?

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:64: host:    req.FormValue("host"),
buildbucket and swarming handlers use "server" name for this. please make it
consistent

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:84: // project is the project.
this is not useful :)

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:110: if as.host == "" {
do this after trimming, so as.host = " " is converted to the default server too

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:113: as.host =
strings.ToLower(strings.TrimSpace(as.host))
please verify that it does not have slashes, so our memcacheKey is constructed
correctly

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:150: }.Warningf(c, "Failed to load
annotation protobuf memcache item.")
i think it is ok to make it Errorf since it may happen often only if there is a
problem with memcache

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:163: C: &http.Client{
http.Client reuses TCP connections, but if we create it each time, we don't take
this advantage

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:173: if err != nil {
nit: consider `case codes.OK:` as an alternative to this if
less nesting and branching

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:180: 
return 403 in case of check codes.PermissionDenied

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:210: Code:   
http.StatusInternalServerError,
it is not an internal error. I think we should `return nil` here and leave
as.item a zero value

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:228: Code:    http.StatusNotImplemented,
how often this is going to happen in practice?
if not often, please add a comment about it otherwise reading this is scary
if often, we need to do something about it

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:251: if t.Step.Ended != nil &&
(latestEndedTime.IsZero() || endedTime.After(latestEndedTime)) {
i think this condition should be just 
if endedTime.After(latestEndedTime)

because
- if t.Step.Ended == nil, endedTime is zero and cannot be after latest
- if latestEndedTime.isZero(), then a non-zero enddTime is after it

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/logDogBuild.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:111: latestTime = now
it should not be now, but time of build completion. we had a case that step
proto never had Finished time (because step hung) and thus its duration was days
since step started till now

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:118: 
this block of added code solves the same problem as the code below. please
revisit

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/stream.proto (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/stream.proto:11: /**
nit: I think we use // everywhere, in particular in milo

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/stream.proto:12: * Item is a full annotation protobuf
state item.
please add a comment that this message is not intended to be used outside of
this package, and actually an implementation detail

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:323: // Compute start and finished times,
and duration.
duration is not computed

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:465: // links.
explain what is the value of this string should be

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-canceled.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-canceled.json:46: "Duration":
3616635310000,
this should not have changed

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-hang.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-hang.json:111: "Duration":
-9167032340000000,
this broke

[dnj, 2016-07-29 19:57:36]

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:1: // Copyright 2016 The LUCI Authors. All
rights reserved.
On 2016/07/29 18:16:38, Ryan Tseng wrote:
> rename this build.go, to match up with the other folder's naming convention
> (build.go implements functions for build.html)

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:40: intermediateCacheLifetime = 10 *
time.Second
On 2016/07/29 18:42:06, nodir wrote:
> this is a bit of regression because with swarming milo always displays fresh
> data. can we omit caching non-terminated streams and see how bad it is?

I think the caching is a good idea. I don't see 10 seconds harming anything.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:52: type AnnotationStream struct{}
On 2016/07/29 18:16:38, Ryan Tseng wrote:
> Move AnnotationStream and it's methods to html.go.  html.go validates inputs
and
> calls the implementation. The idea in the future is to add a prpc.go
(grpc.go?)
> as a separate entry point to call the implementations. 

"AnnotationStream" is explicitly a build.html handler. Why would it go anywhere
other than "build.go"?

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:64: host:    req.FormValue("host"),
On 2016/07/29 18:42:06, nodir wrote:
> buildbucket and swarming handlers use "server" name for this. please make it
> consistent

Lots of other packages, including pRPC, use "host". I don't think it's
unreasonable to use different terms for different components of different
systems. "host" is consistent with pRPC and other LogDog terminology.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:84: // project is the project.
On 2016/07/29 18:42:06, nodir wrote:
> this is not useful :)

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:110: if as.host == "" {
On 2016/07/29 18:42:06, nodir wrote:
> do this after trimming, so as.host = " " is converted to the default server
too

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:113: as.host =
strings.ToLower(strings.TrimSpace(as.host))
On 2016/07/29 18:42:06, nodir wrote:
> please verify that it does not have slashes, so our memcacheKey is constructed
> correctly

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:150: }.Warningf(c, "Failed to load
annotation protobuf memcache item.")
On 2016/07/29 18:42:06, nodir wrote:
> i think it is ok to make it Errorf since it may happen often only if there is
a
> problem with memcache

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:163: C: &http.Client{
On 2016/07/29 18:42:06, nodir wrote:
> http.Client reuses TCP connections, but if we create it each time, we don't
take
> this advantage

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:173: if err != nil {
On 2016/07/29 18:42:06, nodir wrote:
> nit: consider `case codes.OK:` as an alternative to this if
> less nesting and branching

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:180: 
On 2016/07/29 18:42:06, nodir wrote:
> return 403 in case of check codes.PermissionDenied

Since we're authenticating as Milo rather than the user, I am considering
permission denied to be an internal server error. Once we switch to delegation
tokens, this would make more sense.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:210: Code:   
http.StatusInternalServerError,
On 2016/07/29 18:42:06, nodir wrote:
> it is not an internal error. I think we should `return nil` here and leave
> as.item a zero value

I'm not sure a blank page is better than "hey data's not here yet", but OK.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:228: Code:    http.StatusNotImplemented,
On 2016/07/29 18:42:06, nodir wrote:
> how often this is going to happen in practice?
> if not often, please add a comment about it otherwise reading this is scary
> if often, we need to do something about it

It's part of the protocol, and could happen if a given datagram is huge;
however, I discussed this with Ryan months ago and we're okay not handling
datagram reconstruction in the first roll-out. No annotation state datagrams
that are currently emitted are remotely large enough to trigger this. I'll add a
comment to this effect.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:251: if t.Step.Ended != nil &&
(latestEndedTime.IsZero() || endedTime.After(latestEndedTime)) {
On 2016/07/29 18:42:06, nodir wrote:
> i think this condition should be just 
> if endedTime.After(latestEndedTime)
> 
> because
> - if t.Step.Ended == nil, endedTime is zero and cannot be after latest
> - if latestEndedTime.isZero(), then a non-zero enddTime is after it

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/logDogBuild.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:111: latestTime = now
On 2016/07/29 18:42:06, nodir wrote:
> it should not be now, but time of build completion. we had a case that step
> proto never had Finished time (because step hung) and thus its duration was
days
> since step started till now

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:118: 
On 2016/07/29 18:42:06, nodir wrote:
> this block of added code solves the same problem as the code below. please
> revisit

Acknowledged.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/stream.proto (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/stream.proto:11: /**
On 2016/07/29 18:42:06, nodir wrote:
> nit: I think we use // everywhere, in particular in milo

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/stream.proto:12: * Item is a full annotation protobuf
state item.
On 2016/07/29 18:42:06, nodir wrote:
> please add a comment that this message is not intended to be used outside of
> this package, and actually an implementation detail

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/build.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:288: step.FailureDetails.Text = "Text
expired"
On 2016/07/29 18:16:38, Ryan Tseng wrote:
> Task

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:323: // Compute start and finished times,
and duration.
On 2016/07/29 18:42:06, nodir wrote:
> duration is not computed

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:465: // links.
On 2016/07/29 18:42:07, nodir wrote:
> explain what is the value of this string should be

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/build.go:466: type swarmingURLBuilder string
On 2016/07/29 18:16:38, Ryan Tseng wrote:
> nit: Drop the builder, swarmingURL is good enough (also the term "builder" is
> already used in milo elsewhere)

It's a "URLBuilder" implementation.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-canceled.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-canceled.json:46: "Duration":
3616635310000,
On 2016/07/29 18:42:07, nodir wrote:
> this should not have changed

This equals "Finished" - "Started": https://play.golang.org/p/uRhw0WjLWJ

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-hang.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-hang.json:36: "Running
bot_update"
On 2016/07/29 18:16:38, Ryan Tseng wrote:
> This doesn't seem right (And status on L97)

Oh crap forgot to fix this, Done.

[Ryan Tseng, 2016-07-29 20:34:18]

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:52: type AnnotationStream struct{}
On 2016/07/29 19:57:34, dnj wrote:
> On 2016/07/29 18:16:38, Ryan Tseng wrote:
> > Move AnnotationStream and it's methods to html.go.  html.go validates inputs
> and
> > calls the implementation. The idea in the future is to add a prpc.go
> (grpc.go?)
> > as a separate entry point to call the implementations. 
> 
> "AnnotationStream" is explicitly a build.html handler. Why would it go
anywhere
> other than "build.go"?

In other words, all "ThemedHandler" implementations should go in html.go, and
build.go should not import julientschmidt/httprouter.  A reader should be able
to go into html.go and find all endpoints implemented in /logdog/

So html.go should probably just be a simple file that looks like:

type Build struct {}

func (b *Build) GetTemplateName(t settings.Theme) string {
  return "build.html"
}

func (b *Build) Render(c, r, p,) (*templates.Args, error) {
  // Setup
  ...

  // Validation
  ...

  return &templates.Args{...}
}

The goal of this is to decouple entry points from implementation.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:64: host:    req.FormValue("host"),
On 2016/07/29 19:57:35, dnj wrote:
> On 2016/07/29 18:42:06, nodir wrote:
> > buildbucket and swarming handlers use "server" name for this. please make it
> > consistent
> 
> Lots of other packages, including pRPC, use "host". I don't think it's
> unreasonable to use different terms for different components of different
> systems. "host" is consistent with pRPC and other LogDog terminology.

I agree with nodir, we should keep it consistent within milo.  And not make a
situation where
/swarming/task/foobar?server=blah
/logdog/build/project/foobar?host=blah

That's just confusing.
The difference between "server" and "host" is just an implementation detail, it
shouldn't have to be exposed to the user.

If that means changing it from server -> host elsewhere, that's fine too, but I
think "server" makes more sense.

https://codereview.chromium.org/2191693003/diff/60001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/logDogBuild.go (right):

https://codereview.chromium.org/2191693003/diff/60001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:117: case buildCompletedTime.IsZero():
did you mean !buildcompletedTime.IsZero()?

[dnj, 2016-07-29 21:23:37]

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:52: type AnnotationStream struct{}
On 2016/07/29 20:34:17, Ryan Tseng wrote:
> On 2016/07/29 19:57:34, dnj wrote:
> > On 2016/07/29 18:16:38, Ryan Tseng wrote:
> > > Move AnnotationStream and it's methods to html.go.  html.go validates
inputs
> > and
> > > calls the implementation. The idea in the future is to add a prpc.go
> > (grpc.go?)
> > > as a separate entry point to call the implementations. 
> > 
> > "AnnotationStream" is explicitly a build.html handler. Why would it go
> anywhere
> > other than "build.go"?
> 
> In other words, all "ThemedHandler" implementations should go in html.go, and
> build.go should not import julientschmidt/httprouter.  A reader should be able
> to go into html.go and find all endpoints implemented in /logdog/
> 
> So html.go should probably just be a simple file that looks like:
> 
> type Build struct {}
> 
> func (b *Build) GetTemplateName(t settings.Theme) string {
>   return "build.html"
> }
> 
> func (b *Build) Render(c, r, p,) (*templates.Args, error) {
>   // Setup
>   ...
> 
>   // Validation
>   ...
> 
>   return &templates.Args{...}
> }
> 
> The goal of this is to decouple entry points from implementation.

Done.

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:64: host:    req.FormValue("host"),
On 2016/07/29 20:34:17, Ryan Tseng wrote:
> On 2016/07/29 19:57:35, dnj wrote:
> > On 2016/07/29 18:42:06, nodir wrote:
> > > buildbucket and swarming handlers use "server" name for this. please make
it
> > > consistent
> > 
> > Lots of other packages, including pRPC, use "host". I don't think it's
> > unreasonable to use different terms for different components of different
> > systems. "host" is consistent with pRPC and other LogDog terminology.
> 
> I agree with nodir, we should keep it consistent within milo.  And not make a
> situation where
> /swarming/task/foobar?server=blah
> /logdog/build/project/foobar?host=blah
> 
> That's just confusing.
> The difference between "server" and "host" is just an implementation detail,
it
> shouldn't have to be exposed to the user.
> 
> If that means changing it from server -> host elsewhere, that's fine too, but
I
> think "server" makes more sense.

Discussed, using "host" everywhere.

https://codereview.chromium.org/2191693003/diff/60001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/logDogBuild.go (right):

https://codereview.chromium.org/2191693003/diff/60001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/logDogBuild.go:117: case buildCompletedTime.IsZero():
On 2016/07/29 20:34:17, Ryan Tseng wrote:
> did you mean !buildcompletedTime.IsZero()?

oops yeah

[Ryan Tseng, 2016-07-29 22:28:37]

lgtm +1

[nodir, 2016-07-29 23:00:32]

lgtm % comment about read lock

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/handler.go (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/handler.go:180: 
On 2016/07/29 19:57:35, dnj wrote:
> On 2016/07/29 18:42:06, nodir wrote:
> > return 403 in case of check codes.PermissionDenied
> 
> Since we're authenticating as Milo rather than the user, I am considering
> permission denied to be an internal server error. Once we switch to delegation
> tokens, this would make more sense.

until milo uses delegation tokens, it does not support internal streams
perhaps HTTP 501 exists for such cases

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
File appengine/cmd/milo/swarming/expectations/build-canceled.json (right):

https://codereview.chromium.org/2191693003/diff/40001/appengine/cmd/milo/swar...
appengine/cmd/milo/swarming/expectations/build-canceled.json:46: "Duration":
3616635310000,
On 2016/07/29 19:57:35, dnj wrote:
> On 2016/07/29 18:42:07, nodir wrote:
> > this should not have changed
> 
> This equals "Finished" - "Started": https://play.golang.org/p/uRhw0WjLWJ
> 

Acknowledged.

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/build.go (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:79: if strings.IndexRune(as.host, '/') >= 0 {
nit: use string.ContainsRune

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:126: resp, err := client.Tail(c,
&logdog.TailRequest{
please add logging: would be nice to have a log line about the fact that we are
making a request to logdog

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:299: // and now.
why this comment is here?

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/http.go (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/http.go:38: s.initMu.Lock()
insert a s.logdogClient!=nil check with read-lock held. if it is nil, do the
rest

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/internal/stream.proto (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/internal/stream.proto:13: //
nit: remove

[dnj, 2016-07-29 23:24:44]

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/build.go (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:79: if strings.IndexRune(as.host, '/') >= 0 {
On 2016/07/29 23:00:32, nodir wrote:
> nit: use string.ContainsRune

Done.

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:126: resp, err := client.Tail(c,
&logdog.TailRequest{
On 2016/07/29 23:00:31, nodir wrote:
> please add logging: would be nice to have a log line about the fact that we
are
> making a request to logdog

Done.

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/build.go:299: // and now.
On 2016/07/29 23:00:31, nodir wrote:
> why this comment is here?

Done.

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/http.go (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/http.go:38: s.initMu.Lock()
On 2016/07/29 23:00:32, nodir wrote:
> insert a s.logdogClient!=nil check with read-lock held. if it is nil, do the
> rest

Done.

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
File appengine/cmd/milo/logdog/internal/stream.proto (right):

https://codereview.chromium.org/2191693003/diff/80001/appengine/cmd/milo/logd...
appengine/cmd/milo/logdog/internal/stream.proto:13: //
On 2016/07/29 23:00:32, nodir wrote:
> nit: remove

Done.

[dnj, 2016-07-29 23:24:55]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-07-29 23:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com,
nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2191693003/#ps120001
(title: "Remove HTTP client re-use. Thanks Obama^H^H^H^H^HGAE")

[commit-bot: I haz the power, 2016-07-29 23:25:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 23:29:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-29 23:29:56]

Try jobs failed on following builders:
  Luci-go Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3051cb7081fab910)

[dnj, 2016-07-29 23:31:53]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-07-29 23:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com,
nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/2191693003/#ps140001
(title: "Rebarse")

[commit-bot: I haz the power, 2016-07-29 23:32:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dnj, 2016-07-29 23:32:16]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2016-07-30 03:26:07]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-07-30 03:26:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 03:32:00]

Description was changed from

==========
Milo: Add LogDog annotation stream support.

Add LogDog annotation stream loading support. Using the
"/logdog/project/path" URL to a Milo annotation protobuf, this will
render that protobuf as a Milo build. The LogDog host is optionally
specified using the "?host=..." query parameter, and defaults to Chrome
Infra's production LogDog host.

This modifies how LogDog build is generated, which affects Swarming
expectations. This is because previously the Start/Finish time was being
calculated independently; however, now the Start/Finish time is
calculated from the annotation protobuf.

BUG=chromium:624961
TEST=local
  - Ran in `dev_appserver`, loads LogDog streams.
==========

to

==========
Milo: Add LogDog annotation stream support.

Add LogDog annotation stream loading support. Using the
"/logdog/project/path" URL to a Milo annotation protobuf, this will
render that protobuf as a Milo build. The LogDog host is optionally
specified using the "?host=..." query parameter, and defaults to Chrome
Infra's production LogDog host.

This modifies how LogDog build is generated, which affects Swarming
expectations. This is because previously the Start/Finish time was being
calculated independently; however, now the Start/Finish time is
calculated from the annotation protobuf.

BUG=chromium:624961
TEST=local
  - Ran in `dev_appserver`, loads LogDog streams.

Committed:
https://github.com/luci/luci-go/commit/13ee0b897f1429d897f0f5e1b6443b8c406e3419
==========

[commit-bot: I haz the power, 2016-07-30 03:32:01]

Committed patchset #8 (id:140001) as
https://github.com/luci/luci-go/commit/13ee0b897f1429d897f0f5e1b6443b8c406e3419