Pass in custom options to Systrace agents

Pass in custom options to Systrace agents

This CL refactors the Systace agent API to take in a custom config
object tailored to the object. In doing so, this CL:

- Updates Systrace's StartAgentTracing() to take in custom options
- Updates profile_chrome's StartAgentTracing() as well
- Updates the Telemetry call that calls Systrace
- Updates the unit tests accordingly

Note that profile_chrome still needs to be followed to follow Systrace's
controller API. This will be in a separate follow-up CL.

Because so much code is moved between files, this CL looks
artificially large.

BUG=catapult:#2690

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/2629713e67ee64c5448824c93f086efc92a5c2f0

[washingtonp, 2016-08-26 02:58:06]

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

[washingtonp, 2016-08-26 02:58:07]

ptal

[washingtonp, 2016-08-26 02:58:54]

Description was changed from

==========
Pass in custom options to Systrace agents

This CL refactors the Systace agent API to take in a custom config
object tailored to the object. In doing so, this CL:

- Updates Systrace's StartAgentTracing() to take in custom options
- Updates profile_chrome's StartAgentTracing() as well
- Updates the Telemetry call that calls Systrace
- Updates the unit tests accordingly

Note that profile_chrome still needs to be followed to follow Systrace's
controller API. This will be in a separate follow-up CL.

BUG=catapult:#2690
==========

to

==========
Pass in custom options to Systrace agents

This CL refactors the Systace agent API to take in a custom config
object tailored to the object. In doing so, this CL:

- Updates Systrace's StartAgentTracing() to take in custom options
- Updates profile_chrome's StartAgentTracing() as well
- Updates the Telemetry call that calls Systrace
- Updates the unit tests accordingly

Note that profile_chrome still needs to be followed to follow Systrace's
controller API. This will be in a separate follow-up CL.

Because so much code is moved between files, this CL looks
artificially large.

BUG=catapult:#2690
==========

[Sami, 2016-08-26 15:53:43]

Couple of minor suggestions.

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent.py:5: import optparse
Not something for this patch, but we should consider migrating from optparse to
argparse. Optparse has been deprecated and argparse is available since Python
2.7. It also has a nice subparser concept which seems to match what we need
here.

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent.py:122: 
nit: two blank lines around top level entries please (here and elsewhere).

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
File systrace/profile_chrome/atrace_tracing_agent_unittest.py (left):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent_unittest.py:17: categories =
['gfx', 'input', 'view']
Can we keep using these categories? I'm not sure we need to change the test
setup for this patch, do we?

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
File systrace/profile_chrome/profiler.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
systrace/profile_chrome/profiler.py:31: if repr(agent) == 'faketrace':
Can we avoid hardcoding faketrace here? How about we just skip creating configs
for agents that don't appear in AGENT_TYPE_TO_MODULE? (Maybe we could remove
stub config for the startup agent too.)

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
systrace/profile_chrome/profiler.py:132: def get_config(options):
Is this used? If not, let's remove it.

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/systrace_...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/systrace_...
systrace/systrace/systrace_runner.py:85: A list of tuples containing agents and
their corresponding configuration
nit: You're not returning tuples but AgentWithConfig instances. I'd be fine with
these actually being tuples.

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/tracing_a...
File systrace/systrace/tracing_agents/__init__.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/tracing_a...
systrace/systrace/tracing_agents/__init__.py:36: config: TracingOptions subclass
containing agent-specific options
s/TracingOptions/TracingConfig/?

[washingtonp, 2016-08-26 18:50:55]

ptal

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent.py:5: import optparse
On 2016/08/26 15:53:43, Sami wrote:
> Not something for this patch, but we should consider migrating from optparse
to
> argparse. Optparse has been deprecated and argparse is available since Python
> 2.7. It also has a nice subparser concept which seems to match what we need
> here.

I agree with this. The subcommand support, better support for
variable numbers of arguments, and support for positional
arguments have me sold. I will do this in a future CL, and have
created a new Catapult issue for this here:

https://github.com/catapult-project/catapult/issues/2725

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent.py:122: 
On 2016/08/26 15:53:43, Sami wrote:
> nit: two blank lines around top level entries please (here and elsewhere).

