Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291533004:
Wiring Libjingle network related metrics into Chrome (Closed)
|
Created:
6 years, 7 months ago by Mallinath (Gone from Chromium) Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWiring Libjingle network related metrics into Chrome.
R=perkj@chromium.org,asvitkine@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272281
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 12
Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 16 (0 generated)
You also need to modify tools/metrics/histograms/histograms.xml. I suggest you do that in the same cl and asks for a review of it all. https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:319: class NativePCUMAObserver : public webrtc::UMAObserver { Name PeerConnectionUMAObserver? https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.h (right): https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.h:211: talk_base::scoped_refptr<webrtc::UMAObserver> uma_observer_; Dont use libingle class talk_base::scoped_reftpr. Instead use CHromes version. scoped_refptr. If needed - use uma_observer_.get() to create the Chomes scoped_refptr.
I have merged histograms.xml changes into this CL. For some reason I was thinking it was better to keep them in a separate CLs. https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:319: class NativePCUMAObserver : public webrtc::UMAObserver { On 2014/05/19 10:02:04, perkj wrote: > Name PeerConnectionUMAObserver? Done. https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.h (right): https://codereview.chromium.org/291533004/diff/40001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.h:211: talk_base::scoped_refptr<webrtc::UMAObserver> uma_observer_; On 2014/05/19 10:02:04, perkj wrote: > Dont use libingle class talk_base::scoped_reftpr. Instead use CHromes version. > scoped_refptr. If needed - use uma_observer_.get() to create the Chomes > scoped_refptr. Done.
https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:325: webrtc::PeerConnectionUMAMetricsCounter counter) { Nit: OVERRIDE https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:331: webrtc::PeerConnectionUMAMetricsName type, int value) { Nit: OVERRIDE and add an empty line before this. https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:336: 0, 100*1000, 50); // Max 100 seconds. Is there a reason you chose a custom definition here instead of using e.g. UMA_HISTOGRAM_MEDIUM_TIMES()? https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:341: 0, 100, 50); // Max 100 interfaces. Can this be UMA_HISTOGRAM_COUNTS_100()? Same below. https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:403: if (uma_observer_) { Nit: No {}'s https://codereview.chromium.org/291533004/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/291533004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31131: + Counters on IPv4 and IPv6 usage in PeerConnection. These values logged once Nit: "are logged" https://codereview.chromium.org/291533004/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:31139: + Number of IPv4 network interfaces discovered in a PeerConnection Session. Mention that this is logged once per PeerConnection? (Or if not, how often/when?) Same for below.
PTAL https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:325: webrtc::PeerConnectionUMAMetricsCounter counter) { On 2014/05/20 06:40:20, Alexei Svitkine wrote: > Nit: OVERRIDE Done. https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:331: webrtc::PeerConnectionUMAMetricsName type, int value) { On 2014/05/20 06:40:20, Alexei Svitkine wrote: > Nit: OVERRIDE and add an empty line before this. Done. https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:336: 0, 100*1000, 50); // Max 100 seconds. No reason, fact that I didn't knew such macro existed. On 2014/05/20 06:40:20, Alexei Svitkine wrote: > Is there a reason you chose a custom definition here instead of using e.g. > UMA_HISTOGRAM_MEDIUM_TIMES()? https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:341: 0, 100, 50); // Max 100 interfaces. On 2014/05/20 06:40:20, Alexei Svitkine wrote: > Can this be UMA_HISTOGRAM_COUNTS_100()? Same below. Done. https://codereview.chromium.org/291533004/diff/60001/content/renderer/media/r... content/renderer/media/rtc_peer_connection_handler.cc:403: if (uma_observer_) { On 2014/05/20 06:40:20, Alexei Svitkine wrote: > Nit: No {}'s Done.
lgtm with nits https://codereview.chromium.org/291533004/diff/100001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/100001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:402: if (uma_observer_) this can not fail. no need to check. https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31173: + Counters on IPv4 and IPv6 usage in PeerConnection. These values logged once values are logged https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39877: +<enum name="PeerConnectionCounters" type="int"> Should this be moved to just after <histogram name="WebRTC.PeerConnectionIPMetrics" enum="PeerConnectionCounters"> + <owner>mallinath@chromium.org</owner> ? https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39879: + <int value="1" label="PeerConnection enabled with Ipv6."/> What is the difference between these values? IPv4 BestConnection and PeerConnection enabled with IPv4 ? Can one session have both IPV4 and IpV6 enabled but only one best connection?
https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31170: +<histogram name="WebRTC.PeerConnectionIPMetrics" enum="PeerConnectionCounters"> Nit: I suggest to rename these histograms to WebRTC.PeerConnection.IPMetrics, WebRTC.PeerConnection.TimeToConnect and so forth. This will let you bring up all the related metrics easily on the UMA dashboard. https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39877: +<enum name="PeerConnectionCounters" type="int"> On 2014/05/22 07:38:58, perkj wrote: > Should this be moved to just after <histogram > name="WebRTC.PeerConnectionIPMetrics" enum="PeerConnectionCounters"> > + <mailto:owner>mallinath@chromium.org</owner> ? The ordering of entries in this file is enforced by the presubmits.
PTAL https://codereview.chromium.org/291533004/diff/100001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/100001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:402: if (uma_observer_) On 2014/05/22 07:38:58, perkj wrote: > this can not fail. no need to check. Done. https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31170: +<histogram name="WebRTC.PeerConnectionIPMetrics" enum="PeerConnectionCounters"> That's good point. On 2014/05/22 08:35:35, Alexei Svitkine wrote: > Nit: I suggest to rename these histograms to WebRTC.PeerConnection.IPMetrics, > WebRTC.PeerConnection.TimeToConnect and so forth. This will let you bring up all > the related metrics easily on the UMA dashboard. https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:31173: + Counters on IPv4 and IPv6 usage in PeerConnection. These values logged once On 2014/05/22 07:38:58, perkj wrote: > values are logged Done. https://codereview.chromium.org/291533004/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39879: + <int value="1" label="PeerConnection enabled with Ipv6."/> Yes, that's what will happen. When IPv6 is enabled, IPv4 will also be enabled. So we want to know, in how many cases we had Ipv4 best connection and visa versa. On 2014/05/22 07:38:58, perkj wrote: > What is the difference between these values? IPv4 BestConnection and > PeerConnection enabled with IPv4 ? Can one session have both IPV4 and IpV6 > enabled but only one best connection? >
https://codereview.chromium.org/291533004/diff/120001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/120001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:326: UMA_HISTOGRAM_ENUMERATION("WebRTC.PeerConnectionIPMetrics", Update the names here too (i.e. WebRTC.PeerConnectionIPMetrics -> WebRTC.PeerConnection.IPMetrics and same for others below).
https://codereview.chromium.org/291533004/diff/120001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/291533004/diff/120001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:326: UMA_HISTOGRAM_ENUMERATION("WebRTC.PeerConnectionIPMetrics", On 2014/05/22 15:50:42, Alexei Svitkine wrote: > Update the names here too (i.e. WebRTC.PeerConnectionIPMetrics -> > WebRTC.PeerConnection.IPMetrics and same for others below). oops, done now.
LGTM
The CQ bit was checked by mallinath@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mallinath@chromium.org/291533004/140001
The CQ bit was checked by mallinath@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mallinath@chromium.org/291533004/160001
Message was sent while issue was closed.
Change committed as 272281 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
