Reland Replace ValueSetOutputFormatter with HistogramSetJsonOutputFor…

…matter.

Currently, run_benchmark --output=valueset dumps TBMv2 Histograms to JSON, but
it does not convert TBMv1 Values to Histograms, nor does it read existing
results, nor does it support uploading. Also the name is wrong.

This CL renames ValueSetOutputFormatter to HistogramSetJsonOutputFormatter, and
re-conceptualizes it as a subclass of Html2OutputFormatter in order to support
converting TBMv1 Values to Histograms and uploading.
The new HistogramSetJsonOutputFormatter now also reads existing results.

This JSON format will be documented in https://codereview.chromium.org/2563433002

Reverted by https://codereview.chromium.org/2556213002/

BUG=catapult:#3068

Review-Url: https://codereview.chromium.org/2556223002

telemetry/telemetry/internal/results/histogram_set_json_output_formatter.py

@@ -0,0 +1,28 @@
+# 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 json
+import logging
+
+from telemetry.internal.results import output_formatter
+
+
+class HistogramSetJsonOutputFormatter(output_formatter.OutputFormatter):
+  def __init__(self, output_stream, metadata, reset_results):
+    super(HistogramSetJsonOutputFormatter, self).__init__(output_stream)
+    self._metadata = metadata
+    self._reset_results = reset_results
+
+  def Format(self, page_test_results):
+    histograms = page_test_results.AsHistogramDicts(self._metadata)
+    self._output_stream.seek(0)
+    if not self._reset_results:
+      existing = self._output_stream.read()
+      self._output_stream.seek(0)
+      if existing:
+        try:
+          histograms += json.loads(existing)
+        except ValueError:
+          logging.warn('Found existing histograms json but failed to parse it.')
+    json.dump(histograms, self._output_stream)

telemetry/telemetry/internal/results/html2_output_formatter.py

@@ -3,68 +3,38 @@
 # found in the LICENSE file.
 
 import datetime
-import json
 import logging
 import os
-import tempfile
 
-from py_utils import cloud_storage
+from py_utils import cloud_storage  # pylint: disable=import-error
 
-from telemetry.internal.results import chart_json_output_formatter
 from telemetry.internal.results import output_formatter
 
 from tracing import results_renderer
-from tracing.value import convert_chart_json
 
 
 class Html2OutputFormatter(output_formatter.OutputFormatter):
-  def __init__(self, output_stream, metadata, reset_results, upload_results,
+  def __init__(self, output_stream, metadata, reset_results,
                upload_bucket=None):
     super(Html2OutputFormatter, self).__init__(output_stream)
     self._metadata = metadata
-    self._upload_results = upload_results
-    self._upload_bucket = upload_bucket
     self._reset_results = reset_results
-
-  def ConvertChartJson_(self, page_test_results):
-    chart_json = chart_json_output_formatter.ResultsAsChartDict(
-        self._metadata, page_test_results.all_page_specific_values,
-        page_test_results.all_summary_values)
-    info = page_test_results.iteration_info
-    chart_json['label'] = info.label
-    chart_json['benchmarkStartMs'] = info.benchmark_start_ms
-
-    file_descriptor, chart_json_path = tempfile.mkstemp()
-    os.close(file_descriptor)
-    json.dump(chart_json, file(chart_json_path, 'w'))
-
-    vinn_result = convert_chart_json.ConvertChartJson(chart_json_path)
-
-    os.remove(chart_json_path)
-
-    if vinn_result.returncode != 0:
-      logging.error('Error converting chart json to Histograms:\n' +
-          vinn_result.stdout)
-      return []
-    return json.loads(vinn_result.stdout)
+    self._upload_bucket = upload_bucket
 
   def Format(self, page_test_results):
-    histograms = page_test_results.value_set
-    if not histograms:
-      histograms = self.ConvertChartJson_(page_test_results)
-
+    histograms = page_test_results.AsHistogramDicts(self._metadata)
     results_renderer.RenderHTMLView(histograms,
         self._output_stream, self._reset_results)
-    file_path = os.path.abspath(self._output_stream.name)
-    if self._upload_results and self._upload_bucket:
-      remote_path = ('html-results/results-%s' %
-                     datetime.datetime.now().strftime('%Y-%m-%d_%H-%M-%S'))
+
+    if self._upload_bucket:
+      file_name = 'html-results/results-' + datetime.datetime.now().strftime(
+          '%Y-%m-%d_%H-%M-%S')
       try:
-        cloud_storage.Insert(self._upload_bucket, remote_path, file_path)
+        cloud_storage.Insert(self._upload_bucket, file_name,
+                             os.path.abspath(self._output_stream.name))
         print 'View online at',
         print 'http://storage.googleapis.com/{bucket}/{path}'.format(
-            bucket=self._upload_bucket, path=remote_path)
+            bucket=self._upload_bucket, path=file_name)
       except cloud_storage.PermissionError as e:
