Issue 2386783002: Add UMA metrics for ICE regathering reasons.

honghaiz3

Patchset #1 (id:1) has been deleted

4 years, 2 months ago (2016-10-03 06:27:25 UTC) #1

honghaiz3

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

4 years, 2 months ago (2016-10-03 06:27:40 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2386783002/40001

4 years, 2 months ago (2016-10-03 06:27:51 UTC) #4

honghaiz3

Description was changed from ========== Add UMA metrics for ICE regathering reasons. BUG= ========== to ...

4 years, 2 months ago (2016-10-03 06:29:56 UTC) #5

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-03 08:28:22 UTC) #6

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) ...

4 years, 2 months ago (2016-10-03 08:28:23 UTC) #7

Taylor Brandstetter

deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org

4 years, 2 months ago (2016-10-03 20:33:32 UTC) #8

Taylor Brandstetter

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobserver.cc File webrtc/api/fakemetricsobserver.cc (right): https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobserver.cc#newcode50 webrtc/api/fakemetricsobserver.cc:50: } What's the purpose of this change? https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File ...

4 years, 2 months ago (2016-10-03 20:33:35 UTC) #9

honghaiz3

PTAL. Thanks! https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobserver.cc File webrtc/api/fakemetricsobserver.cc (right): https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobserver.cc#newcode50 webrtc/api/fakemetricsobserver.cc:50: } On 2016/10/03 20:33:34, Taylor Brandstetter wrote: ...

4 years, 2 months ago (2016-10-04 01:03:55 UTC) #11

PTAL. Thanks!

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobs...
File webrtc/api/fakemetricsobserver.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/api/fakemetricsobs...
webrtc/api/fakemetricsobserver.cc:50: }
On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> What's the purpose of this change?

If nothing was recorded, it should return 0, not a check error.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (left):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:476: LOG(LS_INFO) << "Start getting
ports";
On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> Why remove this log statement? Is there another one that provides the same
> information?
I moved it to StartGettingPorts(). It is better to be there than here.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:483:
session->SignalIceRegatheringReason(session, reason);
On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> Calling another class's signal breaks encapsulation; only the class that owns
> the signal should emit it. Also, it's not clear that this is expected to
> increment a UMA stat.
> 
> It seems like it would be better if P2PTransportChannel had a pointer to the
> metrics interface directly. Is that possible?

We can, but it would require some wiring of the pointer from peer connection all
the way to the p2ptransportchannel. 
An alternative, simpler approach is to report the event to the allocator and let
the allocator log the metric. This is more in line with other UMA reporting
where we passed the event dtls error all the way up to the webrtcsession and
webrtcsession wrote the metric.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:484: }
On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> I think this should be outside the if/else block, so that once candidate
pooling
> becomes available and applications start to use it, ICE restart stats are
> included for it as well.

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:964:
webrtc::MetricsObserverInterface* observer2 = nullptr) {
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> I'd prefer if the Endpoint owned its own metric observer and hooked it up by
> default. Then the individual test cases don't need to do anything but access
it.
> 
> This is what other tests such as WebRtcSessionTest do.

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1197: 3000, clock);
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> Could we use constants here instead of 3000 and 6000?

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1289: 
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> I think it would be helpful to add a comment explaining that this tests one
> channel with continual gathering enabled and one with it disabled. Same with
the
> test below.

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1360: SIMULATED_WAIT(false,
35000, clock);
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> Could you replace 35000 with a constant?

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel_unittest.cc:1367: LOG(LS_INFO) << "x1";
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> Remember to remove this log statement

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:392: }
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> It seems confusing that the metrics observer is set on the base class, but
only
> used on the derived class, even though every derived class is expected to do
the
> same thing. It's also odd that the enum is defined here, but only two values
are
> relevant to the PortAllocator itself.
> 
> If the P2PTransportChannel had a direct pointer to the metrics observer, I
think
> it would be better if PortAllocatorSession just had two signals: "regathering
> due to network change" and "regathering due to failed networks". Then
> P2PTransportChannel could listen to the signals and increment the stats
itself.