Done.

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
File systrace/profile_chrome/atrace_tracing_agent_unittest.py (left):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/atr...
systrace/profile_chrome/atrace_tracing_agent_unittest.py:17: categories =
['gfx', 'input', 'view']
On 2016/08/26 15:53:43, Sami wrote:
> Can we keep using these categories? I'm not sure we need to change the test
> setup for this patch, do we?

Done. Updated other profile_chrome unit tests as well.

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
File systrace/profile_chrome/profiler.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
systrace/profile_chrome/profiler.py:31: if repr(agent) == 'faketrace':
On 2016/08/26 15:53:43, Sami wrote:
> Can we avoid hardcoding faketrace here? How about we just skip creating
configs
> for agents that don't appear in AGENT_TYPE_TO_MODULE? (Maybe we could remove
> stub config for the startup agent too.)

Done. We cannot remove the stub for the startup agent, by the way,
because adb_profile_chrome_startup directly calls this function.

https://codereview.chromium.org/2276263003/diff/1/systrace/profile_chrome/pro...
systrace/profile_chrome/profiler.py:132: def get_config(options):
On 2016/08/26 15:53:43, Sami wrote:
> Is this used? If not, let's remove it.

Done.

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/systrace_...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/systrace_...
systrace/systrace/systrace_runner.py:85: A list of tuples containing agents and
their corresponding configuration
On 2016/08/26 15:53:43, Sami wrote:
> nit: You're not returning tuples but AgentWithConfig instances. I'd be fine
with
> these actually being tuples.

Done.

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/tracing_a...
File systrace/systrace/tracing_agents/__init__.py (right):

https://codereview.chromium.org/2276263003/diff/1/systrace/systrace/tracing_a...
systrace/systrace/tracing_agents/__init__.py:36: config: TracingOptions subclass
containing agent-specific options
On 2016/08/26 15:53:43, Sami wrote:
> s/TracingOptions/TracingConfig/?

Done.

[Chris Craik, 2016-08-26 20:32:18]

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:72: class AgentWithConfig(object):
This structure feels a bit strange. We always want an agent and config tied
together, since neither makes sense without the other. Now we have a layer of
indirection that appears to only make sense at construction time (since we just
want to deal with agents directly, later on).

Can we just store the config within the agent, in their constructors? Then,
constructing the agents (and implicitly, the configs) would require passing
options, but I think that makes sense.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:128: elif 'sched' in
config.atrace_categories:
this should still take categories as an argument, since atrace_categories is
mutated in StartAgentTracing (currently trying to use the default categories
fails)

(while we could just store the default list in the AtraceConfig options object,
I have a separate bug open to make the default behavior more dynamic)

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_from_file_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_from_file_agent.py:38: options =
optparse.OptionGroup(parser, '')
There's an extra couple newlines and a colon printed out with --help in between
atrace and battor. Is it because of this? Maybe just return None and have the
caller tolerate that?

[Zhen Wang, 2016-08-26 20:39:41]

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
systrace/profile_chrome/atrace_tracing_agent.py:124: class
AtraceConfig(tracing_agents.TracingConfig):
Is the plan to use the AtraceConfig in Systrace after the migration?

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
systrace/profile_chrome/chrome_tracing_agent.py:184: def
_ComputeChromeCategories(options):
This is actually taking config. The naming is confusing here.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:135: # Override --atrace flag if command-line
categories are provided.
Do you mean --atrace-categories instead of --atrace?

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:137: options.atrace_categories = categories
This is also for --ftrace-categories flag.

