[tools/perf] Only send _avg metrics for memory infra metrics

[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
Committed: https://crrev.com/373e9b71d8cc649898fc3f4cbf726d95ca61f580
Cr-Commit-Position: refs/heads/master@{#395603}

[perezju, 2016-05-24 09:55:48]

Description was changed from

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
==========

to

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
==========

[perezju, 2016-05-24 10:07:07]

perezju@chromium.org changed reviewers:
+ nednguyen@google.com, petrcermak@chromium.org

[petrcermak, 2016-05-24 10:23:52]

https://codereview.chromium.org/2005233002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/2005233002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:96: return value.name.endswith('_avg')
Ned: Will this method also be applied to things like num_failed? If so, then we
should do the following:

  return 'memory:' not in value.name or value.name.endswith('_avg')

(I don't think we can use value.name.startswith('memory:') because the tir_label
might have already been added to the front of the value name).

Furthermore, this code will remove all dump counts (because they're simple
scalars), so I think that we should do the following instead:

  _IGNORED_STATS_RE = re.compile(r'_(std|count|max|min|sum|pct_\d{4}(_\d+)?)$')

  return not _IGNORED_STATS_RE.search(value.name)

See
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu....

[perezju, 2016-05-24 11:29:06]

https://codereview.chromium.org/2005233002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/2005233002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:96: return value.name.endswith('_avg')
On 2016/05/24 10:23:52, petrcermak wrote:
> Ned: Will this method also be applied to things like num_failed? If so, then
we
> should do the following:
> 
>   return 'memory:' not in value.name or value.name.endswith('_avg')
> 
> (I don't think we can use value.name.startswith('memory:') because the
tir_label
> might have already been added to the front of the value name).
> 
> Furthermore, this code will remove all dump counts (because they're simple
> scalars), so I think that we should do the following instead:
> 
>   _IGNORED_STATS_RE =
re.compile(r'_(std|count|max|min|sum|pct_\d{4}(_\d+)?)$')
> 
>   return not _IGNORED_STATS_RE.search(value.name)
> 
> See
>
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu....

You're correct, this was throwing away dump counts. I took your suggestion, and
did a few other manual checks to ensure doesn't throw away too much or too
little.

[petrcermak, 2016-05-24 12:36:54]

LGTM with one comment.

Thanks,
Petr

https://codereview.chromium.org/2005233002/diff/20001/tools/perf/benchmarks/m...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/2005233002/diff/20001/tools/perf/benchmarks/m...
tools/perf/benchmarks/memory_infra.py:17: _IGNORED_STATS_RE =
re.compile(r'_(std|count|max|min|sum|pct_\d{4}(_\d+)?)$')
It might be worth saying something like "See
tr.v.Numeric.getSummarizedScalarNumericsWithNames()" and/or add a link to
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu....

[nednguyen, 2016-05-24 13:05:31]

lgtm

[perezju, 2016-05-24 13:16:46]

https://codereview.chromium.org/2005233002/diff/20001/tools/perf/benchmarks/m...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/2005233002/diff/20001/tools/perf/benchmarks/m...
tools/perf/benchmarks/memory_infra.py:17: _IGNORED_STATS_RE =
re.compile(r'_(std|count|max|min|sum|pct_\d{4}(_\d+)?)$')
On 2016/05/24 12:36:54, petrcermak wrote:
> It might be worth saying something like "See
> tr.v.Numeric.getSummarizedScalarNumericsWithNames()" and/or add a link to
>
https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu....

Done.

[perezju, 2016-05-24 13:17:04]

The CQ bit was checked by perezju@chromium.org

[perezju, 2016-05-24 13:17:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/2005233002/#ps40001
(title: "added comment")

[commit-bot: I haz the power, 2016-05-24 13:17:31]

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

[nednguyen, 2016-05-24 13:18:00]

Just curious, why not do this at _MemoryInfra benchmark so all your memory
benchmarks also filter out extra metrics?

[perezju, 2016-05-24 13:21:50]

On 2016/05/24 13:18:00, nednguyen wrote:
> Just curious, why not do this at _MemoryInfra benchmark so all your memory
> benchmarks also filter out extra metrics?

I did not want to affect the V8 benchmarks at the bottom, which use their own
"v8AndMemoryMetrics" and do create more than one dump per story.

[nednguyen, 2016-05-24 15:14:00]

On 2016/05/24 13:21:50, perezju wrote:
> On 2016/05/24 13:18:00, nednguyen wrote:
> > Just curious, why not do this at _MemoryInfra benchmark so all your memory
> > benchmarks also filter out extra metrics?
> 
> I did not want to affect the V8 benchmarks at the bottom, which use their own
> "v8AndMemoryMetrics" and do create more than one dump per story.

I would remove the CQ_EXTRA_TRYBOTS if you already try running this locally

[commit-bot: I haz the power, 2016-05-24 15:18:26]

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

[commit-bot: I haz the power, 2016-05-24 15:18:27]

Try jobs failed on following builders:
  linux_perf_cq on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
  winx64_10_perf_cq on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)

[perezju, 2016-05-24 15:28:52]

Description was changed from

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
==========

to

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
==========

[perezju, 2016-05-24 15:29:23]

On 2016/05/24 15:14:00, nednguyen wrote:
> On 2016/05/24 13:21:50, perezju wrote:
> > On 2016/05/24 13:18:00, nednguyen wrote:
> > > Just curious, why not do this at _MemoryInfra benchmark so all your memory
> > > benchmarks also filter out extra metrics?
> > 
> > I did not want to affect the V8 benchmarks at the bottom, which use their
own
> > "v8AndMemoryMetrics" and do create more than one dump per story.
> 
> I would remove the CQ_EXTRA_TRYBOTS if you already try running this locally

Yep. I did.

[perezju, 2016-05-24 15:29:27]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2016-05-24 15:29:53]

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

[commit-bot: I haz the power, 2016-05-24 15:34:00]

Description was changed from

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
==========

to

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
==========

[commit-bot: I haz the power, 2016-05-24 15:34:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-24 15:35:04]

Description was changed from

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962
==========

to

==========
[tools/perf] Only send _avg metrics for memory infra metrics

This is a stopgap to reduce the load of data sent to dashboards, to
improve the reliability of metrics tracking for these new benchmarks.

BUG=610962

Committed: https://crrev.com/373e9b71d8cc649898fc3f4cbf726d95ca61f580
Cr-Commit-Position: refs/heads/master@{#395603}
==========

[commit-bot: I haz the power, 2016-05-24 15:35:05]

Patchset 3 (id:??) landed as
https://crrev.com/373e9b71d8cc649898fc3f4cbf726d95ca61f580
Cr-Commit-Position: refs/heads/master@{#395603}