-        logging.error('Cannot upload profiling files to cloud storage due ' +
-                      'to permission error: ' + e.message)
-    print 'View result at file://' + file_path
+        logging.error('Cannot upload profiling files to cloud storage due to '
+                      ' permission error: %s' % e.message)

telemetry/telemetry/internal/results/html_output_formatter.py

@@ -39,20 +39,19 @@ def _ShortDatetimeInEs5CompatibleFormat(dt):
 
 # TODO(eakuefner): rewrite template to use Telemetry JSON directly
 class HtmlOutputFormatter(output_formatter.OutputFormatter):
-  def __init__(self, output_stream, metadata, reset_results, upload_results,
-      browser_type, results_label=None, upload_bucket=None):
+  def __init__(self, output_stream, metadata, reset_results, browser_type,
+          results_label=None, upload_bucket=None):
     super(HtmlOutputFormatter, self).__init__(output_stream)
     self._metadata = metadata
     self._reset_results = reset_results
-    self._upload_results = upload_results
-    self._upload_bucket = upload_bucket
     self._build_time = self._GetBuildTime()
     self._combined_results = []
     if results_label:
       self._results_label = results_label
     else:
       self._results_label = '%s (%s)' % (
           metadata.name, _ShortDatetimeInEs5CompatibleFormat(self._build_time))
+    self._upload_bucket = upload_bucket
     self._result = {
         'buildTime': _DatetimeInEs5CompatibleFormat(self._build_time),
         'label': self._results_label,
@@ -160,7 +159,7 @@ def Format(self, page_test_results):
     if page_test_results.value_set:
       html2_formatter = html2_output_formatter.Html2OutputFormatter(
           self._output_stream, self._metadata, self._reset_results,
-          self._upload_results, self._upload_bucket)
+          self._upload_bucket)
       html2_formatter.Format(page_test_results)
       return
 
@@ -182,18 +181,15 @@ def Format(self, page_test_results):
     html = html.replace('%plugins%', self._GetPlugins())
     self._SaveResults(html)
 
