Add instrumentation to track the IceEndpointType.

Add instrumentation to track the IceEndpointType.

The IceEndpointType has the format of <local_endpoint>_<remote_endpoint>. It is recorded on the BestConnection when we have the first OnTransportCompleted signaled.

BUG=webrtc:4918
R=pthatcher@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/3d564c10157d7de1d2d4236f4e2a13ff1363d52b

[guoweis_webrtc, 2015-08-14 20:54:17]

guoweis@webrtc.org changed reviewers:
+ juberti@webrtc.org, pthatcher@webrtc.org

[guoweis_webrtc, 2015-08-14 20:54:17]

PTAL.

[juberti1, 2015-08-14 21:04:59]

https://codereview.webrtc.org/1277263002/diff/40001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1277263002/diff/40001/webrtc/p2p/base/candidate...
webrtc/p2p/base/candidate.h:75: const std::string& first_hop_protocol() const {
Rather than duplicating the protocol value in cases where there is only one hop
(e.g. host, srflx), I suggest making this relay_protocol(), and leaving it empty
for non-relay candidates. (Similar to how tcptype is left empty for non-tcp
candidates.)

[guoweis_webrtc, 2015-08-14 21:14:38]

On 2015/08/14 21:04:59, juberti1 wrote:
>
https://codereview.webrtc.org/1277263002/diff/40001/webrtc/p2p/base/candidate.h
> File webrtc/p2p/base/candidate.h (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/40001/webrtc/p2p/base/candidate...
> webrtc/p2p/base/candidate.h:75: const std::string& first_hop_protocol() const
{
> Rather than duplicating the protocol value in cases where there is only one
hop
> (e.g. host, srflx), I suggest making this relay_protocol(), and leaving it
empty
> for non-relay candidates. (Similar to how tcptype is left empty for non-tcp
> candidates.)

PTAL.

[juberti1, 2015-08-14 21:26:04]

https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametrics.h
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
talk/app/webrtc/umametrics.h:38: kPeerConnectionEnumCounter_IceEndpointTypeUDP,
UDP -> Udp
TCP -> Tcp

Can you add comments explaining what these counters will be measuring (and why
we need to measure udp/tcp separately?)

https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
talk/app/webrtc/umametrics.h:91: enum IceEndpointType {
I think this should be IceCandidatePairType?

https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:1953: PeerConnectionEnumCounterType counter =
kPeerConnectionEnumCounter_Max;
looks like this code needs to be updated

[guoweis_webrtc, 2015-08-16 04:08:34]

Patchset #5 (id:80001) has been deleted

[guoweis_webrtc, 2015-08-16 04:19:39]

Patchset #5 (id:100001) has been deleted

[guoweis_webrtc, 2015-08-16 04:46:10]

Patchset #6 (id:140001) has been deleted

[guoweis_webrtc, 2015-08-16 04:46:16]

Patchset #5 (id:120001) has been deleted

[guoweis_webrtc, 2015-08-16 17:51:56]

Patchset #5 (id:160001) has been deleted

[guoweis_webrtc, 2015-08-16 17:52:07]

Patchset #5 (id:180001) has been deleted

[guoweis_webrtc, 2015-08-16 17:52:15]

Patchset #5 (id:200001) has been deleted

[guoweis_webrtc, 2015-08-16 17:54:34]

On 2015/08/14 21:26:04, juberti1 wrote:
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametrics.h
> File talk/app/webrtc/umametrics.h (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
> talk/app/webrtc/umametrics.h:38:
kPeerConnectionEnumCounter_IceEndpointTypeUDP,
> UDP -> Udp
> TCP -> Tcp
> 
> Can you add comments explaining what these counters will be measuring (and why
> we need to measure udp/tcp separately?)
> 
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
> talk/app/webrtc/umametrics.h:91: enum IceEndpointType {
> I think this should be IceCandidatePairType?
> 
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
> talk/app/webrtc/webrtcsession.cc:1953: PeerConnectionEnumCounterType counter =
> kPeerConnectionEnumCounter_Max;
> looks like this code needs to be updated

PTAL

[guoweis_webrtc, 2015-08-18 17:14:12]

On 2015/08/16 17:54:34, guoweis wrote:
> On 2015/08/14 21:26:04, juberti1 wrote:
> >
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametrics.h
> > File talk/app/webrtc/umametrics.h (right):
> > 
> >
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
> > talk/app/webrtc/umametrics.h:38:
> kPeerConnectionEnumCounter_IceEndpointTypeUDP,
> > UDP -> Udp
> > TCP -> Tcp
> > 
> > Can you add comments explaining what these counters will be measuring (and
why
> > we need to measure udp/tcp separately?)
> > 
> >
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/umametric...
> > talk/app/webrtc/umametrics.h:91: enum IceEndpointType {
> > I think this should be IceCandidatePairType?
> > 
> >
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
> > File talk/app/webrtc/webrtcsession.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1277263002/diff/60001/talk/app/webrtc/webrtcses...
> > talk/app/webrtc/webrtcsession.cc:1953: PeerConnectionEnumCounterType counter
=
> > kPeerConnectionEnumCounter_Max;
> > looks like this code needs to be updated
> 
> PTAL

Ping

[guoweis_webrtc, 2015-08-18 17:21:33]

guoweis@webrtc.org changed reviewers:
+ webrtc-network@google.com

[pthatcher1, 2015-08-18 18:06:23]

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:53: }
Why not just initialize the size in the constructor to the max size, like you do
the array?

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:54: if (counters_[type].size() <
static_cast<size_t>(counter_max)) {
It might be more readable to use a reference:

auto& counters = counters_[type];

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:76: return counters_[type][counter];
Won't this explode without a size check?

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:53: int
GetCounter(PeerConnectionEnumCounterType type, int counter) const;
If you renamed one to IncrementEnumCounter, shouldn't you rename the other to
GetEnumCounter?

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:63: std::vector<std::vector<int>>
counters_;
Can you put a comment about what the indexes are (I'm assuming enum type, and
then counter, but a comment would help out).

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:64: int
int_histogram_samples_[kPeerConnectionMetricsName_Max];
It seems like you should have an array of vectors in counters_ or a a vector in
int_histogram_samples_ to make them consistent.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:194: const int kIceConnectionState_Max
= kIceConnectionClosed + 1;
You could just put kIceConnectionStateMax at the end of the enum.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:39: // For next 2 counters, we track them
separately based on the first hop
next 2 => the next 2

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:39: // For next 2 counters, we track them
separately based on the first hop
What do you mean by "first hop".  I think you mean "the local candidate type in
the case of non-TURN candidates, and the protocol used to connect to the TURN
server in the case of TURN candidates".  But commenting it would be nice.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:43: kPeerConnectionEnumCounter_Max
I'd prefer names without the underscores.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:92: // <local_endpoint>_<remote_endpoint>. It
I think you mean <local_candidate_type>_<remote_candidate_type>.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:94: // signaled.
Might be more clear as "is recorded based on the type of candidate pair used
when the PeerConnection first goes to a connected state".  

Also, you should probably comment on what happens in the non-BUNDLE case.  If
you have separate connections for audio and video, which is recorded?  Both?

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:110: kIceCandidatePair_Prflx_Relay,
I'm not a fan of underscores here either.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:60: using cricket::LOCAL_PORT_TYPE;
I think you could do 

using HOST = cricket::LOCAL_PORT_TYPE;
using SRFLX = cricket::STUN_PORT_TYPE;
using RELAY = cricket::RELAY_PORT_TYPE;
using PRFLX = cricket::PRFLX_PORT_TYPE;

And that might save you some ugliness in the macro.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:101: CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, LOCAL,
kIceCandidatePair_Host_Host);
I think removing the macro and just doing this might be better:

auto l = local.type();
auto r = remote.type();

if (l == HOST && r == HOST) return kIceCandidatePair_Host_Host;
if (l == HOST && r == RELAY) return kIceCandidatePair_Host_Relay;
...

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1956: // Increment the counter for
IceCandidatePairType.
Having an

const Candidate& local = it_info->local_candidate();

Would make the rest a lot more readable.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1957: if (it_info->local_candidate.protocol()
== cricket::TCP_PROTOCOL_NAME ||
Might as well add a "using TCP = cricket::TCP_PROTOCOL_NAME" as well.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1963: cricket::UDP_PROTOCOL_NAME) {
And "using UDP = circket::UDP_PROTOCOL_NAME"

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know if
this is happening in production (even though it shouldn't and the number should
be 0, at least we could verify).

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1989: kPeerConnectionAddressFamilyCounter_Max);
Instead of setting counter in two places and then calling
metrics_observer_->IncrementEnumCounter once, can you just call
metrics_observer_->IncrementEnumCounter in two places?  I think that would be
more readable.

https://codereview.webrtc.org/1277263002/diff/220001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:74: const std::string& relay_protocol() const {
return relay_protocol_; }
Can we use an enum here instead of a string?  

Also, can we make it not "UDP vs TCP", but instead "TURN TCP" vs "TURN UDP"? 
That way, later, we could add "QUIC".

[guoweis_webrtc, 2015-08-18 22:32:43]

Patchset #6 (id:240001) has been deleted

[guoweis_webrtc, 2015-08-18 22:33:57]

PTAL.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:53: }
On 2015/08/18 18:06:22, pthatcher1 wrote:
> Why not just initialize the size in the constructor to the max size, like you
do
> the array?

each counter type's max is different.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:54: if (counters_[type].size() <
static_cast<size_t>(counter_max)) {
On 2015/08/18 18:06:22, pthatcher1 wrote:
> It might be more readable to use a reference:
> 
> auto& counters = counters_[type];

sounds good.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:76: return counters_[type][counter];
On 2015/08/18 18:06:22, pthatcher1 wrote:
> Won't this explode without a size check?

add a DCHECK. I don't want to return silently. It's a bug somewhere else if this
happens.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:53: int
GetCounter(PeerConnectionEnumCounterType type, int counter) const;
On 2015/08/18 18:06:22, pthatcher1 wrote:
> If you renamed one to IncrementEnumCounter, shouldn't you rename the other to
> GetEnumCounter?

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:63: std::vector<std::vector<int>>
counters_;
On 2015/08/18 18:06:22, pthatcher1 wrote:
> Can you put a comment about what the indexes are (I'm assuming enum type, and
> then counter, but a comment would help out).

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.h:64: int
int_histogram_samples_[kPeerConnectionMetricsName_Max];
On 2015/08/18 18:06:22, pthatcher1 wrote:
> It seems like you should have an array of vectors in counters_ or a a vector
in
> int_histogram_samples_ to make them consistent. 

histogram_samples use string to track each sample so they don't need this.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:194: const int kIceConnectionState_Max
= kIceConnectionClosed + 1;
On 2015/08/18 18:06:22, pthatcher1 wrote:
> You could just put kIceConnectionStateMax at the end of the enum.

I was hesitant about this since all the states in enum is according to the spec.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:39: // For next 2 counters, we track them
separately based on the first hop
On 2015/08/18 18:06:22, pthatcher1 wrote:
> What do you mean by "first hop".  I think you mean "the local candidate type
in
> the case of non-TURN candidates, and the protocol used to connect to the TURN
> server in the case of TURN candidates".  But commenting it would be nice.

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:43: kPeerConnectionEnumCounter_Max
On 2015/08/18 18:06:22, pthatcher1 wrote:
> I'd prefer names without the underscores.  

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:92: // <local_endpoint>_<remote_endpoint>. It
On 2015/08/18 18:06:22, pthatcher1 wrote:
> I think you mean <local_candidate_type>_<remote_candidate_type>.

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:94: // signaled.
On 2015/08/18 18:06:22, pthatcher1 wrote:
> Might be more clear as "is recorded based on the type of candidate pair used
> when the PeerConnection first goes to a connected state".  
> 
> Also, you should probably comment on what happens in the non-BUNDLE case.  If
> you have separate connections for audio and video, which is recorded?  Both?

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:110: kIceCandidatePair_Prflx_Relay,
On 2015/08/18 18:06:22, pthatcher1 wrote:
> I'm not a fan of underscores here either.

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:60: using cricket::LOCAL_PORT_TYPE;
On 2015/08/18 18:06:23, pthatcher1 wrote:
> I think you could do 
> 
> using HOST = cricket::LOCAL_PORT_TYPE;
> using SRFLX = cricket::STUN_PORT_TYPE;
> using RELAY = cricket::RELAY_PORT_TYPE;
> using PRFLX = cricket::PRFLX_PORT_TYPE;
> 
> And that might save you some ugliness in the macro.

doesn't alias only work for type?

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1956: // Increment the counter for
IceCandidatePairType.
On 2015/08/18 18:06:23, pthatcher1 wrote:
> Having an
> 
> const Candidate& local = it_info->local_candidate();
> 
> Would make the rest a lot more readable.

Done.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
On 2015/08/18 18:06:22, pthatcher1 wrote:
> Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know if
> this is happening in production (even though it shouldn't and the number
should
> be 0, at least we could verify).  

This is too specific and I don't think UMA is designed for that.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1989: kPeerConnectionAddressFamilyCounter_Max);
On 2015/08/18 18:06:23, pthatcher1 wrote:
> Instead of setting counter in two places and then calling
> metrics_observer_->IncrementEnumCounter once, can you just call
> metrics_observer_->IncrementEnumCounter in two places?  I think that would be
> more readable.

Done.

https://codereview.webrtc.org/1277263002/diff/220001/webrtc/p2p/base/candidate.h
File webrtc/p2p/base/candidate.h (right):

https://codereview.webrtc.org/1277263002/diff/220001/webrtc/p2p/base/candidat...
webrtc/p2p/base/candidate.h:74: const std::string& relay_protocol() const {
return relay_protocol_; }
On 2015/08/18 18:06:23, pthatcher1 wrote:
> Can we use an enum here instead of a string?  
> 
> Also, can we make it not "UDP vs TCP", but instead "TURN TCP" vs "TURN UDP"? 
> That way, later, we could add "QUIC".

For consistency, the previous protocol() is string.

[pthatcher1, 2015-08-19 02:47:53]

Mostly style/readability things now.  

And the bounds check.  I think we need to add one for release builds.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:76: return counters_[type][counter];
On 2015/08/18 22:33:56, guoweis wrote:
> On 2015/08/18 18:06:22, pthatcher1 wrote:
> > Won't this explode without a size check?
> 
> add a DCHECK. I don't want to return silently. It's a bug somewhere else if
this
> happens.

But that only protects debug builds.  I think we should also protect release
builds, with something like:

if (counters_.size() <= type || counters_[type].size() <= counter) {
  return 0;  // or -1?
}

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:60: using cricket::LOCAL_PORT_TYPE;
On 2015/08/18 22:33:57, guoweis wrote:
> On 2015/08/18 18:06:23, pthatcher1 wrote:
> > I think you could do 
> > 
> > using HOST = cricket::LOCAL_PORT_TYPE;
> > using SRFLX = cricket::STUN_PORT_TYPE;
> > using RELAY = cricket::RELAY_PORT_TYPE;
> > using PRFLX = cricket::PRFLX_PORT_TYPE;
> > 
> > And that might save you some ugliness in the macro.
> 
> doesn't alias only work for type? 

Yeah, you're right.  I thought it was more powerful than that in c++11.  I was
wrong.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
On 2015/08/18 22:33:57, guoweis wrote:
> On 2015/08/18 18:06:22, pthatcher1 wrote:
> > Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know
if
> > this is happening in production (even though it shouldn't and the number
> should
> > be 0, at least we could verify).  
> 
> This is too specific and I don't think UMA is designed for that.

Sorry, I meant kPeerConnectionEnumCounter_IceCandidatePairTypeUnknown.

Counters for UDP/TCP/Unknown

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:192: kIceConnectionStateMax,
Can just be kIceConnectionMax (no "state")

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:45: kPeerConnectionEnumCounter_Max
One more underscore.

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:96: // state. For bundle case, only the first
transport gets recorded.
For bundle case => When BUNDLE is enabled

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:100: CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, LOCAL,
kIceCandidatePairHostHost);
I still think this would be better than the macro:

const auto& l = local.type();
const auto& r = remote.type();
const auto& host = LOCAL_PORT_TYPE;
const auto& srflx = STUN_PORT_TYPE;
const auto& relay = RELAY_PORT_TYPE;
const auto& prflx = PRFLX_PORT_TYPE; 

if (l == host && r == host) return kIceCandidatePair_Host_Host;
if (l == host && r == relay) return kIceCandidatePair_Host_Relay;
...
return kIceCandidatePairMax;

[guoweis_webrtc, 2015-08-19 17:56:52]

On 2015/08/19 02:47:53, pthatcher1 wrote:
> Mostly style/readability things now.  
> 
> And the bounds check.  I think we need to add one for release builds.
> 
>
https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
> File talk/app/webrtc/fakemetricsobserver.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
> talk/app/webrtc/fakemetricsobserver.cc:76: return counters_[type][counter];
> On 2015/08/18 22:33:56, guoweis wrote:
> > On 2015/08/18 18:06:22, pthatcher1 wrote:
> > > Won't this explode without a size check?
> > 
> > add a DCHECK. I don't want to return silently. It's a bug somewhere else if
> this
> > happens.
> 
> But that only protects debug builds.  I think we should also protect release
> builds, with something like:
> 
> if (counters_.size() <= type || counters_[type].size() <= counter) {
>   return 0;  // or -1?
> }

I agree such protection on production code but this is test code. What's the
benefit of preventing it from crash such that we could observe it?

> 
>
https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:60: using cricket::LOCAL_PORT_TYPE;
> On 2015/08/18 22:33:57, guoweis wrote:
> > On 2015/08/18 18:06:23, pthatcher1 wrote:
> > > I think you could do 
> > > 
> > > using HOST = cricket::LOCAL_PORT_TYPE;
> > > using SRFLX = cricket::STUN_PORT_TYPE;
> > > using RELAY = cricket::RELAY_PORT_TYPE;
> > > using PRFLX = cricket::PRFLX_PORT_TYPE;
> > > 
> > > And that might save you some ugliness in the macro.
> > 
> > doesn't alias only work for type? 
> 
> Yeah, you're right.  I thought it was more powerful than that in c++11.  I was
> wrong.
> 
>
https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
> On 2015/08/18 22:33:57, guoweis wrote:
> > On 2015/08/18 18:06:22, pthatcher1 wrote:
> > > Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know
> if
> > > this is happening in production (even though it shouldn't and the number
> > should
> > > be 0, at least we could verify).  
> > 
> > This is too specific and I don't think UMA is designed for that.
> 
> Sorry, I meant kPeerConnectionEnumCounter_IceCandidatePairTypeUnknown.
> 
> Counters for UDP/TCP/Unknown
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
> File talk/app/webrtc/peerconnectioninterface.h (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
> talk/app/webrtc/peerconnectioninterface.h:192: kIceConnectionStateMax,
> Can just be kIceConnectionMax (no "state")
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
> File talk/app/webrtc/umametrics.h (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
> talk/app/webrtc/umametrics.h:45: kPeerConnectionEnumCounter_Max
> One more underscore.
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
> talk/app/webrtc/umametrics.h:96: // state. For bundle case, only the first
> transport gets recorded.
> For bundle case => When BUNDLE is enabled
Done
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
> File talk/app/webrtc/webrtcsession.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
> talk/app/webrtc/webrtcsession.cc:100: CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, LOCAL,
> kIceCandidatePairHostHost);
> I still think this would be better than the macro:
> 
> const auto& l = local.type();
> const auto& r = remote.type();
> const auto& host = LOCAL_PORT_TYPE;
> const auto& srflx = STUN_PORT_TYPE;
> const auto& relay = RELAY_PORT_TYPE;
> const auto& prflx = PRFLX_PORT_TYPE; 
> 
> if (l == host && r == host) return kIceCandidatePair_Host_Host;
> if (l == host && r == relay) return kIceCandidatePair_Host_Relay;
> ...
> return kIceCandidatePairMax;
sounds good.

[guoweis_webrtc, 2015-08-19 18:19:42]

Patchset #7 (id:280001) has been deleted

[guoweis_webrtc, 2015-08-19 18:25:52]

PTAL. Hope I didn't miss anything here.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
On 2015/08/19 02:47:52, pthatcher1 wrote:
> On 2015/08/18 22:33:57, guoweis wrote:
> > On 2015/08/18 18:06:22, pthatcher1 wrote:
> > > Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd know
> if
> > > this is happening in production (even though it shouldn't and the number
> > should
> > > be 0, at least we could verify).  
> > 
> > This is too specific and I don't think UMA is designed for that.
> 
> Sorry, I meant kPeerConnectionEnumCounter_IceCandidatePairTypeUnknown.
> 
> Counters for UDP/TCP/Unknown

What's the goal here? To catch a bug? I don't think UMA is designed to catch
bugs like this - see the content in histograms.xml.

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:192: kIceConnectionStateMax,
On 2015/08/19 02:47:53, pthatcher1 wrote:
> Can just be kIceConnectionMax (no "state")

Done.

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
File talk/app/webrtc/umametrics.h (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:45: kPeerConnectionEnumCounter_Max
On 2015/08/19 02:47:53, pthatcher1 wrote:
> One more underscore.

Done.

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/umametri...
talk/app/webrtc/umametrics.h:96: // state. For bundle case, only the first
transport gets recorded.
On 2015/08/19 02:47:53, pthatcher1 wrote:
> For bundle case => When BUNDLE is enabled

Done.

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/260001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:100: CANDIDATE_PAIR_TYPE_ENTRY(LOCAL, LOCAL,
kIceCandidatePairHostHost);
On 2015/08/19 02:47:53, pthatcher1 wrote:
> I still think this would be better than the macro:
> 
> const auto& l = local.type();
> const auto& r = remote.type();
> const auto& host = LOCAL_PORT_TYPE;
> const auto& srflx = STUN_PORT_TYPE;
> const auto& relay = RELAY_PORT_TYPE;
> const auto& prflx = PRFLX_PORT_TYPE; 
> 
> if (l == host && r == host) return kIceCandidatePair_Host_Host;
> if (l == host && r == relay) return kIceCandidatePair_Host_Relay;
> ...
> return kIceCandidatePairMax;

Done.

[pthatcher2, 2015-08-19 18:39:41]

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

[pthatcher2, 2015-08-19 18:39:42]

Sooooo clooooose

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:76: return counters_[type][counter];
On 2015/08/19 02:47:52, pthatcher1 wrote:
> On 2015/08/18 22:33:56, guoweis wrote:
> > On 2015/08/18 18:06:22, pthatcher1 wrote:
> > > Won't this explode without a size check?
> > 
> > add a DCHECK. I don't want to return silently. It's a bug somewhere else if
> this
> > happens.
> 
> But that only protects debug builds.  I think we should also protect release
> builds, with something like:
> 
> if (counters_.size() <= type || counters_[type].size() <= counter) {
>   return 0;  // or -1?
> }

If we want a crash in release builds, then make it  a CHECK instead of a DCHECK.

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/220001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1966: DCHECK(0);
On 2015/08/19 18:25:52, guoweis wrote:
> On 2015/08/19 02:47:52, pthatcher1 wrote:
> > On 2015/08/18 22:33:57, guoweis wrote:
> > > On 2015/08/18 18:06:22, pthatcher1 wrote:
> > > > Should we record a kPeerConnectionEnumCounter_Unknown?  That way we'd
know
> > if
> > > > this is happening in production (even though it shouldn't and the number
> > > should
> > > > be 0, at least we could verify).  
> > > 
> > > This is too specific and I don't think UMA is designed for that.
> > 
> > Sorry, I meant kPeerConnectionEnumCounter_IceCandidatePairTypeUnknown.
> > 
> > Counters for UDP/TCP/Unknown
> 
> What's the goal here? To catch a bug? I don't think UMA is designed to catch
> bugs like this - see the content in histograms.xml.

The goal was to measure how often we screwed up the measurement and hit that
DCHECK(0) in production.  Because release builds will just silently eat that. 
If you're sure that will never happen, you don't need the DCHECK.  If you're not
sure, then you might want to know how often it happens in production.

https://codereview.webrtc.org/1277263002/diff/300001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/300001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:101: return kIceCandidatePairHostHost;
host host is here twice.

[guoweis_webrtc, 2015-08-19 19:01:29]

The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 19:01:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277263002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277263002/340001

[guoweis_webrtc, 2015-08-19 19:03:24]

PTAL. Changed from DCHECK to CHECK.

https://codereview.webrtc.org/1277263002/diff/300001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1277263002/diff/300001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:101: return kIceCandidatePairHostHost;
On 2015/08/19 18:39:42, pthatcher2 wrote:
> host host is here twice.

Done.

[commit-bot: I haz the power, 2015-08-19 19:04:51]

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

[commit-bot: I haz the power, 2015-08-19 19:04:51]

Dry run: Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/9067)

[pthatcher1, 2015-08-19 19:24:20]

lgtm, except for this one enum thing

https://codereview.webrtc.org/1277263002/diff/350018/talk/app/webrtc/objc/pub...
File talk/app/webrtc/objc/public/RTCTypes.h (right):

https://codereview.webrtc.org/1277263002/diff/350018/talk/app/webrtc/objc/pub...
talk/app/webrtc/objc/public/RTCTypes.h:40: } RTCICEConnectionState;
We have Max that isn't the max?

[guoweis_webrtc, 2015-08-19 19:28:23]

The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 19:28:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277263002/350018
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277263002/350018

[guoweis_webrtc, 2015-08-19 21:20:01]

The CQ bit was checked by guoweis@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-19 21:20:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1277263002/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1277263002/370001

[guoweis_webrtc, 2015-08-19 23:51:28]

Committed patchset #11 (id:370001) manually as
3d564c10157d7de1d2d4236f4e2a13ff1363d52b (presubmit successful).

[juberti1, 2015-08-20 23:14:25]

lgtm, just a few minor comments

https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
File talk/app/webrtc/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:39: counters_ =
std::vector<std::vector<int>>();
this could be done via counters_.clear(), I think

https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
talk/app/webrtc/fakemetricsobserver.cc:49: int counter_max) {
I don't understand why we need to pass in a maximum here. It seems unnecessary
and forces us to add Max entries to enums that don't really need them.

https://codereview.webrtc.org/1277263002/diff/370001/webrtc/p2p/base/turnport...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/1277263002/diff/370001/webrtc/p2p/base/turnport...
webrtc/p2p/base/turnport_unittest.cc:737:
EXPECT_EQ(turn_port_->Candidates()[0].relay_protocol(),
expected value comes first in EXPECT_EQ (here and below)

[guoweis_webrtc, 2015-08-21 13:47:07]

On 2015/08/20 23:14:25, juberti1 wrote:
> lgtm, just a few minor comments
> 
>
https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
> File talk/app/webrtc/fakemetricsobserver.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
> talk/app/webrtc/fakemetricsobserver.cc:39: counters_ =
> std::vector<std::vector<int>>();
> this could be done via counters_.clear(), I think
> 
>
https://codereview.webrtc.org/1277263002/diff/370001/talk/app/webrtc/fakemetr...
> talk/app/webrtc/fakemetricsobserver.cc:49: int counter_max) {
> I don't understand why we need to pass in a maximum here. It seems unnecessary
> and forces us to add Max entries to enums that don't really need them.
This is standard UMA interface.
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

> 
>
https://codereview.webrtc.org/1277263002/diff/370001/webrtc/p2p/base/turnport...
> File webrtc/p2p/base/turnport_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1277263002/diff/370001/webrtc/p2p/base/turnport...
> webrtc/p2p/base/turnport_unittest.cc:737:
> EXPECT_EQ(turn_port_->Candidates()[0].relay_protocol(),
> expected value comes first in EXPECT_EQ (here and below)

Will do other clean ups later.