luci-go: Basic support for event logging in Go.

luci-go: Basic support for event logging in Go.

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

[mcgreevy, 2016-11-21 03:40:51]

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

[djd-OOO-Apr2017, 2016-11-21 04:02:24]

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:51: func (c *Client) LogSync(ctx context.Context,
events []*ChromeInfraLogEvent) error {
I'd use the ... form for events here (for the common case, you wouldn't need to
mint a slice).

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:98: // The time at which an event occurrred. Must
have been obtained from Time.Now.
sp. occurred

The "must have been obtained" is the bit that's kinda, uh, funny. It feels like
we should be able to just do that for the user.

Could NewLogEvent just take the enum directly?

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:53: req, err :=
http.NewRequest("POST", c.serverAddr, bytes.NewBuffer(buf))
NewBuffer -> NewReader

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:58: req.Header =
make(http.Header)
You can drop this line.

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:60: resp, err :=
ctxhttp.Do(ctx, c.HTTPClient, req)
We're targetting 1.7+ IIUC

So

req = req.WithContext(ctx)
resp, err := c.HTTPClient.Do(req)

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/settings.go...
common/eventlog/settings.go:18: func (cs *clientSettings) Populate(opts
[]ClientOption) {
This seems like overkill today – can we just have the loop in the one place that
calls Populate?

[mcgreevy, 2016-11-21 04:51:06]

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:51: func (c *Client) LogSync(ctx context.Context,
events []*ChromeInfraLogEvent) error {
On 2016/11/21 at 04:02:24, djd wrote:
> I'd use the ... form for events here (for the common case, you wouldn't need
to mint a slice).

Done.

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:98: // The time at which an event occurrred. Must
have been obtained from Time.Now.
On 2016/11/21 at 04:02:24, djd wrote:
> sp. occurred
> 
> The "must have been obtained" is the bit that's kinda, uh, funny. It feels
like we should be able to just do that for the user.
> 
> Could NewLogEvent just take the enum directly?

Yeah, it could, but I'm worried about making this API too inflexible.  What if
they want to record the time that they started handling a request and only later
create and log an event, passing in the start time? Calling time.Now for them
kind of inhibits that. It would still be doable -- they only have to override
the EventTimeMs in the returned proto, but I don't really want to encourage
that.

Perhaps this is where some helper functions could come in handy:

// Returns Now as a Point
func Point() TypedTime {...}

// Returns Now as a Begin
func Begin() TypedTime {...}

// Returns Now as an End
func End() TypedTime {...}


// Or:

func Now(kind TimeKind) TypedTime {}

type TimeKind int32

const (
  Unknown TimeKind = iota
  Point
  Begin
  End 
)

This pushes the weirdness away from the common usage -- you only need care about
it if you try to construct a TypedTime manually.

WDYT?

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:53: req, err :=
http.NewRequest("POST", c.serverAddr, bytes.NewBuffer(buf))
On 2016/11/21 at 04:02:24, djd wrote:
> NewBuffer -> NewReader

Done.

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:58: req.Header =
make(http.Header)
On 2016/11/21 at 04:02:24, djd wrote:
> You can drop this line.

Done.

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/internal/lo...
common/eventlog/internal/logservice/logservice.go:60: resp, err :=
ctxhttp.Do(ctx, c.HTTPClient, req)
On 2016/11/21 at 04:02:24, djd wrote:
> We're targetting 1.7+ IIUC
> 
> So
> 
> req = req.WithContext(ctx)
> resp, err := c.HTTPClient.Do(req)

Yes, it looks like we moved to 1.7 since I last synced.

Done.

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/settings.go...
common/eventlog/settings.go:18: func (cs *clientSettings) Populate(opts
[]ClientOption) {
On 2016/11/21 at 04:02:24, djd wrote:
> This seems like overkill today – can we just have the loop in the one place
that calls Populate?

Done.

[djd-OOO-Apr2017, 2016-11-21 05:51:26]

lgtm

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

https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
common/eventlog/eventlog.go:98: // The time at which an event occurrred. Must
have been obtained from Time.Now.
On 2016/11/21 at 04:51:06, mcgreevy2 wrote:
> On 2016/11/21 at 04:02:24, djd wrote:
> > sp. occurred
> > 
> > The "must have been obtained" is the bit that's kinda, uh, funny. It feels
like we should be able to just do that for the user.
> > 
> > Could NewLogEvent just take the enum directly?
> 
> Yeah, it could, but I'm worried about making this API too inflexible.  What if
they want to record the time that they started handling a request and only later
create and log an event, passing in the start time? Calling time.Now for them
kind of inhibits that. It would still be doable -- they only have to override
the EventTimeMs in the returned proto, but I don't really want to encourage
that.
> 
> Perhaps this is where some helper functions could come in handy:
> 
> // Returns Now as a Point
> func Point() TypedTime {...}
> 
> // Returns Now as a Begin
> func Begin() TypedTime {...}
> 
> // Returns Now as an End
> func End() TypedTime {...}
> 
> 
> // Or:
> 
> func Now(kind TimeKind) TypedTime {}
> 
> type TimeKind int32
> 
> const (
>   Unknown TimeKind = iota
>   Point
>   Begin
>   End 
> )
> 
> This pushes the weirdness away from the common usage -- you only need care
about it if you try to construct a TypedTime manually.
> 
> WDYT?

I like the first of those options (Point/Begin/End) more. They feel less fussy.

I think making a copy of the enum is interesting, but it's weird you're doing it
for that part of the proto and not the entire structure.

[mcgreevy, 2016-11-21 06:21:34]

On 2016/11/21 at 05:51:26, djd wrote:
> lgtm
> 
> https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go
> File common/eventlog/eventlog.go (right):
> 
>
https://codereview.chromium.org/2517503002/diff/1/common/eventlog/eventlog.go...
> common/eventlog/eventlog.go:98: // The time at which an event occurrred. Must
have been obtained from Time.Now.
> On 2016/11/21 at 04:51:06, mcgreevy2 wrote:
> > On 2016/11/21 at 04:02:24, djd wrote:
> > > sp. occurred
> > > 
> > > The "must have been obtained" is the bit that's kinda, uh, funny. It feels
like we should be able to just do that for the user.
> > > 
> > > Could NewLogEvent just take the enum directly?
> > 
> > Yeah, it could, but I'm worried about making this API too inflexible.  What
if they want to record the time that they started handling a request and only
later create and log an event, passing in the start time? Calling time.Now for
them kind of inhibits that. It would still be doable -- they only have to
override the EventTimeMs in the returned proto, but I don't really want to
encourage that.
> > 
> > Perhaps this is where some helper functions could come in handy:
> > 
> > // Returns Now as a Point
> > func Point() TypedTime {...}
> > 
> > // Returns Now as a Begin
> > func Begin() TypedTime {...}
> > 
> > // Returns Now as an End
> > func End() TypedTime {...}
> > 
> > 
> > // Or:
> > 
> > func Now(kind TimeKind) TypedTime {}
> > 
> > type TimeKind int32
> > 
> > const (
> >   Unknown TimeKind = iota
> >   Point
> >   Begin
> >   End 
> > )
> > 
> > This pushes the weirdness away from the common usage -- you only need care
about it if you try to construct a TypedTime manually.
> > 
> > WDYT?
> 
> I like the first of those options (Point/Begin/End) more. They feel less
fussy.
> 
> I think making a copy of the enum is interesting, but it's weird you're doing
it for that part of the proto and not the entire structure.

Done.

[djd-OOO-Apr2017, 2016-11-21 21:45:55]

LGTM still

[mcgreevy, 2016-11-21 22:44:10]

The CQ bit was checked by mcgreevy@chromium.org

[commit-bot: I haz the power, 2016-11-21 22:44:19]

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-11-21 22:50:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479768250569290,
"parent_rev": "14213b9083041668662a7a22d03e28cb11ab5f60", "commit_rev":
"c34ebd4c33298254cf237ed7b8113687ea583a2b"}

[commit-bot: I haz the power, 2016-11-21 22:50:16]

Description was changed from

==========
luci-go: Basic support for event logging in Go.

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

to

==========
luci-go: Basic support for event logging in Go.

BUG=658441

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

[commit-bot: I haz the power, 2016-11-21 22:50:17]

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