sandwich: Make metrics extraction more customizable.

tools/android/loading/sandwich_swr.py

Index: tools/android/loading/sandwich_swr.py
diff --git a/tools/android/loading/sandwich_swr.py b/tools/android/loading/sandwich_swr.py
index ee4831a08192d9583e3e572e22934511e08c8fcf..e1579bd16bc4b7e0df507ac896c7327dcb343939 100644
--- a/tools/android/loading/sandwich_swr.py
+++ b/tools/android/loading/sandwich_swr.py
@@ -101,6 +101,10 @@ class StaleWhileRevalidateBenchmarkBuilder(task_manager.Builder):
       depends on: <transformer_list_name>/{swr,worstcase}-run/
         depends on: some tasks saved by PopulateCommonPipelines()
     """
+    ADDITIONAL_COLUMN_NAMES = [

[pasko, 2016/05/31 16:00:00]

I think we are not interested in publishing this value outside this file and
unittests => _SWR_COLUMN_NAMES

'SWR' because then it is clearer what the addition is about

[pasko, 2016/05/31 16:00:01]

fits into one line. But if you want to separate it intentionally, it is OK as
well

[gabadie, 2016/06/01 12:04:34]

Done.

[gabadie, 2016/06/01 12:04:34]

Understood. But it is already not publishable since it is in the scope of the
method.

[pasko, 2016/06/01 13:09:22]

Ah, my bad, I thought it is a class constant. Our style guide suggests naming
local constants the same way as local variables. That's not my choice, but for
consistency we'd need to agree with it.

So, this would be: 'swr_column_names' then

[gabadie, 2016/06/01 13:32:16]

I would still prefer additional_column_names because we are already in the swr
module, and swr_column_names suggests that it is all the column's names. Done.

+        'url',
+        'repeat_id']
+
     task_prefix = os.path.join(transformer_list_name, '')
     if enable_swr:
       task_prefix += 'swr'
@@ -124,15 +128,27 @@ class StaleWhileRevalidateBenchmarkBuilder(task_manager.Builder):
 
     @self.RegisterTask(task_prefix + '-metrics.csv', [RunBenchmark])
     def ExtractMetrics():
-      trace_metrics_list = \
-          sandwich_metrics.ExtractMetricsFromRunnerOutputDirectory(
-              None, RunBenchmark.path)
-      trace_metrics_list.sort(key=lambda e: e['repeat_id'])
+      run_metrics_list = []
+      for repeat_id, repeat_dir in sandwich_runner.WalkRepeatedRuns(
+          RunBenchmark.path):
+        trace_path = os.path.join(repeat_dir, sandwich_runner.TRACE_FILENAME)
+        logging.info('processing trace \'%s\'' % trace_path)

[pasko, 2016/05/31 16:00:01]

pylint wants it to be a comma, and quotes are not necessary, I'd write it as:

logging.info('processing trace: %s', trace_path)

[gabadie, 2016/06/01 12:04:34]

Done.

+        trace = loading_trace.LoadingTrace.FromJsonFile(trace_path)
+        run_metrics = {
+            'url': trace.url,
+            'repeat_id': repeat_id,
+        }
+        run_metrics.update(
+            sandwich_metrics.ExtractCommonMetricsFromRepeatDirectory(
+                repeat_dir, trace))
+        run_metrics_list.append(run_metrics)
+
+      run_metrics_list.sort(key=lambda e: e['repeat_id'])

[pasko, 2016/05/31 16:00:01]

is sorting important in the CSV?

[gabadie, 2016/06/01 12:04:34]

Yes to not depend on the underlying file system file ordering and have the order
at which the benchmark are actually run.

       with open(ExtractMetrics.path, 'w') as csv_file:
-        writer = csv.DictWriter(csv_file,
-                                fieldnames=sandwich_metrics.CSV_FIELD_NAMES)
+        writer = csv.DictWriter(csv_file, fieldnames=(ADDITIONAL_COLUMN_NAMES +
+                                    sandwich_metrics.COMMON_CSV_COLUMN_NAMES))
         writer.writeheader()
-        for trace_metrics in trace_metrics_list:
+        for trace_metrics in run_metrics_list:
           writer.writerow(trace_metrics)
 
     self._common_builder.default_final_tasks.append(ExtractMetrics)