The first part, really just follows a few other members in this class where they
were set in the base but used in the derived class. I think the reason is
because the derived class is expected to the same thing as you said. Plus each
derived class has its own control on whether and what observer to set. For
example, we set it the fake metrics observer in the tests. 

Regarding the second part, I think it is not uncommon to define some common
enums in a base class and use them elsewhere including other classes. At least
we defined a lot of constants in transport.h which are then used in many places.

For this particular enum list, some of them will be generated in
BasicPortAllocator and some in p2ptransportchannel. As I have responded
elsewhere, it is simpler to keep the observer pointer in the PortAllocator. 
If you really want to use only the defined enums in BasicPortAllocator, I can
create a separate enum and pass that from p2ptransportchannel to
basicportallocator and translate that enum to the one defined in portallocator,
although I do not see the point of doing that instead of directly passing the
enum defined in portallocator.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:179:
static_cast<int>(IceRegatheringReason::MAX_VALUE));
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> Unless I'm mistaken, pooled BasicPortAllocatorSessions will increment the stat
> even if they aren't used. Is that intended? It could skew the stats once
pooling
> becomes available in Chromium.

It is true but only under the circumstances where you created sessions that were
not used and then network changed or failed. 
Should a session created in a pool being used right away or very soon?
After second thought, if a session was created in a pool and then a network
change happened and an ICE regathering happened, perhaps it should be counted
anyway because an ICE regathering indeed happened, even though it is not used. 
So it is probably intended.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:314: if (!allocation_started_ ||
!network_manager_started_ || IsStopped()) {
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> This seems separate to adding stats. Could you put it in a different CL, or
make
> sure this CL's description mentions it?
Moved away from this CL.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:526: if (network_manager_started_ &&
!IsStopped())
On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> Same here.
This is not a behavior change. 
I just moved the check of IsStopped from the DoAllocate to the outside of the
call. 
I need this change in this CL because I do not want to signal an
ICERegatheringReason when the session has stopped.

honghaiz3

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

4 years, 2 months ago (2016-10-04 01:04:14 UTC) #12

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2386783002/80001

4 years, 2 months ago (2016-10-04 01:04:21 UTC) #13

Taylor Brandstetter

Maybe we should bring Peter into the design discussion. I don't want to hold up ...

4 years, 2 months ago (2016-10-04 02:42:33 UTC) #14

Maybe we should bring Peter into the design discussion. I don't want to hold up
this CL if I'm being too stubborn.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:483:
session->SignalIceRegatheringReason(session, reason);
On 2016/10/04 01:03:54, honghaiz3 wrote:
> On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> > Calling another class's signal breaks encapsulation; only the class that
owns
> > the signal should emit it. Also, it's not clear that this is expected to
> > increment a UMA stat.
> > 
> > It seems like it would be better if P2PTransportChannel had a pointer to the
> > metrics interface directly. Is that possible?
> 
> We can, but it would require some wiring of the pointer from peer connection
all
> the way to the p2ptransportchannel.

I think that's the right thing to do. We'll need to do a similar thing to wire
up the RtcEventLog as well (I assume).

> An alternative, simpler approach is to report the event to the allocator and
let
> the allocator log the metric. This is more in line with other UMA reporting
> where we passed the event dtls error all the way up to the webrtcsession and
> webrtcsession wrote the metric. 

The difference is that in the WebRtcSession case, a parent listened to an event
from its child, and forwarded it upwards; a common pattern. Here, a parent is
creating an event for its child, which the child itself listens to. It's not
very intuitive.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:392: }
On 2016/10/04 01:03:55, honghaiz3 wrote:
> On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > It seems confusing that the metrics observer is set on the base class, but
> only
> > used on the derived class, even though every derived class is expected to do
> the
> > same thing. It's also odd that the enum is defined here, but only two values
> are
> > relevant to the PortAllocator itself.
> > 
> > If the P2PTransportChannel had a direct pointer to the metrics observer, I
> think
> > it would be better if PortAllocatorSession just had two signals:
"regathering
> > due to network change" and "regathering due to failed networks". Then
> > P2PTransportChannel could listen to the signals and increment the stats
> itself.
> 
> The first part, really just follows a few other members in this class where
they
> were set in the base but used in the derived class. I think the reason is
> because the derived class is expected to the same thing as you said. Plus each
> derived class has its own control on whether and what observer to set. For
> example, we set it the fake metrics observer in the tests. 
> 
> Regarding the second part, I think it is not uncommon to define some common
> enums in a base class and use them elsewhere including other classes. At least
> we defined a lot of constants in transport.h which are then used in many
places.
> 
> For this particular enum list, some of them will be generated in
> BasicPortAllocator and some in p2ptransportchannel. As I have responded
> elsewhere, it is simpler to keep the observer pointer in the PortAllocator. 
> If you really want to use only the defined enums in BasicPortAllocator, I can
> create a separate enum and pass that from p2ptransportchannel to
> basicportallocator and translate that enum to the one defined in
portallocator,
> although I do not see the point of doing that instead of directly passing the
> enum defined in portallocator. 
> 
> 

