sandwich: Make speed-index and memory measurement optional from run-all

tools/android/loading/sandwich_metrics.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Pull a sandwich run's output directory's metrics from traces into a CSV.
 
 python pull_sandwich_metrics.py -h
 """
 
 import collections
@@ skipping 25 matching lines @@
     'total_load',
     'js_onload_event',
     'browser_malloc_avg',
     'browser_malloc_max',
     'speed_index',
     'net_emul.name', # Should be in emulation.NETWORK_CONDITIONS.keys()
     'net_emul.download',
     'net_emul.upload',
     'net_emul.latency']
 
+_UNAVAILABLE_CSV_VALUE = 'unavailable'
+
 _TRACKED_EVENT_NAMES = set(['requestStart', 'loadEventStart', 'loadEventEnd'])
 
 # Points of a completeness record.
 #
 # Members:
 #   |time| is in milliseconds,
 #   |frame_completeness| value representing how complete the frame is at a given
 #     |time|. Caution: this completeness might be negative.
 CompletenessPoint = collections.namedtuple('CompletenessPoint',
     ('time', 'frame_completeness'))
@@ skipping 18 matching lines @@
 
 def _GetBrowserDumpEvents(tracing_track):
   """Get the browser memory dump events from a tracing track.
 
   Args:
     tracing_track: The tracing.TracingTrack.
 
   Returns:
     List of memory dump events.
   """
+  assert sandwich_runner.MEMORY_DUMP_CATEGORY in tracing_track.Categories()
   browser_pid = _GetBrowserPID(tracing_track)
   browser_dumps_events = []
   for event in tracing_track.GetEvents():
     if event.category != 'disabled-by-default-memory-infra':
       continue
     if event.type != 'v' or event.name != 'periodic_interval':
       continue
     # Ignore dump events for processes other than the browser process
     if event.pid != browser_pid:
       continue
@@ skipping 30 matching lines @@
     if 'frame' in event.args and event.args['frame'] != main_frame:
       continue
     if event_name in _TRACKED_EVENT_NAMES and event_name not in tracked_events:
       logging.info('found url\'s event \'%s\'' % event_name)
       tracked_events[event_name] = event
   assert len(tracked_events) == len(_TRACKED_EVENT_NAMES)
   return tracked_events
 
 
 def _ExtractMetricsFromLoadingTrace(loading_trace):
-  """Pulls all the metrics from a given trace.
+  """Extract the metrics from a given trace.

[pasko, 2016/04/28 15:04:59]

nit: I don't see a problem with the previous comment. We can call the functions
"ExtractSomething" and explain what it does more colloquially as "Pulls all of
Something".

Up to you

[gabadie, 2016/04/28 15:56:22]

I see your point, but here there was an inconsistency between the verb of the
doc string and the name of the functions. I have updated the doc string with
what I believe is the most adapted. Done.

 
   Args:
     loading_trace: loading_trace_module.LoadingTrace.
 
   Returns:
     Dictionary with all trace extracted fields set.
   """
-  assert all(
-      cat in loading_trace.tracing_track.Categories()
-      for cat in sandwich_runner.ADDITIONAL_CATEGORIES), (
-          'This trace was not generated with the required set of categories '
-          'to be processed by this script.')
-  browser_dump_events = _GetBrowserDumpEvents(loading_trace.tracing_track)
   web_page_tracked_events = _GetWebPageTrackedEvents(
       loading_trace.tracing_track)
+  return {
+    'total_load': (web_page_tracked_events['loadEventEnd'].start_msec -
+                   web_page_tracked_events['requestStart'].start_msec),
+    'js_onload_event': (web_page_tracked_events['loadEventEnd'].start_msec -
+                        web_page_tracked_events['loadEventStart'].start_msec)
+  }
 
+
+def _ExtractMemoryMetricsFromLoadingTrace(loading_trace):

[pasko, 2016/04/28 15:04:59]

Having two functions with very similar long names makes it harder to read. So I
would suggest:

