Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888653003:
increase bucket size and break down video by http/https (Closed)
Descriptionincrease bucket size and break down video by http/https
Effected existing histograms:
Net.HttpContentLength.Http
Net.HttpContentLength.Https
Net.HttpContentLength.Video
Net.HttpContentLength.Http.Video
Net.HttpContentLength.Https.Video
New Histograms:
Net.HttpContentLength.Http.Direct
Net.HttpContentLength.Http.ViaDRP
Net.HttpContentLength.Http.BypassedDRP
Net.HttpContentLength.Http.Other
Net.HttpContentLength.Https.Direct
Net.HttpContentLength.Https.ViaDRP
Net.HttpContentLength.Https.BypassedDRP
Net.HttpContentLength.Https.Other
Net.HttpContentLength.Http.Direct.Video
Net.HttpContentLength.Http.ViaDRP.Video
Net.HttpContentLength.Http.BypassedDRP.Video
Net.HttpContentLength.Http.Other.Video
Net.HttpContentLength.Https.Direct.Video
Net.HttpContentLength.Https.ViaDRP.Video
Net.HttpContentLength.Https.BypassedDRP.Video
Net.HttpContentLength.Https.Other.Video
BUG=689659
Review-Url: https://codereview.chromium.org/2888653003
Cr-Commit-Position: refs/heads/master@{#474922}
Committed: https://chromium.googlesource.com/chromium/src/+/301b00f0c3b91119545da8f1762dcba0f665697e
Patch Set 1 #Patch Set 2 : Add video UMA #Patch Set 3 : fix IIFE #Patch Set 4 : update UMA #Patch Set 5 : cleanup #Patch Set 6 : Revision to tests #
Total comments: 4
Patch Set 7 : formatting #
Total comments: 19
Patch Set 8 : formatting #Patch Set 9 : more formatting #Patch Set 10 : Rebase for trybots #
Total comments: 16
Patch Set 11 : review comments #Patch Set 12 : update histograms.xml #Patch Set 13 : Final formatting fixes #
Total comments: 14
Patch Set 14 : review comments #
Total comments: 15
Patch Set 15 : address some missed comments #Patch Set 16 : address comments #Patch Set 17 : Rebase #
Total comments: 2
Patch Set 18 : Added suffix labels #
Total comments: 2
Patch Set 19 : consistent Proxy capitalization #Patch Set 20 : Address windows build breakage #Patch Set 21 : Better approach to initializing the suffix. #Patch Set 22 : Address trybot failure #
Messages
Total messages: 70 (45 generated)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 to run a CQ dry run
Description was changed from ========== increase bucket size and break down video by http/https Effected histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video BUG=689659 ========== to ========== increase bucket size and break down video by http/https Effected histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video BUG=689659 ==========
ajo@chromium.org changed reviewers: + ryansturm@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== increase bucket size and break down video by http/https Effected histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video BUG=689659 ========== to ========== increase bucket size and break down video by http/https Effected histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video BUG=689659 ==========
ajo@chromium.org changed reviewers: + mdw@chromium.org, ryansturm@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== increase bucket size and break down video by http/https Effected histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video BUG=689659 ========== to ========== increase bucket size and break down video by http/https Effected existing histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video New Histograms: Net.HttpContentLength.Http.Direct Net.HttpContentLength.Http.ViaDRP Net.HttpContentLength.Http.BypassedDRP Net.HttpContentLength.Http.Other Net.HttpContentLength.Https.Direct Net.HttpContentLength.Https.ViaDRP Net.HttpContentLength.Https.BypassedDRP Net.HttpContentLength.Https.Other Net.HttpContentLength.Http.Direct.Video Net.HttpContentLength.Http.ViaDRP.Video Net.HttpContentLength.Http.BypassedDRP.Video Net.HttpContentLength.Http.Other.Video Net.HttpContentLength.Https.Direct.Video Net.HttpContentLength.Https.ViaDRP.Video Net.HttpContentLength.Https.BypassedDRP.Video Net.HttpContentLength.Https.Other.Video BUG=689659 ==========
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...
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...
comments are from a few patches ago, but I believe they still apply. https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89356: <histogram_suffixes name="NetHttpContentLengthType" separator="."> I'll let histograms reviewers decide, but this might be better as nested histogram_suffixes eg: group together HTTP + HTTPS group together <suffix name="ViaDRP"/> <suffix name="BypassedDRP"/> <suffix name="Other"/> group together: <suffix name="Video"/> and list the affected histograms as appropriate within those groups. https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89382: + <suffix name="Http.Direct"/> These should be alphabetical. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:48: base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet( thanks for the switch to this. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:63: std::string connection_type = ".Http"; std::string connection_type = is_https ? ".Https" : ".Http"; https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:130: // content length if the X-Original-Content-Header is not present. mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:165: // HTTP/1.1, otherwise it assumes that the original request would have used mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:182: // |used_drp| is true, this function estimates how many bytes would have mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:213: // Data Saver proxy. mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:262: // not exist. Even though we do not use the |DataUseGroup| here, we want mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:296: // fetching |request|. mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:348: // types. mind moving this back? https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1360: histogram_tester.ExpectTotalCount(kReceivedInsecureDirectHistogramName, 0); Ideally you would test that these histograms get results in one of the tests. I believe these tests would pass without your change.
bengr@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:63: std::string prefix = "Net.HttpContentLength"; Consider using const char* https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:64: std::string connection_type = ".Http"; use ternary operator: std::string connection_type = is_https ? ".Https" : ".Http"; https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:90: prefix + connection_type + suffix + ".Video", received_content_length); I think it's more efficient to use StringPrintf https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:95: // can be added to the Chrome proxy headers. |received_content_length| is Chrome proxy headers -> Chrome-Proxy header https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:98: // |freshness_lifetime| contains information on how long the resource will contains information on -> specifies https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:99: // be fresh for and how long is the usability. What does "how long is the usability" mean?
https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:52: 128 << 20, // Maximum sample size in bytes. Why did you choose 128 MB? can you add a comment on that https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:53: 200, // Bucket count. I'm not sure you need more buckets. More spaced out buckets should be sufficient I would think. Leave this at 50 like the 1M histograms? Perhaps you should only expand this in the case of video?
The CQ bit was checked by ajo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89356: <histogram_suffixes name="NetHttpContentLengthType" separator="."> On 2017/05/24 22:04:09, RyanSturm wrote: > I'll let histograms reviewers decide, but this might be better as nested > histogram_suffixes > > eg: > group together HTTP + HTTPS > > group together > <suffix name="ViaDRP"/> > <suffix name="BypassedDRP"/> > <suffix name="Other"/> > > group together: > <suffix name="Video"/> > > and list the affected histograms as appropriate within those groups. > Done. https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89382: + <suffix name="Http.Direct"/> On 2017/05/24 22:04:09, RyanSturm wrote: > These should be alphabetical. Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:48: base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet( On 2017/05/24 22:04:10, RyanSturm wrote: > thanks for the switch to this. Acknowledged. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:63: std::string connection_type = ".Http"; On 2017/05/24 22:04:10, RyanSturm wrote: > std::string connection_type = is_https ? ".Https" : ".Http"; Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:130: // content length if the X-Original-Content-Header is not present. On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:165: // HTTP/1.1, otherwise it assumes that the original request would have used On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:182: // |used_drp| is true, this function estimates how many bytes would have On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:213: // Data Saver proxy. On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:262: // not exist. Even though we do not use the |DataUseGroup| here, we want On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:296: // fetching |request|. On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:348: // types. On 2017/05/24 22:04:10, RyanSturm wrote: > mind moving this back? Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:63: std::string prefix = "Net.HttpContentLength"; On 2017/05/24 22:17:20, bengr wrote: > Consider using const char* Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:64: std::string connection_type = ".Http"; On 2017/05/24 22:17:20, bengr wrote: > use ternary operator: > std::string connection_type = is_https ? ".Https" : ".Http"; Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:90: prefix + connection_type + suffix + ".Video", received_content_length); On 2017/05/24 22:17:19, bengr wrote: > I think it's more efficient to use StringPrintf Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:95: // can be added to the Chrome proxy headers. |received_content_length| is On 2017/05/24 22:17:19, bengr wrote: > Chrome proxy headers -> Chrome-Proxy header Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:98: // |freshness_lifetime| contains information on how long the resource will On 2017/05/24 22:17:19, bengr wrote: > contains information on -> specifies Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:99: // be fresh for and how long is the usability. On 2017/05/24 22:17:19, bengr wrote: > What does "how long is the usability" mean? I didn't write this comment, so I'm not sure. I removed the hanging clause.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think I don't have any more nits after this :) https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:47: // Records the occurrence of |sample| in |name| histogram. Conventional UMA nit: s/Conventional UMA histograms/UMA macros/ https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:83: // Record aggregate of video and non-video traffic s/traffic/traffic./ https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94: // |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request header Yikes, this is an old q=low comment. Would you mind changing it to remove "q=low" so we can just read it as 'a "Lo-Fi" request header can be added...' https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:137: // Record the new histograms broken down by http/https and video/nonvideo s:/nonvideo/non-video./ s:/"http/https"/"HTTP/HTTPS"/ https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:615: original_content_length, freshness_lifetime, request_type); This is sourced from: https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro... That implementation counts data saver being off as SHORT_BYPASS. Can you file a bug to fix that (perhaps adding a new value to the enum for DATA_SAVER_OFF/DATA_SAVER_EMPTY_CONFIG). For now you can land this CL, but definitely need s a fix for these histograms and a few others. https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89401: + <obsolete> Don't obsolete Http or Https?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:47: // Records the occurrence of |sample| in |name| histogram. Conventional UMA On 2017/05/24 23:15:37, RyanSturm wrote: > nit: s/Conventional UMA histograms/UMA macros/ Done. https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:83: // Record aggregate of video and non-video traffic On 2017/05/24 23:15:37, RyanSturm wrote: > s/traffic/traffic./ Done. https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94: // |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request header On 2017/05/24 23:15:37, RyanSturm wrote: > Yikes, this is an old q=low comment. Would you mind changing it to remove > "q=low" so we can just read it as 'a "Lo-Fi" request header can be added...' Done. https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:137: // Record the new histograms broken down by http/https and video/nonvideo On 2017/05/24 23:15:37, RyanSturm wrote: > s:/nonvideo/non-video./ > s:/"http/https"/"HTTP/HTTPS"/ Done. https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:615: original_content_length, freshness_lifetime, request_type); On 2017/05/24 23:15:37, RyanSturm wrote: > This is sourced from: > > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro... > > That implementation counts data saver being off as SHORT_BYPASS. Can you file a > bug to fix that (perhaps adding a new value to the enum for > DATA_SAVER_OFF/DATA_SAVER_EMPTY_CONFIG). > > For now you can land this CL, but definitely need s a fix for these histograms > and a few others. crbug.com/726411 https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89401: + <obsolete> On 2017/05/24 23:15:37, RyanSturm wrote: > Don't obsolete Http or Https? They shouldn't be used without the suffix.
ajo@chromium.org changed reviewers: + isherman@chromium.org - mdw@chromium.org
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...
Overall this l00ks g00d t0 me, but we also need UMA for Net.HttpOriginalContentLength broken down in a similar way. (The expectation is that HTTPS and BypassedDRP will not have any counts in those histograms, so you can exclude them.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
On 2017/05/25 18:19:09, ajo1 wrote: > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc > (right): > > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:47: > // Records the occurrence of |sample| in |name| histogram. Conventional UMA > On 2017/05/24 23:15:37, RyanSturm wrote: > > nit: s/Conventional UMA histograms/UMA macros/ > > Done. > > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:83: > // Record aggregate of video and non-video traffic > On 2017/05/24 23:15:37, RyanSturm wrote: > > s/traffic/traffic./ > > Done. > > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94: > // |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request header > On 2017/05/24 23:15:37, RyanSturm wrote: > > Yikes, this is an old q=low comment. Would you mind changing it to remove > > "q=low" so we can just read it as 'a "Lo-Fi" request header can be added...' > > Done. > > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:137: > // Record the new histograms broken down by http/https and video/nonvideo > On 2017/05/24 23:15:37, RyanSturm wrote: > > s:/nonvideo/non-video./ > > s:/"http/https"/"HTTP/HTTPS"/ > > Done. > > https://codereview.chromium.org/2888653003/diff/240001/components/data_reduct... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:615: > original_content_length, freshness_lifetime, request_type); > On 2017/05/24 23:15:37, RyanSturm wrote: > > This is sourced from: > > > > > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro... > > > > That implementation counts data saver being off as SHORT_BYPASS. Can you file > a > > bug to fix that (perhaps adding a new value to the enum for > > DATA_SAVER_OFF/DATA_SAVER_EMPTY_CONFIG). > > > > For now you can land this CL, but definitely need s a fix for these histograms > > and a few others. > > crbug.com/726411 > > https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:89401: + <obsolete> > On 2017/05/24 23:15:37, RyanSturm wrote: > > Don't obsolete Http or Https? > > They shouldn't be used without the suffix. Did you mean to upload a new patch?
Yes, uploaded On Thu, May 25, 2017 at 1:20 PM, <ryansturm@chromium.org> wrote: > On 2017/05/25 18:19:09, ajo1 wrote: > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc > > File > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc > > (right): > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc#newcode47 > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc:47: > > // Records the occurrence of |sample| in |name| histogram. Conventional > UMA > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > nit: s/Conventional UMA histograms/UMA macros/ > > > > Done. > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc#newcode83 > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc:83: > > // Record aggregate of video and non-video traffic > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > s/traffic/traffic./ > > > > Done. > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc#newcode94 > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc:94: > > // |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request > header > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > Yikes, this is an old q=low comment. Would you mind changing it to > remove > > > "q=low" so we can just read it as 'a "Lo-Fi" request header can be > added...' > > > > Done. > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc#newcode137 > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc:137: > > // Record the new histograms broken down by http/https and video/nonvideo > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > s:/nonvideo/non-video./ > > > s:/"http/https"/"HTTP/HTTPS"/ > > > > Done. > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc#newcode615 > > > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc:615: > > original_content_length, freshness_lifetime, request_type); > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > This is sourced from: > > > > > > > > > https://cs.chromium.org/chromium/src/components/data_ > reduction_proxy/core/browser/data_reduction_proxy_network_ > delegate.cc?type=cs&l=458 > > > > > > That implementation counts data saver being off as SHORT_BYPASS. Can > you > file > > a > > > bug to fix that (perhaps adding a new value to the enum for > > > DATA_SAVER_OFF/DATA_SAVER_EMPTY_CONFIG). > > > > > > For now you can land this CL, but definitely need s a fix for these > histograms > > > and a few others. > > > > crbug.com/726411 > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > tools/metrics/histograms/histograms.xml > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/2888653003/diff/240001/ > tools/metrics/histograms/histograms.xml#newcode89401 > > tools/metrics/histograms/histograms.xml:89401: + <obsolete> > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > Don't obsolete Http or Https? > > > > They shouldn't be used without the suffix. > > Did you mean to upload a new patch? > > https://codereview.chromium.org/2888653003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89401: + <obsolete> On 2017/05/25 18:19:09, ajo1 wrote: > On 2017/05/24 23:15:37, RyanSturm wrote: > > Don't obsolete Http or Https? > > They shouldn't be used without the suffix. Good point. isherman@ does that seem right? https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:54: 200, // Bucket count. I just noticed you missed my comment from before about bucket size and max bounds, can you look at them (patch set 10) https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:85: // Record aggregate of video and non-video traffic. nit: sort of weird to say "record aggregate" when you aren't summing anything, but rather recording traffic for each request regardless of video/non-video. Feel free to leave it if it makes more sense to you than it does to me.
https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:52: 128 << 20, // Maximum sample size in bytes. On 2017/05/24 22:39:07, RyanSturm wrote: > Why did you choose 128 MB? can you add a comment on that Done. https://codereview.chromium.org/2888653003/diff/180001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:53: 200, // Bucket count. On 2017/05/24 22:39:07, RyanSturm wrote: > I'm not sure you need more buckets. More spaced out buckets should be sufficient > I would think. Leave this at 50 like the 1M histograms? > > Perhaps you should only expand this in the case of video? Done. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:54: 200, // Bucket count. On 2017/05/25 21:05:41, RyanSturm wrote: > I just noticed you missed my comment from before about bucket size and max > bounds, can you look at them (patch set 10) Done. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:85: // Record aggregate of video and non-video traffic. On 2017/05/25 21:05:41, RyanSturm wrote: > nit: sort of weird to say "record aggregate" when you aren't summing anything, > but rather recording traffic for each request regardless of video/non-video. > Feel free to leave it if it makes more sense to you than it does to me. Clarified the language.
https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89401: + <obsolete> On 2017/05/25 21:05:41, RyanSturm wrote: > On 2017/05/25 18:19:09, ajo1 wrote: > > On 2017/05/24 23:15:37, RyanSturm wrote: > > > Don't obsolete Http or Https? > > > > They shouldn't be used without the suffix. > > Good point. isherman@ does that seem right? Yep, this seems reasonable =) https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:54: 200, // Bucket count. 200 is a huge number of buckets. Are you sure you need so many? 50 is a more typical number of buckets. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:56: histogram_pointer->Add(sample); Please use base::UmaHistogramCustomCounts() rather than calling FactoryGet() and Add() directly. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:87: base::StringPrintf("%s%s%s", prefix, connection_type, suffix), Optional nit: I suspect that using std::string rather than char* and a+b+c rather than StringPrintf is at least equally efficient, and IMO easier to read. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:136: std::string prefix = "Net.HttpContentLength"; nit: This is an odd choice of variable name, given how it's used below. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:167: // true, then it's assumed that the original request would have used HTTP/1.1, nit: Spurious diff? https://codereview.chromium.org/2888653003/diff/100002/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/100002/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89422: + <suffix name="ViaDRP"/> Please add a brief label for each suffix.
https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:54: 200, // Bucket count. On 2017/05/25 22:12:31, Ilya Sherman wrote: > 200 is a huge number of buckets. Are you sure you need so many? 50 is a more > typical number of buckets. (Nvm, you've addressed this comment already. Thanks!)
lgtm
https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:56: histogram_pointer->Add(sample); On 2017/05/25 22:12:31, Ilya Sherman wrote: > Please use base::UmaHistogramCustomCounts() rather than calling FactoryGet() and > Add() directly. Done. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:87: base::StringPrintf("%s%s%s", prefix, connection_type, suffix), On 2017/05/25 22:12:32, Ilya Sherman wrote: > Optional nit: I suspect that using std::string rather than char* and a+b+c > rather than StringPrintf is at least equally efficient, and IMO easier to read. Done. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:136: std::string prefix = "Net.HttpContentLength"; On 2017/05/25 22:12:31, Ilya Sherman wrote: > nit: This is an odd choice of variable name, given how it's used below. This naming was chosen in an earlier revision. I removed the variable as it doesn't provide any useful information in the current patch. https://codereview.chromium.org/2888653003/diff/100002/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:167: // true, then it's assumed that the original request would have used HTTP/1.1, On 2017/05/25 22:12:32, Ilya Sherman wrote: > nit: Spurious diff? Erroneous space snuck in. I cleaned it up.
lgtm
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...
Metrics LGTM % a comment: https://codereview.chromium.org/2888653003/diff/310001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/310001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89521: + <suffix name="ViaDRP"/> Please add a brief label for each suffix. (Note: I also made this comment on a previous patch set.)
The CQ bit was checked by ajo@chromium.org to run a CQ dry run
https://codereview.chromium.org/2888653003/diff/310001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/310001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89521: + <suffix name="ViaDRP"/> On 2017/05/25 23:52:37, Ilya Sherman wrote: > Please add a brief label for each suffix. (Note: I also made this comment on a > previous patch set.) Done.
(Still LGTM) https://codereview.chromium.org/2888653003/diff/330001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/330001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89890: + label="Bytes of traffic that bypassed the Data Reduction proxy"/> nit: Mebbe s/proxy/Proxy (uppercase) to match other uses. Any consistently-cased spelling is ok by me, though.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2888653003/diff/330001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2888653003/diff/330001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:89890: + label="Bytes of traffic that bypassed the Data Reduction proxy"/> On 2017/05/26 00:02:28, Ilya Sherman wrote: > nit: Mebbe s/proxy/Proxy (uppercase) to match other uses. Any > consistently-cased spelling is ok by me, 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...
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, ryansturm@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2888653003/#ps350001 (title: "consistent Proxy capitalization")
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, ryansturm@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2888653003/#ps370001 (title: "Address windows build breakage")
The CQ bit was unchecked by ajo@chromium.org
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, ryansturm@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2888653003/#ps390001 (title: "Better approach to initializing the suffix.")
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, ryansturm@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2888653003/#ps410001 (title: "Address trybot failure")
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": 410001, "attempt_start_ts": 1495759315986670,
"parent_rev": "b1e300b0f88b6a65a87c9b27cd3e327499c25e86", "commit_rev":
"301b00f0c3b91119545da8f1762dcba0f665697e"}
Message was sent while issue was closed.
Description was changed from ========== increase bucket size and break down video by http/https Effected existing histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video New Histograms: Net.HttpContentLength.Http.Direct Net.HttpContentLength.Http.ViaDRP Net.HttpContentLength.Http.BypassedDRP Net.HttpContentLength.Http.Other Net.HttpContentLength.Https.Direct Net.HttpContentLength.Https.ViaDRP Net.HttpContentLength.Https.BypassedDRP Net.HttpContentLength.Https.Other Net.HttpContentLength.Http.Direct.Video Net.HttpContentLength.Http.ViaDRP.Video Net.HttpContentLength.Http.BypassedDRP.Video Net.HttpContentLength.Http.Other.Video Net.HttpContentLength.Https.Direct.Video Net.HttpContentLength.Https.ViaDRP.Video Net.HttpContentLength.Https.BypassedDRP.Video Net.HttpContentLength.Https.Other.Video BUG=689659 ========== to ========== increase bucket size and break down video by http/https Effected existing histograms: Net.HttpContentLength.Http Net.HttpContentLength.Https Net.HttpContentLength.Video Net.HttpContentLength.Http.Video Net.HttpContentLength.Https.Video New Histograms: Net.HttpContentLength.Http.Direct Net.HttpContentLength.Http.ViaDRP Net.HttpContentLength.Http.BypassedDRP Net.HttpContentLength.Http.Other Net.HttpContentLength.Https.Direct Net.HttpContentLength.Https.ViaDRP Net.HttpContentLength.Https.BypassedDRP Net.HttpContentLength.Https.Other Net.HttpContentLength.Http.Direct.Video Net.HttpContentLength.Http.ViaDRP.Video Net.HttpContentLength.Http.BypassedDRP.Video Net.HttpContentLength.Http.Other.Video Net.HttpContentLength.Https.Direct.Video Net.HttpContentLength.Https.ViaDRP.Video Net.HttpContentLength.Https.BypassedDRP.Video Net.HttpContentLength.Https.Other.Video BUG=689659 Review-Url: https://codereview.chromium.org/2888653003 Cr-Commit-Position: refs/heads/master@{#474922} Committed: https://chromium.googlesource.com/chromium/src/+/301b00f0c3b91119545da8f1762d... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:410001) as https://chromium.googlesource.com/chromium/src/+/301b00f0c3b91119545da8f1762d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
