Add CSV export, suitable for use with "pivot tables" in spreadsheets.

Add CSV export, suitable for use with "pivot tables" in spreadsheets.

This writes CSV, with each line containing one value + metadata.
Essentially, this is a de-normalized output of the test results which
makes it easier to use for some kinds of analysis, and particularly
with "pivot tables" in common spreadsheet programs.

Use with: --output-format=csv-pivot-table

BUG=

Committed: https://crrev.com/fff37414227c1cedcc64d31299c170eea485bbcb
Cr-Commit-Position: refs/heads/master@{#300083}

[vogelheim, 2014-10-07 16:04:03]

vogelheim@chromium.org changed reviewers:
+ ernstm@chromium.org, tonyg@chromium.org

[vogelheim, 2014-10-07 16:04:04]

I found this to be quite useful when analysing telemetry results. You guys
please decide whether it warrants inclusion as a new output format.

[tonyg, 2014-10-09 01:22:10]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org, nednguyen@google.com
- tonyg@chromium.org

[tonyg, 2014-10-09 01:22:45]

tonyg@chromium.org changed reviewers:
+ tonyg@chromium.org

[tonyg, 2014-10-09 01:22:45]

Adding this sgtm. Dave or Ned would be better reviewers though.

[nednguyen, 2014-10-09 17:05:14]

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
File tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py
(right):

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py:54:
value_dict = value.AsDict()
I don't like the usage of value dict and this dynamic lookup since it creates
dependency between your header field names and the value dict. 

Please use direct fields: 
page_set --> page_set.Name()
page --> page.display_name
name --> value.name
units --> value.units

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py:58: 
Nits: extra blank line here and in .._unittest file

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/results_options.py:32: 'Can be %s.' % ',
'.join(_OUTPUT_FORMAT_CHOICES))
How would telemetry users know that --output-trace-tag can be used with
--cvs-pivot-table as described in csv_pivot_table_output_formatter.py? 

Can you improve the help string here?

[vogelheim, 2014-10-10 11:27:43]

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
File tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py
(right):

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py:54:
value_dict = value.AsDict()
On 2014/10/09 17:05:14, nednguyen wrote:
> I don't like the usage of value dict and this dynamic lookup since it creates
> dependency between your header field names and the value dict. 
> 
> Please use direct fields: 
> page_set --> page_set.Name()
> page --> page.display_name
> name --> value.name
> units --> value.units

Done.

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter.py:58: 
On 2014/10/09 17:05:14, nednguyen wrote:
> Nits: extra blank line here and in .._unittest file

Done.

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
File tools/telemetry/telemetry/results/results_options.py (right):

https://codereview.chromium.org/631213003/diff/1/tools/telemetry/telemetry/re...
tools/telemetry/telemetry/results/results_options.py:32: 'Can be %s.' % ',
'.join(_OUTPUT_FORMAT_CHOICES))
On 2014/10/09 17:05:14, nednguyen wrote:
> How would telemetry users know that --output-trace-tag can be used with
> --cvs-pivot-table as described in csv_pivot_table_output_formatter.py? 
> 
> Can you improve the help string here?

Done. (I think...)

I've added the description to the --output-trace-tag help string, not to this
one. I only mentioned the formats the trace tag can be used with but not the
specifics of csv-pivot-table, since 1, I guess the help string should be
reasonably brief, and 2, it's really a way of saying a value with an embedded
comma works fine.

I thought the code to accomplish in the csv_pivot_table_output_formatter looks a
bit non-obvious, which is why I put in an explanation there. For a user I'd
think this was a fairly natural way of handling this.

[nednguyen, 2014-10-10 14:21:36]

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py
(right):

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:29:
def make_formatter(self, trace_tag=''):
Telemetry's function naming convention is MakeFormatter().

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:34:
def simulate_benchmark_run(self, list_of_list_of_values=None):
The API of this function make it hard to see which page get which values. How
about:
def SimulateBenchmarkRunForPage(self, page, page_values):

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:44:
def format(self):
Format(self)

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:48:
def test_simple(self):
testSimple()

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:57:
self.assertEqual(expected, self.format())
Can you add other test cases:
1) Case results contain runs for page 'foo.com'  & page 'bar.com'. At least one
of the pages has more than 1 run.
2) Some cases that make trace_tag values to appear on more than 1 row.

[vogelheim, 2014-10-13 12:18:48]

Patchset #3 (id:130001) has been deleted

[vogelheim, 2014-10-13 12:22:53]

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py
(right):

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:29:
def make_formatter(self, trace_tag=''):
On 2014/10/10 14:21:36, nednguyen wrote:
> Telemetry's function naming convention is MakeFormatter(). 

Done.

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:34:
def simulate_benchmark_run(self, list_of_list_of_values=None):
On 2014/10/10 14:21:35, nednguyen wrote:
> The API of this function make it hard to see which page get which values. How
> about:
> def SimulateBenchmarkRunForPage(self, page, page_values):

Done.

That is: I now made it a dictionary of Page -> [Value], which should make it
rather clear which values go to which page objects.

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:44:
def format(self):
On 2014/10/10 14:21:35, nednguyen wrote:
> Format(self)

Done.

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:48:
def test_simple(self):
On 2014/10/10 14:21:36, nednguyen wrote:
> testSimple() 

Done.

https://codereview.chromium.org/631213003/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:57:
self.assertEqual(expected, self.format())
On 2014/10/10 14:21:36, nednguyen wrote:
> Can you add other test cases:
> 1) Case results contain runs for page 'foo.com'  & page 'bar.com'. At least
one
> of the pages has more than 1 run.
> 2) Some cases that make trace_tag values to appear on more than 1 row.

Done.

