LogDog: Add Butler Output package.

LogDog: Add Butler Output package.

BUG=

Committed: https://github.com/luci/luci-go/commit/76ae8a45fa7252fe26db819f027236c3faf3ae32

[dnj, 2015-07-01 01:52:27]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2015-07-01 01:52:28]

PTAL. Feel free to pull someone else in, too. The kickoff document is here:
https://docs.google.com/document/d/1AnAjx46zbqT_RJcwAMcUy7OlXxbUZ2-3Es0bhBRoJ...

[dnj, 2015-11-03 19:07:26]

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

[dnj, 2015-11-03 19:07:26]

Updated from Butler v2.

[dnj, 2015-11-03 20:37:48]

Review order:
(butlerproto) https://codereview.chromium.org/1321273002/
(StreamServer) https://codereview.chromium.org/1429993002/
(Bundler) https://codereview.chromium.org/1412063008/
(Output) https://codereview.chromium.org/1211053004/

[iannucci, 2015-11-13 07:17:57]

https://chromiumcodereview.appspot.com/1211053004/diff/40001/client/internal/...
File client/internal/logdog/butler/output/gcps/output.go (right):

https://chromiumcodereview.appspot.com/1211053004/diff/40001/client/internal/...
client/internal/logdog/butler/output/gcps/output.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
can has doc.go's?

[dnj, 2015-11-13 23:37:05]

https://chromiumcodereview.appspot.com/1211053004/diff/40001/client/internal/...
File client/internal/logdog/butler/output/gcps/output.go (right):

https://chromiumcodereview.appspot.com/1211053004/diff/40001/client/internal/...
client/internal/logdog/butler/output/gcps/output.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/11/13 07:17:57, iannucci wrote:
> can has doc.go's?

Done.

[dnj, 2015-11-19 19:16:32]

(Ping)

[estaab, 2015-11-19 23:57:24]

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/doc.go:9: // Output instances must be
goroutine-safe. The Butler may elect to output
Wait, "must be" or "are"? I'm reading this is as: users can safely use Output
instances in goroutines but anyone changing this code must ensure it stays this
way. Is that right? It sort of looks like instructions to the user of the code
so perhaps disambiguate this statement by explicitly mentioning caller vs
author.

(and only here, not in the implementation files, it's much more obvious where
the audience is there)

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/doc.go:13: //   - gcps: Write logs to
Google Cloud Pub/Sub.
A better name might be just "pubsub" since it's instantly recognizable.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
Generally we don't mix message types across records in recordio (at least the
one I'm familiar with :)) and we'd achieve the same functionality with a message
that encapsulates both ButlerMetadata and ButlerLogBundle. Then the recipient
wouldn't need to understand recordio, either. What are the advantages to doing
it with 2 records instead that I'm missing?

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
Generally we don't mix message types across records in recordio. Could the same
functionality be achieved with a message that encapsulates both ButlerMetadata
and ButlerLogBundle?

Advantages of having all pubsub messages being proto instead of recordio:
1) sender and receiver only need to speak proto, not recordio
2) we have command line tools to print protos as ascii even without knowing the
message type (you just get tag numbers, but still useful for figuring out what's
going on)

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/gcpsOutput.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:22:
"google.golang.org/cloud/pubsub"
I think some encapsulation is being broken here - gcps seems to be the interface
we want to use to interact with this package, not use it directly. Can you
possibly just add

package gcps

type Message struct {
  pubsub.Message
}

