[Systrace] Fix systrace clock syncing issue with BattOr.

systrace/systrace/output_generator.py

Index: systrace/systrace/output_generator.py
diff --git a/systrace/systrace/output_generator.py b/systrace/systrace/output_generator.py
index f0ff06aa156a8aa8286cb5d560ac0ed10ee0df6c..45b16c246e5cc1cb7086f5e605345452054d8f02 100644
--- a/systrace/systrace/output_generator.py
+++ b/systrace/systrace/output_generator.py
@@ -12,6 +12,7 @@ import StringIO
 
 from systrace import tracing_controller
 from systrace import trace_result
+from tracing.trace_data import trace_data
 
 
 # TODO(alexandermont): Current version of trace viewer does not support
@@ -20,6 +21,21 @@ from systrace import trace_result
 # trace viewer is working again.
 OUTPUT_CONTROLLER_TRACE_ = False
 CONTROLLER_TRACE_DATA_KEY = 'controllerTraceDataKey'
+_SYSTRACE_TO_TRACE_DATA_NAME_MAPPING = {
+    'systemTraceEvents': trace_data.ATRACE_PART,
+    'powerTraceAsString': trace_data.BATTOR_TRACE_PART,
+    'systraceController': trace_data.TELEMETRY_PART,
+    'traceEvents': trace_data.CHROME_TRACE_PART,
+}
+
+
+def NewGenerateHTMLOutput(trace_results, output_file_name):
+  trace_data_builder = trace_data.TraceDataBuilder()
+  for trace in trace_results:
+    trace_data_part = _SYSTRACE_TO_TRACE_DATA_NAME_MAPPING.get(
+        trace.source_name)
+    trace_data_builder.AddTraceFor(trace_data_part, trace.raw_data)
+  trace_data_builder.AsData().Serialize(output_file_name, 'Systrace')

[charliea (OOO until 10-5), 2017/03/21 19:47:23]

'Systrace' should probably be a constant

[rnephew (Reviews Here), 2017/03/23 15:14:15]

Done.

 
 
 def GenerateHTMLOutput(trace_results, output_file_name):
@@ -33,6 +49,11 @@ def GenerateHTMLOutput(trace_results, output_file_name):
   def _ReadAsset(src_dir, filename):
     return open(os.path.join(src_dir, filename)).read()
 
+  # The old method does not support anything except atrace.

[charliea (OOO until 10-5), 2017/03/20 14:37:38]

ccraik@chromium.org, now that the output generator is in tracing/ and not
telemetry/, I'd like to reevaluate moving to a world where we _only_  use the
trace data builder. My rationale is that I've worked in a codebase before where
we had lots of:

if detect_new_way():
  # Do new way...
  return

# Do old way...

and it's a nightmare to keep a mental map of the logical branches in the long
run. This seems like a clear case where we don't need to do that and can just
kill the last 40 LoC in this function because they're equivalent.

What's the benefit of continuing to use the old way of generating traces here?
More specifically, what assurances does it give us about things not breaking
systrace that wouldn't be better given by just adding a tracing unit test to
make sure that the old systrace trace format continues to import successfully?

[Chris Craik, 2017/03/20 17:38:37]

I agree it's gross, but I really don't want to break java systrace capture in
the monitor tool while we still have to support it. I'm reaching out to android
tools team to figure out when in the release process it's reasonable to drop it.

I'm still not clear though on what moving to the benefit of the new format
immediately is. I totally agree that we shouldn't have our own output generator
code in the long term, but I don't understand why time sync wouldn't be working
in the old system.

The problem with testing importing is that it doesn't actually run any of the JS
prefix code, which is the part I worry about breaking. We've had many times in
the past that the prefix has to be updated to point to changed method or class
names for importing the trace change. Continuing to use the old way of
generating traces ensures that if prefix.html is broken, output is broken for
both entry points the same way.

As to unit testing, I don't know of a way of validating that a given generated
html file not only opens, but correctly displays a trace. If there's some way to
do that, I'd be happy to add that, and enable us to switch over entirely.

Only thought I have is that one could get pretty far at checking prefix.html by
parsing/loading it in a js test and exec()-ing it, but that seems pretty gross,
and I don't think we want exec() in our codebase :P

[charliea (OOO until 10-5), 2017/03/21 19:47:23]

Randy, could you add a TODO describing why we have two pathways when one seems
like it should work?

[charliea (OOO until 10-5), 2017/03/21 19:47:23]

Decision based on VC: we'll maintain the dual pathways for the short amount of
time that we expect Java systrace to continue existing, then pare down to just
the Python output formatter.

[rnephew (Reviews Here), 2017/03/23 15:14:15]

Added comment about removing once java no longer uses the old way.

+  if len(trace_results) > 2:

[Chris Craik, 2017/03/21 19:52:23]

The comment above makes it sound like we should be checking for atrace only
here, in addition to length < 2. Should we be, or do we know in advance that
only atrace traces will have single entry here?

[rnephew (Reviews Here), 2017/03/23 15:14:15]

It should work with any single trace. I reworded it to reflect that reality. 

+    NewGenerateHTMLOutput(trace_results, output_file_name)
+    return os.path.abspath(output_file_name)
+
   systrace_dir = os.path.abspath(os.path.dirname(__file__))
 
   try:
@@ -63,9 +84,6 @@ def GenerateHTMLOutput(trace_results, output_file_name):
   # for each tracing agent (including the controller tracing agent).
   html_file.write('<!-- BEGIN TRACE -->\n')
   for result in trace_results:
-    if (result.source_name == tracing_controller.TRACE_DATA_CONTROLLER_NAME and
-        not OUTPUT_CONTROLLER_TRACE_):
-      continue
     html_file.write('  <script class="trace-data" type="application/text">\n')
     html_file.write(_ConvertToHtmlString(result.raw_data))
     html_file.write('  </script>\n')
@@ -102,8 +120,6 @@ def GenerateJSONOutput(trace_results, output_file_name):
   results = _ConvertTraceListToDictionary(trace_results)
   results[CONTROLLER_TRACE_DATA_KEY] = (
       tracing_controller.TRACE_DATA_CONTROLLER_NAME)
-  if not OUTPUT_CONTROLLER_TRACE_:
-    results[tracing_controller.TRACE_DATA_CONTROLLER_NAME] = []
   with open(output_file_name, 'w') as json_file:
     json.dump(results, json_file)
   final_path = os.path.abspath(output_file_name)