The enum definition doesn't matter too much. What I'm more concerned about is
the confusing relationship between PortAllocator, BasicPortAllocator,
MetricsObserverInterface and P2PTransportChannel. Mostly, that
P2PTransportChannel shouldn't be directly calling a signal of another class, and
relying on that signal to increment a stat. An alternative would be to add a
method such as "RecordIceRegatheringReason", but it's still an odd roundabout
approach. The PortAllocator becomes responsible for recording metrics for
another class.

If the main reason for this design is that it's easier to pass the
MetricsObserverInterface from PeerConnection->PortAllocator as opposed to
WebRtcSession->TransportController->P2PTransportChannel, I don't think that's a
strong enough reason.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:179:
static_cast<int>(IceRegatheringReason::MAX_VALUE));
On 2016/10/04 01:03:55, honghaiz3 wrote:
> On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > Unless I'm mistaken, pooled BasicPortAllocatorSessions will increment the
stat
> > even if they aren't used. Is that intended? It could skew the stats once
> pooling
> > becomes available in Chromium.
> 
> It is true but only under the circumstances where you created sessions that
were
> not used and then network changed or failed. 
> Should a session created in a pool being used right away or very soon?
> After second thought, if a session was created in a pool and then a network
> change happened and an ICE regathering happened, perhaps it should be counted
> anyway because an ICE regathering indeed happened, even though it is not used.

> So it is probably intended. 

If we do count regathering that occurs for pooled sessions, I think it needs to
increment a different stat than for in-use sessions. Because otherwise, we can't
answer a question like: "When regathering occurs for an active connection, how
often is it due to a network change vs. an ICE restart?"

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:526: if (network_manager_started_ &&
!IsStopped())
On 2016/10/04 01:03:55, honghaiz3 wrote:
> On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > Same here.
> This is not a behavior change. 
> I just moved the check of IsStopped from the DoAllocate to the outside of the
> call. 
> I need this change in this CL because I do not want to signal an
> ICERegatheringReason when the session has stopped. 

Acknowledged.

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-04 03:04:52 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

4 years, 2 months ago (2016-10-04 03:04:53 UTC) #16

honghaiz3

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portallocator.h#newcode392 webrtc/p2p/base/portallocator.h:392: } On 2016/10/04 02:42:32, Taylor Brandstetter wrote: > On ...