Have you tested ftrace?

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:161: script_dir, options)
Merge this line to the previous line.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:31: options: List of command line options.
options is not a list.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:90: result =
[AgentWithConfig(module.try_create_agent(options),
try_create_agent() should not take options. It should take the config as the
input parameter.

And since try_create_agent() could return None, we need to check that before
putting the result to the list.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:72: config: Command line
config.
In systrace_runner.py, you are passing in options instead of config. The naming
is confusing here.

And this function should take tracing config instead of options.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:74: if config.target !=
'android':
target should be included in atrace config. Then you do not need to pass in
options.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:77: return False
Should this function return False? This is not your problem as the old code
already returns false. We should return None here. And the systrace_runner.py,
if the return value is None, we do not include it in the agent list.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:521: trace_t):
s/trace_t/trace_time

[Zhen Wang, 2016-08-26 20:51:18]

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:72: class AgentWithConfig(object):
On 2016/08/26 20:32:18, Chris Craik wrote:
> This structure feels a bit strange. We always want an agent and config tied
> together, since neither makes sense without the other. Now we have a layer of
> indirection that appears to only make sense at construction time (since we
just
> want to deal with agents directly, later on).
> 
> Can we just store the config within the agent, in their constructors? Then,
> constructing the agents (and implicitly, the configs) would require passing
> options, but I think that makes sense.

I think the root cause of this is because config is not passed to agent when the
agent is constructed, but when StartAgentTracing(config, ...) is called.

For systrace itself, it is probably fine with either way (maybe even cleaner if
do it in constructor). But some clients, e.g., Telemetry, uses the 2nd way,
i.e., pass config when calling StartAgentTracing. And it is not easy to change
that.

Is it ok to separate the construction of the agent and the configuration of the
agent? This may be a little awkward. But the good thing is that the agent does
not have access to all options, it only has access to its own config. The only
function having access to all options is get_config(), which is easier to
control.

[Chris Craik, 2016-08-26 20:58:05]

> I think the root cause of this is because config is not passed to agent when
the agent is constructed, but when StartAgentTracing(config, ...) is called.
> 
> For systrace itself, it is probably fine with either way (maybe even cleaner
if do it in constructor). But some clients, e.g., Telemetry, uses the 2nd way,
i.e., pass config when calling StartAgentTracing. And it is not easy to change
that.

Its config would still be available at the time of StartAgentTracing if it was
built at construction time - can you explain more about the difficulty? I like
the idea of the configuration not being stateful, and just being immediately
constructed and configured all at once.
 
> Is it ok to separate the construction of the agent and the configuration of
the agent? This may be a little awkward. But the good thing is that the agent
does not have access to all options, it only has access to its own config. The
only function having access to all options is get_config(), which is easier to
control.

[Zhen Wang, 2016-08-26 21:41:48]

On 2016/08/26 20:58:05, Chris Craik wrote:
> > I think the root cause of this is because config is not passed to agent when
> the agent is constructed, but when StartAgentTracing(config, ...) is called.
> > 
> > For systrace itself, it is probably fine with either way (maybe even cleaner
> if do it in constructor). But some clients, e.g., Telemetry, uses the 2nd way,
> i.e., pass config when calling StartAgentTracing. And it is not easy to change
> that.
> 
> Its config would still be available at the time of StartAgentTracing if it was
> built at construction time - can you explain more about the difficulty? I like
> the idea of the configuration not being stateful, and just being immediately
> constructed and configured all at once.

The API is like the following:

class TracingAgent(object):
  def __init__(self):
    pass

  def StartAgentTracing(self, config, timeout=None):
    ...

When tracing agent is constructed, it takes nothing. Then when
StartAgentTracing(config) is called, config is passed in from the controller. So
the agent is not configured when constructed.


If now we move to:

class TracingAgent(object):
  def __init__(self, config):
    self._config = config // or get values from config

  def StartAgentTracing(self, timeout=None):
    // now use self._config

The problem becomes the controller needs to have the config when constructing
the agents. This is ok for systrace. This may not be easy for Telemetry (can be
done, but needs refactor in Telemetry).

Both Telemetry and Chrome are passing config to agent when calling
StartAgentTracing. So each tracing session (between start and stop) can have
different configurations. Systrace does not have this need because systrace just
have one tracing session and then exit. But if we keep it function in the same
way, it may be easier.

[washingtonp, 2016-08-27 02:22:44]

ptal

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
systrace/profile_chrome/atrace_tracing_agent.py:124: class
AtraceConfig(tracing_agents.TracingConfig):
On 2016/08/26 20:39:41, Zhen Wang wrote:
> Is the plan to use the AtraceConfig in Systrace after the migration?

Yes, this is just a temporary config since the atrace agent in profile_chrome
doesn't support most of the options that the Systrace agent supports.

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/profile_chrome...
systrace/profile_chrome/chrome_tracing_agent.py:184: def
_ComputeChromeCategories(options):
On 2016/08/26 20:39:41, Zhen Wang wrote:
> This is actually taking config. The naming is confusing here.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:135: # Override --atrace flag if command-line
categories are provided.
On 2016/08/26 20:39:41, Zhen Wang wrote:
> Do you mean --atrace-categories instead of --atrace?

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:137: options.atrace_categories = categories
On 2016/08/26 20:39:41, Zhen Wang wrote:
> This is also for --ftrace-categories flag.
> 
> Have you tested ftrace?

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:161: script_dir, options)
On 2016/08/26 20:39:41, Zhen Wang wrote:
> Merge this line to the previous line.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
File systrace/systrace/systrace_runner.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:31: options: List of command line options.
On 2016/08/26 20:39:41, Zhen Wang wrote:
> options is not a list.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/systr...
systrace/systrace/systrace_runner.py:90: result =
[AgentWithConfig(module.try_create_agent(options),
On 2016/08/26 20:39:41, Zhen Wang wrote:
> try_create_agent() should not take options. It should take the config as the
> input parameter.
> 
> And since try_create_agent() could return None, we need to check that before
> putting the result to the list.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:72: config: Command line
config.
On 2016/08/26 20:39:41, Zhen Wang wrote:
> In systrace_runner.py, you are passing in options instead of config. The
naming
> is confusing here.
> 
> And this function should take tracing config instead of options.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:74: if config.target !=
'android':
On 2016/08/26 20:39:41, Zhen Wang wrote:
> target should be included in atrace config. Then you do not need to pass in
> options.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:77: return False
On 2016/08/26 20:39:41, Zhen Wang wrote:
> Should this function return False? This is not your problem as the old code
> already returns false. We should return None here. And the systrace_runner.py,
> if the return value is None, we do not include it in the agent list.

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:128: elif 'sched' in
config.atrace_categories:
On 2016/08/26 20:32:18, Chris Craik wrote:
> this should still take categories as an argument, since atrace_categories is
> mutated in StartAgentTracing (currently trying to use the default categories
> fails)
> 
> (while we could just store the default list in the AtraceConfig options
object,
> I have a separate bug open to make the default behavior more dynamic)

Done.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:521: trace_t):
On 2016/08/26 20:39:41, Zhen Wang wrote:
> s/trace_t/trace_time

I did this because this agent also uses py_trace_event.trace_time, so there was
a name
collision error. To avoid this, I have imported py_trace_event.trace_time as
something
else (trace_time_handler), but let me know if this is not the preferred solution
to this.

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_from_file_agent.py (right):

https://codereview.chromium.org/2276263003/diff/30001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_from_file_agent.py:38: options =
optparse.OptionGroup(parser, '')
On 2016/08/26 20:32:18, Chris Craik wrote:
> There's an extra couple newlines and a colon printed out with --help in
between
> atrace and battor. Is it because of this? Maybe just return None and have the
> caller tolerate that?

Done.

[Zhen Wang, 2016-08-27 15:32:42]

Took a quick look. Mostly look ok to me. Some nits.

https://codereview.chromium.org/2276263003/diff/50001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/profile_chrome...
systrace/profile_chrome/chrome_startup_tracing_agent.py:99: # command line
options.
The comments need some revision. startup tracing's options are specified in
adb_profile_chrome_startup. Currently it does not support working with other
agents.

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:163: options.ftrace_categories = categories
Those two should be assigned based on options.target.

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_agent.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:14: from py_trace_event import
trace_time as trace_time_handler
s/trace_time_handler/trace_time_module

[washingtonp, 2016-08-29 06:40:02]

ptal

https://codereview.chromium.org/2276263003/diff/50001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/profile_chrome...
systrace/profile_chrome/chrome_startup_tracing_agent.py:99: # command line
options.
On 2016/08/27 15:32:41, Zhen Wang wrote:
> The comments need some revision. startup tracing's options are specified in
> adb_profile_chrome_startup. Currently it does not support working with other
> agents.

Done.

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:163: options.ftrace_categories = categories
On 2016/08/27 15:32:41, Zhen Wang wrote:
> Those two should be assigned based on options.target.

Done.

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/traci...
File systrace/systrace/tracing_agents/atrace_agent.py (right):

https://codereview.chromium.org/2276263003/diff/50001/systrace/systrace/traci...
systrace/systrace/tracing_agents/atrace_agent.py:14: from py_trace_event import
trace_time as trace_time_handler
On 2016/08/27 15:32:41, Zhen Wang wrote:
> s/trace_time_handler/trace_time_module

Done.

[Zhen Wang, 2016-08-29 16:30:21]

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/atrace_tracing_agent.py:148: return []
Are there default ones that we want to include? You can use the one specified in
systrace.

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/chrome_startup_tracing_agent.py:100: #
bin/adb_profile_chrome_startup.
I noticed that adb_profile_chrome_startup actually supports chrome and atrace.
Can you move the ChromeStartupTracingAgent related ones here, e.g.,
package_info, cold, url.

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

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/main.py:53: parser.add_option('-b', '--browser',
help='Select among installed browsers. '
This is only related to chrome.

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:88: help='chose tracing target (android or
linux)')
Can you put the above 3 options into their original place? So it will not show
up as a change.

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:165: options.ftrace_categories = categories
if options.target is neither android nor linux, raise runtime error with a
message.

https://codereview.chromium.org/2276263003/diff/70001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/tracing_agent/atrace_tracing_agent.py
(right):

https://codereview.chromium.org/2276263003/diff/70001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/tracing_agent/atrace_tracing_agent.py:50:
self._config = AtraceOpts(str(self._device),
Are you intend to use AtraceConfig instead of AtraceOpts here?

[Chris Craik, 2016-08-29 19:10:13]

LGTM, but please fix Zhen's remaining comments before submitting.

[washingtonp, 2016-08-29 20:00:41]

ptal

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
File systrace/profile_chrome/atrace_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/atrace_tracing_agent.py:148: return []
On 2016/08/29 16:30:20, Zhen Wang wrote:
> Are there default ones that we want to include? You can use the one specified
in
> systrace.

Done.

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
File systrace/profile_chrome/chrome_startup_tracing_agent.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/chrome_startup_tracing_agent.py:100: #
bin/adb_profile_chrome_startup.
On 2016/08/29 16:30:20, Zhen Wang wrote:
> I noticed that adb_profile_chrome_startup actually supports chrome and atrace.
> Can you move the ChromeStartupTracingAgent related ones here, e.g.,
> package_info, cold, url.

Done.

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

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/main.py:53: parser.add_option('-b', '--browser',
help='Select among installed browsers. '
On 2016/08/29 16:30:20, Zhen Wang wrote:
> This is only related to chrome.

options.browser is only used to generate package_info, which
is used both for the Chrome and DDMS agents. In general, in these
cases where there is one option that is relevant to only 2 or 3
agents but not all agents, would you like to create a separate
OptionGroup for only those two agents? I did this atrace+ftrace
in systrace, but I did this because 1) atrace and ftrace are similar
and debatable mutually exclusive and 2) atrace and ftrace have a
lot of common options. I am not sure that it would be as clean
to do this for Chrome+DDMS.

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:88: help='chose tracing target (android or
linux)')
On 2016/08/29 16:30:20, Zhen Wang wrote:
> Can you put the above 3 options into their original place? So it will not show
> up as a change.

