Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2804113004:
Add UMA to break down data usage by http/https and video/non_video (Closed)
DescriptionAdd UMA to break down data usage by http/https and video/non_video
BUG=689659
Review-Url: https://codereview.chromium.org/2804113004
Cr-Commit-Position: refs/heads/master@{#464465}
Committed: https://chromium.googlesource.com/chromium/src/+/049f4387ba7724fa3c5ac1117a438fabb504f563
Patch Set 1 #
Total comments: 4
Patch Set 2 : format and add to histograms.xml #Patch Set 3 : Add tests #Patch Set 4 : gn changes were sticking around #Patch Set 5 : rename incorrect local variable #Patch Set 6 : rename incorrect local variable #
Total comments: 8
Patch Set 7 : formatting #
Total comments: 5
Patch Set 8 : address review comments #Patch Set 9 : fix erroneous UMA in unit test #
Total comments: 11
Patch Set 10 : review comments #
Total comments: 1
Patch Set 11 : rename args #Patch Set 12 : rebase #
Messages
Total messages: 37 (18 generated)
Description was changed from ========== Add UMA to break down data usage by http/https and video/non_video BUG= ========== to ========== Add UMA to break down data usage by http/https and video/non_video BUG= ==========
ajo@chromium.org changed reviewers: + bengr@chromium.org, sclittle@chromium.org
The basic approach here looks good, I'll review it more thoroughly once you've added the tests. https://codereview.chromium.org/2804113004/diff/1/.gn File .gn (left): https://codereview.chromium.org/2804113004/diff/1/.gn#oldcode44 .gn:44: v8_enable_inspector = true Should this change be in this CL? https://codereview.chromium.org/2804113004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:84: UMA_HISTOGRAM_COUNTS("Net.HttpContentLength.Https", received_content_length); style nit: 80 column limit, you can just run 'git cl format' to auto format stuff like this though.
https://codereview.chromium.org/2804113004/diff/1/.gn File .gn (left): https://codereview.chromium.org/2804113004/diff/1/.gn#oldcode44 .gn:44: v8_enable_inspector = true On 2017/04/07 20:07:34, sclittle wrote: > Should this change be in this CL? No, removed. This change actually got replicated in HEAD. https://codereview.chromium.org/2804113004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:84: UMA_HISTOGRAM_COUNTS("Net.HttpContentLength.Https", received_content_length); On 2017/04/07 20:07:34, sclittle wrote: > style nit: 80 column limit, you can just run 'git cl format' to auto format > stuff like this though. Done.
The CQ bit was checked by ajo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ajo@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:428: bool is_video_request = false; nit: rename as is_video_response, because that's what you're checking. https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:432: is_video_request = net::MatchesMimeType("video/*", mime_type); Hmm. Does video always have the proper MIME type? https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:198: DCHECK(!(use_secure_proxy && bypass_proxy)); This seems busted. Seems like there are really three states: use secure proxy, use insecure proxy, or bypass proxy. Ideally there'd also be a DCHECK that's conceptually !(use_insecure_proxy && bypass_proxy) and another one that's !(use_secure_proxy && use_insecure_proxy)
PTAL https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:428: bool is_video_request = false; On 2017/04/10 20:48:12, bengr wrote: > nit: rename as is_video_response, because that's what you're checking. Done. https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:432: is_video_request = net::MatchesMimeType("video/*", mime_type); On 2017/04/10 20:48:12, bengr wrote: > Hmm. Does video always have the proper MIME type? I believe so. I found another example of this mime type check in the codebase, and https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats suggests that browsers won't play video unless it is served with the proper mime type. https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:198: DCHECK(!(use_secure_proxy && bypass_proxy)); On 2017/04/10 20:48:12, bengr wrote: > This seems busted. Seems like there are really three states: use secure proxy, > use insecure proxy, or bypass proxy. Ideally there'd also be a DCHECK that's > conceptually !(use_insecure_proxy && bypass_proxy) and another one that's > !(use_secure_proxy && use_insecure_proxy) Changed to an enum to avoid wrangling with complex DCHECKs
https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:82: UMA_HISTOGRAM_COUNTS("Net.HttpContentLength.Https", UMA_HISTOGRAM_COUNTS is deprecated and we prefer new code to use the macros which has a max-value as a suffix. UMA_HISTOGRAM_COUNTS is equivalent to UMA_HISTOGRAM_COUNTS_1M. Please use that version and change the other in this file at the same time. (You can also decide if a different max is more appropriate - but if you change max for existing macros, you'll need to rename them - so it might be fine to keep using _1M max throughout these measurements.) https://codereview.chromium.org/2804113004/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2804113004/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34309: +<histogram name="Net.HttpContentLength.Https" units="bytes"> Please use histogram_suffixes construct for these to avoid repetition. See the top of the file for details on how to.
https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:432: is_video_request = net::MatchesMimeType("video/*", mime_type); On 2017/04/10 21:54:45, ajo1 wrote: > On 2017/04/10 20:48:12, bengr wrote: > > Hmm. Does video always have the proper MIME type? > > I believe so. I found another example of this mime type check in the codebase, > and https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats > suggests that browsers won't play video unless it is served with the proper mime > type. Please verify with bolian@ or tombergan@ https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:197: void Init(ProxyConfig proxy_config, bool enable_brotli_globally) { ProxyConfig is already a concept in Chromium. I'd rename, e.g., ProxyTestConfig
https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/100001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:432: is_video_request = net::MatchesMimeType("video/*", mime_type); On 2017/04/11 18:51:43, bengr wrote: > On 2017/04/10 21:54:45, ajo1 wrote: > > On 2017/04/10 20:48:12, bengr wrote: > > > Hmm. Does video always have the proper MIME type? > > > > I believe so. I found another example of this mime type check in the codebase, > > and https://developer.mozilla.org/en-US/docs/Web/HTML/Supported_media_formats > > suggests that browsers won't play video unless it is served with the proper > mime > > type. > > Please verify with bolian@ or tombergan@ Verified. Some requests are served with incorrect mime types, but there's no way to tell without trying to parse them and it's a negligible portion of the total video requests. https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:82: UMA_HISTOGRAM_COUNTS("Net.HttpContentLength.Https", On 2017/04/11 18:44:12, Alexei Svitkine (very slow) wrote: > UMA_HISTOGRAM_COUNTS is deprecated and we prefer new code to use the macros > which has a max-value as a suffix. UMA_HISTOGRAM_COUNTS is equivalent to > UMA_HISTOGRAM_COUNTS_1M. > > Please use that version and change the other in this file at the same time. > > (You can also decide if a different max is more appropriate - but if you change > max for existing macros, you'll need to rename them - so it might be fine to > keep using _1M max throughout these measurements.) Done. https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:197: void Init(ProxyConfig proxy_config, bool enable_brotli_globally) { On 2017/04/11 18:51:43, bengr wrote: > ProxyConfig is already a concept in Chromium. I'd rename, e.g., ProxyTestConfig Done.
https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:471: request.response_headers()->GetMimeType(&mime_type); nit: GetMimeType() returns a bool, you could reorganize this as: is_video_response = request.response_headers()->GetMimeType(&mime_type) && net::MatchesMimeType("video/*", mime_type); https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:200: case BYPASS_PROXY: nit: instead of switching on an enum here, have you considered just passing in the net::ProxyServer here, and having net::ProxyServer constants for the secure and insecure kinds of proxies? e.g., void Init(const net::ProxyServer& proxy_server, bool enable_brotli_globally) { // ... initializes using |proxy_server| ... } You could handle the BYPASS_PROXY check below by checking whether the |proxy_server| is direct. WDYT? https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:985: "Net.HttpContentLength_Https"; nit: Assuming you split this test up (see below), these string constants seem unnecessary, could you inline these strings instead to make it easier to see which histogram checks refer to which histograms? https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1049: std::string video_response_headers = Could you split this video request into it's own test? https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1067: const struct { nit: While you're at it, could you split these LoFi tests into their own test? https://codereview.chromium.org/2804113004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2804113004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:121586: +<histogram_suffixes name="NetHttpContentLengthType"> nit: Did you mean to set the separator to "." like the below suffixes block?
Description was changed from ========== Add UMA to break down data usage by http/https and video/non_video BUG= ========== to ========== Add UMA to break down data usage by http/https and video/non_video BUG=689659 ==========
https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:471: request.response_headers()->GetMimeType(&mime_type); On 2017/04/11 20:36:11, sclittle wrote: > nit: GetMimeType() returns a bool, you could reorganize this as: > > is_video_response = request.response_headers()->GetMimeType(&mime_type) && > net::MatchesMimeType("video/*", mime_type); Used the boolean, but organized it in a way that feels more readable to me. PTAL. https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:200: case BYPASS_PROXY: On 2017/04/11 20:36:11, sclittle wrote: > nit: instead of switching on an enum here, have you considered just passing in > the net::ProxyServer here, and having net::ProxyServer constants for the secure > and insecure kinds of proxies? > > e.g., > > void Init(const net::ProxyServer& proxy_server, bool enable_brotli_globally) { > // ... initializes using |proxy_server| ... > } > > You could handle the BYPASS_PROXY check below by checking whether the > |proxy_server| is direct. > > WDYT? I like this approach better, since I'd want to create a new proxy on each test invocation, which makes having net::ProxyServer constants trickier(i.e. I'd be replace USE_INSECURE_PROXY with GetInsecureProxy()) https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1049: std::string video_response_headers = On 2017/04/11 20:36:11, sclittle wrote: > Could you split this video request into it's own test? Done. https://codereview.chromium.org/2804113004/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1067: const struct { On 2017/04/11 20:36:11, sclittle wrote: > nit: While you're at it, could you split these LoFi tests into their own test? I don't want to mix that into this change if I can avoid it. https://codereview.chromium.org/2804113004/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2804113004/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:121586: +<histogram_suffixes name="NetHttpContentLengthType"> On 2017/04/11 20:36:11, sclittle wrote: > nit: Did you mean to set the separator to "." like the below suffixes block? I didn't realize you could do that! Changed to '.'
lgtm
lgtm with nit. https://codereview.chromium.org/2804113004/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2804113004/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:480: is_https_request, is_video_response, optional nit: The prototype for RecordContentLengthHistograms uses the names 'is_https_request' and 'is_video', which made me wonder if we should change the names to be more consistent. Maybe we should get rid of '_request' and '_response', and have: RecordContentLengthHistograms(is_https, is_video, ...) I also tried to come up with a method name that includes "Request" to make it more obvious that this method recorded at the request level, but I couldn't come up with a name that was obviously better, e.g., ReportRequestDataUse.
sclittle@chromium.org changed reviewers: - sclittle@chromium.org
deferring review to bengr
The CQ bit was checked by ajo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by ajo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2804113004/#ps200001 (title: "rename args")
The CQ bit was checked by ajo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ajo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2804113004/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492102126839450,
"parent_rev": "50b1bcc54ad5763ef3e17f3710b1b46aee5c9cfb", "commit_rev":
"049f4387ba7724fa3c5ac1117a438fabb504f563"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA to break down data usage by http/https and video/non_video BUG=689659 ========== to ========== Add UMA to break down data usage by http/https and video/non_video BUG=689659 Review-Url: https://codereview.chromium.org/2804113004 Cr-Commit-Position: refs/heads/master@{#464465} Committed: https://chromium.googlesource.com/chromium/src/+/049f4387ba7724fa3c5ac1117a43... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/049f4387ba7724fa3c5ac1117a43... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
