Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2324853002:
Adding logs to track potential cause of not starting port allocation.
DescriptionAdding logs to track potential cause of not starting port allocation.
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/9ecb08576ed43f374b6b9ee7209ba229004b7eaf
Cr-Commit-Position: refs/heads/master@{#14244}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Log set local description as verbose in transportcontroller and log SetICEParameters in p2ptranspor… #
Total comments: 4
Patch Set 3 : log remote transport description and use INFO level #Patch Set 4 : Remove the ufrag pwd logging in transportcontroller because they are logged in p2ptransportchannel #Messages
Total messages: 22 (7 generated)
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/2324853002/diff/1/webrtc/p2p/base/transportcont... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2324853002/diff/1/webrtc/p2p/base/transportcont... webrtc/p2p/base/transportcontroller.cc:460: if (transport_name == CN_AUDIO) { We don't always bundle on audio, and this would log an error for every single PeerConnection that uses only data channels. You might be better off logging all transports that are in the TransportController after every call to PeerConnecction::SetLocalDescription.
https://codereview.webrtc.org/2324853002/diff/1/webrtc/p2p/base/transportcont... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2324853002/diff/1/webrtc/p2p/base/transportcont... webrtc/p2p/base/transportcontroller.cc:460: if (transport_name == CN_AUDIO) { On 2016/09/08 17:55:27, pthatcher1 wrote: > We don't always bundle on audio, and this would log an error for every single > PeerConnection that uses only data channels. > > You might be better off logging all transports that are in the > TransportController after every call to PeerConnecction::SetLocalDescription. If we are using data-channel only, then the transport should not have been deleted due to bundle (so transport should not be nullptr), right?
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
We don't intend to land this CL, right? Isn't it only intended to be used for debugging an issue?
On 2016/09/08 23:58:24, Taylor Brandstetter wrote: > We don't intend to land this CL, right? Isn't it only intended to be used for > debugging an issue? Actually, the P2PTransportChannel log seems generally useful. But the TransportController log would produce false positive error logs in some cases.
On 2016/09/09 00:06:20, Taylor Brandstetter wrote: > On 2016/09/08 23:58:24, Taylor Brandstetter wrote: > > We don't intend to land this CL, right? Isn't it only intended to be used for > > debugging an issue? > > Actually, the P2PTransportChannel log seems generally useful. But the > TransportController log would produce false positive error logs in some cases. OK. I guess if there is only video and data, it will be bundled on video or data. Made change to log set local transport description if transport is found. Also log SetIceParameters in p2ptransportchannels
On 2016/09/09 17:11:21, honghaiz3 wrote: > On 2016/09/09 00:06:20, Taylor Brandstetter wrote: > > On 2016/09/08 23:58:24, Taylor Brandstetter wrote: > > > We don't intend to land this CL, right? Isn't it only intended to be used > for > > > debugging an issue? > > > > Actually, the P2PTransportChannel log seems generally useful. But the > > TransportController log would produce false positive error logs in some cases. > > OK. I guess if there is only video and data, it will be bundled on video or > data. > > Made change to log set local transport description if transport is found. > Also log SetIceParameters in p2ptransportchannels Are you OK with the change? We can also make the logging in transportcontroller to be INFO or remove that logging if you prefer.
lgtm https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:315: << " on transport "<< transport_name(); nit: Missing whitespace, can probably do git cl format
https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:498: nit: If we log setting the local transport description, we may want to log the remote as well for symmetry. Someone may be confused when looking at the log and seeing one but not the other.
I also changed the log level in transportcontroller to INFO to log it in a normal chrome logging. transport description is only set a few times anyway. https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel.cc:315: << " on transport "<< transport_name(); On 2016/09/14 20:39:04, Taylor Brandstetter wrote: > nit: Missing whitespace, can probably do git cl format Done. https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2324853002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:498: On 2016/09/14 20:40:27, Taylor Brandstetter wrote: > nit: If we log setting the local transport description, we may want to log the > remote as well for symmetry. Someone may be confused when looking at the log and > seeing one but not the other. Done.
removed duplicate logging ufrag/pwd
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2324853002/#ps60001 (title: "Remove the ufrag pwd logging in transportcontroller because they are logged in p2ptransportchannel")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Adding logs to track potential cause of not starting port allocation. ========== to ========== Adding logs to track potential cause of not starting port allocation. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9ecb08576ed43f374b6b9ee72... ==========
Description was changed from ========== Adding logs to track potential cause of not starting port allocation. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/9ecb08576ed43f374b6b9ee72... ========== to ========== Adding logs to track potential cause of not starting port allocation. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/9ecb08576ed43f374b6b9ee7209ba229004b7eaf Cr-Commit-Position: refs/heads/master@{#14244} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 9ecb08576ed43f374b6b9ee7209ba229004b7eaf (presubmit successful).
Patchset 4 (id:??) landed as https://crrev.com/9ecb08576ed43f374b6b9ee7209ba229004b7eaf Cr-Commit-Position: refs/heads/master@{#14244} |