to gcps so we can remove this? If that's not valid go or against some idioms
then nevermind. :)

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:92:
log.Errorf(log.SetError(o.ctx, err), "Failed to build PubSub Message from
bundle.")
Do the rules about lowercase first letter and no punctuation apply here?

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/output.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/output.go:91: // Errors implements Stas.
Stats (and all of the copy pastes above)

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/retry.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/retry.go:17: // TODO: Tune backoff
parameters.
Should we allow any of these to be controlled by flags in case we need to change
something without needing to do a code push?

[dnj, 2015-11-20 01:55:12]

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/doc.go:9: // Output instances must be
goroutine-safe. The Butler may elect to output
On 2015/11/19 23:57:24, estaab wrote:
> Wait, "must be" or "are"? I'm reading this is as: users can safely use Output
> instances in goroutines but anyone changing this code must ensure it stays
this
> way. Is that right? It sort of looks like instructions to the user of the code
> so perhaps disambiguate this statement by explicitly mentioning caller vs
> author.
> 
> (and only here, not in the implementation files, it's much more obvious where
> the audience is there)

Must. The Output interface contract promises goroutine-safety.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/doc.go:13: //   - gcps: Write logs to
Google Cloud Pub/Sub.
On 2015/11/19 23:57:24, estaab wrote:
> A better name might be just "pubsub" since it's instantly recognizable.

Hmm ... okay, agreed.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
On 2015/11/19 23:57:24, estaab wrote:
> Generally we don't mix message types across records in recordio (at least the
> one I'm familiar with :)) and we'd achieve the same functionality with a
message
> that encapsulates both ButlerMetadata and ButlerLogBundle. Then the recipient
> wouldn't need to understand recordio, either. What are the advantages to doing
> it with 2 records instead that I'm missing?

ButlerLogBundle may be compressed. The compression is defined in ButlerMetadata,
so the former must be in a known format and the latter's format is defined by
the former.

Now, I could embed ButlerLogBundle as a "Data []byte" field in ButlerMetadata,
but if I do that, the user has to read the full stream and allocate/copy the
bytes once on ButlerMetadata deserialization and then again while actually
processing. 

With this layout, a client can read this as a stream. The implicit protocol is
"there is one RecordIO frame, and it is a ButlerMetadata". The ButlerMetadata
describes the remainder of the stream, which is "there is another ReceordIO
frame, and it is a ButlerLogBundle compressed/etc.".

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/gcpsOutput.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:22:
"google.golang.org/cloud/pubsub"
On 2015/11/19 23:57:24, estaab wrote:
> I think some encapsulation is being broken here - gcps seems to be the
interface
> we want to use to interact with this package, not use it directly. Can you
> possibly just add
> 
> package gcps
> 
> type Message struct {
>   pubsub.Message
> }
> 
> to gcps so we can remove this? If that's not valid go or against some idioms
> then nevermind. :)

"gcps" package is never used outside of the pubsub package (and the CLI
interface to couple to it). I don't see the value in this abstraction.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:92:
log.Errorf(log.SetError(o.ctx, err), "Failed to build PubSub Message from
bundle.")
On 2015/11/19 23:57:24, estaab wrote:
> Do the rules about lowercase first letter and no punctuation apply here?

Nope, this isn't a Go error, it's a logging error message.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/output.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/output.go:91: // Errors implements Stas.
On 2015/11/19 23:57:24, estaab wrote:
> Stats (and all of the copy pastes above)

Done.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/retry.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/retry.go:17: // TODO: Tune backoff
parameters.
On 2015/11/19 23:57:24, estaab wrote:
> Should we allow any of these to be controlled by flags in case we need to
change
> something without needing to do a code push?

I originally had this. Then I read a ToTT on flag bloat saying, "if a flag isn't
being used, don't define it" and agreed with it.

Actually going to delete this altogether since the "retry" library defines this
now.

[estaab, 2015-11-20 04:37:28]

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
On 2015/11/20 01:55:12, dnj wrote:
> On 2015/11/19 23:57:24, estaab wrote:
> > Generally we don't mix message types across records in recordio (at least
the
> > one I'm familiar with :)) and we'd achieve the same functionality with a
> message
> > that encapsulates both ButlerMetadata and ButlerLogBundle. Then the
recipient
> > wouldn't need to understand recordio, either. What are the advantages to
doing
> > it with 2 records instead that I'm missing?
> 
> ButlerLogBundle may be compressed. The compression is defined in
ButlerMetadata,
> so the former must be in a known format and the latter's format is defined by
> the former.
> 
> Now, I could embed ButlerLogBundle as a "Data []byte" field in ButlerMetadata,
> but if I do that, the user has to read the full stream and allocate/copy the
> bytes once on ButlerMetadata deserialization and then again while actually
> processing. 
> 
> With this layout, a client can read this as a stream. The implicit protocol is
> "there is one RecordIO frame, and it is a ButlerMetadata". The ButlerMetadata
> describes the remainder of the stream, which is "there is another ReceordIO
> frame, and it is a ButlerLogBundle compressed/etc.".

Pubsub messages already get gzip-compressed at the HTTP level. Why do we need to
complicate this code by doing it at the application level also?

HTTP client created in pubsub.NewClient:
https://github.com/GoogleCloudPlatform/gcloud-golang/blob/master/pubsub/pubsu...

which in turn calls transport.NewHTTPClient:
https://github.com/GoogleCloudPlatform/gcloud-golang/blob/master/internal/tra...

Which in turn calls oauth2.NewClient, and DisableCompression isn't getting set
anywhere I can find down the call stack.

And any competent library performing RPCs should be compressing messages anyway
- I only dug into the implementation just to confirm it.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/gcpsOutput.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:22:
"google.golang.org/cloud/pubsub"
On 2015/11/20 01:55:12, dnj wrote:
> On 2015/11/19 23:57:24, estaab wrote:
> > I think some encapsulation is being broken here - gcps seems to be the
> interface
> > we want to use to interact with this package, not use it directly. Can you
> > possibly just add
> > 
> > package gcps
> > 
> > type Message struct {
> >   pubsub.Message
> > }
> > 
> > to gcps so we can remove this? If that's not valid go or against some idioms
> > then nevermind. :)
> 
> "gcps" package is never used outside of the pubsub package (and the CLI
> interface to couple to it). I don't see the value in this abstraction.