4 years, 2 months ago (2016-10-04 04:06:42 UTC) #17

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:392: }
On 2016/10/04 02:42:32, Taylor Brandstetter wrote:
> On 2016/10/04 01:03:55, honghaiz3 wrote:
> > On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > > It seems confusing that the metrics observer is set on the base class, but
> > only
> > > used on the derived class, even though every derived class is expected to
do
> > the
> > > same thing. It's also odd that the enum is defined here, but only two
values
> > are
> > > relevant to the PortAllocator itself.
> > > 
> > > If the P2PTransportChannel had a direct pointer to the metrics observer, I
> > think
> > > it would be better if PortAllocatorSession just had two signals:
> "regathering
> > > due to network change" and "regathering due to failed networks". Then
> > > P2PTransportChannel could listen to the signals and increment the stats
> > itself.
> > 
> > The first part, really just follows a few other members in this class where
> they
> > were set in the base but used in the derived class. I think the reason is
> > because the derived class is expected to the same thing as you said. Plus
each
> > derived class has its own control on whether and what observer to set. For
> > example, we set it the fake metrics observer in the tests. 
> > 
> > Regarding the second part, I think it is not uncommon to define some common
> > enums in a base class and use them elsewhere including other classes. At
least
> > we defined a lot of constants in transport.h which are then used in many
> places.
> > 
> > For this particular enum list, some of them will be generated in
> > BasicPortAllocator and some in p2ptransportchannel. As I have responded
> > elsewhere, it is simpler to keep the observer pointer in the PortAllocator. 
> > If you really want to use only the defined enums in BasicPortAllocator, I
can
> > create a separate enum and pass that from p2ptransportchannel to
> > basicportallocator and translate that enum to the one defined in
> portallocator,
> > although I do not see the point of doing that instead of directly passing
the
> > enum defined in portallocator. 
> > 
> > 
> 
> The enum definition doesn't matter too much. What I'm more concerned about is
> the confusing relationship between PortAllocator, BasicPortAllocator,
> MetricsObserverInterface and P2PTransportChannel. Mostly, that
> P2PTransportChannel shouldn't be directly calling a signal of another class,
and
> relying on that signal to increment a stat. An alternative would be to add a
> method such as "RecordIceRegatheringReason", but it's still an odd roundabout
> approach. The PortAllocator becomes responsible for recording metrics for
> another class.
This is what I was doing in the second patch. 

> 
> If the main reason for this design is that it's easier to pass the
> MetricsObserverInterface from PeerConnection->PortAllocator as opposed to
> WebRtcSession->TransportController->P2PTransportChannel, I don't think that's
a
> strong enough reason.
In fact we can pass from PortAllocator to P2PTransportChannel directly. We then
have the following options: 
1. Pass the allocator from WebRTCSession->TransportController->(perhaps also
transport and DtlsTransportChannel)->P2PTransportChannel
2. Then why not the following simpler approach?
 record  allocator->metric_observer in p2ptransportchannel 
3. Or could we just use allocator->metric_observer() every time?
4. Or should we wrap the method of reporting the events by doing something like
allocator->Report...

pthatcher2

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

4 years, 2 months ago (2016-10-04 06:03:16 UTC) #18

pthatcher2

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portallocator.h#newcode83 webrtc/p2p/base/portallocator.h:83: ICE_RESTART_WHEN_DISCONNECTED, Shouldn't these be "_WHILE_" rather than "_WHEN_"? https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portallocator.h#newcode85 ...

4 years, 2 months ago (2016-10-04 06:03:18 UTC) #19

Taylor Brandstetter

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portallocator.h#newcode392 webrtc/p2p/base/portallocator.h:392: } On 2016/10/04 04:06:42, honghaiz3 wrote: > In fact ...

4 years, 2 months ago (2016-10-04 17:44:15 UTC) #20

honghaiz3

Addressed all comments. PTAL. https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransportchannel.cc#newcode483 webrtc/p2p/base/p2ptransportchannel.cc:483: session->SignalIceRegatheringReason(session, reason); On 2016/10/04 02:42:32, ...

4 years, 2 months ago (2016-10-05 05:01:53 UTC) #21

