Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
577343002:
Adds UMA to measure when the data reduction proxy via header is missing (Closed)
DescriptionAdds UMA to measure when the data reduction proxy via header is missing
Keep track of the situations when Chrome expects the data reduction
proxy via header to be present in a response, but the data reduction
proxy via header is missing.
BUG=412888
Committed: https://crrev.com/ba1e1e3f7ea6edae987e6cfa92086807bf74260b
Cr-Commit-Position: refs/heads/master@{#296297}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 18
Patch Set 4 : Addressed comments #Patch Set 5 : Addressed comments #
Total comments: 18
Patch Set 6 : Addressed comments #
Total comments: 2
Patch Set 7 : Addressed comments #Patch Set 8 : Sync to head #
Total comments: 1
Messages
Total messages: 40 (16 generated)
sclittle@chromium.org changed reviewers: + asvitkine@chromium.org, bengr@chromium.org, mmenke@chromium.org
bengr: all mmenke: chrome_network_delegate asvitkine: histograms.xml Thanks in advance!
https://codereview.chromium.org/577343002/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/577343002/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:593: data_reduction_proxy_usage_stats_->RecordMissingViaHeaderBytes(request); This function name is very confusing, since it's always called, even when we have a via header. Also seems weird that this takes request and the next takes *request. I think to keep things simplest, you should just merge them into DataReductionProxyUsageStats::RecordByteHistograms(*request, *data_decution_proxy_enable_, proxy_config_getter_.Run()); https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:409: || HasDataReductionProxyViaHeader(request->response_headers(), NULL)) { Searching through the header every time we get a k or two seems needlessly heavy weight. Maybe use UserData to associate data-reduction-proxy information with the request, when headers are first read? https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:416: && request->GetResponseCode() < net::HTTP_INTERNAL_SERVER_ERROR) { nit: Not sure if the DRP code uses another style, but the generally preferred style is the operator on the previous line.
I'll wait for you to address mmenke's point before doing a detailed review. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:53: void HeadersToRaw(std::string* headers) { I think duplicates of this function live elsewhere in the data reduction proxy component. would you please coalesce these all into one *test_utils file? https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:90: Here and above, I don't think the blank lines improve readability. Remove. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:283: kPrimaryHistogramName)); move up a with previous line? https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:286: kFallbackHistogramName)); move up a with previous line? https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:418: WasDataReductionProxyUsed(fake_request.get(), NULL)) Indentation seems off. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:438: Remove extra blank line
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/577343002/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/577343002/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:593: data_reduction_proxy_usage_stats_->RecordMissingViaHeaderBytes(request); On 2014/09/18 18:39:30, mmenke wrote: > This function name is very confusing, since it's always called, even when we > have a via header. Also seems weird that this takes request and the next takes > *request. > > I think to keep things simplest, you should just merge them into > DataReductionProxyUsageStats::RecordByteHistograms(*request, > *data_decution_proxy_enable_, proxy_config_getter_.Run()); Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:409: || HasDataReductionProxyViaHeader(request->response_headers(), NULL)) { On 2014/09/18 18:39:30, mmenke wrote: > Searching through the header every time we get a k or two seems needlessly heavy > weight. Maybe use UserData to associate data-reduction-proxy information with > the request, when headers are first read? Aren't there a lot of things that search through all the headers multiple times every time we get a k or two, such as the http caching logic? If searching through the headers is so slow, then wouldn't it make more sense to make the optimization in HttpResponseHeaders so that everything gets faster? Since every via header value corresponds to a proxy or something that the response passed through, the total number of via header values would typically be very small. The complication with setting some UserData for the via header presence when the headers are first read is that we would have to ensure that the via header is not overridden or removed before the request completes. I don't see any reason that something might want to remove it, but do you think that any performance gains of doing this would be worth the additional complexity? https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:416: && request->GetResponseCode() < net::HTTP_INTERNAL_SERVER_ERROR) { On 2014/09/18 18:39:30, mmenke wrote: > nit: Not sure if the DRP code uses another style, but the generally preferred > style is the operator on the previous line. Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:53: void HeadersToRaw(std::string* headers) { On 2014/09/18 19:55:57, bengr1 wrote: > I think duplicates of this function live elsewhere in the data reduction proxy > component. would you please coalesce these all into one *test_utils file? Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:90: On 2014/09/18 19:55:57, bengr1 wrote: > Here and above, I don't think the blank lines improve readability. Remove. Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:283: kPrimaryHistogramName)); On 2014/09/18 19:55:57, bengr1 wrote: > move up a with previous line? Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:286: kFallbackHistogramName)); On 2014/09/18 19:55:57, bengr1 wrote: > move up a with previous line? Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:418: WasDataReductionProxyUsed(fake_request.get(), NULL)) On 2014/09/18 19:55:57, bengr1 wrote: > Indentation seems off. Done. https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:438: On 2014/09/18 19:55:57, bengr1 wrote: > Remove extra blank line Done.
https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:409: || HasDataReductionProxyViaHeader(request->response_headers(), NULL)) { On 2014/09/19 01:05:01, sclittle wrote: > On 2014/09/18 18:39:30, mmenke wrote: > > Searching through the header every time we get a k or two seems needlessly > heavy > > weight. Maybe use UserData to associate data-reduction-proxy information with > > the request, when headers are first read? > > Aren't there a lot of things that search through all the headers multiple times > every time we get a k or two, such as the http caching logic? If searching > through the headers is so slow, then wouldn't it make more sense to make the > optimization in HttpResponseHeaders so that everything gets faster? Since every > via header value corresponds to a proxy or something that the response passed > through, the total number of via header values would typically be very small. > > The complication with setting some UserData for the via header presence when the > headers are first read is that we would have to ensure that the via header is > not overridden or removed before the request completes. I don't see any reason > that something might want to remove it, but do you think that any performance > gains of doing this would be worth the additional complexity? Oh, sorry - was thinking this was being called on every read, not on complete, which seemed like a ton of work purely for metrics (Just imagine a 4 GB download in 1000k chunks...Probably wouldn't slow things down noticeably, but still seems like a ton of completely pointless work). Since we only do this on complete, I'm fine with this.
On 2014/09/19 01:39:03, mmenke wrote: > https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... > File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc > (right): > > https://codereview.chromium.org/577343002/diff/1/components/data_reduction_pr... > components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:409: > || HasDataReductionProxyViaHeader(request->response_headers(), NULL)) { > On 2014/09/19 01:05:01, sclittle wrote: > > On 2014/09/18 18:39:30, mmenke wrote: > > > Searching through the header every time we get a k or two seems needlessly > > heavy > > > weight. Maybe use UserData to associate data-reduction-proxy information > with > > > the request, when headers are first read? > > > > Aren't there a lot of things that search through all the headers multiple > times > > every time we get a k or two, such as the http caching logic? If searching > > through the headers is so slow, then wouldn't it make more sense to make the > > optimization in HttpResponseHeaders so that everything gets faster? Since > every > > via header value corresponds to a proxy or something that the response passed > > through, the total number of via header values would typically be very small. > > > > The complication with setting some UserData for the via header presence when > the > > headers are first read is that we would have to ensure that the via header is > > not overridden or removed before the request completes. I don't see any reason > > that something might want to remove it, but do you think that any performance > > gains of doing this would be worth the additional complexity? > > Oh, sorry - was thinking this was being called on every read, not on complete, > which seemed like a ton of work purely for metrics (Just imagine a 4 GB download > in 1000k chunks...Probably wouldn't slow things down noticeably, but still seems > like a ton of completely pointless work). Since we only do this on complete, > I'm fine with this. Err... 1000k = 1k
https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:82: // The data reduction proxy via header is present, so don't record anything. Would it be useful to count these so we know the fraction of responses without via headers? https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:65: // Record all the data reduction proxy bytes-related histograms for the "Records" https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:70: const net::ProxyConfig& data_reduction_proxy_config); Forward declare ProxyConfig https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:77: // RecordBytesHistograms instead of calling this method directly. What test uses it? Can't you friend the test or make this protected? https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:99: DCHECK( Strange indentation. How about: DCHECK(test_job... url::kHttpScheme, test_job_interceptor_)); https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:405: WasDataReductionProxyUsed(fake_request.get(), NULL)).WillRepeatedly( Indentation seems wrong.
https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:82: // The data reduction proxy via header is present, so don't record anything. On 2014/09/19 21:42:56, bengr1 wrote: > Would it be useful to count these so we know the fraction of responses without > via headers? That's what the other UMA added in this CL is for: "DataReductionProxy.MissingViaHeader{4xx|Other}ResponseBytes", which can be compared to BypassedBytes.NotBypassed as well as the other bypassed bytes histograms. https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:65: // Record all the data reduction proxy bytes-related histograms for the On 2014/09/19 21:42:56, bengr1 wrote: > "Records" Done. https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:70: const net::ProxyConfig& data_reduction_proxy_config); On 2014/09/19 21:42:56, bengr1 wrote: > Forward declare ProxyConfig Done. https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:77: // RecordBytesHistograms instead of calling this method directly. On 2014/09/19 21:42:56, bengr1 wrote: > What test uses it? Can't you friend the test or make this protected? This particular method isn't actually yet used by any tests; I hope that this is soon rectified and that tests are added for it like the tests for RecordMissingViaHeaderBytes. The issue here is that we want to test RecordBypassedBytesHistograms separately from RecordMissingViaHeaderBytes because it significantly simplifies the tests, and to allow this the tests must be able to directly call one or the other. This is a question of coding style; the main options I see here are: (1) Make RecordBypassedBytes and RecordMissingViaHeaderBytes public like they are here, so that the tests can call them directly (2) Make RecordBypassedBytes and RecordMissingViaHeaderBytes private, friend the test, and add "proxy" methods in the DataReductionProxyUsageStatsTest class that forward calls and arguments on to the real RecordBypassedBytes and RecordMissingViaHeaderBytes methods. I originally chose (1) because it was simpler and easier, and adding "proxy" methods in the test class that forward arguments as described in (2) seemed ugly to me. Do you think otherwise or see another option? https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:99: DCHECK( On 2014/09/19 21:42:56, bengr1 wrote: > Strange indentation. How about: > > DCHECK(test_job... > url::kHttpScheme, test_job_interceptor_)); Done. https://codereview.chromium.org/577343002/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:405: WasDataReductionProxyUsed(fake_request.get(), NULL)).WillRepeatedly( On 2014/09/19 21:42:56, bengr1 wrote: > Indentation seems wrong. Done.
https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:190: const BooleanPrefMember& data_reduction_proxy_enabled, Forward declare? https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:412: URLRequest& request) { Can these methods all be const? https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:417: if (!data_reduction_proxy_params_->WasDataReductionProxyUsed(&request, nit: I'd move &request, down to the next line with "NULL" and indent 4 from !data_reduction... https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:21: class HttpResponseHeaders; Alphabetize forward declarations. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:69: net::URLRequest& request, Don't use non-const refs. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:78: // RecordBytesHistograms instead of calling this method directly. As discussed, please just friend the tests. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:92: void RecordMissingViaHeaderBytes(net::URLRequest& request); Don't use non-const refs.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:190: const BooleanPrefMember& data_reduction_proxy_enabled, On 2014/09/22 19:36:16, bengr1 wrote: > Forward declare? BooleanPrefMember is a typedef, so we can't forward declare it. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:412: URLRequest& request) { On 2014/09/22 19:36:16, bengr1 wrote: > Can these methods all be const? Unfortunately, no. It calls request->received_response_content_length(), which is a non-const method for some reason. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:417: if (!data_reduction_proxy_params_->WasDataReductionProxyUsed(&request, On 2014/09/22 19:36:16, bengr1 wrote: > nit: I'd move &request, down to the next line with "NULL" and indent 4 from > !data_reduction... Since the "&" is gone now that |request| is a pointer, this all fits on one line now. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:21: class HttpResponseHeaders; On 2014/09/22 19:36:16, bengr1 wrote: > Alphabetize forward declarations. Done. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:69: net::URLRequest& request, On 2014/09/22 19:36:16, bengr1 wrote: > Don't use non-const refs. Done. I'll change RecordBypassedBytesHistograms as well. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:78: // RecordBytesHistograms instead of calling this method directly. On 2014/09/22 19:36:16, bengr1 wrote: > As discussed, please just friend the tests. Done. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:92: void RecordMissingViaHeaderBytes(net::URLRequest& request); On 2014/09/22 19:36:16, bengr1 wrote: > Don't use non-const refs. Done.
lgtm, with nit https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:190: const BooleanPrefMember& data_reduction_proxy_enabled, On 2014/09/22 20:54:25, sclittle wrote: > On 2014/09/22 19:36:16, bengr1 wrote: > > Forward declare? > > BooleanPrefMember is a typedef, so we can't forward declare it. Can you include it then? https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:412: URLRequest& request) { On 2014/09/22 20:54:25, sclittle wrote: > On 2014/09/22 19:36:16, bengr1 wrote: > > Can these methods all be const? > > Unfortunately, no. It calls request->received_response_content_length(), which > is a non-const method for some reason. Would you mind filing a bug if you believe received_response_content_length() should be const?
https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:190: const BooleanPrefMember& data_reduction_proxy_enabled, On 2014/09/22 22:01:26, bengr1 wrote: > On 2014/09/22 20:54:25, sclittle wrote: > > On 2014/09/22 19:36:16, bengr1 wrote: > > > Forward declare? > > > > BooleanPrefMember is a typedef, so we can't forward declare it. > > Can you include it then? Done. https://codereview.chromium.org/577343002/diff/60001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:412: URLRequest& request) { On 2014/09/22 22:01:26, bengr1 wrote: > On 2014/09/22 20:54:25, sclittle wrote: > > On 2014/09/22 19:36:16, bengr1 wrote: > > > Can these methods all be const? > > > > Unfortunately, no. It calls request->received_response_content_length(), which > > is a non-const method for some reason. > > Would you mind filing a bug if you believe received_response_content_length() > should be const? Sure.
net/chrome_network_delegate.cc LGTM
https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:81: bool is_primary, const net::HttpResponseHeaders* headers) { Nit: 1 param per line if first one is wrapped. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:90: UMA_HISTOGRAM_CUSTOM_ENUMERATION( It's better to use an UMA_HISTOGRAM_SPARSE_SLOWLY() when logging response codes - that way the underlying histogram doesn't preallocate an array for all possible response codes. Then, you don't need the util functions you call below. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:91: "DataReductionProxy.MissingViaHeaderResponseCodePrimary", Consider adding .'s to delimit different parts of the name, so that you get better grouping on the dashboard. e.g.: DataReductionProxy.MissingViaHeader.ResponseCode.Primary DataReductionProxy.MissingViaHeader.ResponseCode.Fallback And similarly for the ones below. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:42: bool is_primary, const net::HttpResponseHeaders* headers); Nit: 1 param per line if first one is wrapped. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:53: const char* histogram_name) { Pass these as const std::string&. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:55: base::StatisticsRecorder::FindHistogram(histogram_name); Out of curiosity, have you looked at the histogram_tester.cc class to check if it would be a better fit in this case? (FWIW, I'm okay with not changing this existing code if it doesn't provide substantial benefit, just wondering whether that class didn't fit your nits or whether you didn't know about it.) https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:61: const base::HistogramSamples& initial_samples, const char* histogram_name) { Nit: 1 param per line if the first param is on a new line in function signature. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:112: const GURL& url, const std::string& raw_response_headers) { Nit: 1 param per line if the first param is on a new line in function signature.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:81: bool is_primary, const net::HttpResponseHeaders* headers) { On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Nit: 1 param per line if first one is wrapped. Done. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:90: UMA_HISTOGRAM_CUSTOM_ENUMERATION( On 2014/09/23 15:12:13, Alexei Svitkine wrote: > It's better to use an UMA_HISTOGRAM_SPARSE_SLOWLY() when logging response codes > - that way the underlying histogram doesn't preallocate an array for all > possible response codes. Then, you don't need the util functions you call below. Done. I'll keep the call to MapStatusCodeForHistogram though, since the response code could conceivably be any non-negative integer specified by a server. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:91: "DataReductionProxy.MissingViaHeaderResponseCodePrimary", On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Consider adding .'s to delimit different parts of the name, so that you get > better grouping on the dashboard. > > e.g.: > > DataReductionProxy.MissingViaHeader.ResponseCode.Primary > DataReductionProxy.MissingViaHeader.ResponseCode.Fallback > > And similarly for the ones below. Done. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:42: bool is_primary, const net::HttpResponseHeaders* headers); On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Nit: 1 param per line if first one is wrapped. Done. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:53: const char* histogram_name) { On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Pass these as const std::string&. Removed functions. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:55: base::StatisticsRecorder::FindHistogram(histogram_name); On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Out of curiosity, have you looked at the histogram_tester.cc class to check if > it would be a better fit in this case? (FWIW, I'm okay with not changing this > existing code if it doesn't provide substantial benefit, just wondering whether > that class didn't fit your nits or whether you didn't know about it.) Thanks, I had no idea histogram_tester existed. I've changed these tests to use it. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:61: const base::HistogramSamples& initial_samples, const char* histogram_name) { On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Nit: 1 param per line if the first param is on a new line in function signature. Removed functions. https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:112: const GURL& url, const std::string& raw_response_headers) { On 2014/09/23 15:12:13, Alexei Svitkine wrote: > Nit: 1 param per line if the first param is on a new line in function signature. Done.
https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:90: UMA_HISTOGRAM_CUSTOM_ENUMERATION( On 2014/09/23 18:13:41, sclittle wrote: > On 2014/09/23 15:12:13, Alexei Svitkine wrote: > > It's better to use an UMA_HISTOGRAM_SPARSE_SLOWLY() when logging response > codes > > - that way the underlying histogram doesn't preallocate an array for all > > possible response codes. Then, you don't need the util functions you call > below. > > Done. I'll keep the call to MapStatusCodeForHistogram though, since the response > code could conceivably be any non-negative integer specified by a server. Sorry, I don't understand what you mean for your reason for keeping MapStatusCodeForHistogram. The sparse macro allows logging any non-negative values, so I don't think the extra MapStatusCodeForHistogram() call is needed, but perhaps I'm just not understanding your motivation. Could you elaborate? https://codereview.chromium.org/577343002/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:109: net::TestJobInterceptor* test_job_interceptor_; Nit: Add a comment that this is owned by |test_job_factory_|.
https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/577343002/diff/120001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:90: UMA_HISTOGRAM_CUSTOM_ENUMERATION( On 2014/09/23 18:20:06, Alexei Svitkine wrote: > On 2014/09/23 18:13:41, sclittle wrote: > > On 2014/09/23 15:12:13, Alexei Svitkine wrote: > > > It's better to use an UMA_HISTOGRAM_SPARSE_SLOWLY() when logging response > > codes > > > - that way the underlying histogram doesn't preallocate an array for all > > > possible response codes. Then, you don't need the util functions you call > > below. > > > > Done. I'll keep the call to MapStatusCodeForHistogram though, since the > response > > code could conceivably be any non-negative integer specified by a server. > > Sorry, I don't understand what you mean for your reason for keeping > MapStatusCodeForHistogram. The sparse macro allows logging any non-negative > values, so I don't think the extra MapStatusCodeForHistogram() call is needed, > but perhaps I'm just not understanding your motivation. Could you elaborate? On second thought, you're right, we don't need this call. The idea behind keeping the call was to prevent the logging of response codes that don't mean anything anyway, but we don't need to be filtering those out here in Chrome. I've removed the call. https://codereview.chromium.org/577343002/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:109: net::TestJobInterceptor* test_job_interceptor_; On 2014/09/23 18:20:06, Alexei Svitkine wrote: > Nit: Add a comment that this is owned by |test_job_factory_|. Done.
LGTM, thanks!
The CQ bit was checked by sclittle@chromium.org
The CQ bit was unchecked by sclittle@chromium.org
The CQ bit was checked by sclittle@chromium.org
The CQ bit was unchecked by sclittle@chromium.org
The CQ bit was checked by sclittle@chromium.org
The CQ bit was unchecked by sclittle@chromium.org
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/577343002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sclittle@chromium.org
The CQ bit was unchecked by sclittle@chromium.org
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/577343002/220001
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as 788ee83ba3cc2ea57b3f5beda20df04edf0ab361
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ba1e1e3f7ea6edae987e6cfa92086807bf74260b Cr-Commit-Position: refs/heads/master@{#296297}
Message was sent while issue was closed.
https://codereview.chromium.org/577343002/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc (right): https://codereview.chromium.org/577343002/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc:64: DCHECK(test_job_factory_.SetProtocolHandler(url::kHttpScheme, You can't do this. SetProtocolHandler() won't run in Release mode when DCHECKs are disabled. |