Besides clean abstraction being a general good practice I can think of at least
one practical reason: if cloud ever releases a v2 API none of this code will
need to change, just luci-go/common/gcloud/gcps. Less likely: we switch to
another pubsub implementation. Since it's easy I'm not sure what the argument
against abstracting it is. I guess the only reason pubsub is abstracted at all
is for the testability of this file?

Perhaps leaky abstractions are more common in go since building is fast. In C++
or java you'd be able to run gcpsOutput_test without building any underlying
pubsub code, making binaries smaller and causing fewer tests to trigger when
doing an affected tests dependency calculation on a patch.

[dnj (Google), 2015-11-20 04:45:48]

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

[dnj (Google), 2015-11-20 04:45:48]

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
> Pubsub messages already get gzip-compressed at the HTTP level. Why do we need
to
> complicate this code by doing it at the application level also?

PubSub enforces a maximum message size, evaluating the raw Message data size
(regardless of HTTP compression). This lets us pack more into that space.

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/gcpsOutput.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:22:
"google.golang.org/cloud/pubsub"
On 2015/11/20 04:37:28, estaab wrote:
> On 2015/11/20 01:55:12, dnj wrote:
> > On 2015/11/19 23:57:24, estaab wrote:
> > > I think some encapsulation is being broken here - gcps seems to be the
> > interface
> > > we want to use to interact with this package, not use it directly. Can you
> > > possibly just add
> > > 
> > > package gcps
> > > 
> > > type Message struct {
> > >   pubsub.Message
> > > }
> > > 
> > > to gcps so we can remove this? If that's not valid go or against some
idioms
> > > then nevermind. :)
> > 
> > "gcps" package is never used outside of the pubsub package (and the CLI
> > interface to couple to it). I don't see the value in this abstraction.
> 
> Besides clean abstraction being a general good practice I can think of at
least
> one practical reason: if cloud ever releases a v2 API none of this code will
> need to change, just luci-go/common/gcloud/gcps. Less likely: we switch to
> another pubsub implementation. Since it's easy I'm not sure what the argument
> against abstracting it is. I guess the only reason pubsub is abstracted at all
> is for the testability of this file?

Yeah, Pub/Sub abstraction is in place so we can test. This is directly bound to
the interface, and I'd kill the abstraction too if it weren't necessary to do
proper testing.

If Pub/Sub upgrades to a v2 API, this code may have to change, as will the
abstraction. It's a fairly dead-simple interface. If I built a "send this data
through Pub/Sub" interface, it'd basically be the Pub/Sub interface, so not much
to gain by shielding this code from it.

[estaab, 2015-11-20 05:15:48]

lgtm

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
On 2015/11/20 04:45:48, dnj (Google) wrote:
> > Pubsub messages already get gzip-compressed at the HTTP level. Why do we
need
> to
> > complicate this code by doing it at the application level also?
> 
> PubSub enforces a maximum message size, evaluating the raw Message data size
> (regardless of HTTP compression). This lets us pack more into that space.