Done.

https://codereview.chromium.org/2276263003/diff/70001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:165: options.ftrace_categories = categories
On 2016/08/29 16:30:20, Zhen Wang wrote:
> if options.target is neither android nor linux, raise runtime error with a
> message.

Done.

https://codereview.chromium.org/2276263003/diff/70001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/tracing_agent/atrace_tracing_agent.py
(right):

https://codereview.chromium.org/2276263003/diff/70001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/tracing_agent/atrace_tracing_agent.py:50:
self._config = AtraceOpts(str(self._device),
On 2016/08/29 16:30:20, Zhen Wang wrote:
> Are you intend to use AtraceConfig instead of AtraceOpts here?

Done.

[Zhen Wang, 2016-08-29 21:00:26]

lgtm with nits

Please wait for Sami to take a look, too.

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

https://codereview.chromium.org/2276263003/diff/70001/systrace/profile_chrome...
systrace/profile_chrome/main.py:53: parser.add_option('-b', '--browser',
help='Select among installed browsers. '
On 2016/08/29 20:00:41, washingtonp wrote:
> On 2016/08/29 16:30:20, Zhen Wang wrote:
> > This is only related to chrome.
> 
> options.browser is only used to generate package_info, which
> is used both for the Chrome and DDMS agents. In general, in these
> cases where there is one option that is relevant to only 2 or 3
> agents but not all agents, would you like to create a separate
> OptionGroup for only those two agents? I did this atrace+ftrace
> in systrace, but I did this because 1) atrace and ftrace are similar
> and debatable mutually exclusive and 2) atrace and ftrace have a
> lot of common options. I am not sure that it would be as clean
> to do this for Chrome+DDMS.

