sandwich: Make metrics extraction more customizable.

tools/android/loading/sandwich_swr.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.
 
 import csv
 import logging
 import os
 import shutil
 
 import chrome_cache
@@ skipping 83 matching lines @@
           in Task names (prefer names without spaces and special characters).
       transformer_list: An ordered list of function that takes an instance of
           SandwichRunner as parameter, would be applied immediately before
           SandwichRunner.Run() in the given order.
 
     Here is the full dependency of the added tree for the returned task:
     <transformer_list_name>/{swr,worstcase}-metrics.csv
       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'
     else:
       task_prefix += 'worstcase'
 
     @self.RegisterTask(task_prefix + '-run/', [self._patched_cache_task])
     def RunBenchmark():
       runner = self._common_builder.CreateSandwichRunner()
       for transformer in transformer_list:
         transformer(runner)
       runner.wpr_archive_path = self._common_builder.original_wpr_task.path
       runner.wpr_out_log_path = os.path.join(
           RunBenchmark.path, sandwich_runner.WPR_LOG_FILENAME)
       runner.cache_archive_path = self._patched_cache_task.path
       runner.cache_operation = sandwich_runner.CacheOperation.PUSH
       runner.output_dir = RunBenchmark.path
       if enable_swr:
         runner.chrome_args.append('--enable-features=StaleWhileRevalidate2')
       runner.Run()
 
     @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)