Addressed all comments. PTAL.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
File webrtc/p2p/base/p2ptransportchannel.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/p2ptransp...
webrtc/p2p/base/p2ptransportchannel.cc:483:
session->SignalIceRegatheringReason(session, reason);
On 2016/10/04 02:42:32, Taylor Brandstetter wrote:
> On 2016/10/04 01:03:54, honghaiz3 wrote:
> > On 2016/10/03 20:33:34, Taylor Brandstetter wrote:
> > > Calling another class's signal breaks encapsulation; only the class that
> owns
> > > the signal should emit it. Also, it's not clear that this is expected to
> > > increment a UMA stat.
> > > 
> > > It seems like it would be better if P2PTransportChannel had a pointer to
the
> > > metrics interface directly. Is that possible?
> > 
> > We can, but it would require some wiring of the pointer from peer connection
> all
> > the way to the p2ptransportchannel.
> 
> 
> I think that's the right thing to do. We'll need to do a similar thing to wire
> up the RtcEventLog as well (I assume).
> 
> > An alternative, simpler approach is to report the event to the allocator and
> let
> > the allocator log the metric. This is more in line with other UMA reporting
> > where we passed the event dtls error all the way up to the webrtcsession and
> > webrtcsession wrote the metric. 
> 
> The difference is that in the WebRtcSession case, a parent listened to an
event
> from its child, and forwarded it upwards; a common pattern. Here, a parent is
> creating an event for its child, which the child itself listens to. It's not
> very intuitive.

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:392: }
On 2016/10/04 17:44:15, Taylor Brandstetter wrote:
> On 2016/10/04 04:06:42, honghaiz3 wrote:
> > In fact we can pass from PortAllocator to P2PTransportChannel directly. We
> then
> > have the following options: 
> > 1. Pass the allocator from WebRTCSession->TransportController->(perhaps also
> > transport and DtlsTransportChannel)->P2PTransportChannel
> > 2. Then why not the following simpler approach?
> >  record  allocator->metric_observer in p2ptransportchannel 
> > 3. Or could we just use allocator->metric_observer() every time?
> > 4. Or should we wrap the method of reporting the events by doing something
> like
> > allocator->Report...
> > 
> > 
> 
> I'd still prefer #1, because I don't think it should be PortAllocator's
> responsibility to assist other classes in recording metrics. The approach
won't
> work if we need to record a metric for something that doesn't have a pointer
to
> a PortAllocator.
> 
> I think the approaches that make sense are:
> 
> 1. Bubble events up to WebRtcSession (what we've been doing until now).
> 2. Trickle the metric observer down to the things that need it.
> 
> And I think it makes sense to start taking the "trickle down" approach (like
is
> used for RtcEventLog), because it requires a lot less boilerplate code. And
> although it means more objects have a pointer to a shared object (the metric
> observer), this is fine since it's only used for logging.

Done.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:179:
static_cast<int>(IceRegatheringReason::MAX_VALUE));
On 2016/10/04 02:42:32, Taylor Brandstetter wrote:
> On 2016/10/04 01:03:55, honghaiz3 wrote:
> > On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > > Unless I'm mistaken, pooled BasicPortAllocatorSessions will increment the
> stat
> > > even if they aren't used. Is that intended? It could skew the stats once
> > pooling
> > > becomes available in Chromium.
> > 
> > It is true but only under the circumstances where you created sessions that
> were
> > not used and then network changed or failed. 
> > Should a session created in a pool being used right away or very soon?
> > After second thought, if a session was created in a pool and then a network
> > change happened and an ICE regathering happened, perhaps it should be
counted
> > anyway because an ICE regathering indeed happened, even though it is not
used.
> 
> > So it is probably intended. 
> 
> If we do count regathering that occurs for pooled sessions, I think it needs
to
> increment a different stat than for in-use sessions. Because otherwise, we
can't
> answer a question like: "When regathering occurs for an active connection, how
> often is it due to a network change vs. an ICE restart?"

Do not count the regathering for pooled sessions for now.  We could add that
later.

https://codereview.webrtc.org/2386783002/diff/40001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:526: if (network_manager_started_ &&
!IsStopped())
On 2016/10/04 02:42:32, Taylor Brandstetter wrote:
> On 2016/10/04 01:03:55, honghaiz3 wrote:
> > On 2016/10/03 20:33:35, Taylor Brandstetter wrote:
> > > Same here.
> > This is not a behavior change. 
> > I just moved the check of IsStopped from the DoAllocate to the outside of
the
> > call. 
> > I need this change in this CL because I do not want to signal an
> > ICERegatheringReason when the session has stopped. 
> 
> Acknowledged.

Acknowledged.

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portalloc...
File webrtc/p2p/base/portallocator.h (right):

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:83: ICE_RESTART_WHEN_DISCONNECTED,
On 2016/10/04 06:03:17, pthatcher2 wrote:
> Shouldn't these be "_WHILE_" rather than "_WHEN_"?