I see. Let's keep it here for now.

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

https://codereview.chromium.org/2276263003/diff/90001/systrace/bin/adb_profil...
systrace/bin/adb_profile_chrome_startup:37:
parser.add_option_group(chrome_startup_tracing_agent.add_options(parser))
Move this after all parser.add_option() function calls.

https://codereview.chromium.org/2276263003/diff/90001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/90001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:168: 'platform must be either Linux or
Android.')
For consistency, you mentioned "atrace/ftrace", in which atrace is before
ftrace. So s/Linux or Android/Android or Linux.

[washingtonp, 2016-08-29 21:13:17]

Sami, ptal when you have a chance.

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

https://codereview.chromium.org/2276263003/diff/90001/systrace/bin/adb_profil...
systrace/bin/adb_profile_chrome_startup:37:
parser.add_option_group(chrome_startup_tracing_agent.add_options(parser))
On 2016/08/29 21:00:26, Zhen Wang wrote:
> Move this after all parser.add_option() function calls.

Done.

https://codereview.chromium.org/2276263003/diff/90001/systrace/systrace/run_s...
File systrace/systrace/run_systrace.py (right):

https://codereview.chromium.org/2276263003/diff/90001/systrace/systrace/run_s...
systrace/systrace/run_systrace.py:168: 'platform must be either Linux or
Android.')
On 2016/08/29 21:00:26, Zhen Wang wrote:
> For consistency, you mentioned "atrace/ftrace", in which atrace is before
> ftrace. So s/Linux or Android/Android or Linux.

