Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1274213006:
Add instrumentation to track the IceCandidatePairType and IceConnectionState (Closed)
|
Created:
5 years, 4 months ago by guoweis_left_chromium Modified:
5 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd instrumentation to track the IceCandidatePairType and IceConnectionState.
Based on onging CL https://codereview.webrtc.org/1277263002
BUG=webrtc:4918
Committed: https://crrev.com/bb99c4d9b16b69f9c504e292105b5cd60e907687
Cr-Commit-Position: refs/heads/master@{#344617}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 10
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 16
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 28 (11 generated)
Patchset #2 (id:20001) has been deleted
guoweis@chromium.org changed reviewers: + perkj@chromium.org, pthatcher@chromium.org, rkaplow@chromium.org
rkaplow@chromium.org: Please review changes in histograms.xml perkj@chromium.org: Please review changes in rtc_peer_connection_handler.*
Patchset #3 (id:60001) has been deleted
If rkaplow is OOO until Aug 24 and perkj is OOO until September, who is going to review these UMA CLs? I have one too! https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:512: case webrtc::kEnumCounterAddressFamily: Would it have made more sense to pass in a string to IncrementEnumCounter than an enum? https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1386: ResetUMAStats(); This makes it so that we get a UMA for ICE every state for every ICE restart. Is that what we want? https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1543: if (ice_state_tracking_[new_state]) I'd rename ice_state_tracking_ to ice_state_seen_. https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50672: + first BestConnection of a PeerConnection. BestConnection => selected candidate pair https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50681: + PeerConnection gets into that state for the first time. Or after an ICE restart? Whichever we decide, we should make the comment match the measurement.
guoweis@chromium.org changed reviewers: - perkj@chromium.org, rkaplow@chromium.org
guoweis@chromium.org changed reviewers: + asvitkine@chromium.org, tommi@chromium.org
Peter, PTAL. asvitkine@chromium.org: Please review changes in histograms.xml tommi@chromium.org: Please review changes in rtc_peer_connection_handler.* https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:512: case webrtc::kEnumCounterAddressFamily: On 2015/08/19 02:59:06, pthatcher2 wrote: > Would it have made more sense to pass in a string to IncrementEnumCounter than > an enum? Can you elaborate? https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1386: ResetUMAStats(); On 2015/08/19 02:59:06, pthatcher2 wrote: > This makes it so that we get a UMA for ICE every state for every ICE restart. > Is that what we want? I think we should also track the stat after restart. https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:1543: if (ice_state_tracking_[new_state]) On 2015/08/19 02:59:06, pthatcher2 wrote: > I'd rename ice_state_tracking_ to ice_state_seen_. Done. https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50672: + first BestConnection of a PeerConnection. On 2015/08/19 02:59:06, pthatcher2 wrote: > BestConnection => selected candidate pair Done. https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50681: + PeerConnection gets into that state for the first time. On 2015/08/19 02:59:06, pthatcher2 wrote: > Or after an ICE restart? Whichever we decide, we should make the comment match > the measurement. Done.
guoweis@chromium.org changed reviewers: - asvitkine@chromium.org
guoweis@chromium.org changed reviewers: + isherman@chromium.org
isherman@chromium.org: Please review changes in histograms.xml
histograms lgtm https://codereview.chromium.org/1274213006/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:524: NOTREACHED(); nit: It's best practice to leave off the default case, and instead enumerate all possible cases. That way, the compiler can actually catch forgotten cases.
Agree. Changed. https://codereview.chromium.org/1274213006/diff/120001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/120001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:524: NOTREACHED(); On 2015/08/19 18:50:18, Ilya Sherman wrote: > nit: It's best practice to leave off the default case, and instead enumerate all > possible cases. That way, the compiler can actually catch forgotten cases. Done.
lgtm
https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:514: counter_max); missing break? https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:523: case webrtc::kPeerConnectionEnumCounterMax: default: too? https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:743: ResetUMAStats(); might not need this if the array is initialized. I'd prefer that actually. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1332: ReportICEState(new_state); move below the thread check https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1542: void RTCPeerConnectionHandler::ReportICEState( add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1551: void RTCPeerConnectionHandler::ResetUMAStats() { add DCHECK(thread_checker_.CalledOnValidThread()); https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.h (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.h:260: bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax]; What about initializing here: bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax] = {};
PTAL. Thanks for catching the missing of break. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:514: counter_max); On 2015/08/19 19:29:25, tommi wrote: > missing break? Thanks for catching this. Since this is based on an ongoing CL from webrtc and I need to get them both into M46, I took shortcut to start this one and didn't always try to compile this whenever the other one is updated. Won't check this in until the WebRTC CL is rolled. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:523: case webrtc::kPeerConnectionEnumCounterMax: On 2015/08/19 19:29:26, tommi wrote: > default: too? I purposedly removed the default so all enums will be listed here and if anyone is missing in the future, it's a compiler error. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:743: ResetUMAStats(); On 2015/08/19 19:29:25, tommi wrote: > might not need this if the array is initialized. I'd prefer that actually. Done. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1332: ReportICEState(new_state); On 2015/08/19 19:29:26, tommi wrote: > move below the thread check Done. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1542: void RTCPeerConnectionHandler::ReportICEState( On 2015/08/19 19:29:26, tommi wrote: > add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:1551: void RTCPeerConnectionHandler::ResetUMAStats() { On 2015/08/19 19:29:26, tommi wrote: > add DCHECK(thread_checker_.CalledOnValidThread()); Done. https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.h (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.h:260: bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax]; On 2015/08/19 19:29:26, tommi wrote: > What about initializing here: > > bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax] = {}; Done. However, what is the best practice here? Should we start to use c+11 initialization even if anything else is not using that?
lgtm. I'll leave it up to you if you want to update the other variables https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.cc:523: case webrtc::kPeerConnectionEnumCounterMax: On 2015/08/19 19:37:29, guoweis_chromium wrote: > On 2015/08/19 19:29:26, tommi wrote: > > default: too? > > I purposedly removed the default so all enums will be listed here and if anyone > is missing in the future, it's a compiler error. sgtm https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... File content/renderer/media/rtc_peer_connection_handler.h (right): https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... content/renderer/media/rtc_peer_connection_handler.h:260: bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax]; On 2015/08/19 19:37:29, guoweis_chromium wrote: > On 2015/08/19 19:29:26, tommi wrote: > > What about initializing here: > > > > bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax] = {}; > > Done. > > However, what is the best practice here? Should we start to use c+11 > initialization even if anything else is not using that? Hmm... there aren't that many variables that need explicit initialization. Do you mind moving them to in-class? Alternatively I can do it after you land this cl.
The CQ bit was checked by guoweis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, pthatcher@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1274213006/#ps180001 (title: " ")
On 2015/08/19 19:46:18, tommi wrote: > lgtm. I'll leave it up to you if you want to update the other variables > > https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... > content/renderer/media/rtc_peer_connection_handler.cc:523: case > webrtc::kPeerConnectionEnumCounterMax: > On 2015/08/19 19:37:29, guoweis_chromium wrote: > > On 2015/08/19 19:29:26, tommi wrote: > > > default: too? > > > > I purposedly removed the default so all enums will be listed here and if > anyone > > is missing in the future, it's a compiler error. > > sgtm > > https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... > File content/renderer/media/rtc_peer_connection_handler.h (right): > > https://codereview.chromium.org/1274213006/diff/140001/content/renderer/media... > content/renderer/media/rtc_peer_connection_handler.h:260: bool > ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax]; > On 2015/08/19 19:37:29, guoweis_chromium wrote: > > On 2015/08/19 19:29:26, tommi wrote: > > > What about initializing here: > > > > > > bool ice_state_seen_[webrtc::PeerConnectionInterface::kIceConnectionMax] = > {}; > > > > Done. > > > > However, what is the best practice here? Should we start to use c+11 > > initialization even if anything else is not using that? > > Hmm... there aren't that many variables that need explicit initialization. Do > you mind moving them to in-class? Alternatively I can do it after you land this > cl. I moved others into in-class initialization as well.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274213006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274213006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by guoweis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1274213006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1274213006/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bb99c4d9b16b69f9c504e292105b5cd60e907687 Cr-Commit-Position: refs/heads/master@{#344617}
Message was sent while issue was closed.
pthatcher@ I'm around now if you're looking for a reviewer. On Tue, Aug 18, 2015 at 10:59 PM, <pthatcher@chromium.org> wrote: > If rkaplow is OOO until Aug 24 and perkj is OOO until September, who is > going to > review these UMA CLs? I have one too! > > > https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... > content/renderer/media/rtc_peer_connection_handler.cc:512: case > webrtc::kEnumCounterAddressFamily: > Would it have made more sense to pass in a string to > IncrementEnumCounter than an enum? > > https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... > content/renderer/media/rtc_peer_connection_handler.cc:1386: > ResetUMAStats(); > This makes it so that we get a UMA for ICE every state for every ICE > restart. Is that what we want? > > https://codereview.chromium.org/1274213006/diff/80001/content/renderer/media/... > content/renderer/media/rtc_peer_connection_handler.cc:1543: if > (ice_state_tracking_[new_state]) > I'd rename ice_state_tracking_ to ice_state_seen_. > > https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:50672: + first BestConnection > of a PeerConnection. > BestConnection => selected candidate pair > > https://codereview.chromium.org/1274213006/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:50681: + PeerConnection gets > into that state for the first time. > Or after an ICE restart? Whichever we decide, we should make the > comment match the measurement. > > https://codereview.chromium.org/1274213006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