1) in separate test case.
2) added to testTraceTag.

[vogelheim, 2014-10-13 12:25:04]

Patchset #3 (id:150001) has been deleted

[nednguyen, 2014-10-13 15:47:46]

lgtm

https://codereview.chromium.org/631213003/diff/210001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py
(right):

https://codereview.chromium.org/631213003/diff/210001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:38:
def SimulateBenchmarkRun(self, dict_of_values=None):
nits: since the implementation doesn't support case dict_of_values=None, you can
remove the default value for dict_of_values.

[vogelheim, 2014-10-13 16:53:20]

https://codereview.chromium.org/631213003/diff/210001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py
(right):

https://codereview.chromium.org/631213003/diff/210001/tools/telemetry/telemet...
tools/telemetry/telemetry/results/csv_pivot_table_output_formatter_unittest.py:38:
def SimulateBenchmarkRun(self, dict_of_values=None):
On 2014/10/13 15:47:46, nednguyen wrote:
> nits: since the implementation doesn't support case dict_of_values=None, you
can
> remove the default value for dict_of_values.

Done.

[dtu, 2014-10-14 00:15:30]

Would anyone object if we just named it "csv," replacing the current csv option?
I don't think it has any users.

[vogelheim, 2014-10-14 09:52:55]

I'd be happy to do that (in a separate CL, I guess?)

Code Search finds one use: [cs]
<https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...>
I
wouldn't know whether that is replaceable or whether there are additional
users; I'd need to rely on your judgement.

Generally, my thoughts:
- Both are useful primarily as input to other programs. Changing from one
format to another might be burdensome.
- "csv" aggregates (averages) the data. That makes the output much more
readable; but makes it much worse if you intend to do actual analysis with
the data. So IMHO use depends on what you want to do with the output.
- "csv-pivot-table" can generate rather large outputs, since the "pivot
table" writes one row per value. Essentially, the pivot table input is a
fully de-normalized rendering of the result set. That might be inconvenient
for some uses.

Please let me know what you guys think.

On Tue, Oct 14, 2014 at 2:15 AM, <dtu@chromium.org> wrote:

> Would anyone object if we just named it "csv," replacing the current csv
> option?
> I don't think it has any users.
>
> https://codereview.chromium.org/631213003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dtu, 2014-10-14 17:43:41]

On 2014/10/14 09:52:55, vogelheim wrote:
> I'd be happy to do that (in a separate CL, I guess?)
> 
> Code Search finds one use: [cs]
>
<https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...>
> I
> wouldn't know whether that is replaceable or whether there are additional
> users; I'd need to rely on your judgement.
> 
> Generally, my thoughts:
> - Both are useful primarily as input to other programs. Changing from one
> format to another might be burdensome.
> - "csv" aggregates (averages) the data. That makes the output much more
> readable; but makes it much worse if you intend to do actual analysis with
> the data. So IMHO use depends on what you want to do with the output.
> - "csv-pivot-table" can generate rather large outputs, since the "pivot
> table" writes one row per value. Essentially, the pivot table input is a
> fully de-normalized rendering of the result set. That might be inconvenient
> for some uses.
> 
> Please let me know what you guys think.
> 
> On Tue, Oct 14, 2014 at 2:15 AM, <mailto:dtu@chromium.org> wrote:
> 
> > Would anyone object if we just named it "csv," replacing the current csv
> > option?
> > I don't think it has any users.
> >
> > https://codereview.chromium.org/631213003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.


I guess it's not that important. Seems easier to punt on that.

lgtm

[nednguyen, 2014-10-15 23:09:02]

On 2014/10/14 17:43:41, dtu wrote:
> On 2014/10/14 09:52:55, vogelheim wrote:
> > I'd be happy to do that (in a separate CL, I guess?)
> > 
> > Code Search finds one use: [cs]
> >
>
<https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...>
> > I
> > wouldn't know whether that is replaceable or whether there are additional
> > users; I'd need to rely on your judgement.
> > 
> > Generally, my thoughts:
> > - Both are useful primarily as input to other programs. Changing from one
> > format to another might be burdensome.
> > - "csv" aggregates (averages) the data. That makes the output much more
> > readable; but makes it much worse if you intend to do actual analysis with
> > the data. So IMHO use depends on what you want to do with the output.
> > - "csv-pivot-table" can generate rather large outputs, since the "pivot
> > table" writes one row per value. Essentially, the pivot table input is a
> > fully de-normalized rendering of the result set. That might be inconvenient
> > for some uses.
> > 
> > Please let me know what you guys think.
> > 
> > On Tue, Oct 14, 2014 at 2:15 AM, <mailto:dtu@chromium.org> wrote:
> > 
> > > Would anyone object if we just named it "csv," replacing the current csv
> > > option?
> > > I don't think it has any users.
> > >
> > > https://codereview.chromium.org/631213003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> 
> I guess it's not that important. Seems easier to punt on that.
> 
> lgtm

Actually, can you make sure that you coordinate this effort with Ari to always
output csv-pivot-table results as file?

[nednguyen, 2014-10-15 23:09:14]

nednguyen@google.com changed reviewers:
+ ariblue@google.com

[vogelheim, 2014-10-17 09:41:49]

The CQ bit was checked by vogelheim@chromium.org

[commit-bot: I haz the power, 2014-10-17 09:42:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631213003/260001

[commit-bot: I haz the power, 2014-10-17 09:54:55]

Committed patchset #4 (id:260001)

[commit-bot: I haz the power, 2014-10-17 09:58:27]

Patchset 4 (id:??) landed as
https://crrev.com/fff37414227c1cedcc64d31299c170eea485bbcb
Cr-Commit-Position: refs/heads/master@{#300083}