-    if self._upload_results and self._upload_bucket:
-      file_path = os.path.abspath(self._output_stream.name)
-      file_name = 'html-results/results-%s' % datetime.datetime.now().strftime(
+    if self._upload_bucket:
+      file_name = 'html-results/results-' + datetime.datetime.now().strftime(
           '%Y-%m-%d_%H-%M-%S')
       try:
-        cloud_storage.Insert(self._upload_bucket, file_name, file_path)
+        cloud_storage.Insert(self._upload_bucket, file_name,
+                             os.path.abspath(self._output_stream.name))
         print 'View online at',
         print 'http://storage.googleapis.com/{bucket}/{path}'.format(
             bucket=self._upload_bucket, path=file_name)
       except cloud_storage.PermissionError as e:
         logging.error('Cannot upload profiling files to cloud storage due to '
                       ' permission error: %s' % e.message)
-    print
-    print 'View result at file://%s' % os.path.abspath(
-        self._output_stream.name)

telemetry/telemetry/internal/results/html_output_formatter_unittest.py

@@ -67,7 +67,7 @@ def test_basic_summary(self):
     results.DidRunPage(test_story_set.stories[1])
 
     formatter = DeterministicHtmlOutputFormatter(
-        output_file, FakeMetadataForTest(), False, False, 'browser_type')
+        output_file, FakeMetadataForTest(), False, 'browser_type')
     formatter.Format(results)
     expected = {
       "platform": "browser_type",
@@ -121,7 +121,7 @@ def test_basic_summary(self):
     results.DidRunPage(test_story_set.stories[1])
 
     formatter = DeterministicHtmlOutputFormatter(
-        output_file, FakeMetadataForTest(), False, False, 'browser_type')
+        output_file, FakeMetadataForTest(), False, 'browser_type')
     formatter.Format(results)
     expected = [
       {
@@ -213,7 +213,7 @@ def test_basic_summary(self):
     results.DidRunPage(test_story_set.stories[1])
 
     formatter = DeterministicHtmlOutputFormatter(
-       output_file, FakeMetadataForTest(), True, False, 'browser_type')
+       output_file, FakeMetadataForTest(), True, 'browser_type')
     formatter.Format(results)
     expected = [{
       "platform": "browser_type",

telemetry/telemetry/internal/results/output_formatter.py

@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import os
+
 from telemetry.value import summary as summary_module
 
 class OutputFormatter(object):
@@ -33,6 +35,9 @@ def Format(self, page_test_results):
     """
     raise NotImplementedError()
 
+  def PrintViewResults(self):
+    print 'View result at file://' + os.path.abspath(self.output_stream.name)
+
   def FormatDisabled(self):
     """Formats disabled results into the output stream.
 

telemetry/telemetry/internal/results/page_test_results.py

@@ -5,21 +5,26 @@
 import collections
 import copy
 import datetime
+import json
 import logging
+import os
 import random
 import sys
+import tempfile
 import traceback
 
 from py_utils import cloud_storage  # pylint: disable=import-error
 
+from telemetry import value as value_module
+from telemetry.internal.results import chart_json_output_formatter
 from telemetry.internal.results import json_output_formatter
 from telemetry.internal.results import progress_reporter as reporter_module
 from telemetry.internal.results import story_run
-from telemetry import value as value_module
 from telemetry.value import failure
 from telemetry.value import skip
 from telemetry.value import trace
 
+from tracing.value import convert_chart_json
 
 class IterationInfo(object):
   def __init__(self):
@@ -168,6 +173,30 @@ def iteration_info(self):
   def value_set(self):
     return self._value_set
 
+  def AsHistogramDicts(self, benchmark_metadata):
+    if self.value_set:
+      return self.value_set
+    chart_json = chart_json_output_formatter.ResultsAsChartDict(
+        benchmark_metadata, self.all_page_specific_values,
+        self.all_summary_values)
+    info = self.iteration_info
+    chart_json['label'] = info.label
+    chart_json['benchmarkStartMs'] = info.benchmark_start_ms
+
+    file_descriptor, chart_json_path = tempfile.mkstemp()
+    os.close(file_descriptor)
+    json.dump(chart_json, file(chart_json_path, 'w'))
+
+    vinn_result = convert_chart_json.ConvertChartJson(chart_json_path)
+
+    os.remove(chart_json_path)
+
+    if vinn_result.returncode != 0:
+      logging.error('Error converting chart json to Histograms:\n' +
+          vinn_result.stdout)
+      return []
+    return json.loads(vinn_result.stdout)
+
   def __copy__(self):
     cls = self.__class__
     result = cls.__new__(cls)
@@ -339,6 +368,7 @@ def PrintSummary(self):
         self._SerializeTracesToDirPath(self._output_dir)
       for output_formatter in self._output_formatters:
         output_formatter.Format(self)
+        output_formatter.PrintViewResults()
     else:
       for output_formatter in self._output_formatters:
         output_formatter.FormatDisabled()

telemetry/telemetry/internal/results/results_options.py

@@ -14,16 +14,16 @@
 from telemetry.internal.results import chart_json_output_formatter
 from telemetry.internal.results import csv_pivot_table_output_formatter
 from telemetry.internal.results import gtest_progress_reporter
+from telemetry.internal.results import histogram_set_json_output_formatter
 from telemetry.internal.results import html2_output_formatter
 from telemetry.internal.results import json_output_formatter
 from telemetry.internal.results import page_test_results
 from telemetry.internal.results import progress_reporter
-from telemetry.internal.results import valueset_output_formatter
 
 # Allowed output formats. The default is the first item in the list.
 
 _OUTPUT_FORMAT_CHOICES = ('html', 'gtest', 'json', 'chartjson',
-    'csv-pivot-table', 'valueset', 'none')
+    'csv-pivot-table', 'histograms', 'none')
 
 
 # Filenames to use for given output formats.
@@ -32,7 +32,7 @@
     'json': 'results.json',
     'chartjson': 'results-chart.json',
     'csv-pivot-table': 'results-pivot-table.csv',
-    'valueset': 'results-valueset.json'
+    'histograms': 'histograms.json',
 }
 
 
@@ -140,18 +140,18 @@ def CreateResults(benchmark_metadata, options,
     elif output_format == 'html':
       output_formatters.append(html2_output_formatter.Html2OutputFormatter(
           output_stream, benchmark_metadata, options.reset_results,
-          options.upload_results, upload_bucket=upload_bucket))
+          upload_bucket))
     elif output_format == 'json':
       output_formatters.append(json_output_formatter.JsonOutputFormatter(
           output_stream, benchmark_metadata))
     elif output_format == 'chartjson':
       output_formatters.append(
           chart_json_output_formatter.ChartJsonOutputFormatter(
               output_stream, benchmark_metadata))
-    elif output_format == 'valueset':
+    elif output_format == 'histograms':
       output_formatters.append(
-          valueset_output_formatter.ValueSetOutputFormatter(
-              output_stream))
+          histogram_set_json_output_formatter.HistogramSetJsonOutputFormatter(
+              output_stream, benchmark_metadata, options.reset_results))
     else:
       # Should never be reached. The parser enforces the choices.
       raise Exception('Invalid --output-format "%s". Valid choices are: %s'