Ok, I buy that argument. Am I looking at the right place where I see requests
can be up to 10MB? Is that a limit we expect to approach?
Looking here: https://cloud.google.com/pubsub/quotas

If the need for the extra complexity isn't already documented somewhere can you
add a note?

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/gcpsOutput.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/gcpsOutput.go:22:
"google.golang.org/cloud/pubsub"
On 2015/11/20 04:45:48, dnj (Google) wrote:
> On 2015/11/20 04:37:28, estaab wrote:
> > On 2015/11/20 01:55:12, dnj wrote:
> > > On 2015/11/19 23:57:24, estaab wrote:
> > > > I think some encapsulation is being broken here - gcps seems to be the
> > > interface
> > > > we want to use to interact with this package, not use it directly. Can
you
> > > > possibly just add
> > > > 
> > > > package gcps
> > > > 
> > > > type Message struct {
> > > >   pubsub.Message
> > > > }
> > > > 
> > > > to gcps so we can remove this? If that's not valid go or against some
> idioms
> > > > then nevermind. :)
> > > 
> > > "gcps" package is never used outside of the pubsub package (and the CLI
> > > interface to couple to it). I don't see the value in this abstraction.
> > 
> > Besides clean abstraction being a general good practice I can think of at
> least
> > one practical reason: if cloud ever releases a v2 API none of this code will
> > need to change, just luci-go/common/gcloud/gcps. Less likely: we switch to
> > another pubsub implementation. Since it's easy I'm not sure what the
argument
> > against abstracting it is. I guess the only reason pubsub is abstracted at
all
> > is for the testability of this file?
> 
> Yeah, Pub/Sub abstraction is in place so we can test. This is directly bound
to
> the interface, and I'd kill the abstraction too if it weren't necessary to do
> proper testing.
> 
> If Pub/Sub upgrades to a v2 API, this code may have to change, as will the
> abstraction. It's a fairly dead-simple interface. If I built a "send this data
> through Pub/Sub" interface, it'd basically be the Pub/Sub interface, so not
much
> to gain by shielding this code from it.

Ok, that's fine. I think it's a good habit but it seems reasonable to have a
thin abstraction for testability.

[dnj (Google), 2015-11-20 16:40:02]

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
File client/internal/logdog/butler/output/gcps/doc.go (right):

https://codereview.chromium.org/1211053004/diff/60001/client/internal/logdog/...
client/internal/logdog/butler/output/gcps/doc.go:9: // RecordIO frames:
On 2015/11/20 05:15:48, estaab wrote:
> On 2015/11/20 04:45:48, dnj (Google) wrote:
> > > Pubsub messages already get gzip-compressed at the HTTP level. Why do we
> need
> > to
> > > complicate this code by doing it at the application level also?
> > 
> > PubSub enforces a maximum message size, evaluating the raw Message data size
> > (regardless of HTTP compression). This lets us pack more into that space.
> 
> Ok, I buy that argument. Am I looking at the right place where I see requests
> can be up to 10MB? Is that a limit we expect to approach?
> Looking here: https://cloud.google.com/pubsub/quotas

Not really expect to approach, but it depends. Most builds will send more than
10MB of logs. The remainder is then buffering/timeout configuration. I don't
think we'll tune to retain 10MB and burst it, but this implementation doesn't
make those decisions.

In this case I'm baking wire versatility into the protocol even if it's not
strictly necessary on rollout day. This makes it so things like buffering
parameters are tunable without rebooting the entire stack.

> If the need for the extra complexity isn't already documented somewhere can
you
> add a note?

This whole thing is actually documented in detail in
./common/logdog/butlerproto. It's just restated here for reference. I'll add a
note to this effect.

[dnj (Google), 2015-11-20 16:40:07]

The CQ bit was checked by dnj@google.com

[dnj (Google), 2015-11-20 16:40:08]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/1211053004/#ps80001
(title: "Relocate butlerproto to common, document.")

[commit-bot: I haz the power, 2015-11-20 16:40:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211053004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1211053004/80001

[commit-bot: I haz the power, 2015-11-20 16:42:42]

Committed patchset #5 (id:80001) as
https://github.com/luci/luci-go/commit/76ae8a45fa7252fe26db819f027236c3faf3ae32