Use Systrace tracing controller in profile_chrome

Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662

TBR=ccraik@google.com

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/32f19b159dd6a951b71d1825271fc6856c2b429e

[washingtonp, 2016-09-01 01:51:21]

Description was changed from

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

BUG=catapult:#2662
==========

to

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662
==========

[washingtonp, 2016-09-01 01:52:33]

washingtonp@google.com changed reviewers:
+ ccraik@google.com, skyostil@chromium.org, zhenw@chromium.org

[washingtonp, 2016-09-01 01:52:34]

ptal (This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace)

[Sami, 2016-09-01 12:16:00]

Overall looks good. Couple of comments below.

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
systrace/bin/adb_profile_chrome_startup:74: options.chrome_startup = True
Why are we overriding the options set by the user here?

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
systrace/profile_chrome/chrome_startup_tracing_agent.py:111: return False
I think it would be more pythonic to either return the agent instance or None,
not a bool.

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
systrace/profile_chrome/chrome_startup_tracing_agent.py:122:
options.chrome_startup = True
I wouldn't expect a function called add_options to also set some options. Should
we do this elsewhere -- in the front-end perhaps?

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
systrace/profile_chrome/main.py:96: # they both use the same tracing
controller).
I assume this is temporary? Could you add a TODO as a reminder here?

[Chris Craik, 2016-09-01 17:24:05]

https://codereview.chromium.org/2297403003/diff/1/systrace/systrace/tracing_c...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/systrace/tracing_c...
systrace/systrace/tracing_controller.py:218: return ' + '.join(map(str,
self.child_agents))
should this sort the agents too, for consistency?

[washingtonp, 2016-09-01 17:44:41]

ptal

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
systrace/bin/adb_profile_chrome_startup:74: options.chrome_startup = True
On 2016/09/01 12:15:59, Sami wrote:
> Why are we overriding the options set by the user here?

adb_profile_chrome_startup does not provide the user with all of the
command line options that adb_profile_chrome and systrace provide.
However, these 3 clients share the same underlying tracing controller,
which expects some value (even if it is None) in the options
object it is passed. There are two solutions here:

1) Set these options that do not appear to the user when running
adb_profile_chrome_startup to None / another default value here
(what is currently being done)

or 

2) In each Get*Config() function in the agents, check if each
option in the option object exists, and if not, create it and set
it to None.

Would you like me to move to this second design? I can see the benefits
of this small design change, as it takes the burden away from the client
(for example adb_profile_chrome_startup) that does not care about
irrelevant options. I am actually leaning towards this but wondering
what others think.

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
systrace/profile_chrome/chrome_startup_tracing_agent.py:111: return False
On 2016/09/01 12:15:59, Sami wrote:
> I think it would be more pythonic to either return the agent instance or None,
> not a bool.

Done.

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/chr...
systrace/profile_chrome/chrome_startup_tracing_agent.py:122:
options.chrome_startup = True
On 2016/09/01 12:15:59, Sami wrote:
> I wouldn't expect a function called add_options to also set some options.
Should
> we do this elsewhere -- in the front-end perhaps?

Done.

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
systrace/profile_chrome/main.py:96: # they both use the same tracing
controller).
On 2016/09/01 12:15:59, Sami wrote:
> I assume this is temporary? Could you add a TODO as a reminder here?

Done. This reminds me - are you okay with adding the additional atrace
agent functionality / command line options to the profile_chrome
interface as well? This would expand profile_chrome's atrace tracing
functionality and would be the cleanest way to remove these options
from here.

https://codereview.chromium.org/2297403003/diff/1/systrace/systrace/tracing_c...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/systrace/tracing_c...
systrace/systrace/tracing_controller.py:218: return ' + '.join(map(str,
self.child_agents))
On 2016/09/01 17:24:05, Chris Craik wrote:
> should this sort the agents too, for consistency?

Done.

[Sami, 2016-09-02 12:57:43]

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
systrace/bin/adb_profile_chrome_startup:74: options.chrome_startup = True
On 2016/09/01 17:44:41, washingtonp wrote:
> On 2016/09/01 12:15:59, Sami wrote:
> > Why are we overriding the options set by the user here?
> 
> adb_profile_chrome_startup does not provide the user with all of the
> command line options that adb_profile_chrome and systrace provide.
> However, these 3 clients share the same underlying tracing controller,
> which expects some value (even if it is None) in the options
> object it is passed. There are two solutions here:
> 
> 1) Set these options that do not appear to the user when running
> adb_profile_chrome_startup to None / another default value here
> (what is currently being done)
> 
> or 
> 
> 2) In each Get*Config() function in the agents, check if each
> option in the option object exists, and if not, create it and set
> it to None.
> 
> Would you like me to move to this second design? I can see the benefits
> of this small design change, as it takes the burden away from the client
> (for example adb_profile_chrome_startup) that does not care about
> irrelevant options. I am actually leaning towards this but wondering
> what others think.

