Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642553002:
Adding UMA for data reduction proxy page load size/savings (Closed)
|
Created:
3 years, 11 months ago by RyanSturm Modified:
3 years, 11 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding UMA for data reduction proxy page load size/savings
This Adds UMA to track bytes savings, percent savings, total requests,
percent requests throug the proxy, byte usage, etc.
This UMA will be compared against existing PLM data as well as sliced by
various experiments in data reduction proxy.
BUG=682022
Review-Url: https://codereview.chromium.org/2642553002
Cr-Commit-Position: refs/heads/master@{#444859}
Committed: https://chromium.googlesource.com/chromium/src/+/485df9ed214f7924eaf8ed067070146e3d12aa58
Patch Set 1 #
Total comments: 31
Patch Set 2 : tbansal comments, tests, small const var refactor. #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : tbansal nits #Patch Set 5 : fixing rebase artifact #
Total comments: 12
Patch Set 6 : Add TODO, bmcquade, asvitkine comments #
Total comments: 6
Patch Set 7 : asvitkine comments #
Dependent Patchsets: Messages
Total messages: 49 (31 generated)
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: Can you decide if you think any of these histograms are too duplicative and if so, suggest some to remove. FYI, I still need to add testing (and probably moving const strings into internal namespace, etc.). I'll add metrics reviewer after we decide on a set of histograms.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:162: void DataReductionProxyMetricsObserver::RecordPageSizeUMA() { const method? https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:164: return; Why return? It might be useful to know if there are pages that are 100% cached (may be PWA or due to SW)? https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:169: 100 * num_data_reduction_proxy_requests_ / num_network_requests_; do we need parenthesis here? to get around the ambiguity of what happens first: division or multiplication similar comments below https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:187: if (original_network_bytes_ > 0 && original_network_bytes_ > network_bytes_) { s/>/>=/ for the second condition? If the two are equal, I think we should record that as 0% savings rather than 0% inflation. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:191: .append("Experimental.Bytes.Network.CompressionRatio"), Should we record savings percent because that's what we use in all other histograms? https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:193: } else if (network_bytes_ > 0) { Can network_bytes_ be 0 when num_network_requests_ > 0? The latter is already checked at Line 163 above. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:194: int inflation_ratio = 100 * original_network_bytes_ / network_bytes_; This histogram is difficult to parse. eg., if original_n_b = 1, n_b = 10; then inflation_ratio is 10% original_n_b = 1, n_b = 20; then inflation_ratio is 5% whereas, clearly inflation was more in the second case. Another possible way is to record: (n_b - o_n_b)/o_n_b * 100 https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:222: .append("Experimental.Bytes.Network"), Use KB or KiloBytes in the histogram name to avoid confusion same comment below https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:230: static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50); Can you add a comment that upper bucket is 500 MB. Here and below. you can also use UMA_HISTOGRAM_COUNTS_1M if you are willing to use 1 GB as upper bound. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:237: static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50); s/network_bytes_proxied_/network_bytes_ - network_bytes_proxied_/ https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:248: if (network_bytes_ < original_network_bytes_) { s/</<=/ https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:102: void RecordPageSizeUMA(); s/Record/Records/ https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:116: // The number of request that used the network. How is this determined? What happens if the cached resource is validated? https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:123: // The total network bytes loaded through data reduction proxy missing period at end. https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42724: + as a percent when the user saw savings. Recorded per page load when the user "when the user saw savings" This is not clear. This seems like it is somehow related to user seeing the savings in settings UI. https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42744: + units="%"> Isn't the units ratio times 100?
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + bmcquade@chromium.org
tbansal, bmcquade: PTAL https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:162: void DataReductionProxyMetricsObserver::RecordPageSizeUMA() { On 2017/01/18 22:35:19, tbansal1 wrote: > const method? Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:164: return; On 2017/01/18 22:35:19, tbansal1 wrote: > Why return? It might be useful to know if there are pages that are 100% cached > (may be PWA or due to SW)? This is happens when the main page request uses DRP but doesn't complete before OnComplete (e.g., navigation away from the page) https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:169: 100 * num_data_reduction_proxy_requests_ / num_network_requests_; On 2017/01/18 22:35:19, tbansal1 wrote: > do we need parenthesis here? to get around the ambiguity of what happens first: > division or multiplication > > similar comments below No, but I'll add it for you. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:187: if (original_network_bytes_ > 0 && original_network_bytes_ > network_bytes_) { On 2017/01/18 22:35:19, tbansal1 wrote: > s/>/>=/ for the second condition? > If the two are equal, I think we should record that as 0% savings rather than 0% > inflation. Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:191: .append("Experimental.Bytes.Network.CompressionRatio"), On 2017/01/18 22:35:19, tbansal1 wrote: > Should we record savings percent because that's what we use in all other > histograms? I'd prefer compression ratio because it shows when users get above 100% savings. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:193: } else if (network_bytes_ > 0) { On 2017/01/18 22:35:19, tbansal1 wrote: > Can network_bytes_ be 0 when num_network_requests_ > 0? The latter is already > checked at Line 163 above. I assume so. This is body bytes, and I believe body bytes could theoretically be 0. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:194: int inflation_ratio = 100 * original_network_bytes_ / network_bytes_; On 2017/01/18 22:35:19, tbansal1 wrote: > This histogram is difficult to parse. > eg., if original_n_b = 1, n_b = 10; then inflation_ratio is 10% > original_n_b = 1, n_b = 20; then inflation_ratio is 5% > > whereas, clearly inflation was more in the second case. > > Another possible way is to record: > (n_b - o_n_b)/o_n_b * 100 Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:222: .append("Experimental.Bytes.Network"), On 2017/01/18 22:35:19, tbansal1 wrote: > Use KB or KiloBytes in the histogram name to avoid confusion > > same comment below It's in the units on histograms.xml and I am standardizing on PLM exsiting metrics. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:230: static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50); On 2017/01/18 22:35:19, tbansal1 wrote: > Can you add a comment that upper bucket is 500 MB. Here and below. > > you can also use UMA_HISTOGRAM_COUNTS_1M if you are willing to use 1 GB as upper > bound. Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:237: static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50); On 2017/01/18 22:35:19, tbansal1 wrote: > s/network_bytes_proxied_/network_bytes_ - network_bytes_proxied_/ Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:248: if (network_bytes_ < original_network_bytes_) { On 2017/01/18 22:35:19, tbansal1 wrote: > s/</<=/ Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h (right): https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:102: void RecordPageSizeUMA(); On 2017/01/18 22:35:20, tbansal1 wrote: > s/Record/Records/ Done. https://codereview.chromium.org/2642553002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:116: // The number of request that used the network. On 2017/01/18 22:35:20, tbansal1 wrote: > How is this determined? What happens if the cached resource is validated? Done. https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42724: + as a percent when the user saw savings. Recorded per page load when the user On 2017/01/18 22:35:20, tbansal1 wrote: > "when the user saw savings" > This is not clear. This seems like it is somehow related to user seeing the > savings in settings UI. Done. https://codereview.chromium.org/2642553002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42744: + units="%"> On 2017/01/18 22:35:20, tbansal1 wrote: > Isn't the units ratio times 100? Changed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % nits. https://codereview.chromium.org/2642553002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:207: } else if (network_bytes_ > 0) { s/network_bytes_ > 0/original_network_bytes_ > 0/ to avoid divide-by-zero. https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42721: + The ratio of network bytes received to network bytes the user would have Add: times 100 https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42723: + that had its main resource was loaded through data reduction proxy. Recorded Remove "was"? https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42736: + The number of network bytes (not including headers) that the data reduction kilobytes? If the reader does not look at the units, they may assume it is in bytes.
The CQ bit was checked by ryansturm@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 ryansturm@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...
https://codereview.chromium.org/2642553002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:207: } else if (network_bytes_ > 0) { On 2017/01/18 23:34:49, tbansal1 wrote: > s/network_bytes_ > 0/original_network_bytes_ > 0/ > to avoid divide-by-zero. Done. https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42721: + The ratio of network bytes received to network bytes the user would have On 2017/01/18 23:34:49, tbansal1 wrote: > Add: times 100 Done. https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42723: + that had its main resource was loaded through data reduction proxy. Recorded On 2017/01/18 23:34:49, tbansal1 wrote: > Remove "was"? Done. https://codereview.chromium.org/2642553002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42736: + The number of network bytes (not including headers) that the data reduction On 2017/01/18 23:34:49, tbansal1 wrote: > kilobytes? > If the reader does not look at the units, they may assume it is in bytes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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-...)
The CQ bit was checked by ryansturm@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: This issue passed the CQ dry run.
last comment. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:90: const char kBytesProxied[] = "Experimental.Bytes.Network.Proxied"; Actually, I still do not understand why we can't replace Bytes by KB in the histogram name. Are we sure that there is a need to precisely match the names of the other existing histograms. Searching for ".*KB.*" in the UMA dashboard shows plenty of histograms that use KB in their name. I would prefer if the histogram names are clear enough (or at least not misleading).
On 2017/01/19 03:12:40, tbansal1 wrote: > last comment. > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc > (right): > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:90: > const char kBytesProxied[] = "Experimental.Bytes.Network.Proxied"; > Actually, I still do not understand why we can't replace Bytes by KB in the > histogram name. Are we sure that there is a need to precisely match the names of > the other existing histograms. > > Searching for ".*KB.*" in the UMA dashboard shows plenty of histograms that use > KB in their name. > > I would prefer if the histogram names are clear enough (or at least not > misleading). I agree, however, I want PageLoad.Experimental.Bytes.Network to be comparable to PageLoad.Clients.DRP.Experimental.Bytes.Network and I want that to be comparable to the rest of these. I also want consistent names so I can search experimental.bytes and see all of them on UMA. I don't want to change existing hiatograms, so for consistency, it makes sense to do it this way. Either way the curve will show the right picture, and if you look at raw numbers without units, you should be able to determine it's not in bytes (or at least determine that you should check units).
ryansturm@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: PTAL @ histograms, Thanks!
This is quite a lot of histograms. Are you sure you need all of these? (Since histograms consume user bandwidth it's best to minimize them when possible.) https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:196: static_cast<int>((100 * network_bytes_proxied_) / network_bytes_)); Is static_cast needed here? Seems you don't need it above on line 189 - if not I suggest removing it. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:225: .append(internal::kRequestsProxied), Nit: How about making a helper function for this to make all of these take 1 line instead of two? e.g. GetConstHistogramWithSuffix(internal::kRequestsProxied)? https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43216: + name="PageLoad.Clients.DataReductionProxy.PageLoad.Experimental.Requests.Network.Proxied"> Nit: add units="requests" Also to the other ones that don't specify it.
LGTM overall, thanks! https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:79: const char kRequestsPercentProxied[] = one challenge we've encountered with percent histograms is that it is difficult to distinguish between e.g. 50% meaning 1 in 2 requests vs 50% meaning 50 of 100 requests - yet those cases are very different. One possible way to address this would be to have a couple histograms here that are segmented by total number of requests on the page, e.g. Experimental.Bytes.Network.PercentProxied.LessThan10Requests, and .AtLeast10Requests (maybe there are shorter ways to express this?). You could also have an overall histo that collects stats for all page loads. This is an optional suggestion - feel free to proceed w/o this. I've just found it confusing to reason about these percentage histograms due to the above issue. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:208: // Inflation should never be above one hundred percent. what happens if it is over 100%? does it just get recorded in the last bucket?
> This is quite a lot of histograms. Are you sure you need all of these? (Since > histograms consume user bandwidth it's best to minimize them when possible.) I tried to eliminate a few of these early in writing this CL, but they all provide different information that is interesting (e.g., comparing network bytes vs non drp pages, comparing drp bytes vs non-drp bytes vs total network bytes, etc.) If you do think it is overkill, I wouldn't be too upset if you felt I should get rid of *.NonProxied (two histograms). Any others seem pretty important to doing analysis on this. WDYT? https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:79: const char kRequestsPercentProxied[] = On 2017/01/19 18:44:18, Bryan McQuade wrote: > one challenge we've encountered with percent histograms is that it is difficult > to distinguish between e.g. 50% meaning 1 in 2 requests vs 50% meaning 50 of 100 > requests - yet those cases are very different. > > One possible way to address this would be to have a couple histograms here that > are segmented by total number of requests on the page, e.g. > Experimental.Bytes.Network.PercentProxied.LessThan10Requests, and > .AtLeast10Requests (maybe there are shorter ways to express this?). You could > also have an overall histo that collects stats for all page loads. > > This is an optional suggestion - feel free to proceed w/o this. I've just found > it confusing to reason about these percentage histograms due to the above issue. Hmm. I'll open a bug to add a LessThan10Requests histogram later based on what we see in the UMA initially. I think it is a good idea, but I don't want to add too much new UMA. Thanks for the suggestion. crbug.com/682765 https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:90: const char kBytesProxied[] = "Experimental.Bytes.Network.Proxied"; On 2017/01/19 03:12:40, tbansal1 wrote: > Actually, I still do not understand why we can't replace Bytes by KB in the > histogram name. Are we sure that there is a need to precisely match the names of > the other existing histograms. > > Searching for ".*KB.*" in the UMA dashboard shows plenty of histograms that use > KB in their name. > > I would prefer if the histogram names are clear enough (or at least not > misleading). Acknowledged. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:196: static_cast<int>((100 * network_bytes_proxied_) / network_bytes_)); On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > Is static_cast needed here? > > Seems you don't need it above on line 189 - if not I suggest removing it. For the percent ones, it might not be necessary, but I think I'd prefer to be safe since this will be an int64_t after multiplication/division. Otherwise it will be implicitly casted. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:208: // Inflation should never be above one hundred percent. On 2017/01/19 18:44:18, Bryan McQuade wrote: > what happens if it is over 100%? does it just get recorded in the last bucket? Correct, which for now is an approach we are comfortable with. If we see that we have a lot in that bucket, we will address the issue then. https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:225: .append(internal::kRequestsProxied), On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > Nit: How about making a helper function for this to make all of these take 1 > line instead of two? > > e.g. GetConstHistogramWithSuffix(internal::kRequestsProxied)? Done. https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43216: + name="PageLoad.Clients.DataReductionProxy.PageLoad.Experimental.Requests.Network.Proxied"> On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > Nit: add units="requests" > > Also to the other ones that don't specify it. Done.
On 2017/01/19 19:32:46, Ryan Sturm wrote: > > This is quite a lot of histograms. Are you sure you need all of these? (Since > > histograms consume user bandwidth it's best to minimize them when possible.) > > I tried to eliminate a few of these early in writing this CL, but they all > provide different information that is interesting (e.g., comparing network bytes > vs non drp pages, comparing drp bytes vs non-drp bytes vs total network bytes, > etc.) > > If you do think it is overkill, I wouldn't be too upset if you felt I should get > rid of *.NonProxied (two histograms). Any others seem pretty important to doing > analysis on this. > > WDYT? I don't enough about the domain to judge how important they are. Certainly, if they're not important - don't log them. But also I'm OK if you add a TODO to revisit these and re-evaluate if they're all still needed after we've collected some data for it and decide if they're all still needed. Waiting for the updated patchset. > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > File > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc > (right): > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:79: > const char kRequestsPercentProxied[] = > On 2017/01/19 18:44:18, Bryan McQuade wrote: > > one challenge we've encountered with percent histograms is that it is > difficult > > to distinguish between e.g. 50% meaning 1 in 2 requests vs 50% meaning 50 of > 100 > > requests - yet those cases are very different. > > > > One possible way to address this would be to have a couple histograms here > that > > are segmented by total number of requests on the page, e.g. > > Experimental.Bytes.Network.PercentProxied.LessThan10Requests, and > > .AtLeast10Requests (maybe there are shorter ways to express this?). You could > > also have an overall histo that collects stats for all page loads. > > > > This is an optional suggestion - feel free to proceed w/o this. I've just > found > > it confusing to reason about these percentage histograms due to the above > issue. > > Hmm. I'll open a bug to add a LessThan10Requests histogram later based on what > we see in the UMA initially. I think it is a good idea, but I don't want to add > too much new UMA. > > Thanks for the suggestion. crbug.com/682765 > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:90: > const char kBytesProxied[] = "Experimental.Bytes.Network.Proxied"; > On 2017/01/19 03:12:40, tbansal1 wrote: > > Actually, I still do not understand why we can't replace Bytes by KB in the > > histogram name. Are we sure that there is a need to precisely match the names > of > > the other existing histograms. > > > > Searching for ".*KB.*" in the UMA dashboard shows plenty of histograms that > use > > KB in their name. > > > > I would prefer if the histogram names are clear enough (or at least not > > misleading). > > Acknowledged. > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:196: > static_cast<int>((100 * network_bytes_proxied_) / network_bytes_)); > On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > > Is static_cast needed here? > > > > Seems you don't need it above on line 189 - if not I suggest removing it. > > For the percent ones, it might not be necessary, but I think I'd prefer to be > safe since this will be an int64_t after multiplication/division. Otherwise it > will be implicitly casted. > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:208: > // Inflation should never be above one hundred percent. > On 2017/01/19 18:44:18, Bryan McQuade wrote: > > what happens if it is over 100%? does it just get recorded in the last bucket? > > Correct, which for now is an approach we are comfortable with. If we see that we > have a lot in that bucket, we will address the issue then. > > https://codereview.chromium.org/2642553002/diff/80001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:225: > .append(internal::kRequestsProxied), > On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > > Nit: How about making a helper function for this to make all of these take 1 > > line instead of two? > > > > e.g. GetConstHistogramWithSuffix(internal::kRequestsProxied)? > > Done. > > https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2642553002/diff/80001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:43216: + > name="PageLoad.Clients.DataReductionProxy.PageLoad.Experimental.Requests.Network.Proxied"> > On 2017/01/19 18:39:50, Alexei Svitkine (slow) wrote: > > Nit: add units="requests" > > > > Also to the other ones that don't specify it. > > Done.
The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run
On 2017/01/19 19:37:44, Alexei Svitkine (slow) wrote: > On 2017/01/19 19:32:46, Ryan Sturm wrote: > > > This is quite a lot of histograms. Are you sure you need all of these? > (Since > > > histograms consume user bandwidth it's best to minimize them when possible.) > > > > I tried to eliminate a few of these early in writing this CL, but they all > > provide different information that is interesting (e.g., comparing network > bytes > > vs non drp pages, comparing drp bytes vs non-drp bytes vs total network bytes, > > etc.) > > > > If you do think it is overkill, I wouldn't be too upset if you felt I should > get > > rid of *.NonProxied (two histograms). Any others seem pretty important to > doing > > analysis on this. > > > > WDYT? > > I don't enough about the domain to judge how important they are. Certainly, if > they're not important - don't log them. > > But also I'm OK if you add a TODO to revisit these and re-evaluate if they're > all still needed after we've collected some data for it and decide if they're > all still needed. > > Waiting for the updated patchset. > Added a TODO and bug.
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/2642553002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:184: if (num_network_requests_ == 0) { Nit: No {}'s https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:194: (100 * num_data_reduction_proxy_requests_) / num_network_requests_); Nit: Add the static_cast<int>() cast here if you're keeping the casts. https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:236: static_cast<int>(network_bytes_ / 1024), 1, 500 * 1024, 50); Since you're re-using these macro params for all of these, suggest defining a custom macro for it so you only need to specify them once. Also, you can do the division by 1024 inside it to make the callsites cleaner.
The CQ bit was checked by ryansturm@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...
https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc (right): https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:184: if (num_network_requests_ == 0) { On 2017/01/19 20:12:46, Alexei Svitkine (slow) wrote: > Nit: No {}'s Done. https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:194: (100 * num_data_reduction_proxy_requests_) / num_network_requests_); On 2017/01/19 20:12:46, Alexei Svitkine (slow) wrote: > Nit: Add the static_cast<int>() cast here if you're keeping the casts. I only want to cast int64_t->int, and this (and other num_request vars) would be int->int. https://codereview.chromium.org/2642553002/diff/100001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:236: static_cast<int>(network_bytes_ / 1024), 1, 500 * 1024, 50); On 2017/01/19 20:12:46, Alexei Svitkine (slow) wrote: > Since you're re-using these macro params for all of these, suggest defining a > custom macro for it so you only need to specify them once. > > Also, you can do the division by 1024 inside it to make the callsites cleaner. Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/2642553002/#ps120001 (title: "asvitkine comments")
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": 120001, "attempt_start_ts": 1484863742927660,
"parent_rev": "44ed361a0e940d4cff7b5a91ecf1fb4ac96b38f7", "commit_rev":
"485df9ed214f7924eaf8ed067070146e3d12aa58"}
Message was sent while issue was closed.
Description was changed from ========== Adding UMA for data reduction proxy page load size/savings This Adds UMA to track bytes savings, percent savings, total requests, percent requests throug the proxy, byte usage, etc. This UMA will be compared against existing PLM data as well as sliced by various experiments in data reduction proxy. BUG=682022 ========== to ========== Adding UMA for data reduction proxy page load size/savings This Adds UMA to track bytes savings, percent savings, total requests, percent requests throug the proxy, byte usage, etc. This UMA will be compared against existing PLM data as well as sliced by various experiments in data reduction proxy. BUG=682022 Review-Url: https://codereview.chromium.org/2642553002 Cr-Commit-Position: refs/heads/master@{#444859} Committed: https://chromium.googlesource.com/chromium/src/+/485df9ed214f7924eaf8ed067070... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/485df9ed214f7924eaf8ed067070... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
