[Telemetry] Platform-level tracing

[Telemetry] Platform-level tracing

Platform-level tracing doc:
https://docs.google.com/document/d/1Uv8SzOq2ugvzLiOcP85g7qPiaxwsn4tgOdxpe3YjUkM/edit?usp=sharing

BUG=515323, 535731
Committed: https://crrev.com/04d142b7aa08e23002a1082a5fdc3eb79a5afcee
Cr-Commit-Position: refs/heads/master@{#351220}

[Zhen Wang, 2015-09-21 20:46:59]

zhenw@chromium.org changed reviewers:
+ nednguyen@google.com

[Zhen Wang, 2015-09-23 00:23:56]

Patchset #5 (id:80001) has been deleted

[Zhen Wang, 2015-09-23 14:14:58]

ping

[Zhen Wang, 2015-09-28 15:40:30]

ping :)

[nednguyen, 2015-09-28 17:18:59]

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/tracing_controller_unittest.py (right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/tracing_controller_unittest.py:95: class
TracingControllerStartupTracingTest(unittest.TestCase):
Can you reuse the browser_test_case.BrowserTestCase instead of creating this
test from scratch?

Later we would want to group all telemetry unittests that actually invoke the
browsers.

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:92:
if trace_config_file and self.HasStartupTracingSupport():
Checking for trace_config_file here seems redundant..

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:121:
def HasStartupTracingSupport(self):
@property
def support_startup_tracing(self):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:124:
# crrev.com/1353583002).
Add a TODO to remove this once stable chrome has passed 2512

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py
(right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py:87:
config = tracing_config.TracingConfig(trace_options, category_filter)
Can you add comment explaining why we need to start both StartupTracing &
DevtoolTracing here. S.t along the lines of:

Chrome Tracing Agent needs to start tracing for chrome browsers that are not yet
started, and for the ones that already are. For the former, we first setup the
trace_config_file, which allows browsers that starts after this point to use it
for enabling tracing upon starting. For the latter, we invoke start tracing
command through devtools for browsers that are already started and tracked by
chrome_tracing_devtools_manager.

[Zhen Wang, 2015-09-28 20:42:49]

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/tracing_controller_unittest.py (right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/tracing_controller_unittest.py:95: class
TracingControllerStartupTracingTest(unittest.TestCase):
On 2015/09/28 17:18:59, nednguyen wrote:
> Can you reuse the browser_test_case.BrowserTestCase instead of creating this
> test from scratch?
> 
> Later we would want to group all telemetry unittests that actually invoke the
> browsers.

For browser_test_case tests, the browser is started in setUpClass(). But we want
to start browser after start tracing in this test.

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:92:
if trace_config_file and self.HasStartupTracingSupport():
On 2015/09/28 17:18:59, nednguyen wrote:
> Checking for trace_config_file here seems redundant..

Replaced by platform_backend.IsStartupTracingSupported().

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:121:
def HasStartupTracingSupport(self):
On 2015/09/28 17:18:59, nednguyen wrote:
> @property
> def support_startup_tracing(self):

Done.

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:124:
# crrev.com/1353583002).
On 2015/09/28 17:18:59, nednguyen wrote:
> Add a TODO to remove this once stable chrome has passed 2512

Done.

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py
(right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/platform/tracing_agent/chrome_tracing_agent.py:87:
config = tracing_config.TracingConfig(trace_options, category_filter)
On 2015/09/28 17:18:59, nednguyen wrote:
> Can you add comment explaining why we need to start both StartupTracing &
> DevtoolTracing here. S.t along the lines of:
> 
> Chrome Tracing Agent needs to start tracing for chrome browsers that are not
yet
> started, and for the ones that already are. For the former, we first setup the
> trace_config_file, which allows browsers that starts after this point to use
it
> for enabling tracing upon starting. For the latter, we invoke start tracing
> command through devtools for browsers that are already started and tracked by
> chrome_tracing_devtools_manager.

Done.

[nednguyen, 2015-09-28 21:36:24]

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/tracing_controller_unittest.py (right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/tracing_controller_unittest.py:95: class
TracingControllerStartupTracingTest(unittest.TestCase):
On 2015/09/28 20:42:49, Zhen Wang wrote:
> On 2015/09/28 17:18:59, nednguyen wrote:
> > Can you reuse the browser_test_case.BrowserTestCase instead of creating this
> > test from scratch?
> > 
> > Later we would want to group all telemetry unittests that actually invoke
the
> > browsers.
> 
> For browser_test_case tests, the browser is started in setUpClass(). But we
want
> to start browser after start tracing in this test. 

Can you override the setUpClass()? Or you can also close the browser temporarily
& restart it in your test methods.

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:91:
self.support_startup_tracing):
Sorry for back n forth on this, I think we can just make this cleaner with
"if self.support_startup_tracing:"

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:127:
return self.GetChromeBranchNumber() >= 2512
return self.GetChromeBranchNumber() >= 2512 and
"self._app_backend.platform_backend.platform.GetOSName() in ('Mac', 'Win',...)

[Zhen Wang, 2015-09-28 23:05:12]

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/tracing_controller_unittest.py (right):

https://codereview.chromium.org/1338423007/diff/120001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/tracing_controller_unittest.py:95: class
TracingControllerStartupTracingTest(unittest.TestCase):
On 2015/09/28 21:36:24, nednguyen wrote:
> On 2015/09/28 20:42:49, Zhen Wang wrote:
> > On 2015/09/28 17:18:59, nednguyen wrote:
> > > Can you reuse the browser_test_case.BrowserTestCase instead of creating
this
> > > test from scratch?
> > > 
> > > Later we would want to group all telemetry unittests that actually invoke
> the
> > > browsers.
> > 
> > For browser_test_case tests, the browser is started in setUpClass(). But we
> want
> > to start browser after start tracing in this test. 
> 
> Can you override the setUpClass()? Or you can also close the browser
temporarily
> & restart it in your test methods.

Done.

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
File
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py
(right):

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:91:
self.support_startup_tracing):
On 2015/09/28 21:36:24, nednguyen wrote:
> Sorry for back n forth on this, I think we can just make this cleaner with
> "if self.support_startup_tracing:"

Done.

https://codereview.chromium.org/1338423007/diff/140001/tools/telemetry/teleme...
tools/telemetry/telemetry/internal/backends/chrome_inspector/devtools_client_backend.py:127:
return self.GetChromeBranchNumber() >= 2512
On 2015/09/28 21:36:24, nednguyen wrote:
> return self.GetChromeBranchNumber() >= 2512 and
> "self._app_backend.platform_backend.platform.GetOSName() in ('Mac', 'Win',...)

Done.

[nednguyen, 2015-09-28 23:10:00]

lgtm

[Zhen Wang, 2015-09-28 23:12:27]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2015-09-28 23:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338423007/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338423007/160001

[commit-bot: I haz the power, 2015-09-29 00:50:52]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-09-29 00:53:12]

Patchset 8 (id:??) landed as
https://crrev.com/04d142b7aa08e23002a1082a5fdc3eb79a5afcee
Cr-Commit-Position: refs/heads/master@{#351220}