_ExtractAllMetrics(loading_trace) and _ExtractMemoryMetrics(loading_trace)

Just a friendly optional suggestion. Current naming is not problematic, and I
would not object to it too much.

[gabadie, 2016/04/28 15:56:22]

Used _ExtractDefaultMetrics and _ExtractMemoryMetrics

+  """Extract the memory metrics from a given trace.
+
+  Args:
+    loading_trace: loading_trace_module.LoadingTrace.
+
+  Returns:
+    Dictionary with all trace extracted fields set.
+  """
+  if (sandwich_runner.MEMORY_DUMP_CATEGORY not in
+          loading_trace.tracing_track.Categories()):
+    return {
+      'browser_malloc_avg': _UNAVAILABLE_CSV_VALUE,
+      'browser_malloc_max': _UNAVAILABLE_CSV_VALUE
+    }
+  browser_dump_events = _GetBrowserDumpEvents(loading_trace.tracing_track)
   browser_malloc_sum = 0
   browser_malloc_max = 0
   for dump_event in browser_dump_events:
     attr = dump_event.args['dumps']['allocators']['malloc']['attrs']['size']
     assert attr['units'] == 'bytes'
     size = int(attr['value'], 16)
     browser_malloc_sum += size
     browser_malloc_max = max(browser_malloc_max, size)
-
   return {
-    'total_load': (web_page_tracked_events['loadEventEnd'].start_msec -
-                   web_page_tracked_events['requestStart'].start_msec),
-    'js_onload_event': (web_page_tracked_events['loadEventEnd'].start_msec -
-                        web_page_tracked_events['loadEventStart'].start_msec),
     'browser_malloc_avg': browser_malloc_sum / float(len(browser_dump_events)),
     'browser_malloc_max': browser_malloc_max
   }
 
 
 def _ExtractCompletenessRecordFromVideo(video_path):
   """Extracts the completeness record from a video.
 
   The video must start with a filled rectangle of orange (RGB: 222, 100, 13), to
   give the view-port size/location from where to compute the completeness.
@@ skipping 58 matching lines @@
     run_directory_path: Path of the run directory.
 
   Returns:
     Dictionary of extracted metrics.
   """
   trace_path = os.path.join(run_directory_path, 'trace.json')
   logging.info('processing trace \'%s\'' % trace_path)
   loading_trace = loading_trace_module.LoadingTrace.FromJsonFile(trace_path)
   run_metrics = {'url': loading_trace.url}
   run_metrics.update(_ExtractMetricsFromLoadingTrace(loading_trace))
+  run_metrics.update(_ExtractMemoryMetricsFromLoadingTrace(loading_trace))
   video_path = os.path.join(run_directory_path, 'video.mp4')
   if os.path.isfile(video_path):
     logging.info('processing speed-index video \'%s\'' % video_path)
     completeness_record = _ExtractCompletenessRecordFromVideo(video_path)
     run_metrics['speed_index'] = ComputeSpeedIndex(completeness_record)
   else:
-    run_metrics['speed_index'] = 'disabled'
+    run_metrics['speed_index'] = _UNAVAILABLE_CSV_VALUE
   for key, value in loading_trace.metadata['network_emulation'].iteritems():
     run_metrics['net_emul.' + key] = value
   return run_metrics
 
 
 def ExtractMetricsFromRunnerOutputDirectory(output_directory_path):
   """Extracts all the metrics from all the traces of a sandwich runner output
   directory.
 
   Args:
@@ skipping 13 matching lines @@
     except ValueError:
       continue
     run_directory_path = os.path.join(output_directory_path, node_name)
     run_metrics = _ExtractMetricsFromRunDirectory(run_directory_path)
     run_metrics['repeat_id'] = repeat_id
     assert set(run_metrics.keys()) == set(CSV_FIELD_NAMES)
     metrics.append(run_metrics)
   assert len(metrics) > 0, ('Looks like \'{}\' was not a sandwich runner ' +
                             'output directory.').format(output_directory_path)
   return metrics