Add Chart JSON processing to Telemetry.

tools/telemetry/telemetry/value/chart_json.py

+# Copyright 2014 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.
+
+from telemetry.value import summary as summary_module
+
+def ResultsAsChartDict(page_specific_values, summary_values, metadata):

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

s/metadata/benchmark_metadata/ (metadata is too generic)

Also, let's use that as first parameter.

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

This isn't really a "value" but more a summary of results. So maybe put this
under telemetry.results? I'd be fine with introducing a new package too (such as
telemetry.output) if you feel appropriate. Feel free to suggest one.

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

Why does this take page_specific_values and summary_values instead of
PageTestResults?

[eakuefner, 2014/08/15 22:47:31]

You're right that there isn't a really clear conceptual place for this. I think
results is the right place for it based on the fact that it's a results-oriented
concept, even though it isn't itself an OutputFormatter.

[eakuefner, 2014/08/15 22:47:32]

We leave that up to the caller; this makes testing simpler and ensures that we
don't overtest -- I've refactored the tests in the latest patchset to take
advantage of this fact and they're now much shorter and don't rely on
functionality of PageTestResults to be correct.

[eakuefner, 2014/08/15 22:47:32]

Done.
Done.

[chrishenry, 2014/08/19 17:36:21]

If you need a unit test equivalent, please create a private API that the public
API will call. Unless you feel that there is reasons why you'd want to call this
without passing PageTestResults. I prefer the API to accept PageTestResults
since it frees us to rewrite the implementation details of chart_json (e.g., to
do summarization based on PageRun).

[eakuefner, 2014/08/21 01:11:17]

Eventually we want to make Chart JSON actually a transformation of Telemetry
JSON, so in this case I think the workflow would be to make sure that page runs
are accurately represented in Telemetry JSON and then go from there. For that
reason I'm going to leave it this way for now.

+  """Produces a dict for serialization to Chart JSON format from raw values.
+
+  Chart JSON is a transformation of the basic Telemetry JSON format that

[chrishenry - DO NOT USE, 2014/08/13 05:12:02]

This should doc the schema as well or point to documentation of the v0.1 schema.

[eakuefner, 2014/08/15 22:47:32]

Done.

+  removes the page map, summarizes the raw values, and organizes the results
+  by chart and trace name. This function takes the key pieces of data needed to
+  perform this transformation (namely, lists of values and a benchmark metadata
+  object) and processes them into a dict which can be serialized using the json
+  module.
+
+  page_specific_values: list of all values with an associated page
+  summary_values: list of all values without an associated page
+  metadata: a benchmark.BenchmarkMetadata object
+  """

[nednguyen, 2014/08/13 02:17:03]

Assert summary_values contains "all values without an associated page"?

[eakuefner, 2014/08/15 22:47:31]

We want this invariant to be enforced higher up the chain, so I've changed the
description to simply call such values "summary values" instead of making an
informal assertion about their insides.

+#  had_failures = len(page_test_results.failures) > 0

[nduca, 2014/08/13 00:55:39]

funky?

[eakuefner, 2014/08/13 01:03:43]

Will decide on failure handling best effort & fix this before commit.

+  had_failures = False
+  summary = summary_module.Summary(page_specific_values, had_failures)
+  charts = {}
+
+  _HandleComputedValues(summary, charts)
+  _HandleSummaryValues(summary_values, charts)
+
+  result_dict = {
+    'format_version': '0.1',
+    'test_name': metadata.name,
+    'charts': charts
+  }
+
+  return result_dict
+
+def _HandleComputedValues(summary, charts):

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

Best if we can have this yield chart_name, chart_content and have the caller add
it to charts dict.

[eakuefner, 2014/08/15 22:47:32]

This style matches the way we are handling results in page_runner etc. by
threading a results object through the stack, which is why I did it this way.
There is also corner case that this style accounts for, that I've now added a
unittest for: Suppose a page specific value having a simple (non-compound) name
is added, but a summary value is added with the same name. Then the summary of
the page specific value will be that chart's "summary" key, and the non-computed
summary will conflict. This is bad benchmark authorship, and the asserts in both
of these functions are designed to catch this. Does this sound reasonable?

+  """Organizes computed per-page and summary values into a charts dict.
+
+  summary: summary.Summary object containing computed values
+  charts: dict into which chartified values will be inserted
+  """
+
+  for value in summary.interleaved_computed_per_page_values_and_summaries:
+    if value.page:
+      chart_name, trace_name = (
+          value.GetChartAndTraceNameForPerPageResult())
+    else:
+      chart_name, trace_name = (
+          value.GetChartAndTraceNameForComputedSummaryResult(
+              None))

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

nit: can fit in 80-col?

[eakuefner, 2014/08/15 22:47:31]

Done.

+      if chart_name == trace_name:
+        if chart_name in charts:
+          assert 'summary' not in charts[chart_name]
+        trace_name = 'summary'
+
+    if chart_name in charts:
+      charts[chart_name]['traces'][trace_name] = value

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

A little odd, can you document this briefly? Do all values with the same
chart_name guaranteed to have different trace_name (I'm guessing it does after
summarizing, but worth doc'ing briefly I think)?

Also, you're assigning this to value and not value.AsDict()?

[eakuefner, 2014/08/15 22:47:31]

Added a couple comments to attempt to explain -- the principle is that if you
have a value named "x" instead of "x.y" it represents both a chart and a trace,
and in this case since we're representing the data hierarchically we want to
store the value with a key called "summary" to disambiguate.
Done.

+    else:
+      charts[chart_name] = {
+        'traces': {trace_name: value.AsDict()}
+      }
+
+def _HandleSummaryValues(summary_values, charts):
+  """Organizes summary values into a charts dict.
+
+  While summary values can be created by a Summary object, they can also be
+  added directly during the course of a page run. Those values are not subject
+  to summarization and thus we handle them directly in this routine.
+
+  page_test_results: a PageTestResults object containing overall values

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

Outdated doc.

[eakuefner, 2014/08/15 22:47:32]

Done.

+  charts: dict into which chartified values will be inserted
+  """
+
+  # If any pages failed, we want to invalidate the overall results
+  # TODO(eakuefner): figure out how to handle this correctly
+  had_failures = False
+  if had_failures:
+    return
+
+  for value in summary_values:
+    chart_name, trace_name = (
+        value.GetChartAndTraceNameForComputedSummaryResult(
+            None))
+    if chart_name == trace_name:
+      trace_name = 'summary'
+    if chart_name in charts:
+      assert trace_name not in charts[chart_name]
+      charts[chart_name][trace_name] = value

[chrishenry - DO NOT USE, 2014/08/13 05:12:01]

value.AsDict()?

[eakuefner, 2014/08/15 22:47:32]

Done.

+    else:
+      charts[chart_name] = {
+        'units': value.units,
+        'traces': {trace_name: value.AsDict()}
+      }