Pass in custom options to Systrace agents

systrace/systrace/systrace_runner.py

Index: systrace/systrace/systrace_runner.py
diff --git a/systrace/systrace/systrace_runner.py b/systrace/systrace/systrace_runner.py
index a7ad9fa2b796b05d5c805ae4f9c4b3852bf2e41f..f41ac31c5bc949c26373b9cfcaed9b40eecc79c4 100644
--- a/systrace/systrace/systrace_runner.py
+++ b/systrace/systrace/systrace_runner.py
@@ -9,6 +9,7 @@ necessary tracing agents for systrace, runs them, and outputs the results
 as an HTML or JSON file.'''
 
 from systrace import output_generator
+from systrace import tracing_agents
 from systrace import tracing_controller
 from systrace.tracing_agents import atrace_agent
 from systrace.tracing_agents import atrace_from_file_agent
@@ -21,24 +22,23 @@ AGENT_MODULES_ = [atrace_agent, atrace_from_file_agent,
 
 
 class SystraceRunner(object):
-  def __init__(self, script_dir, options, categories):
+  def __init__(self, script_dir, options):
     """Constructor.
 
     Args:
         script_dir: Directory containing the trace viewer script
                     (systrace_trace_viewer.html)
         options: List of command line options.

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

options is not a list.

[washingtonp, 2016/08/27 02:22:43]

Done.

-        categories: List of trace categories to capture.
     """
     # Parse command line arguments and create agents.
     self._script_dir = script_dir
     self._out_filename = options.output_file
-    agents = CreateAgents(options)
+    agents_with_config = _CreateAgentsWithConfig(options)
+    controller_config = _GetControllerConfig(options)
 
     # Set up tracing controller.
-    self._tracing_controller = tracing_controller.TracingController(options,
-                                                                    categories,
-                                                                    agents)
+    self._tracing_controller = tracing_controller.TracingController(
+        agents_with_config, controller_config)
 
   def StartTracing(self):
     self._tracing_controller.StartTracing()
@@ -68,15 +68,51 @@ class SystraceRunner(object):
     print '\nWrote trace %s file: file://%s\n' % (('JSON' if write_json
                                                    else 'HTML'), result)
 
-def CreateAgents(options):
+
+class AgentWithConfig(object):

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

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.

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

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.

+  def __init__(self, agent, config):
+    self.agent = agent
+    self.config = config
+
+
+def _CreateAgentsWithConfig(options):
   """Create tracing agents.
 
   This function will determine which tracing agents are valid given the
-  options and create those agents.
+  options and create those agents along with their corresponding configuration
+  object.
   Args:
     options: The command-line options.
   Returns:
-    The list of systrace agents.
+    A list of AgentWithConfig options containing agents and their corresponding
+    configuration object.
   """
-  result = [module.try_create_agent(options) for module in AGENT_MODULES_]
-  return [x for x in result if x]
+  result = [AgentWithConfig(module.try_create_agent(options),

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

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.

[washingtonp, 2016/08/27 02:22:43]

Done.

+            module.get_config(options))
+            for module in AGENT_MODULES_]
+  return [x for x in result if x and x.agent]
+
+
+class TracingControllerConfig(tracing_agents.TracingConfig):
+  def __init__(self, output_file, trace_time, list_categories, write_json,
+               link_assets, asset_dir, timeout, collection_timeout,
+               device_serial_number, target):
+    tracing_agents.TracingConfig.__init__(self)
+    self.output_file = output_file
+    self.trace_time = trace_time
+    self.list_categories = list_categories
+    self.write_json = write_json
+    self.link_assets = link_assets
+    self.asset_dir = asset_dir
+    self.timeout = timeout
+    self.collection_timeout = collection_timeout
+    self.device_serial_number = device_serial_number
+    self.target = target
+
+
+def _GetControllerConfig(options):
+  return TracingControllerConfig(options.output_file, options.trace_time,
+                                 options.list_categories, options.write_json,
+                                 options.link_assets, options.asset_dir,
+                                 options.timeout, options.collection_timeout,
+                                 options.device_serial_number, options.target)