My worry is that in 1) the list will quickly get out of date and in 2) we need
to add lots of boilerplate that can have typos.

I wonder if we could do something where the list of agents can be configured
dynamically instead of being static? adb_profile_chrome_startup would then
override the list of agents to the ones it supports and all others would just
use the default set minus the startup agent. WDYT?

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
systrace/profile_chrome/main.py:96: # they both use the same tracing
controller).
On 2016/09/01 17:44:41, washingtonp wrote:
> On 2016/09/01 12:15:59, Sami wrote:
> > I assume this is temporary? Could you add a TODO as a reminder here?
> 
> Done. This reminds me - are you okay with adding the additional atrace
> agent functionality / command line options to the profile_chrome
> interface as well? This would expand profile_chrome's atrace tracing
> functionality and would be the cleanest way to remove these options
> from here.

Yeah, that would be great.

[washingtonp, 2016-09-02 20:10:27]

ptal

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/bin/adb_profile_ch...
systrace/bin/adb_profile_chrome_startup:74: options.chrome_startup = True
On 2016/09/02 12:57:42, Sami wrote:
> On 2016/09/01 17:44:41, washingtonp wrote:
> > On 2016/09/01 12:15:59, Sami wrote:
> > > Why are we overriding the options set by the user here?
> > 
> > adb_profile_chrome_startup does not provide the user with all of the
> > command line options that adb_profile_chrome and systrace provide.
> > However, these 3 clients share the same underlying tracing controller,
> > which expects some value (even if it is None) in the options
> > object it is passed. There are two solutions here:
> > 
> > 1) Set these options that do not appear to the user when running
> > adb_profile_chrome_startup to None / another default value here
> > (what is currently being done)
> > 
> > or 
> > 
> > 2) In each Get*Config() function in the agents, check if each
> > option in the option object exists, and if not, create it and set
> > it to None.
> > 
> > Would you like me to move to this second design? I can see the benefits
> > of this small design change, as it takes the burden away from the client
> > (for example adb_profile_chrome_startup) that does not care about
> > irrelevant options. I am actually leaning towards this but wondering
> > what others think.
> 
> My worry is that in 1) the list will quickly get out of date and in 2) we need
> to add lots of boilerplate that can have typos.
> 
> I wonder if we could do something where the list of agents can be configured
> dynamically instead of being static? adb_profile_chrome_startup would then
> override the list of agents to the ones it supports and all others would just
> use the default set minus the startup agent. WDYT?

I like that idea, and have implemented it in the most recent
code patch. LMK if this is what you had in mind.

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/1/systrace/profile_chrome/mai...
systrace/profile_chrome/main.py:96: # they both use the same tracing
controller).
On 2016/09/02 12:57:42, Sami wrote:
> On 2016/09/01 17:44:41, washingtonp wrote:
> > On 2016/09/01 12:15:59, Sami wrote:
> > > I assume this is temporary? Could you add a TODO as a reminder here?
> > 
> > Done. This reminds me - are you okay with adding the additional atrace
> > agent functionality / command line options to the profile_chrome
> > interface as well? This would expand profile_chrome's atrace tracing
> > functionality and would be the cleanest way to remove these options
> > from here.
> 
> Yeah, that would be great.

Great! This will be done in the CL that makes profile_chrome call
Systrace's atrace agent directly.

[Sami, 2016-09-05 11:05:29]

lgtm, looks much cleaner this way, thanks.

[Zhen Wang, 2016-09-07 16:37:11]