renamed class and variables as suggested in other comments.

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:85: CONTINUAL_GATHERING_BY_NETWORK_FAILURE,
On 2016/10/04 06:03:17, pthatcher2 wrote:
> Shouldn't these be "_FOR_" rather than "_BY_"?

Renames this too.

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/base/portalloc...
webrtc/p2p/base/portallocator.h:233: sigslot::signal1<IceRegatheringReason>
SignalIceRegatheringReason;
On 2016/10/04 06:03:17, pthatcher2 wrote:
> Would it make more sense to call this SignalIceRegathering?

Done.

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/client/basicpo...
File webrtc/p2p/client/basicportallocator.cc (right):

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:174: void
BasicPortAllocator::ReportIceRegatheringReason(
On 2016/10/04 06:03:17, pthatcher2 wrote:
> Similarly, I think calling this ReportIceRegathering would make more sense.

This method is removed.

https://codereview.webrtc.org/2386783002/diff/80001/webrtc/p2p/client/basicpo...
webrtc/p2p/client/basicportallocator.cc:341:
IceRegatheringReason::CONTINUAL_GATHERING_BY_NETWORK_FAILURE);
On 2016/10/04 06:03:17, pthatcher2 wrote:
> Actually, this makes it feel like we have two different stats:  ICE restarts
and
> ICE regatherings, which are not the same thing.  The ICE restarts can be
> recorded by the IceTransport (P2pTransportChannel) and the ICE regatherings
can
> be recorded by the IceGatherer (PortAllocator).
> 2 different stats for 2 different things.
> 
> I think that would make the stats more clear and keep the relationship between
> the IceTransport and IceGatherer more clear, which would address Taylor's
> concern.

Done.

Taylor Brandstetter

lgtm. I know it's more boilerplate code but I like how this turned out. https://codereview.webrtc.org/2386783002/diff/140001/webrtc/p2p/base/portallocator.h ...

4 years, 2 months ago (2016-10-05 17:40:02 UTC) #22

honghaiz3

Thanks! https://codereview.webrtc.org/2386783002/diff/140001/webrtc/p2p/base/portallocator.h File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2386783002/diff/140001/webrtc/p2p/base/portallocator.h#newcode398 webrtc/p2p/base/portallocator.h:398: std::deque<std::unique_ptr<PortAllocatorSession>>& pooled_sessions() { On 2016/10/05 17:40:02, Taylor Brandstetter ...

4 years, 2 months ago (2016-10-05 18:04:38 UTC) #23

honghaiz3

The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the ...

4 years, 2 months ago (2016-10-05 18:05:17 UTC) #25

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2386783002/160001

4 years, 2 months ago (2016-10-05 18:05:25 UTC) #26

honghaiz3

Description was changed from ========== Add UMA metrics for ICE regathering reasons. BUG=webrtc:6462 ========== to ...

4 years, 2 months ago (2016-10-05 18:47:44 UTC) #27

honghaiz3

4 years, 2 months ago (2016-10-05 18:47:45 UTC) #28

Message was sent while issue was closed.

Committed patchset #6 (id:160001) manually as
d93f50cd5754ae20a97c8ed1d0469b912c0b05af (presubmit successful).

commit-bot: I haz the power

4 years, 2 months ago (2016-10-05 18:47:47 UTC) #29

Message was sent while issue was closed.

Description was changed from

==========
Add UMA metrics for ICE regathering reasons.

BUG=webrtc:6462
R=deadbeef@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/d93f50cd5754ae20a97c8ed1d...
==========

to

==========
Add UMA metrics for ICE regathering reasons.

BUG=webrtc:6462
R=deadbeef@webrtc.org

Committed: https://crrev.com/d93f50cd5754ae20a97c8ed1d0469b912c0b05af
Cr-Commit-Position: refs/heads/master@{#14531}
==========

Issue 2386783002: Add UMA metrics for ICE regathering reasons. (Closed)

Description

Patch Set 1 : . #

Patch Set 2 : Address comments #

Patch Set 3 : Pass the metric observer from transportcontroller to the transport channel and etc #

Patch Set 4 : revert some unwanted auto-formatting. #

Patch Set 5 : fix comments #

Patch Set 6 : Add const to the pooled_sessions method. #

Messages