Introduce TracingController and TracingControllerBackend skeleton

Introduce TracingController and TracingControllerBackend skeleton

BUG=356763
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287874

[nduca, 2014-08-04 19:30:25]

ptal

[nednguyen, 2014-08-04 22:12:07]

LGTM overall. Some nits & questions.

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/__init__.py:89: def
SetFullPerformanceModeEnabled(self, enabled):
Dead code?

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/platform_backend.py:70:
self.SetFullPerformanceModeEnabled(True)
Can you add some unittest for this logic of setting & unsetting this mode? If
there isn't an easy way to do it, I am fine with skipping it.

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_controller.py (right):

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_controller.py:16: """
Should we add assert self.AreOptionsSupported(trace_options) here?

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
(right):

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:65: return
self._platform_backend.running_browsers_and_backends.values()
Should we add a DidCreateBrowser(self, browser, browser_backend) to support the
case of running_browser_backends == 0 like you mention above? The use case would
be important if we want to support cold-start benchmarking case.

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/tracing_options.py (right):

https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/tracing_options.py:13:
self.enable_chrome_trace = False
Shouldn't we default chrome trace to True? Other tracing system can be default
to False.

[nduca, 2014-08-05 00:55:52]

> tools/telemetry/telemetry/core/platform/platform_backend.py:70:
self.SetFullPerformanceModeEnabled(True)
> Can you add some unittest for this logic of setting & unsetting this mode? If
there isn't an easy way to do it, I am fine with skipping it.
Kinda pain to test. Wasn't tested before. :(


>
https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/tracing_controller.py:16: """
> Should we add assert self.AreOptionsSupported(trace_options) here?

I think the contract I'm going to have is that Start() with unsupported is
allowed, because what-tracers-are-supported isn't knowable until we've started
the processes.

I updated docs accordingly. Eventually, we could have the return of the Stop
method contain metadata indicating which of the requested tracers actually
turned on.

>
https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/tracing_controller_backend.py:65:
return self._platform_backend.running_browsers_and_backends.values()
> Should we add a DidCreateBrowser(self, browser, browser_backend) to support
the case of running_browser_backends == 0 like you mention above? The use case
would be important if we want to support cold-start benchmarking case.

There is a DidCreateBrowser sent to the platform. I can add the tracer. Note
that supporting trace startup is going to be a bit of work, but its tractable.
Its something we could attack as we do the cold startup cleanup work perhaps.
The key thing is that the commandline for startup tracing needs to be extended
to support TraceOptions, and we need a trace-startup-and-leave-it-going instead
of trace-for-N-seconds. Probably you know all this code by now. :)

>
https://codereview.chromium.org/414253004/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/tracing_options.py:13:
self.enable_chrome_trace = False
> Shouldn't we default chrome trace to True? Other tracing system can be default
to False.

I'd like it to be very very explicit.

[nduca, 2014-08-05 01:04:09]

> There is a DidCreateBrowser sent to the platform. I can add the tracer. Note
that supporting trace startup is going to be a bit of work, but its tractable.
Its something we could attack as we do the cold startup cleanup work perhaps.
The key thing is that the commandline for startup tracing needs to be extended
to support TraceOptions, and we need a trace-startup-and-leave-it-going instead
of trace-for-N-seconds. Probably you know all this code by now. :)

Hmm actually we'll need a different hook for this case: for this to work, the
browser doesn't exist only the backend. At that point, it needs to ask the
tracing controller, "do you want to add any command line flags before I launch
this process." That seems a lot like "hey browser finder, i'd like to start this
browser with this startup url"

Lets leave this for a followup.

[nednguyen, 2014-08-05 01:17:27]

On 2014/08/05 01:04:09, nduca wrote:
> > There is a DidCreateBrowser sent to the platform. I can add the tracer. Note
> that supporting trace startup is going to be a bit of work, but its tractable.
> Its something we could attack as we do the cold startup cleanup work perhaps.
> The key thing is that the commandline for startup tracing needs to be extended
> to support TraceOptions, and we need a trace-startup-and-leave-it-going
instead
> of trace-for-N-seconds. Probably you know all this code by now. :)
> 
> Hmm actually we'll need a different hook for this case: for this to work, the
> browser doesn't exist only the backend. At that point, it needs to ask the
> tracing controller, "do you want to add any command line flags before I launch
> this process." That seems a lot like "hey browser finder, i'd like to start
this
> browser with this startup url"
> 
> Lets leave this for a followup.

SGTM

[nduca, 2014-08-06 18:46:22]

The CQ bit was checked by nduca@chromium.org

[commit-bot: I haz the power, 2014-08-06 18:47:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nduca@chromium.org/414253004/60001

[commit-bot: I haz the power, 2014-08-06 22:19:42]

Change committed as 287874