https://codereview.chromium.org/2297403003/diff/40001/systrace/bin/adb_profil...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/bin/adb_profil...
systrace/bin/adb_profile_chrome_startup:29: CHROME_STARTUP_MODULES =
[atrace_tracing_agent,
This should be private if it is not used by other files.

https://codereview.chromium.org/2297403003/diff/40001/systrace/profile_chrome...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/profile_chrome...
systrace/profile_chrome/main.py:24: PROFILE_CHROME_AGENT_MODULES =
[chrome_tracing_agent, ddms_tracing_agent,
This should be private if it is not used in other files.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:112: self.child_agents = None
Can you keep this private and create a property function for it? So
profile_chrome can access this, but not modify this.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:152: self.child_agents = succ_agents
Change this back to use private member |self._child_agents|.

Same for other places in this file.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:291: def
GetStartupControllerConfig(options):
s/GetStartupControllerConfig/GetChromeStartupControllerConfig

[washingtonp, 2016-09-07 18:36:08]

ptal

https://codereview.chromium.org/2297403003/diff/40001/systrace/bin/adb_profil...
File systrace/bin/adb_profile_chrome_startup (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/bin/adb_profil...
systrace/bin/adb_profile_chrome_startup:29: CHROME_STARTUP_MODULES =
[atrace_tracing_agent,
On 2016/09/07 16:37:11, Zhen Wang wrote:
> This should be private if it is not used by other files.

Done.

https://codereview.chromium.org/2297403003/diff/40001/systrace/profile_chrome...
File systrace/profile_chrome/main.py (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/profile_chrome...
systrace/profile_chrome/main.py:24: PROFILE_CHROME_AGENT_MODULES =
[chrome_tracing_agent, ddms_tracing_agent,
On 2016/09/07 16:37:11, Zhen Wang wrote:
> This should be private if it is not used in other files.

Done.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:112: self.child_agents = None
On 2016/09/07 16:37:11, Zhen Wang wrote:
> Can you keep this private and create a property function for it? So
> profile_chrome can access this, but not modify this.

Done.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:152: self.child_agents = succ_agents
On 2016/09/07 16:37:11, Zhen Wang wrote:
> Change this back to use private member |self._child_agents|.
> 
> Same for other places in this file.

Done.

https://codereview.chromium.org/2297403003/diff/40001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:291: def
GetStartupControllerConfig(options):
On 2016/09/07 16:37:11, Zhen Wang wrote:
> s/GetStartupControllerConfig/GetChromeStartupControllerConfig

Done.

[Zhen Wang, 2016-09-07 21:05:09]

lgtm with nit

https://codereview.chromium.org/2297403003/diff/60001/systrace/systrace/traci...
File systrace/systrace/tracing_controller.py (right):

https://codereview.chromium.org/2297403003/diff/60001/systrace/systrace/traci...
systrace/systrace/tracing_controller.py:115: def get_child_agents(self):
s/get_child_agents/child_agents/

[washingtonp, 2016-09-09 17:27:52]

@ccraik: ptal when you get the chance

[ChrisCraik, 2016-09-11 04:40:39]

chriscraik@gmail.com changed reviewers:
+ ChrisCraik@gmail.com

[ChrisCraik, 2016-09-11 04:40:40]

LGTM

(not sure if this will work - don't have access to work account right now)

[washingtonp, 2016-09-11 18:24:21]

The CQ bit was checked by washingtonp@google.com

[washingtonp, 2016-09-11 18:24:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2297403003/#ps60001
(title: "Added property method for tracing controller's child agents")

[commit-bot: I haz the power, 2016-09-11 18:24:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 18:44:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-11 18:44:18]

Try jobs failed on following builders:
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[washingtonp, 2016-09-11 21:23:08]

Description was changed from

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662
==========

to

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662

TBR=ccraik@google.com
==========

[washingtonp, 2016-09-11 21:23:08]

washingtonp@google.com changed reviewers:
+ chriscraik@gmail.com
- ChrisCraik@gmail.com

[washingtonp, 2016-09-11 21:24:07]

The CQ bit was checked by washingtonp@google.com

[commit-bot: I haz the power, 2016-09-11 21:24:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-11 21:25:34]

Description was changed from

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662

TBR=ccraik@google.com
==========

to

==========
Use Systrace tracing controller in profile_chrome

In the continuing effort to merge profile_chrome and Systrace,
profiler.py in profile_chrome needs to directly use the tracing
controller in Systrace. This involves refactoring Systrace's tracing
controller to handle various types of tracing categories (Chrome + Perf
in addition to its current atrace categories).

Note: This CL finishes laying the groundwork for moving
the profile_chrome agents to Systrace. After this CL,
5 parallel CLs will go out; each one will be an easy
move of one of the 5 profile_chrome agents into
Systrace.

BUG=catapult:#2662

TBR=ccraik@google.com

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-09-11 21:25:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[Chris Craik, 2016-09-12 17:10:13]

lgtm