Add two UMA counters for IPv4 and IPv6 local candidates gathered in WebRTC PeerConnection.

Add two UMA counters for IPv4 and IPv6 local candidates gathered in WebRTC PeerConnection.

Counter will be accumulated for each OnIceCandidate callback and report once the IceGathering is completed.

BUG=411086
Committed: https://crrev.com/8fb8b1b7ffa66807f5313ccb367a9d4891dab404
Cr-Commit-Position: refs/heads/master@{#296965}

[guoweis2, 2014-09-25 05:14:33]

guoweis@google.com changed reviewers:
+ guoweis@google.com, vrk@chromium.org

[guoweis2, 2014-09-25 05:14:33]

Victoria, could you review this change? The change is to hook up 2 new counters
to keep track of how many IPv4 and IPv6 candidates we have in a PeerConnection.

[guoweis2, 2014-09-25 06:20:23]

On 2014/09/25 05:14:33, guoweis2 wrote:
> Victoria, could you review this change? The change is to hook up 2 new
counters
> to keep track of how many IPv4 and IPv6 candidates we have in a
PeerConnection.

Found an issue that I missed a place on unit test file. Will address this
tomorrow.

[vrk (LEFT CHROMIUM), 2014-09-25 08:12:31]

The CQ bit was checked by vrk@chromium.org

[vrk (LEFT CHROMIUM), 2014-09-25 08:12:31]

lgtm

[vrk (LEFT CHROMIUM), 2014-09-25 08:12:35]

The CQ bit was unchecked by vrk@chromium.org

[vrk (LEFT CHROMIUM), 2014-09-25 08:13:36]

vrk@chromium.org changed reviewers:
+ asvitkine@chromium.org
- guoweis@google.com

[vrk (LEFT CHROMIUM), 2014-09-25 08:13:37]

Adding asvitkine for histogram OWNERS

[Alexei Svitkine (slow), 2014-09-25 15:10:52]

LGTM % comment, thanks!

https://codereview.chromium.org/600163004/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/600163004/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:37850: +    Number of IPv4 Candidates
gathered in a PeerConnection Session.
Please mention when this is logged, same for the one below.

[guoweis2, 2014-09-25 19:46:10]

guoweis@google.com changed reviewers:
+ guoweis@google.com, sergeyu@chromium.org
- asvitkine@chromium.org, vrk@chromium.org

[guoweis2, 2014-09-25 19:46:10]

@sergeyu: could you OWNER review
content/renderer/media/webrtc/mock_peer_connection_dependency_factory.cc?

[juberti2, 2014-09-25 20:05:56]

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

[juberti2, 2014-09-25 20:05:56]

https://codereview.chromium.org/600163004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/600163004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:980:
num_local_candidates_ipv4_++;
Need to reset this when gathering restarts

[juberti2, 2014-09-25 20:10:43]

https://codereview.chromium.org/600163004/diff/20001/content/renderer/media/r...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/600163004/diff/20001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:900:
UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
Remind me, is the goal to count all candidates? Or just the number of candidates
per media stream/m= line?

[chromium-reviews, 2014-09-25 20:20:53]

I plan to correlate this with TimeToConnect. Looking at the code, my
understanding is it tracks all m lines, correct? If so, I'd say we should
count all candidates.

Thanks,
Guowei

On Thu, Sep 25, 2014 at 1:10 PM, <juberti@chromium.org> wrote:

>
> https://codereview.chromium.org/600163004/diff/20001/
> content/renderer/media/rtc_peer_connection_handler.cc
> File content/renderer/media/rtc_peer_connection_handler.cc (right):
>
> https://codereview.chromium.org/600163004/diff/20001/
> content/renderer/media/rtc_peer_connection_handler.cc#newcode900
> content/renderer/media/rtc_peer_connection_handler.cc:900:
> UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
> Remind me, is the goal to count all candidates? Or just the number of
> candidates per media stream/m= line?
>
> https://codereview.chromium.org/600163004/
>

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

[juberti2, 2014-09-25 20:26:37]

On 2014/09/25 20:20:53, chromium-reviews wrote:
> I plan to correlate this with TimeToConnect. Looking at the code, my
> understanding is it tracks all m lines, correct? If so, I'd say we should
> count all candidates.
> 
> Thanks,
> Guowei
> 
> On Thu, Sep 25, 2014 at 1:10 PM, <mailto:juberti@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/600163004/diff/20001/
> > content/renderer/media/rtc_peer_connection_handler.cc
> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
> >
> > https://codereview.chromium.org/600163004/diff/20001/
> > content/renderer/media/rtc_peer_connection_handler.cc#newcode900
> > content/renderer/media/rtc_peer_connection_handler.cc:900:
> > UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
> > Remind me, is the goal to count all candidates? Or just the number of
> > candidates per media stream/m= line?
> >
> > https://codereview.chromium.org/600163004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

OK. The part I am concerned about is that not all candidates will ping each
other, so if you have 8 candidates for a single m= line, that will result in 64
checks, versus 50 checks for 2 m= lines each with 5 candidates. So it might make
sense to only count candidates for a single component (and perhaps also
correlate # of components with TimeToConnect)

[chromium-reviews, 2014-09-25 20:37:36]

I see. Please let me know if below sounds reasonable.

1. I'm going to naively divide the # of candidates by # of m lines. I'm not
aware of any situation the number should be different by m line
2. I'll add the # of components in a different CL.

Thanks,
Guowei

Thanks,
Guowei

On Thu, Sep 25, 2014 at 1:26 PM, <juberti@chromium.org> wrote:

> On 2014/09/25 20:20:53, chromium-reviews wrote:
>
>> I plan to correlate this with TimeToConnect. Looking at the code, my
>> understanding is it tracks all m lines, correct? If so, I'd say we should
>> count all candidates.
>>
>
>  Thanks,
>> Guowei
>>
>
>  On Thu, Sep 25, 2014 at 1:10 PM, <mailto:juberti@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.org/600163004/diff/20001/
>> > content/renderer/media/rtc_peer_connection_handler.cc
>> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
>> >
>> > https://codereview.chromium.org/600163004/diff/20001/
>> > content/renderer/media/rtc_peer_connection_handler.cc#newcode900
>> > content/renderer/media/rtc_peer_connection_handler.cc:900:
>> > UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
>> > Remind me, is the goal to count all candidates? Or just the number of
>> > candidates per media stream/m= line?
>> >
>> > https://codereview.chromium.org/600163004/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> OK. The part I am concerned about is that not all candidates will ping each
> other, so if you have 8 candidates for a single m= line, that will result
> in 64
> checks, versus 50 checks for 2 m= lines each with 5 candidates. So it
> might make
> sense to only count candidates for a single component (and perhaps also
> correlate # of components with TimeToConnect)
>
> https://codereview.chromium.org/600163004/
>

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

[juberti2, 2014-09-25 20:43:28]

On 2014/09/25 20:37:36, chromium-reviews wrote:
> I see. Please let me know if below sounds reasonable.
> 
> 1. I'm going to naively divide the # of candidates by # of m lines. I'm not
> aware of any situation the number should be different by m line
> 2. I'll add the # of components in a different CL.

I think the easiest thing to do would be to only count candidates for m= line 0
(sdpMIndex = 0) and component 0. This will avoid miscounting when doing BUNDLE
or rtcp mux.
> 
> Thanks,
> Guowei
> 
> Thanks,
> Guowei
> 
> On Thu, Sep 25, 2014 at 1:26 PM, <mailto:juberti@chromium.org> wrote:
> 
> > On 2014/09/25 20:20:53, chromium-reviews wrote:
> >
> >> I plan to correlate this with TimeToConnect. Looking at the code, my
> >> understanding is it tracks all m lines, correct? If so, I'd say we should
> >> count all candidates.
> >>
> >
> >  Thanks,
> >> Guowei
> >>
> >
> >  On Thu, Sep 25, 2014 at 1:10 PM, <mailto:juberti@chromium.org> wrote:
> >>
> >
> >  >
> >> > https://codereview.chromium.org/600163004/diff/20001/
> >> > content/renderer/media/rtc_peer_connection_handler.cc
> >> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
> >> >
> >> > https://codereview.chromium.org/600163004/diff/20001/
> >> > content/renderer/media/rtc_peer_connection_handler.cc#newcode900
> >> > content/renderer/media/rtc_peer_connection_handler.cc:900:
> >> > UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
> >> > Remind me, is the goal to count all candidates? Or just the number of
> >> > candidates per media stream/m= line?
> >> >
> >> > https://codereview.chromium.org/600163004/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > OK. The part I am concerned about is that not all candidates will ping each
> > other, so if you have 8 candidates for a single m= line, that will result
> > in 64
> > checks, versus 50 checks for 2 m= lines each with 5 candidates. So it
> > might make
> > sense to only count candidates for a single component (and perhaps also
> > correlate # of components with TimeToConnect)
> >
> > https://codereview.chromium.org/600163004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[guoweis_left_chromium, 2014-09-25 22:49:41]

On 2014/09/25 20:43:28, juberti2 wrote:
> On 2014/09/25 20:37:36, chromium-reviews wrote:
> > I see. Please let me know if below sounds reasonable.
> > 
> > 1. I'm going to naively divide the # of candidates by # of m lines. I'm not
> > aware of any situation the number should be different by m line
> > 2. I'll add the # of components in a different CL.
> 
> I think the easiest thing to do would be to only count candidates for m= line
0
> (sdpMIndex = 0) and component 0. This will avoid miscounting when doing BUNDLE
> or rtcp mux.
> > 
> > Thanks,
> > Guowei
> > 
> > Thanks,
> > Guowei
> > 
> > On Thu, Sep 25, 2014 at 1:26 PM, <mailto:juberti@chromium.org> wrote:
> > 
> > > On 2014/09/25 20:20:53, chromium-reviews wrote:
> > >
> > >> I plan to correlate this with TimeToConnect. Looking at the code, my
> > >> understanding is it tracks all m lines, correct? If so, I'd say we should
> > >> count all candidates.
> > >>
> > >
> > >  Thanks,
> > >> Guowei
> > >>
> > >
> > >  On Thu, Sep 25, 2014 at 1:10 PM, <mailto:juberti@chromium.org> wrote:
> > >>
> > >
> > >  >
> > >> > https://codereview.chromium.org/600163004/diff/20001/
> > >> > content/renderer/media/rtc_peer_connection_handler.cc
> > >> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
> > >> >
> > >> > https://codereview.chromium.org/600163004/diff/20001/
> > >> > content/renderer/media/rtc_peer_connection_handler.cc#newcode900
> > >> > content/renderer/media/rtc_peer_connection_handler.cc:900:
> > >> > UMA_HISTOGRAM_COUNTS_100("WebRTC.PeerConnection.IPv4LocalCandidates",
> > >> > Remind me, is the goal to count all candidates? Or just the number of
> > >> > candidates per media stream/m= line?
> > >> >
> > >> > https://codereview.chromium.org/600163004/
> > >> >
> > >>
> > >
> > >  To unsubscribe from this group and stop receiving emails from it, send an
> > >>
> > > email
> > >
> > >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >>
> > >
> > > OK. The part I am concerned about is that not all candidates will ping
each
> > > other, so if you have 8 candidates for a single m= line, that will result
> > > in 64
> > > checks, versus 50 checks for 2 m= lines each with 5 candidates. So it
> > > might make
> > > sense to only count candidates for a single component (and perhaps also
> > > correlate # of components with TimeToConnect)
> > >
> > > https://codereview.chromium.org/600163004/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL

[juberti2, 2014-09-26 01:45:37]

lgtm with the following change

https://codereview.chromium.org/600163004/diff/40001/content/renderer/media/r...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/600163004/diff/40001/content/renderer/media/r...
content/renderer/media/rtc_peer_connection_handler.cc:905: } else if
(ice_gathering_current_state_ ==
wouldn't it be simpler to do this if new_state == kIceGatheringStarted (or
whatever the name is?)

[chromium-reviews, 2014-09-26 04:33:45]

I was concerned with the fact that we could have one IceGatheringGathering
per component and if for some reason one component was started late, it'll
clear the accumulated counter. It would be nice if I could only clear the
counter if it's the first component of first m line but the information was
not available from the call back.


On Sep 25, 2014 6:45 PM, <juberti@chromium.org> wrote:

> lgtm with the following change
>
>
> https://codereview.chromium.org/600163004/diff/40001/
> content/renderer/media/rtc_peer_connection_handler.cc
> File content/renderer/media/rtc_peer_connection_handler.cc (right):
>
> https://codereview.chromium.org/600163004/diff/40001/
> content/renderer/media/rtc_peer_connection_handler.cc#newcode905
> content/renderer/media/rtc_peer_connection_handler.cc:905: } else if
> (ice_gathering_current_state_ ==
> wouldn't it be simpler to do this if new_state == kIceGatheringStarted
> (or whatever the name is?)
>
> https://codereview.chromium.org/600163004/
>

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

[chromium-reviews, 2014-09-26 04:59:28]

Ok. I removed the tracking of the old state. In all my testing, all the
IceGatheringGathering state are always fired together before any
OnIceCandidate. I looked the code a bit and can't tell whether such race
could ever occur or not.

I'm trusting your point and going with your comment now.

Thanks,
Guowei

On Thu, Sep 25, 2014 at 9:33 PM, Guo-wei Shieh <guoweis@google.com> wrote:

> I was concerned with the fact that we could have one IceGatheringGathering
> per component and if for some reason one component was started late, it'll
> clear the accumulated counter. It would be nice if I could only clear the
> counter if it's the first component of first m line but the information was
> not available from the call back.
>
>
> On Sep 25, 2014 6:45 PM, <juberti@chromium.org> wrote:
>
>> lgtm with the following change
>>
>>
>> https://codereview.chromium.org/600163004/diff/40001/
>> content/renderer/media/rtc_peer_connection_handler.cc
>> File content/renderer/media/rtc_peer_connection_handler.cc (right):
>>
>> https://codereview.chromium.org/600163004/diff/40001/
>> content/renderer/media/rtc_peer_connection_handler.cc#newcode905
>> content/renderer/media/rtc_peer_connection_handler.cc:905: } else if
>> (ice_gathering_current_state_ ==
>> wouldn't it be simpler to do this if new_state == kIceGatheringStarted
>> (or whatever the name is?)
>>
>> https://codereview.chromium.org/600163004/
>>
>

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

[guoweis2, 2014-09-26 05:00:38]

The CQ bit was checked by guoweis@google.com

[commit-bot: I haz the power, 2014-09-26 05:01:10]

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

[juberti2, 2014-09-26 05:17:09]

On 2014/09/26 05:01:10, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/600163004/60001

Yes, you should always get the Gathering state before the first ICE candidate.

There is one edge case to watch out for; if you have an audio m-line working,
and then add video, the state will go back to gathering, but there won't be any
new audio candidates.

The way to work around this would be to not report stats upon going to Completed
if you have gathered zero v4 and v6 candidates. But not a big deal.

[commit-bot: I haz the power, 2014-09-26 06:56:15]

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

[commit-bot: I haz the power, 2014-09-26 06:56:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[guoweis2, 2014-09-26 16:28:23]

The CQ bit was checked by guoweis@google.com

[commit-bot: I haz the power, 2014-09-26 16:29:26]

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

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

Committed patchset #4 (id:60001) as 5d17bf75ca70d1787dbac647925e804509191fc7

[commit-bot: I haz the power, 2014-09-26 17:08:46]

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