De-client tracing.TraceController interface

De-client tracing.TraceController interface

The tracing service logically provides one service, TraceCoordinator, which can
start and stop tracing sessions. It also can connect to any number of
TraceControllers each of which can supply tracing information on demand.

Whenever an app connects to the tracing service, the tracing service attempts to
connect to that app's TraceController interface. If the app provides this
interface then tracing data will be requested whenever tracing starts. If the
app doesn't, then the pipe just closes. Thus apps that want to be traced can do
so by creating however many connections to the tracing service they want and
registering a TraceController implementation on each outgoing connection.

The shell connects in a similar fashion by connecting to the tracing service and
providing a TraceController implementation. The code looks a bit different since
the shell is special.

BUG=451319
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/47225cba1cfeb74f6b3f6ff245c5f791a6581a7e

[jamesr, 2014-12-19 02:00:58]

jamesr@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[jamesr, 2014-12-19 02:00:59]

https://codereview.chromium.org/791493006/diff/1/services/tracing/main.cc
File services/tracing/main.cc (right):

https://codereview.chromium.org/791493006/diff/1/services/tracing/main.cc#new...
services/tracing/main.cc:67: // TODO: We really should keep track of how many
connections we have here
what i really want do to is join all controller message pipes up to some timeout

[qsr, 2014-12-19 09:47:14]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2014-12-19 09:47:15]

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc#newcode134
shell/context.cc:134: StrongBinding<ServiceProvider> binding_;
I might be missing something, but if I understand correctly, this means that
this object will be auto-deleted when the pipe is closed, but you still keep it
in a scoped_ptr on the context. Wouldn't that generate a double free if the
handle is closed?

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc#newcode208
shell/context.cc:208: spp.Bind(pipe.handle1.Pass());
Did we lose the ability to bury the pipes with the new mojo::Bindings interface?
Or should TracingServiceProvider takes an InterfaceRequest so that we can do:

ServiceProviderPtr spp;
InterfaceRequest<ServiceProvider> irsp = GetProxy(&spp);
tracing_sp_.reset(new Context::TracingServiceProvider(irsp.Pass()));

It is conceptually exactly the same, but I was on the impression that we
prefered burying pipe when possible.

[jamesr, 2014-12-29 18:57:59]

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc#newcode134
shell/context.cc:134: StrongBinding<ServiceProvider> binding_;
On 2014/12/19 09:47:14, qsr (OoO until Jan 5) wrote:
> I might be missing something, but if I understand correctly, this means that
> this object will be auto-deleted when the pipe is closed, but you still keep
it
> in a scoped_ptr on the context. Wouldn't that generate a double free if the
> handle is closed?

You are correct.  I'll get rid of the scoped_ptr (there's no need to retain this
on the Context)

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc#newcode208
shell/context.cc:208: spp.Bind(pipe.handle1.Pass());
On 2014/12/19 09:47:14, qsr (OoO until Jan 5) wrote:
> Did we lose the ability to bury the pipes with the new mojo::Bindings
interface?
> Or should TracingServiceProvider takes an InterfaceRequest so that we can do:
> 
> ServiceProviderPtr spp;
> InterfaceRequest<ServiceProvider> irsp = GetProxy(&spp);
> tracing_sp_.reset(new Context::TracingServiceProvider(irsp.Pass()));
> 
> It is conceptually exactly the same, but I was on the impression that we
> prefered burying pipe when possible.

I think hiding these makes this code a lot harder to understand.

[qsr, 2015-01-05 13:38:31]

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/791493006/diff/20001/shell/context.cc#newcode208
shell/context.cc:208: spp.Bind(pipe.handle1.Pass());
On 2014/12/29 18:57:58, jamesr wrote:
> On 2014/12/19 09:47:14, qsr (OoO until Jan 5) wrote:
> > Did we lose the ability to bury the pipes with the new mojo::Bindings
> interface?
> > Or should TracingServiceProvider takes an InterfaceRequest so that we can
do:
> > 
> > ServiceProviderPtr spp;
> > InterfaceRequest<ServiceProvider> irsp = GetProxy(&spp);
> > tracing_sp_.reset(new Context::TracingServiceProvider(irsp.Pass()));
> > 
> > It is conceptually exactly the same, but I was on the impression that we
> > prefered burying pipe when possible.
> 
> I think hiding these makes this code a lot harder to understand.

 Hum... Ok, I have a tendency to think the opposite, because burying the pipe
makes it type safe (as everything is linked to the final type, instead of
passing a pipe and TracingServiceProvider constructor is explicit about what it
expects).

[jamesr, 2015-01-06 00:03:30]

The code you propose has types, but it's not type safe.  Type safety means that
the type system checks for bugs.  It's possible to write all sorts of code that
uses types and is type-correct but does the wrong thing.  Since we're starting
from scratch here (i.e. we don't have any endpoints) the type system can't
really help us correctly allocate a pipe and pass the different endpoints to
different places.  Burying the actual code here just makes it really hard to see
what's happening without helping.

If we were getting a handle from bindings, say, where there is external type
information then the type system would be very useful in keeping the types
consistent throughout.

[qsr, 2015-01-07 18:28:06]

On 2015/01/06 00:03:30, jamesr wrote:
> The code you propose has types, but it's not type safe.  Type safety means
that
> the type system checks for bugs.

 Hum, not sure to understand you. The code I propose checks that the
implementation of one side of the pipe correspond to the proxy on the other
side. That's what I meant by type safety. I agree that at this point, it is not
that hard to check that when writing the code, but when we start to refactor, it
is very helpful.

[jamesr, 2015-01-29 23:29:29]

jamesr@chromium.org changed reviewers:
+ eseidel@chromium.org
- qsr@chromium.org, viettrungluu@chromium.org

[jamesr, 2015-01-29 23:29:30]

OK, this sat for a while now it's reworked a fair bit.  It's now type-safer and
(imo) simpler overall.

Eric - wanna take a look?  skydb is currently the only thing that actually knows
how to activate and use this system

[viettrungluu, 2015-02-02 21:22:43]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-02-02 21:22:44]

lgtm w/nits

https://codereview.chromium.org/791493006/diff/60001/mojo/common/trace_contro...
File mojo/common/trace_controller_impl.h (right):

https://codereview.chromium.org/791493006/diff/60001/mojo/common/trace_contro...
mojo/common/trace_controller_impl.h:33: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/791493006/diff/60001/mojo/common/tracing_impl.cc
File mojo/common/tracing_impl.cc (right):

https://codereview.chromium.org/791493006/diff/60001/mojo/common/tracing_impl...
mojo/common/tracing_impl.cc:22: connection->AddService(this);
As discussed in person and not applying to this CL in particular:
* Calling AddService() after ConnectToApplication() is pretty unintuitive.
* I learned about ConfigureOutgoingConnection(), which is probably morally wrong
(since it implies that you might want to provide services on outgoing
connections independent on the target application, which is wrong). We should
probably remove it, before someone gets the idea that they should use it.

https://codereview.chromium.org/791493006/diff/60001/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/791493006/diff/60001/shell/context.cc#newcode166
shell/context.cc:166: };
DISALLOW_COPY_AND_ASSIGN

[jamesr, 2015-02-03 00:56:39]

Committed patchset #4 (id:60001) manually as
47225cba1cfeb74f6b3f6ff245c5f791a6581a7e (presubmit successful).