Implements peer connection stats measurements in WebRTC call test.

Implements peer connection stats measurements in WebRTC call test.

The telemetry test for WebRTC has so far been measuring CPU and memory.
Since there's a stats API for WebRTC (see http://goo.gl/lfQKzE), we try
to use that do extract interesting properties about the call. The
properties we look at are such that we expect them to remain stable, so
a sudden change in any of them will warrant investigation.

This implementation is based on a trick where we instrument the
constructor for webkitRTCPeerConnection. Whenever a peer connection is
constructed, we attach a polling function which periodically polls the
connection for new stats (that's how the stats API works). Then we
make it easy for a measurement to extract this data, aggregate it into
time series and report it through Telemetry.

BUG=412787
Committed: https://crrev.com/1d2d8d71fc4ce119e6847a8c8aa667cf003c87ad
Cr-Commit-Position: refs/heads/master@{#300249}

[phoglund_chromium, 2014-09-10 15:28:08]

phoglund@chromium.org changed reviewers:
+ nduca@chromium.org

[phoglund_chromium, 2014-09-10 15:28:09]

Nat: please review :) Feel free to reassign to any other telemetry reviewer if
you're busy.

[phoglund_chromium, 2014-09-12 08:26:07]

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
File tools/perf/metrics/webrtc_stats.py (right):

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:64: for i in xrange(num_peer_connections):
This is arguably complex enough to warrant a unit test; what do you think?

[phoglund_chromium, 2014-09-23 16:48:50]

phoglund@chromium.org changed reviewers:
+ tonyg@chromium.org
- nduca@chromium.org

[phoglund_chromium, 2014-09-23 16:49:57]

nduca suggested you'd be a good reviewer, Tony. Can you take a look?

[phoglund_chromium, 2014-09-26 08:07:36]

On 2014/09/23 16:49:57, phoglund wrote:
> nduca suggested you'd be a good reviewer, Tony. Can you take a look?

friendly ping

[tonyg, 2014-09-29 16:45:22]

Apologies for the delay. One round of comments and I think this should be good
to go.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
File tools/perf/metrics/webrtc_stats.py (right):

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:59: if not self._all_reports:
Are there cases when this is expected? If not, perhaps we want an assertion or
at least a logging.error.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:64: for i in xrange(num_peer_connections):
Better written as:
for i, peer_connection in enumerate(reports):
  ...

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:64: for i in xrange(num_peer_connections):
On 2014/09/12 08:26:07, phoglund wrote:
> This is arguably complex enough to warrant a unit test; what do you think?

Definitely. The main value is that without a unittest, the stats could just
unexpectedly vanish and the perf dashboard won't let us know. With a unittest,
then any breakage to the stats themselves would fail at the CQ level.

I'd recommend keeping it very simple, just play the shortest webrtc stream and
verify the stats you want are all > 0.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:68: trace_name = 'pc_%d_%s' % (i,
camel_case.ToUnderscore(stat_name))
What is "pc_"? I recommend a more friendly name here.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
File tools/perf/page_sets/webrtc_cases.py (right):

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:8: TRACK_CREATED_PEER_CONNECTIONS = '''
This looks significant enough to warrant pulling out to a .js file which we read
in. For example, see tools/perf/metrics/media.js

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:21: window.peerConnectionReports = [];
Looks like everything besides this should go into the functional closure to
avoid polluting window.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:80: self.script_to_evaluate_on_commit =
TRACK_CREATED_PEER_CONNECTIONS
Why is this on page2 but not page1? If that's a mistake, I recommend moving this
injection into the Measurement because any page set with the webrtc measurement
requires it. If that's not a mistake, then this is the correct place, but I'd
like to understand why.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:82: def RunEndure(self, action_runner):
Unrelated: Endure is gone. I think this can be removed now.

[phoglund_chromium, 2014-09-30 13:37:49]

PTAL

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
File tools/perf/metrics/webrtc_stats.py (right):

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:59: if not self._all_reports:
On 2014/09/29 16:45:21, tonyg wrote:
> Are there cases when this is expected? If not, perhaps we want an assertion or
> at least a logging.error.

Yes, the list will be empty if a page didn't end up invoking the peer connection
constructor.

I can add a log statement here. If someone runs with this measurement on a page
that doesn't create peer connections, the measurement is meaningless.

Note that if the javascript from webrtc_cases doesn't load for whatever reason,
we'll fail on #56 already, so the code already makes that distinction at least.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:64: for i in xrange(num_peer_connections):
On 2014/09/29 16:45:22, tonyg wrote:
> Better written as:
> for i, peer_connection in enumerate(reports):
>   ...

Done.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:64: for i in xrange(num_peer_connections):
On 2014/09/29 16:45:22, tonyg wrote:
> On 2014/09/12 08:26:07, phoglund wrote:
> > This is arguably complex enough to warrant a unit test; what do you think?
> 
> Definitely. The main value is that without a unittest, the stats could just
> unexpectedly vanish and the perf dashboard won't let us know. With a unittest,
> then any breakage to the stats themselves would fail at the CQ level.
> 
> I'd recommend keeping it very simple, just play the shortest webrtc stream and
> verify the stats you want are all > 0.

Hm. I don't think I should play an actual WebRTC stream, since the js code that
does the constructor overriding and stats recording is in webrtc_cases.py. I did
make a test which ensures this code does the right thing given valid JSON,
though. I think that test exercises this code well.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:68: trace_name = 'pc_%d_%s' % (i,
camel_case.ToUnderscore(stat_name))
On 2014/09/29 16:45:21, tonyg wrote:
> What is "pc_"? I recommend a more friendly name here.

Done.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
File tools/perf/page_sets/webrtc_cases.py (right):

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:8: TRACK_CREATED_PEER_CONNECTIONS = '''
On 2014/09/29 16:45:22, tonyg wrote:
> This looks significant enough to warrant pulling out to a .js file which we
read
> in. For example, see tools/perf/metrics/media.js

Done.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:21: window.peerConnectionReports = [];
On 2014/09/29 16:45:22, tonyg wrote:
> Looks like everything besides this should go into the functional closure to
> avoid polluting window.

Done.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:80: self.script_to_evaluate_on_commit =
TRACK_CREATED_PEER_CONNECTIONS
On 2014/09/29 16:45:22, tonyg wrote:
> Why is this on page2 but not page1? If that's a mistake, I recommend moving
this
> injection into the Measurement because any page set with the webrtc
measurement
> requires it. If that's not a mistake, then this is the correct place, but I'd
> like to understand why.
The first test page only does a getUserMedia and puts the media stream into a
local video tag, whereas page 2 (and 3, which I'll just now rebase in), sets up
peer connections as well.

So for page 1, the measurement will not measure anything.

https://codereview.chromium.org/561803002/diff/20001/tools/perf/page_sets/web...
tools/perf/page_sets/webrtc_cases.py:82: def RunEndure(self, action_runner):
On 2014/09/29 16:45:22, tonyg wrote:
> Unrelated: Endure is gone. I think this can be removed now.

Done.

[phoglund_chromium, 2014-10-01 13:17:45]

https://codereview.chromium.org/561803002/diff/80001/tools/perf/metrics/webrt...
File tools/perf/metrics/webrtc_stats.py (right):

https://codereview.chromium.org/561803002/diff/80001/tools/perf/metrics/webrt...
tools/perf/metrics/webrtc_stats.py:59: if not self._all_reports:
Actually, I ended up not logging or asserting here after all. My first solution
only loaded the tracking js if I expected the page to create peer connections.
Now it will always load the js, but it just won't do anything if no peer
connection are created. Thus, it seems reasonable to just return here if the
page didn't create any peer connections but just did getUserMedia and a local
stream.

[phoglund_chromium, 2014-10-03 12:15:47]

Friendly ping!

[phoglund_chromium, 2014-10-09 15:46:46]

On 2014/10/03 12:15:47, phoglund wrote:
> Friendly ping!

Ping!

[phoglund_chromium, 2014-10-09 15:47:52]

On 2014/10/09 15:46:46, phoglund wrote:
> On 2014/10/03 12:15:47, phoglund wrote:
> > Friendly ping!
> 
> Ping!

Heh, I see you have 82 incoming reviews, so I understand it takes time. Next
time, should I choose someone else as reviewer and in that case who?

[tonyg, 2014-10-15 00:42:02]

lgtm

Apologies again for the long delay here. dtu@ or skyostil@ might both be great
reviewers for these types of things as well.

[phoglund_chromium, 2014-10-15 06:12:19]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-10-15 06:13:05]

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

[commit-bot: I haz the power, 2014-10-15 06:15:50]

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

[commit-bot: I haz the power, 2014-10-15 06:15:51]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[phoglund_chromium, 2014-10-20 10:21:40]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2014-10-20 10:22:10]

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

[commit-bot: I haz the power, 2014-10-20 11:41:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-20 11:41:41]

Patchset 6 (id:??) landed as
https://crrev.com/1d2d8d71fc4ce119e6847a8c8aa667cf003c87ad
Cr-Commit-Position: refs/heads/master@{#300249}

[phoglund_chromium, 2014-10-21 15:54:13]

On 2014/10/15 00:42:02, tonyg wrote:
> lgtm
> 
> Apologies again for the long delay here. dtu@ or skyostil@ might both be great
> reviewers for these types of things as well.

Thank you Tony. I'll try with them to ease your review load a bit :)