Add debug-log server api

Add debug-log server api

Add the server side end-point for the debug-log api
call.  This is implemented as a websocket, with the
request parameters defining the filtering behaviour,
and the log lines that match being streamed back to
the user over the web-socket.

The first line over the websocket is the an error
structure.  If there are any issues either processing
the parameters or opening the log file, there is
an error, and if not, the Error component is 'null'.

The capabilities of the server match the older python
version as closely as possible.

https://code.launchpad.net/~thumper/juju-core/debug-log-apiserver/+merge/214455

(do not edit description out of merge proposal)

[thumper, 2014-04-06 22:15:39]

Please take a look.

[axw, 2014-04-07 02:21:09]

Haven't been through the tests yet, but here's my comments so far.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go
File state/apiserver/debuglog.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
tagsto include in the response
s/tagsto/tags to/

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
tagsto include in the response
I think "agent" might be the wrong term here. The user cares about the entity,
not the agent responsible for it. Entity is the name we use in state and API
when passing tags around like this.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:40: //      may finish with a '*' to match a prefix
eg: unit-mysql-*, machine-2
"tags may finish..."
(would be nice to also begin the line with a hyphen like in the following line,
so it's clear that it's not part of the previous sentence)

and while I'm being pedantic,
s/eg/e.g./

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:47: //   limit -> int - show this many lines then
exit
show *at most* this many lines

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:49: //   replay -> string - one of [true, false], if
true, start the file from the start
replay does not suggest going back to the beginning to me. how about rewind?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:59: stream, err := newLogStream(req.URL.Query())
Not a fan of constructor + init, it's too easy to overlook the need to
initialise later. Is there a good reason not to combine init() into
newLogStream?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:104: return nil, fmt.Errorf("maxLines value %q is
not a valid unsigned number", value)
add the error here too?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:113: return nil, fmt.Errorf("replay value %q is not
a valid boolean", value)
ditto

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:122: return nil, fmt.Errorf("backlog value %q is not
a valid unsigned number", value)
ditto

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:166: // authenticate parses HTTP basic
authentication and authorizes the
This is a bit out of place, and appears to be a copy-and-paste from
apiserver/httphandler.go
Can you please factor them?

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:218: if strings.HasSuffix(agent, ":") {
When does it not have the suffix?

[axw, 2014-04-07 03:36:04]

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
File state/apiserver/debuglog_internal_test.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
state/apiserver/debuglog_internal_test.go:272: expected := `line 1
How about a test that has maxLines>available?

[thumper, 2014-04-07 05:02:09]

Please take a look.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go
File state/apiserver/debuglog.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
tagsto include in the response
On 2014/04/07 02:21:10, axw wrote:
> s/tagsto/tags to/

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
tagsto include in the response
On 2014/04/07 02:21:10, axw wrote:
> I think "agent" might be the wrong term here. The user cares about the entity,
> not the agent responsible for it. Entity is the name we use in state and API
> when passing tags around like this.

OK. Will change, although I imagine issues later... but we can deal with that
later.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:40: //      may finish with a '*' to match a prefix
eg: unit-mysql-*, machine-2
On 2014/04/07 02:21:10, axw wrote:
> "tags may finish..."
> (would be nice to also begin the line with a hyphen like in the following
line,
> so it's clear that it's not part of the previous sentence)

Done.

> and while I'm being pedantic,
> s/eg/e.g./

Done

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:47: //   limit -> int - show this many lines then
exit
On 2014/04/07 02:21:10, axw wrote:
> show *at most* this many lines

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:49: //   replay -> string - one of [true, false], if
true, start the file from the start
On 2014/04/07 02:21:10, axw wrote:
> replay does not suggest going back to the beginning to me. how about rewind?

I kept 'replay' because it was what pyjuju used.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:59: stream, err := newLogStream(req.URL.Query())
On 2014/04/07 02:21:10, axw wrote:
> Not a fan of constructor + init, it's too easy to overlook the need to
> initialise later. Is there a good reason not to combine init() into
> newLogStream?

Probably a bad use of 'init'.  Will change to "start".

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:104: return nil, fmt.Errorf("maxLines value %q is
not a valid unsigned number", value)
On 2014/04/07 02:21:10, axw wrote:
> add the error here too?

I had wanted to provide a more useful and specific error to the caller. But I'll
add it.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:113: return nil, fmt.Errorf("replay value %q is not
a valid boolean", value)
On 2014/04/07 02:21:10, axw wrote:
> ditto

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:122: return nil, fmt.Errorf("backlog value %q is not
a valid unsigned number", value)
On 2014/04/07 02:21:10, axw wrote:
> ditto

Done.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:166: // authenticate parses HTTP basic
authentication and authorizes the
On 2014/04/07 02:21:10, axw wrote:
> This is a bit out of place, and appears to be a copy-and-paste from
> apiserver/httphandler.go
> Can you please factor them?

Yep. Didn't notice this was a copy and paste thing...

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
state/apiserver/debuglog.go:218: if strings.HasSuffix(agent, ":") {
On 2014/04/07 02:21:10, axw wrote:
> When does it not have the suffix?

Then it is unexpected, and the agent field in the logLine is not set.

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
File state/apiserver/debuglog_internal_test.go (right):

https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
state/apiserver/debuglog_internal_test.go:272: expected := `line 1
On 2014/04/07 03:36:04, axw wrote:
> How about a test that has maxLines>available?

Good call. Caught an issue.

[axw, 2014-04-07 05:11:06]

On 2014/04/07 05:02:09, thumper wrote:
> Please take a look.
> 
> https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go
> File state/apiserver/debuglog.go (right):
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
> tagsto include in the response
> On 2014/04/07 02:21:10, axw wrote:
> > s/tagsto/tags to/
> 
> Done.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:39: //   includeAgent -> []string - lists agent
> tagsto include in the response
> On 2014/04/07 02:21:10, axw wrote:
> > I think "agent" might be the wrong term here. The user cares about the
entity,
> > not the agent responsible for it. Entity is the name we use in state and API
> > when passing tags around like this.
> 
> OK. Will change, although I imagine issues later... but we can deal with that
> later.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:40: //      may finish with a '*' to match a
prefix
> eg: unit-mysql-*, machine-2
> On 2014/04/07 02:21:10, axw wrote:
> > "tags may finish..."
> > (would be nice to also begin the line with a hyphen like in the following
> line,
> > so it's clear that it's not part of the previous sentence)
> 
> Done.
> 
> > and while I'm being pedantic,
> > s/eg/e.g./
> 
> Done
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:47: //   limit -> int - show this many lines then
> exit
> On 2014/04/07 02:21:10, axw wrote:
> > show *at most* this many lines
> 
> Done.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:49: //   replay -> string - one of [true, false],
if
> true, start the file from the start
> On 2014/04/07 02:21:10, axw wrote:
> > replay does not suggest going back to the beginning to me. how about rewind?
> 
> I kept 'replay' because it was what pyjuju used.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:59: stream, err := newLogStream(req.URL.Query())
> On 2014/04/07 02:21:10, axw wrote:
> > Not a fan of constructor + init, it's too easy to overlook the need to
> > initialise later. Is there a good reason not to combine init() into
> > newLogStream?
> 
> Probably a bad use of 'init'.  Will change to "start".
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:104: return nil, fmt.Errorf("maxLines value %q is
> not a valid unsigned number", value)
> On 2014/04/07 02:21:10, axw wrote:
> > add the error here too?
> 
> I had wanted to provide a more useful and specific error to the caller. But
I'll
> add it.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:113: return nil, fmt.Errorf("replay value %q is
not
> a valid boolean", value)
> On 2014/04/07 02:21:10, axw wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:122: return nil, fmt.Errorf("backlog value %q is
not
> a valid unsigned number", value)
> On 2014/04/07 02:21:10, axw wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:166: // authenticate parses HTTP basic
> authentication and authorizes the
> On 2014/04/07 02:21:10, axw wrote:
> > This is a bit out of place, and appears to be a copy-and-paste from
> > apiserver/httphandler.go
> > Can you please factor them?
> 
> Yep. Didn't notice this was a copy and paste thing...
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog.go#ne...
> state/apiserver/debuglog.go:218: if strings.HasSuffix(agent, ":") {
> On 2014/04/07 02:21:10, axw wrote:
> > When does it not have the suffix?
> 
> Then it is unexpected, and the agent field in the logLine is not set.
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
> File state/apiserver/debuglog_internal_test.go (right):
> 
>
https://codereview.appspot.com/84880044/diff/1/state/apiserver/debuglog_inter...
> state/apiserver/debuglog_internal_test.go:272: expected := `line 1
> On 2014/04/07 03:36:04, axw wrote:
> > How about a test that has maxLines>available?
> 
> Good call. Caught an issue.

LGTM, thanks