Add batch logging support to eventlog.

Add batch logging support to eventlog.

BUG=658441
Committed: https://github.com/luci/luci-go/commit/7477886a355c7267508550a313fcaeed8bd12ada

[mcgreevy, 2016-12-06 00:11:21]

Description was changed from

==========
Add batch logging support to eventlog.

BUG=658441
==========

to

==========
Add batch logging support to eventlog.

BUG=658441
==========

[mcgreevy, 2016-12-06 00:11:22]

mcgreevy@chromium.org changed reviewers:
+ djd@chromium.org

[mcgreevy, 2016-12-06 02:33:39]

djd says he doesn't think this was published, so I'm publishing it now.

[djd-OOO-Apr2017, 2016-12-06 06:31:27]

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/eventlog.go
File common/eventlog/eventlog.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:75: if err != nil {
It would be nice if we could guarantee that (err == nil) when NewEventLog was
used.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:56: bl.stopc = make(chan
struct{})
Move these up into the struct literal.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:97: bl.pending = nil
maybe bl.pending = bl.pending[:0] to reuse the slice?

(Might not be important given the relatively low number of events?)

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:100: if pending == nil &&
bl.retries == nil {
len(pending) == 0 seems less fragile to future changes

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:117: dropLen :=
len(toUpload)
Where is toUpload modified?

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger_test.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:62: // log another
event.
You should also try the case where there are pending logs on close too.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:135: if got :=
rsl.records; !reflect.DeepEqual(got, want) {
It feels like you should be asserting as you go along too?

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:248: func
emptyEventSlice() []*logpb.LogRequestLite_LogEventLite {
Can you just make this a var? (If it has cap 0, then it can't be re-used between
after appends)

[mcgreevy, 2016-12-06 23:57:36]

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/eventlog.go
File common/eventlog/eventlog.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:75: if err != nil {
On 2016/12/06 06:31:27, djd wrote:
> It would be nice if we could guarantee that (err == nil) when NewEventLog was
> used.

There's nothing stopping the user making aribtrary changes to the proto after it
is returned by NewEventLog, so I'm not sure there's much we can do in this
respect.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:56: bl.stopc = make(chan
struct{})
On 2016/12/06 06:31:27, djd wrote:
> Move these up into the struct literal.

Done.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:97: bl.pending = nil
On 2016/12/06 06:31:27, djd wrote:
> maybe bl.pending = bl.pending[:0] to reuse the slice?
> 
> (Might not be important given the relatively low number of events?)

If I were to do that, I would have to lock the mutex while attempting the log
rpc, which would suck.  Or, set it to nil, and then after doing the RPC, take
the lock again and do 

if bl.pending == nil {
  bl.pending = bl.pending[:0]
}

But that's starting to get icky and I think prematurely optimized.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:100: if pending == nil &&
bl.retries == nil {
On 2016/12/06 06:31:27, djd wrote:
> len(pending) == 0 seems less fragile to future changes

Done.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:117: dropLen :=
len(toUpload)
On 2016/12/06 06:31:27, djd wrote:
> Where is toUpload modified?

It is passed to bl.retries.AppendAll as a destination slice to append to
(AppendAll then returns the resultant slice).

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger_test.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:62: // log another
event.
On 2016/12/06 06:31:27, djd wrote:
> You should also try the case where there are pending logs on close too.

Added a separate test.

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:135: if got :=
rsl.records; !reflect.DeepEqual(got, want) {
On 2016/12/06 06:31:27, djd wrote:
> It feels like you should be asserting as you go along too?

That would make the test a bit more precise, but also clutter it with a bunch of
assertions that make the test even longer and harder to read. I've now chosen to
just use a channel for this instead of a slice. I think it's a lot better now.
WDYT?

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger_test.go:248: func
emptyEventSlice() []*logpb.LogRequestLite_LogEventLite {
On 2016/12/06 06:31:27, djd wrote:
> Can you just make this a var? (If it has cap 0, then it can't be re-used
between
> after appends)

Good idea. Done.

[djd-OOO-Apr2017, 2016-12-07 00:31:25]

Mind uploading PS2?

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:117: dropLen :=
len(toUpload)
On 2016/12/06 at 23:57:36, mcgreevy2 wrote:
> On 2016/12/06 06:31:27, djd wrote:
> > Where is toUpload modified?
> 
> It is passed to bl.retries.AppendAll as a destination slice to append to
(AppendAll then returns the resultant slice).

It looks like the len of toUpload can't change though? (You're not passing
&toUpload to AppendAll.)
So, can you bring dropLen down to the if on :126?
You look convinced.

[djd-OOO-Apr2017, 2016-12-07 00:31:35]

:D

[mcgreevy, 2016-12-07 02:42:37]

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
File common/eventlog/internal/logservice/batch_logger.go (right):

https://codereview.chromium.org/2557593002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/batch_logger.go:117: dropLen :=
len(toUpload)
On 2016/12/07 00:31:25, djd wrote:
> On 2016/12/06 at 23:57:36, mcgreevy2 wrote:
> > On 2016/12/06 06:31:27, djd wrote:
> > > Where is toUpload modified?
> > 
> > It is passed to bl.retries.AppendAll as a destination slice to append to
> (AppendAll then returns the resultant slice).
> 
> It looks like the len of toUpload can't change though? (You're not passing
> &toUpload to AppendAll.)
> So, can you bring dropLen down to the if on :126?
> You look convinced.

Done, but also un-inlined the call to AppendAll to make things a bit clearer. I
hope.

[djd-OOO-Apr2017, 2016-12-07 02:54:03]

lgtm

[mcgreevy, 2016-12-07 02:59:39]

The CQ bit was checked by mcgreevy@chromium.org

[commit-bot: I haz the power, 2016-12-07 02:59:51]

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-12-07 03:07:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481079579943080,
"parent_rev": "588899c32ba02a93d9f8710cd0e1f11212385c02", "commit_rev":
"7477886a355c7267508550a313fcaeed8bd12ada"}

[commit-bot: I haz the power, 2016-12-07 03:07:27]

Description was changed from

==========
Add batch logging support to eventlog.

BUG=658441
==========

to

==========
Add batch logging support to eventlog.

BUG=658441

Committed:
https://github.com/luci/luci-go/commit/7477886a355c7267508550a313fcaeed8bd12ada
==========

[commit-bot: I haz the power, 2016-12-07 03:07:28]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/7477886a355c7267508550a313fcaeed8bd12ada