Add Chart JSON processing to Telemetry.

Add Chart JSON processing to Telemetry.

This patch implements Chart JSON for Telemetry, which represents a JSON format specifically for conveying data to Buildbot to be sent to the perf dashboard in charted/summarized form. 
Consult go/telemetry-json for further specs on this format.
This is an 0.1 since the overall metadata and description portions of the format are still in flux.

BUG=399970
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291185

[nduca, 2014-08-12 02:18:08]

some thoughts. lookin good

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:1: # Copyright 2014 The Chromium
Authors. All rights reserved.
this should be in values

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
why pass in a full results object? you just need page_specific_values and the
summary values as lists? That simplifies unit tests. Always use the most narrow
set of data you can --- that simplifies testing.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
AsChartDict?

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
Please create an extensive docstring explaining what this does, what it
produces. The short form docs we've had in design doc should be good.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:11: summary =
summary_module.Summary(page_test_results.all_page_specific_values,
someone needs to update this to handle failures so that 1 page that produces 1
failure and 3 successes summarizes right

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:12: had_failures)
unit tests for this file shoudl include pages that had failures

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:19: 'format_version': '0.1',
what happens when you bump json format version?

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:64: # If any pages failed, we want to
invalidate the overall results
are you sure this is the right behavior now? That was the case with failure
values, but should it be the case now?

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:71:
value.GetBuildbotMeasurementAndTraceNameForComputedSummaryResult(
Please generalize. Its no longer buildbot only. So we should clarify the purpose
of this function.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:74: trace_name = 'overall'
im wondering about this still. 'summary'?

which reminds me, do we have a unit test that ensures complete failure if you
have a value and  a summary value with the same name?

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:85: def _ValueAsDictForChartJson(value):
not needed?

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:90: def
ResultsAsChartJson(page_test_results, metadata):
dont think we really need this. the caller can do that.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/chart_json_unittest.py (right):

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/chart_json_unittest.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
this should be next to the chart_json.py

[tonyg, 2014-08-12 02:23:23]

I'm happy once Nat is.

[eakuefner, 2014-08-12 21:18:41]

On the overall note of handling and unittesting with failures: it's not clear to
me what the appropriate summarization behavior wrt to failures and skips should
be. I think it's probably a good idea to have a conversation about this and then
change summary to reason about failures appropriately in a separate CL and then
update this one once we've fixed that. 

summary.py still looks at page_test_results.failures where it should really be
reasoning about FailureValue and SkipValue in some more detailed way on its own.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:1: # Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/08/12 02:18:08, nduca wrote:
> this should be in values

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
On 2014/08/12 02:18:08, nduca wrote:
> why pass in a full results object? you just need page_specific_values and the
> summary values as lists? That simplifies unit tests. Always use the most
narrow
> set of data you can --- that simplifies testing.

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
On 2014/08/12 02:18:08, nduca wrote:
> AsChartDict?

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:9: def ResultsAsDict(page_test_results,
metadata):
On 2014/08/12 02:18:08, nduca wrote:
> Please create an extensive docstring explaining what this does, what it
> produces. The short form docs we've had in design doc should be good.

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:19: 'format_version': '0.1',
On 2014/08/12 02:18:08, nduca wrote:
> what happens when you bump json format version?

This is something I haven't quite worked out yet -- I think it will be enough to
check the version number from the dashboard handler that I'm going to be writing
and handle the JSON differently based on the version number. We could also make
it so that the handler will always expect the latest version and just use the
number as a sanity check, which I think is better from a standpoint of not
encouraging backwards compatibility cruft.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:71:
value.GetBuildbotMeasurementAndTraceNameForComputedSummaryResult(
On 2014/08/12 02:18:08, nduca wrote:
> Please generalize. Its no longer buildbot only. So we should clarify the
purpose
> of this function.

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:74: trace_name = 'overall'
On 2014/08/12 02:18:07, nduca wrote:
> im wondering about this still. 'summary'?
Done.
> 
> which reminds me, do we have a unit test that ensures complete failure if you
> have a value and  a summary value with the same name?
See overall comment about failure behavior.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:85: def _ValueAsDictForChartJson(value):
On 2014/08/12 02:18:07, nduca wrote:
> not needed?

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/chart_json.py:90: def
ResultsAsChartJson(page_test_results, metadata):
On 2014/08/12 02:18:08, nduca wrote:
> dont think we really need this. the caller can do that.

Done.

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/chart_json_unittest.py (right):

https://codereview.chromium.org/439613003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/chart_json_unittest.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
On 2014/08/12 02:18:08, nduca wrote:
> this should be next to the chart_json.py

Done.

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

lgtm but please make it do what you think is right wrt failure.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:20: _OUTPUT_FORMAT_CHOICES
= ('html', 'buildbot', 'block', 'csv', 'gtest', 'json',
this change isn't needed?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:21: #  had_failures =
len(page_test_results.failures) > 0
funky?

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

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:21: #  had_failures =
len(page_test_results.failures) > 0
On 2014/08/13 00:55:39, nduca wrote:
> funky?

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

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

lgtm

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:20: """
Assert summary_values contains "all values without an associated page"?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json_unittest.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json_unittest.py:58: v0 =
scalar.ScalarValue(results.current_page, 'foo', 'seconds', 3)
Please don't use the results.current_page pattern but use self._page_set[0]
instead. We want to remove this in the future.

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

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/json_output_formatter.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/json_output_formatter.py:9: def
ResultsAsDict(page_test_results, metadata):
metadata is very confusing. Can you rename this and document (on JsonOF
constructor).

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/json_output_formatter.py:12: 'test_name':
metadata.name,
metadata is always a BenchmarkMetadata anyway, so benchmark_name is probably
fine. test_name implies something about the PageTest or a unit test's test name.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:134: chart, _ =
_ConvertValueNameToChartAndTraceName(self.name)
chart_name for consistency

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
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.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
Why does this take page_specific_values and summary_values instead of
PageTestResults?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
s/metadata/benchmark_metadata/ (metadata is too generic)

Also, let's use that as first parameter.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:10: Chart JSON is a transformation
of the basic Telemetry JSON format that
This should doc the schema as well or point to documentation of the v0.1 schema.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:37: def
_HandleComputedValues(summary, charts):
Best if we can have this yield chart_name, chart_content and have the caller add
it to charts dict.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:51: None))
nit: can fit in 80-col?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:58:
charts[chart_name]['traces'][trace_name] = value
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()?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:71: page_test_results: a
PageTestResults object containing overall values
Outdated doc.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:89: charts[chart_name][trace_name]
= value
value.AsDict()?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json_unittest.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json_unittest.py:128:
self.assertTrue('overall' in d['charts']['foo']['traces'])
I don't see any tests that test the actual values of
d.charts.foo.traces.trace_name = ???

[eakuefner, 2014-08-15 22:47:33]

Chris: PTAL; I've addressed your comments with discussion on some of them below.

Others: I think the confusion re: failure handling has been sorted out; I
believe the source of the confusion to have been a comment that described what
is going on inaccurately.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/json_output_formatter.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/json_output_formatter.py:9: def
ResultsAsDict(page_test_results, metadata):
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> metadata is very confusing. Can you rename this and document (on JsonOF
> constructor).

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/json_output_formatter.py:12: 'test_name':
metadata.name,
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> metadata is always a BenchmarkMetadata anyway, so benchmark_name is probably
> fine. test_name implies something about the PageTest or a unit test's test
name.

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/results_options.py:20: _OUTPUT_FORMAT_CHOICES
= ('html', 'buildbot', 'block', 'csv', 'gtest', 'json',
On 2014/08/13 00:55:39, nduca wrote:
> this change isn't needed?

Ack. Fills the column better though.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:134: chart, _ =
_ConvertValueNameToChartAndTraceName(self.name)
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> chart_name for consistency

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> s/metadata/benchmark_metadata/ (metadata is too generic)
Done.
> 
> Also, let's use that as first parameter.
Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> Why does this take page_specific_values and summary_values instead of
> PageTestResults?

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.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> 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.

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.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:10: Chart JSON is a transformation
of the basic Telemetry JSON format that
On 2014/08/13 05:12:02, chrishenry - DO NOT USE wrote:
> This should doc the schema as well or point to documentation of the v0.1
schema.

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:20: """
On 2014/08/13 02:17:03, nednguyen wrote:
> Assert summary_values contains "all values without an associated page"?

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.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:37: def
_HandleComputedValues(summary, charts):
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> Best if we can have this yield chart_name, chart_content and have the caller
add
> it to charts dict.

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?

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:51: None))
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> nit: can fit in 80-col?

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:58:
charts[chart_name]['traces'][trace_name] = value
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> 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)?
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.
> 
> Also, you're assigning this to value and not value.AsDict()?

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:71: page_test_results: a
PageTestResults object containing overall values
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> Outdated doc.

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:89: charts[chart_name][trace_name]
= value
On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> value.AsDict()?

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json_unittest.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json_unittest.py:58: v0 =
scalar.ScalarValue(results.current_page, 'foo', 'seconds', 3)
On 2014/08/13 02:17:03, nednguyen wrote:
> Please don't use the results.current_page pattern but use self._page_set[0]
> instead. We want to remove this in the future. 

