Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
784253002:
Measure network error rates with and without data reduction proxy (Closed)
DescriptionMeasure network error rates with and without data reduction proxy
Adds new histograms that measure the network errors that requests are
completing with when using the data reduction proxy, and also measure
the network error rates for all HTTP requests for comparison. Single out
main frame resource loads because most net errors that would occur due
the network being unusable with the data reduction proxy would occur on
main frame requests.
BUG=439589
Committed: https://crrev.com/3c16159e8509dc95e72758296c66531b202eee6d
Cr-Commit-Position: refs/heads/master@{#308396}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments #Patch Set 3 : Rebased on master #
Total comments: 4
Patch Set 4 : Switched to using LOAD_MAIN_FRAME load flag #
Total comments: 7
Patch Set 5 : Addressed comments #
Messages
Total messages: 35 (18 generated)
sclittle@chromium.org changed reviewers: + bengr@chromium.org, jar@chromium.org, mef@chromium.org
bengr: all mef: chrome_network_delegate.cc jar: histograms.xml Thanks in advance!
https://codereview.chromium.org/784253002/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/784253002/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:574: data_reduction_proxy_usage_stats_->OnUrlRequestCompleted( After megjablon@'s CL lands, this will be in DataReductionProxyNetworkDelegate. Note that this, unlike ChromeNetworkDelegate, cannot depend on content. https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:459: }; Add a blank line. https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:461: {false, false, true, net::OK}, Indent only 2. https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:502: histogram_tester.ExpectUniqueSample(kPrimaryHistogramName, -net_error_int, I'd put all three parameters on one line. Move the first two down.
https://codereview.chromium.org/784253002/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/784253002/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:574: data_reduction_proxy_usage_stats_->OnUrlRequestCompleted( On 2014/12/09 23:44:13, bengr wrote: > After megjablon@'s CL lands, this will be in DataReductionProxyNetworkDelegate. > Note that this, unlike ChromeNetworkDelegate, cannot depend on content. Darn. Being able to single out main frame resource loads from other requests using the Net.ErrorCodesForMainFrame3 histogram has helped identify net errors that are likely a problem for the DRP, so having them singled out here could be quite helpful as well. There must be a way to plumb this information through, even if it requires injecting a callback, or putting some stuff in components/data_reduction_proxy/content. How close is megjablon@'s CL to landing? Should I hold off on this CL so that they can be integrated together more nicely? https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:459: }; On 2014/12/09 23:44:13, bengr wrote: > Add a blank line. Done. https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:461: {false, false, true, net::OK}, On 2014/12/09 23:44:13, bengr wrote: > Indent only 2. Done. https://codereview.chromium.org/784253002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:502: histogram_tester.ExpectUniqueSample(kPrimaryHistogramName, -net_error_int, On 2014/12/09 23:44:13, bengr wrote: > I'd put all three parameters on one line. Move the first two down. Done.
jar@chromium.org changed reviewers: + asvitkine@chromium.org - jar@chromium.org
+cc asvitkine for histograms.xml
Patchset #3 (id:40001) has been deleted
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
sclittle@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: profile_impl_io_data.cc I've rebased this on megjablon@'s refactor, and I've injected the IsMainFrameResourceRequest check as a callback. I'm not sure that profile_impl_io_data.cc is the right place to put the implementation of IsMainFrameResourceRequest, but I'm not sure that it would be better to create and pull in a new file just for this simple 3 line function. Thanks in advance!
https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:135: is_main_frame_resource_callback_.Run(request); We have a load flag for this (Admittedly, one I'd like to get rid of, but as long as we have it, might as well use it).
lgtm with a nit https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:62: bool(const net::URLRequest*)>& is_main_frame_resource_callback) { Nit: This wrapping is pretty awful. Maybe give the param a shorter name and wrap it similar to set_unavailable_callback() above?
https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:135: is_main_frame_resource_callback_.Run(request); On 2014/12/12 15:35:48, mmenke wrote: > We have a load flag for this (Admittedly, one I'd like to get rid of, but as > long as we have it, might as well use it). I didn't know about that load flag, thanks. I've changed the code to use it instead of injecting this callback. https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/784253002/diff/60001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:62: bool(const net::URLRequest*)>& is_main_frame_resource_callback) { On 2014/12/12 20:25:48, asvitkine (OOO until Jan4) wrote: > Nit: This wrapping is pretty awful. Maybe give the param a shorter name and wrap > it similar to set_unavailable_callback() above? I've removed this function entirely in favor of using the LOAD_MAIN_FRAME load flag.
https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:164: "Net.HttpRequestCompletionErrorCodes.MainFrame", Not needed. See Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2
https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:164: "Net.HttpRequestCompletionErrorCodes.MainFrame", On 2014/12/12 21:09:30, mmenke wrote: > Not needed. See Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2 Please clarify, for "not needed", are you referring to both the histograms here or just the MainFrame one? The idea behind having these histograms is that we could compare net error rates for HTTP requests between users who have the data reduction proxy enabled and users who don't. By looking at these histograms together with DRP.RequestCompletionErrorCodes, we could work out what the net error rates are for requests that bypass the DRP, and get a better overall idea of the effects of network flakiness on DRP usage. The idea behind drilling down on the main frame requests is that when a client is on a network on which the DRP isn't immediately usable (e.g. captive portal), errors caused by this would typically show up on main frame resource loads, and since these captive portals or other things could handle HTTP traffic differently than other traffic (such as HTTPS), it would be nice to look at just HTTP main frame request net error rates. Also, from looking at Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2, the rates of some errors between these two differ in interesting ways, so it could be useful to narrow that down to just HTTP requests so we can better apply this data to DRP usage. If you believe that the existing Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2 histograms already allow for this sort of thing, then I'll remove these new ones.
lgtm with nits https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:126: if (data_reduction_proxy_params_->WasDataReductionProxyUsed(request, #include "components/data_reduction_proxy/common/core/data_reduction_proxy_params.h" https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:447: "DataReductionProxy.RequestCompletionErrorCodes.Primary"; Can these be const char foo[] = "..." ??
https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:126: if (data_reduction_proxy_params_->WasDataReductionProxyUsed(request, On 2014/12/13 00:51:59, bengr wrote: > #include > "components/data_reduction_proxy/common/core/data_reduction_proxy_params.h" Done. https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/784253002/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:447: "DataReductionProxy.RequestCompletionErrorCodes.Primary"; On 2014/12/13 00:52:00, bengr wrote: > Can these be const char foo[] = "..." ?? Unfortunately, no, they have to be std::string. HistogramTester only takes std::string for histogram names.
LGTM https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:164: "Net.HttpRequestCompletionErrorCodes.MainFrame", On 2014/12/12 22:10:20, sclittle wrote: > On 2014/12/12 21:09:30, mmenke wrote: > > Not needed. See Net.ErrorCodesForMainFrame3 and > Net.ErrorCodesForSubresources2 > > Please clarify, for "not needed", are you referring to both the histograms here > or just the MainFrame one? > > The idea behind having these histograms is that we could compare net error rates > for HTTP requests between users who have the data reduction proxy enabled and > users who don't. By looking at these histograms together with > DRP.RequestCompletionErrorCodes, we could work out what the net error rates are > for requests that bypass the DRP, and get a better overall idea of the effects > of network flakiness on DRP usage. > > The idea behind drilling down on the main frame requests is that when a client > is on a network on which the DRP isn't immediately usable (e.g. captive portal), > errors caused by this would typically show up on main frame resource loads, and > since these captive portals or other things could handle HTTP traffic > differently than other traffic (such as HTTPS), it would be nice to look at just > HTTP main frame request net error rates. Also, from looking at > Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2, the rates of > some errors between these two differ in interesting ways, so it could be useful > to narrow that down to just HTTP requests so we can better apply this data to > DRP usage. > > If you believe that the existing Net.ErrorCodesForMainFrame3 and > Net.ErrorCodesForSubresources2 histograms already allow for this sort of thing, > then I'll remove these new ones. Ahh...you're right, they're different, I missed the http restriction. Suppose these also cover requests that don't come through the RDH. This does include incognito requests, though, which would bypass the DRP even if it were enabled, so not completely comparable, but close. I'm still not a huge fan of having all these near identical histograms logged all over the place. Histograms aren't free. Don't have a better suggestion, though. Could just compare the other histograms with DRP on and off, but that doesn't give you the fallback vs non-fallback error numbers, I suppose.
On 2014/12/15 16:55:44, mmenke wrote: > LGTM > > https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... > File chrome/browser/net/chrome_network_delegate.cc (right): > > https://codereview.chromium.org/784253002/diff/80001/chrome/browser/net/chrom... > chrome/browser/net/chrome_network_delegate.cc:164: > "Net.HttpRequestCompletionErrorCodes.MainFrame", > On 2014/12/12 22:10:20, sclittle wrote: > > On 2014/12/12 21:09:30, mmenke wrote: > > > Not needed. See Net.ErrorCodesForMainFrame3 and > > Net.ErrorCodesForSubresources2 > > > > Please clarify, for "not needed", are you referring to both the histograms > here > > or just the MainFrame one? > > > > The idea behind having these histograms is that we could compare net error > rates > > for HTTP requests between users who have the data reduction proxy enabled and > > users who don't. By looking at these histograms together with > > DRP.RequestCompletionErrorCodes, we could work out what the net error rates > are > > for requests that bypass the DRP, and get a better overall idea of the effects > > of network flakiness on DRP usage. > > > > The idea behind drilling down on the main frame requests is that when a client > > is on a network on which the DRP isn't immediately usable (e.g. captive > portal), > > errors caused by this would typically show up on main frame resource loads, > and > > since these captive portals or other things could handle HTTP traffic > > differently than other traffic (such as HTTPS), it would be nice to look at > just > > HTTP main frame request net error rates. Also, from looking at > > Net.ErrorCodesForMainFrame3 and Net.ErrorCodesForSubresources2, the rates of > > some errors between these two differ in interesting ways, so it could be > useful > > to narrow that down to just HTTP requests so we can better apply this data to > > DRP usage. > > > > If you believe that the existing Net.ErrorCodesForMainFrame3 and > > Net.ErrorCodesForSubresources2 histograms already allow for this sort of > thing, > > then I'll remove these new ones. > > Ahh...you're right, they're different, I missed the http restriction. Suppose > these also cover requests that don't come through the RDH. This does include > incognito requests, though, which would bypass the DRP even if it were enabled, > so not completely comparable, but close. > > I'm still not a huge fan of having all these near identical histograms logged > all over the place. Histograms aren't free. Don't have a better suggestion, > though. > > Could just compare the other histograms with DRP on and off, but that doesn't > give you the fallback vs non-fallback error numbers, I suppose. I don't suppose we could just always hook up the DRP stats object, whether or not it's in use, and gather stats there for enabled vs not? Makes for the most directly comparable numbers, and nice to have it all in one place. Fine as-is, just something to think about.
The CQ bit was checked by sclittle@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784253002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3c16159e8509dc95e72758296c66531b202eee6d Cr-Commit-Position: refs/heads/master@{#308396} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
