rpc: implement introspection

rpc: implement introspection

This unearths an old branch that provided introspection
into the methods provided by the rpc package.
I've moved all the rpc method introspection stuff
into a new package, rpc/rpcreflect, that has
a number of potentially interesting use cases.

The "discarded method" log warnings are now gone;
an upcoming branch will add a test to state/apiserver/*
that exactly the expected methods are discarded.

https://code.launchpad.net/~rogpeppe/juju-core/411-rpc-introspection/+merge/186909

(do not edit description out of merge proposal)

[rog, 2013-09-21 06:03:02]

Please take a look.

[rog, 2013-09-22 19:16:29]

Please take a look.

[rog, 2013-09-22 19:49:45]

Please take a look.

[axw1, 2013-09-23 07:26:52]

LGTM, though you might want to wait for someone better versed in this code.

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go
File rpc/rpcreflect/methods.go (right):

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:49: var names []string
Fairly minor, but I'd prefer if this were sized up front. Also cached or
precomputed, depending on how heavily this is likely to be used (I guess not
much?)

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:62: return RootMethod{}, ErrMethodNotFound
Should this error message not include the input name?

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:244: log.Infof("considering method %#v\n", m)
Please make this Tracef, if it's needed at all.

[rog, 2013-09-23 09:06:29]

Please take a look.

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go
File rpc/rpcreflect/methods.go (right):

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:49: var names []string
On 2013/09/23 07:26:52, axw1 wrote:
> Fairly minor, but I'd prefer if this were sized up front. Also cached or
> precomputed, depending on how heavily this is likely to be used (I guess not
> much?)

I started that way and then told myself to not to be
silly - this method will not be called much at all.
Still, done anyway; and in ObjType.MethodNames too.

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:62: return RootMethod{}, ErrMethodNotFound
On 2013/09/23 07:26:52, axw1 wrote:
> Should this error message not include the input name?

I considered that, but generally the caller is in a better
place to make a nice error message. I think the error
atom works ok here, but could be persuaded otherwise.

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:244: log.Infof("considering method %#v\n", m)
On 2013/09/23 07:26:52, axw1 wrote:
> Please make this Tracef, if it's needed at all.

debugging remnant, removed, thanks.

[rog, 2013-09-23 09:08:55]

Please take a look.

[fwereade, 2013-09-23 09:10:01]

LGTM, very nice.

But I think we do want to identify the missing method :).

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go
File rpc/rpcreflect/methods.go (right):

https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
rpc/rpcreflect/methods.go:62: return RootMethod{}, ErrMethodNotFound
On 2013/09/23 07:26:52, axw1 wrote:
> Should this error message not include the input name?

If it does, and a client annotates the error with the context it knows, you get
a nasty stuttery error message. If it doesn't, and nor does the client, you get
a nasty useless error message.  The latter would seem to be strictly worse that
the former, and given that the method's exposed (and so we can't control further
annotations) it seems like we really should include the name.

[rog, 2013-09-23 09:35:44]

Please take a look.

[rog, 2013-09-23 09:36:38]

On 23 September 2013 10:10,  <fwereade@gmail.com> wrote:
> LGTM, very nice.
>
> But I think we do want to identify the missing method :).
>
>
>
> https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go
> File rpc/rpcreflect/methods.go (right):
>
>
https://codereview.appspot.com/13249054/diff/13001/rpc/rpcreflect/methods.go#...
> rpc/rpcreflect/methods.go:62: return RootMethod{}, ErrMethodNotFound
> On 2013/09/23 07:26:52, axw1 wrote:
>>
>> Should this error message not include the input name?
>
>
> If it does, and a client annotates the error with the context it knows,
> you get a nasty stuttery error message. If it doesn't, and nor does the
> client, you get a nasty useless error message.  The latter would seem to
> be strictly worse that the former, and given that the method's exposed
> (and so we can't control further annotations) it seems like we really
> should include the name.

As discussed online, I left it as is, but changed the docs to make it
more obvious
that it's the only error that can be returned.