sandwich: Make metrics extraction more customizable.

sandwich: Make metrics extraction more customizable.

BUG=582080
Committed: https://crrev.com/928ffadce5e22bdddeb231d357468b32dd92936b
Cr-Commit-Position: refs/heads/master@{#397134}

[pasko, 2016-05-31 13:00:33]

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

[pasko, 2016-05-31 13:00:34]

General idea looks good, thank you.

A few minor comments about sandwich_runner.py.

Ping me when the remaining changes are ready.

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:255: def
IterRepeatedRuns(runner_output_dir):
naming:

this is a generator, so let's call it: GenerateRepeatedRuns(...)

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
Let's add "Not ordered."

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
s/repeat_dir/repeat_directory/

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:272: assert repeated_run_count > 0,
('Looks like \'{}\' was not a sandwich runner '
nit: I prefer error messages to mention the important bits of the state at the
end.

Like:
Error: not a sandwich runner output directory: {}

[gabadie, 2016-05-31 14:32:25]

Description was changed from

==========
DRAFT: sandwich: Make metrics extraction more customizable.

BUG=582080
==========

to

==========
sandwich: Make metrics extraction more customizable.

BUG=582080
==========

[gabadie, 2016-05-31 14:33:02]

Thanks Egor. Uploaded a new revision. PTAL.

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:255: def
IterRepeatedRuns(runner_output_dir):
On 2016/05/31 13:00:33, pasko wrote:
> naming:
> 
> this is a generator, so let's call it: GenerateRepeatedRuns(...)

WalkRepeatedRuns as agreed offline.

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
On 2016/05/31 13:00:33, pasko wrote:
> Let's add "Not ordered."

Done.

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
On 2016/05/31 13:00:33, pasko wrote:
> s/repeat_dir/repeat_directory/

Line are not very wide and git -C tools/android/loading grep "dir\W" | wc -l ->
236

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:272: assert repeated_run_count > 0,
('Looks like \'{}\' was not a sandwich runner '
On 2016/05/31 13:00:33, pasko wrote:
> nit: I prefer error messages to mention the important bits of the state at the
> end.
> 
> Like:
> Error: not a sandwich runner output directory: {}

Done.

[pasko, 2016-05-31 16:00:01]

looking good, a few relatively minor comments

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
On 2016/05/31 14:33:01, gabadie wrote:
> On 2016/05/31 13:00:33, pasko wrote:
> > s/repeat_dir/repeat_directory/
> 
> Line are not very wide and git -C tools/android/loading grep "dir\W" | wc -l
->
> 236

LOL, this was about the comment that fits into the line. Not a problem for me in
either case, just adding more human language to this comment would make it a
tiny bit more like a comment.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_swr.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
fits into one line. But if you want to separate it intentionally, it is OK as
well

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
I think we are not interested in publishing this value outside this file and
unittests => _SWR_COLUMN_NAMES

'SWR' because then it is clearer what the addition is about

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:135: logging.info('processing trace
\'%s\'' % trace_path)
pylint wants it to be a comma, and quotes are not necessary, I'd write it as:

logging.info('processing trace: %s', trace_path)

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:146: run_metrics_list.sort(key=lambda e:
e['repeat_id'])
is sorting important in the CSV?

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:15: from loading_trace import
LoadingTrace
more consistent: 'import loading_trace'.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:250: # TODO PERF
IMPROVMENT(gabadie): Can load trace only once an use it for
do you want to make this improvement in a followup change or in this one?

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:250: # TODO PERF
IMPROVMENT(gabadie): Can load trace only once an use it for
The format of TODO is somewhat strict to allow some grepping, so we'd be better
with:
# TODO(gabadie): Performance improvement: load each trace only once and use it
for validation and extraction of metrics later.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:260: logging.info('processing
trace \'%s\'' % trace_path)
logging.info('processing trace: %s', trace_path)

[gabadie, 2016-06-01 12:04:34]

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2009883002/diff/1/tools/android/loading/sandw...
tools/android/loading/sandwich_runner.py:256: """Yields repeat_id, repeat_dir.
On 2016/05/31 16:00:00, pasko wrote:
> On 2016/05/31 14:33:01, gabadie wrote:
> > On 2016/05/31 13:00:33, pasko wrote:
> > > s/repeat_dir/repeat_directory/
> > 
> > Line are not very wide and git -C tools/android/loading grep "dir\W" | wc -l
> ->
> > 236
> 
> LOL, this was about the comment that fits into the line. Not a problem for me
in
> either case, just adding more human language to this comment would make it a
> tiny bit more like a comment.

Oh my bad, I have not seen that this comment was actually on the doc string.
Done.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_swr.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
On 2016/05/31 16:00:01, pasko wrote:
> fits into one line. But if you want to separate it intentionally, it is OK as
> well

Done.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
On 2016/05/31 16:00:00, pasko wrote:
> I think we are not interested in publishing this value outside this file and
> unittests => _SWR_COLUMN_NAMES
> 
> 'SWR' because then it is clearer what the addition is about

Understood. But it is already not publishable since it is in the scope of the
method.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:135: logging.info('processing trace
\'%s\'' % trace_path)
On 2016/05/31 16:00:01, pasko wrote:
> pylint wants it to be a comma, and quotes are not necessary, I'd write it as:
> 
> logging.info('processing trace: %s', trace_path)

Done.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:146: run_metrics_list.sort(key=lambda e:
e['repeat_id'])
On 2016/05/31 16:00:01, pasko wrote:
> is sorting important in the CSV?

Yes to not depend on the underlying file system file ordering and have the order
at which the benchmark are actually run.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_task_builder.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:15: from loading_trace import
LoadingTrace
On 2016/05/31 16:00:01, pasko wrote:
> more consistent: 'import loading_trace'.

Done.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:250: # TODO PERF
IMPROVMENT(gabadie): Can load trace only once an use it for
On 2016/05/31 16:00:01, pasko wrote:
> do you want to make this improvement in a followup change or in this one?

Separate.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:250: # TODO PERF
IMPROVMENT(gabadie): Can load trace only once an use it for
On 2016/05/31 16:00:01, pasko wrote:
> The format of TODO is somewhat strict to allow some grepping, so we'd be
better
> with:
> # TODO(gabadie): Performance improvement: load each trace only once and use it
> for validation and extraction of metrics later.

Done.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_task_builder.py:260: logging.info('processing
trace \'%s\'' % trace_path)
On 2016/05/31 16:00:01, pasko wrote:
> logging.info('processing trace: %s', trace_path)

Done.

[pasko, 2016-06-01 13:09:22]

lgtm, thank you Guillaume

apologies for slow response, it was actually quick-n-easy to finalize the review

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_swr.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
On 2016/06/01 12:04:34, gabadie wrote:
> On 2016/05/31 16:00:00, pasko wrote:
> > I think we are not interested in publishing this value outside this file and
> > unittests => _SWR_COLUMN_NAMES
> > 
> > 'SWR' because then it is clearer what the addition is about
> 
> Understood. But it is already not publishable since it is in the scope of the
> method.

Ah, my bad, I thought it is a class constant. Our style guide suggests naming
local constants the same way as local variables. That's not my choice, but for
consistency we'd need to agree with it.

So, this would be: 'swr_column_names' then

https://codereview.chromium.org/2009883002/diff/40001/tools/android/loading/s...
File tools/android/loading/sandwich_runner.py (right):

https://codereview.chromium.org/2009883002/diff/40001/tools/android/loading/s...
tools/android/loading/sandwich_runner.py:260: """Yields unordered (repeat id,
path of the repeat directory).
nice, thank you

[gabadie, 2016-06-01 13:32:16]

Thanks Egor! Landing.

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
File tools/android/loading/sandwich_swr.py (right):

https://codereview.chromium.org/2009883002/diff/20001/tools/android/loading/s...
tools/android/loading/sandwich_swr.py:104: ADDITIONAL_COLUMN_NAMES = [
On 2016/06/01 13:09:22, pasko wrote:
> On 2016/06/01 12:04:34, gabadie wrote:
> > On 2016/05/31 16:00:00, pasko wrote:
> > > I think we are not interested in publishing this value outside this file
and
> > > unittests => _SWR_COLUMN_NAMES
> > > 
> > > 'SWR' because then it is clearer what the addition is about
> > 
> > Understood. But it is already not publishable since it is in the scope of
the
> > method.
> 
> Ah, my bad, I thought it is a class constant. Our style guide suggests naming
> local constants the same way as local variables. That's not my choice, but for
> consistency we'd need to agree with it.
> 
> So, this would be: 'swr_column_names' then

I would still prefer additional_column_names because we are already in the swr
module, and swr_column_names suggests that it is all the column's names. Done.

[gabadie, 2016-06-01 13:32:25]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-06-01 13:32:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2009883002/#ps60001
(title: "s/ADDITIONAL_COLUMN_NAMES/additional_column_names/g")

[commit-bot: I haz the power, 2016-06-01 13:32:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009883002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009883002/60001

[commit-bot: I haz the power, 2016-06-01 14:12:10]

Description was changed from

==========
sandwich: Make metrics extraction more customizable.

BUG=582080
==========

to

==========
sandwich: Make metrics extraction more customizable.

BUG=582080
==========

[commit-bot: I haz the power, 2016-06-01 14:12:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-01 14:14:48]

Description was changed from

==========
sandwich: Make metrics extraction more customizable.

BUG=582080
==========

to

==========
sandwich: Make metrics extraction more customizable.

BUG=582080

Committed: https://crrev.com/928ffadce5e22bdddeb231d357468b32dd92936b
Cr-Commit-Position: refs/heads/master@{#397134}
==========

[commit-bot: I haz the power, 2016-06-01 14:14:49]

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