Done.

[Sami, 2016-08-30 18:24:49]

lgtm.

[washingtonp, 2016-08-30 20:41:19]

The CQ bit was checked by washingtonp@google.com

[washingtonp, 2016-08-30 20:41:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ccraik@google.com,
zhenw@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2276263003/#ps130001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-30 20:41: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-08-30 20:53:09]

Description was changed from

==========
Pass in custom options to Systrace agents

This CL refactors the Systace agent API to take in a custom config
object tailored to the object. In doing so, this CL:

- Updates Systrace's StartAgentTracing() to take in custom options
- Updates profile_chrome's StartAgentTracing() as well
- Updates the Telemetry call that calls Systrace
- Updates the unit tests accordingly

Note that profile_chrome still needs to be followed to follow Systrace's
controller API. This will be in a separate follow-up CL.

Because so much code is moved between files, this CL looks
artificially large.

BUG=catapult:#2690
==========

to

==========
Pass in custom options to Systrace agents

This CL refactors the Systace agent API to take in a custom config
object tailored to the object. In doing so, this CL:

- Updates Systrace's StartAgentTracing() to take in custom options
- Updates profile_chrome's StartAgentTracing() as well
- Updates the Telemetry call that calls Systrace
- Updates the unit tests accordingly

Note that profile_chrome still needs to be followed to follow Systrace's
controller API. This will be in a separate follow-up CL.

Because so much code is moved between files, this CL looks
artificially large.

BUG=catapult:#2690

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

[commit-bot: I haz the power, 2016-08-30 20:53:10]

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