Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
531973002:
[Telemetry] Add option to output Chart JSON summary. (Closed)
Description[Telemetry] Add option to output Chart JSON summary.
This CL adds an option --chartjson to the test runner to automatically perform
summarization to the specialized Chart JSON format that the perf dashboard has
been modified to accept. Passing --chartjson is mutually exclusive with the --output-formatter
option.
Committed: https://crrev.com/a3d2110e14741bdfa16b076a026b319c3e5e0359
Cr-Commit-Position: refs/heads/master@{#293089}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Ned's comments #
Total comments: 2
Patch Set 3 : Address Ned's comments #
Messages
Total messages: 14 (3 generated)
eakuefner@chromium.org changed reviewers: + chrishenry@google.com, nednguyen@google.com
https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... tools/telemetry/telemetry/benchmark.py:29: class BenchmarkMetadata(object): I thought we remove this and make name just a field of benchmark? https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... tools/telemetry/telemetry/benchmark.py:102: if finder_options.chartjson: This is the wrong place to print the chart_json format. https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... File tools/telemetry/telemetry/results/results_options.py (right): https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... tools/telemetry/telemetry/results/results_options.py:26: help='Output Chart JSON. Ignores --output-format.') Why does enabling this ignores --output-format? Can we make it an output format choice instead? https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... tools/telemetry/telemetry/results/results_options.py:119: progress_reporter=reporter) Is the formatting change necessary?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... tools/telemetry/telemetry/benchmark.py:29: class BenchmarkMetadata(object): On 2014/09/02 22:41:50, nednguyen wrote: > I thought we remove this and make name just a field of benchmark? We may eventually want the output formatters to be aware of more facts about benchmarks, which we would add as fields to this class. https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... tools/telemetry/telemetry/benchmark.py:102: if finder_options.chartjson: On 2014/09/02 22:41:51, nednguyen wrote: > This is the wrong place to print the chart_json format. If we don't make it an output formatter (see comment on the other file), where would be a better place for it to go? PageTestResults doesn't know about the options and it seems like this is the earliest place outside of the results object where we can be sure that the tests have already been run. https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... File tools/telemetry/telemetry/results/results_options.py (right): https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... tools/telemetry/telemetry/results/results_options.py:26: help='Output Chart JSON. Ignores --output-format.') On 2014/09/02 22:41:51, nednguyen wrote: > Why does enabling this ignores --output-format? Can we make it an output format > choice instead? Per nduca's comments on the CL where this functionality was introduced we decided not to make it an output formatter because it represents a one-off "distillation" of the raw JSON format and an antipattern that we want to eventually eliminate (ideally, we would have the dashboard able to accept the raw JSON format eventually and perform summarization itself). Nat can weigh in for more of the rationale on this. https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... tools/telemetry/telemetry/results/results_options.py:119: progress_reporter=reporter) On 2014/09/02 22:41:51, nednguyen wrote: > Is the formatting change necessary? No, reverted. I double checked the style guide and how it was before is fine.
On 2014/09/02 23:24:48, eakuefner wrote: > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > File tools/telemetry/telemetry/benchmark.py (right): > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > tools/telemetry/telemetry/benchmark.py:29: class BenchmarkMetadata(object): > On 2014/09/02 22:41:50, nednguyen wrote: > > I thought we remove this and make name just a field of benchmark? > > We may eventually want the output formatters to be aware of more facts about > benchmarks, which we would add as fields to this class. > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > tools/telemetry/telemetry/benchmark.py:102: if finder_options.chartjson: > On 2014/09/02 22:41:51, nednguyen wrote: > > This is the wrong place to print the chart_json format. > > If we don't make it an output formatter (see comment on the other file), where > would be a better place for it to go? PageTestResults doesn't know about the > options and it seems like this is the earliest place outside of the results > object where we can be sure that the tests have already been run. > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > File tools/telemetry/telemetry/results/results_options.py (right): > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > tools/telemetry/telemetry/results/results_options.py:26: help='Output Chart > JSON. Ignores --output-format.') > On 2014/09/02 22:41:51, nednguyen wrote: > > Why does enabling this ignores --output-format? Can we make it an output > format > > choice instead? > > Per nduca's comments on the CL where this functionality was introduced we > decided not to make it an output formatter because it represents a one-off > "distillation" of the raw JSON format and an antipattern that we want to > eventually eliminate (ideally, we would have the dashboard able to accept the > raw JSON format eventually and perform summarization itself). Nat can weigh in > for more of the rationale on this. I see. Don't you have a results deserializer? What about the approach that dashboard use your deserializer and apply the chart_json on results itself? > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > tools/telemetry/telemetry/results/results_options.py:119: > progress_reporter=reporter) > On 2014/09/02 22:41:51, nednguyen wrote: > > Is the formatting change necessary? > > No, reverted. I double checked the style guide and how it was before is fine.
https://codereview.chromium.org/531973002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/531973002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:106: results.all_summary_values)) Maybe results.PrintSummary in the else block of this.
On 2014/09/02 23:44:57, nednguyen wrote: > On 2014/09/02 23:24:48, eakuefner wrote: > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > > File tools/telemetry/telemetry/benchmark.py (right): > > > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > > tools/telemetry/telemetry/benchmark.py:29: class BenchmarkMetadata(object): > > On 2014/09/02 22:41:50, nednguyen wrote: > > > I thought we remove this and make name just a field of benchmark? > > > > We may eventually want the output formatters to be aware of more facts about > > benchmarks, which we would add as fields to this class. > > > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/be... > > tools/telemetry/telemetry/benchmark.py:102: if finder_options.chartjson: > > On 2014/09/02 22:41:51, nednguyen wrote: > > > This is the wrong place to print the chart_json format. > > > > If we don't make it an output formatter (see comment on the other file), where > > would be a better place for it to go? PageTestResults doesn't know about the > > options and it seems like this is the earliest place outside of the results > > object where we can be sure that the tests have already been run. > > > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > > File tools/telemetry/telemetry/results/results_options.py (right): > > > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > > tools/telemetry/telemetry/results/results_options.py:26: help='Output Chart > > JSON. Ignores --output-format.') > > On 2014/09/02 22:41:51, nednguyen wrote: > > > Why does enabling this ignores --output-format? Can we make it an output > > format > > > choice instead? > > > > Per nduca's comments on the CL where this functionality was introduced we > > decided not to make it an output formatter because it represents a one-off > > "distillation" of the raw JSON format and an antipattern that we want to > > eventually eliminate (ideally, we would have the dashboard able to accept the > > raw JSON format eventually and perform summarization itself). Nat can weigh in > > for more of the rationale on this. > > I see. Don't you have a results deserializer? What about the approach that > dashboard use your deserializer and apply the chart_json on results itself? I think that's eventually going to be a good idea -- for the time being we haven't figured out quite how to make the Dashboard code depend on Telemetry, but that's exactly what I want to have happen eventually. > > > > > > https://codereview.chromium.org/531973002/diff/1/tools/telemetry/telemetry/re... > > tools/telemetry/telemetry/results/results_options.py:119: > > progress_reporter=reporter) > > On 2014/09/02 22:41:51, nednguyen wrote: > > > Is the formatting change necessary? > > > > No, reverted. I double checked the style guide and how it was before is fine.
https://codereview.chromium.org/531973002/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/benchmark.py (right): https://codereview.chromium.org/531973002/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/benchmark.py:106: results.all_summary_values)) On 2014/09/02 23:45:05, nednguyen wrote: > Maybe results.PrintSummary in the else block of this. Done.
lgtm
The CQ bit was checked by eakuefner@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eakuefner@chromium.org/531973002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as 4caff4517b8de9bee7f8228b55012931985a47b9
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a3d2110e14741bdfa16b076a026b319c3e5e0359 Cr-Commit-Position: refs/heads/master@{#293089} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