Done.

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json_unittest.py:128:
self.assertTrue('overall' in d['charts']['foo']['traces'])
On 2014/08/13 05:12:02, chrishenry - DO NOT USE wrote:
> I don't see any tests that test the actual values of
> d.charts.foo.traces.trace_name = ???

Added a smoke test to deal with this.

[chrishenry, 2014-08-19 17:36:22]

lgtm

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
On 2014/08/15 22:47:32, eakuefner wrote:
> On 2014/08/13 05:12:01, chrishenry - DO NOT USE wrote:
> > Why does this take page_specific_values and summary_values instead of
> > PageTestResults?
> 
> 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.

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).

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:21: page_specific_values: list
of page-specific values
Follow style guide at go/pyguide. This should be
Args:
  page_specific_values: ...
  summary_values: ...
  ...

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:61:
charts[chart_name]['traces'][trace_name] = value.AsDict()
Let's use defaultdict if possible.

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:61:
charts[chart_name]['traces'][trace_name] = value.AsDict()
Should this also be asserting that trace_name doesn't exist?

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:90: assert trace_name not in
charts[chart_name]
Who set trace_name in charts[chart_name]? I only see it being set in
charts[chart_name]['traces']

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

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/chart_json.py:7: def
ResultsAsChartDict(page_specific_values, summary_values, metadata):
On 2014/08/19 17:36:21, chrishenry wrote:
> 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).

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.

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/results/chart_json.py (right):

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:21: page_specific_values: list
of page-specific values
On 2014/08/19 17:36:22, chrishenry wrote:
> Follow style guide at go/pyguide. This should be
> Args:
>   page_specific_values: ...
>   summary_values: ...
>   ...

Done.

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:61:
charts[chart_name]['traces'][trace_name] = value.AsDict()
On 2014/08/19 17:36:22, chrishenry wrote:
> Should this also be asserting that trace_name doesn't exist?

Done.

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:61:
charts[chart_name]['traces'][trace_name] = value.AsDict()
On 2014/08/19 17:36:22, chrishenry wrote:
> Let's use defaultdict if possible.

Done. Thanks for pointing this out -- again this was a case where this Python
hint led to me being able to eliminate most of this spaghetti for a much more
concise solution.

https://codereview.chromium.org/439613003/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/chart_json.py:90: assert trace_name not in
charts[chart_name]
On 2014/08/19 17:36:22, chrishenry wrote:
> Who set trace_name in charts[chart_name]? I only see it being set in
> charts[chart_name]['traces']

Done.

[eakuefner, 2014-08-21 01:11:23]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-08-21 01:12:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/439613003/120001

[commit-bot: I haz the power, 2014-08-21 02:13:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 02:15:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 02:15:55]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[eakuefner, 2014-08-21 17:09:17]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:10:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/439613003/120001

[commit-bot: I haz the power, 2014-08-21 21:11:53]

Committed patchset #7 (120001) as 291185