Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
298883011:
Record errors that trigger a data reduction proxy bypass (Closed)
|
Created:
6 years, 7 months ago by bengr Modified:
6 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, jar (doing other things), asvitkine+watch_chromium.org, Matt Welsh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionUpdate UMA to track bypasses due to 4xx responses that are missing the proxy's via header and bypasses due to network errors.
BUG=376148
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273839
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Addressed comments from mdw #
Total comments: 13
Patch Set 3 : Addressed comments #
Total comments: 7
Patch Set 4 : addressed comments from mef #
Total comments: 10
Patch Set 5 : addressed more comments from mef #
Total comments: 3
Patch Set 6 : Removed new bypass logic #Patch Set 7 : rebase #
Messages
Total messages: 24 (0 generated)
zea: google_apis/*, jingle/* mef: net/* asvitkine: tools/metrics/*
https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_l... File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:514: //std::string pac_string = base::StringPrintf( remove comments? https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_t... net/http/http_network_transaction.cc:1016: // originated at the proxy. We shouldn't be bypassing all 4xx errors. This should only be happening if the Via header is missing. Many origin sites produce (valid) 4xx errors and the last thing we want is all of them to turn into bypasses. https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... net/http/http_response_headers.cc:1514: // Do not bypass other 4xx responses on an incorrect or missing Via Wait, according to your doc, you should be doing a 1-5 minute bypass in the case of a 4xx with a missing Via and a missing GFE header. https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2257: net::ProxyService::BYPASS_EVENT_TYPE_MAX, I think this is wrong - you should do a 1-5 minute bypass in this case. https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1463: UMA_HISTOGRAM_CUSTOM_ENUMERATION( Shouldn't there be an else here to prevent double-counting in the is_primary case? https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.h:290: // Bypass the proxy because the proxy, not the origin, sent a 4xx response. I don't understand why this is a different case. CLIENT_ERROR_BYPASS is also confusing; what does this have to do with a "client error"? https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.h:305: // proxy (|is_primary| is true) or the dta reduction proxy fallback. typo: data
https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_l... File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:514: //std::string pac_string = base::StringPrintf( On 2014/05/23 23:56:30, Matt Welsh wrote: > remove comments? Done. https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_network_t... net/http/http_network_transaction.cc:1016: // originated at the proxy. On 2014/05/23 23:56:30, Matt Welsh wrote: > We shouldn't be bypassing all 4xx errors. This should only be happening if the > Via header is missing. Many origin sites produce (valid) 4xx errors and the last > thing we want is all of them to turn into bypasses. > GetDataReductionBypassEventType decides if we should bypass. This code just sets up the right bypass if we should. https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... net/http/http_response_headers.cc:1514: // Do not bypass other 4xx responses on an incorrect or missing Via On 2014/05/23 23:56:30, Matt Welsh wrote: > Wait, according to your doc, you should be doing a 1-5 minute bypass in the case > of a 4xx with a missing Via and a missing GFE header. Discussed offline. Changed. https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2257: net::ProxyService::BYPASS_EVENT_TYPE_MAX, On 2014/05/23 23:56:30, Matt Welsh wrote: > I think this is wrong - you should do a 1-5 minute bypass in this case. Done. https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1463: UMA_HISTOGRAM_CUSTOM_ENUMERATION( On 2014/05/23 23:56:30, Matt Welsh wrote: > Shouldn't there be an else here to prevent double-counting in the is_primary > case? I return in the first case. https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.h:290: // Bypass the proxy because the proxy, not the origin, sent a 4xx response. On 2014/05/23 23:56:30, Matt Welsh wrote: > I don't understand why this is a different case. CLIENT_ERROR_BYPASS is also > confusing; what does this have to do with a "client error"? 4xx's are commonly called client errors. I agree it is confusing that we only bypass if the client error originated at the proxy. Changed. https://codereview.chromium.org/298883011/diff/120001/net/proxy/proxy_service... net/proxy/proxy_service.h:305: // proxy (|is_primary| is true) or the dta reduction proxy fallback. On 2014/05/23 23:56:30, Matt Welsh wrote: > typo: data Done.
lgtm
https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1460: GetAllErrorCodesForUma()); Instead, it's better to use a sparse histogram here and below. See Variations.FailedRequestErrorCode for example. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2682: + Positive net error code that caused a the fallback data reduction proxy to Nit: "a the" -> "the" https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2683: + be bypassed an put on the proxy retry list. Nit: "an" -> "and" https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2684: + </summary> Nit: mention when this is logged - is it when the error occurs or something else? Same below. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2691: + Positive net error code that caused a the primary data reduction proxy to be Nit: "a the" -> "the" https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2692: + bypassed an put on the proxy retry list. Nit: "an" -> "and"
https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1460: GetAllErrorCodesForUma()); On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Instead, it's better to use a sparse histogram here and below. See > Variations.FailedRequestErrorCode for example. Done. Do we lose any fidelity by using the sparse histogram? https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2682: + Positive net error code that caused a the fallback data reduction proxy to On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Nit: "a the" -> "the" Done. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2683: + be bypassed an put on the proxy retry list. On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Nit: "an" -> "and" Done. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2684: + </summary> On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Nit: mention when this is logged - is it when the error occurs or something > else? Same below. Done. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2691: + Positive net error code that caused a the primary data reduction proxy to be On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Nit: "a the" -> "the" Done. https://codereview.chromium.org/298883011/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:2692: + bypassed an put on the proxy retry list. On 2014/05/26 17:25:08, Alexei Svitkine wrote: > Nit: "an" -> "and" Done.
LGTM https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/140001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1460: GetAllErrorCodesForUma()); On 2014/05/27 15:59:54, bengr1 wrote: > On 2014/05/26 17:25:08, Alexei Svitkine wrote: > > Instead, it's better to use a sparse histogram here and below. See > > Variations.FailedRequestErrorCode for example. > > Done. Do we lose any fidelity by using the sparse histogram? No, we don't. It's merely more efficient on the client side (only allocates memory for the buckets that are seen) with same results from the server-side POV.
lgtm
sorry for the delay. https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... net/http/http_response_headers.cc:1512: if (HasHeaderValue("Server", "GFE/2.0")) Um, why do you need to hardcode GFE here? https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... File net/http/http_response_headers_unittest.cc (right): https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... net/http/http_response_headers_unittest.cc:2264: "Server: GFE/2.0\n" What is expected if this was "Server: MyServer/1.5\n"? https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1197: true, result->proxy_server(), net_error); is |is_primary| expected to be |true| in both cases? https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service... net/proxy/proxy_service.h:145: int net_error, please add comment in regards to usage of |net_error| and its values.
https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/160001/net/http/http_response_... net/http/http_response_headers.cc:1512: if (HasHeaderValue("Server", "GFE/2.0")) On 2014/05/28 15:39:57, mef wrote: > Um, why do you need to hardcode GFE here? A missing via header with this server error indicates that the proxy front end rejected the request. The proxy should be bypasses for any retry of this request. In contrast, if a 4xx is returned that lacks the proxy via header and lacks the proxy front end server header, something more serious is wrong with the connection (e.g., a middle box is stripping headers). In that case, the proxy should be bypassed for longer. I added a comment to address concerns about deprecation. Note: In the next few weeks, I plan to move all DataReductionProxy code from HttpResponseHeaders to components/data_reduction_proxy. https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1197: true, result->proxy_server(), net_error); On 2014/05/28 15:39:57, mef wrote: > is |is_primary| expected to be |true| in both cases? No. Only in the first case: if (result->proxy_server().isDataReductionProxy())... https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service.h File net/proxy/proxy_service.h (right): https://codereview.chromium.org/298883011/diff/160001/net/proxy/proxy_service... net/proxy/proxy_service.h:145: int net_error, On 2014/05/28 15:39:57, mef wrote: > please add comment in regards to usage of |net_error| and its values. Done.
https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:546: "Server: good-proxy\r\n" Is this the data that is expected from server? Why did it change to 'good-proxy'? https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:594: ProxyRetryInfoMap::const_iterator i = Given that this is a map I think it is too brittle to expect particular order of proxies. I'd suggest EXPECT_EQ(1u, proxy_service_->proxy_retry_info().count(data_reduction_proxy_fallback)); https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:710: // when the data reduction proxy via header is absent on a 4xx. Is this a valid comment? I see 'Via' header in mock reads... https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... net/http/http_response_headers.cc:1517: return ProxyService::PROXY_4XX_BYPASS; I'm still uneasy about hardcoded "GFE/2.0", and I'm looking for suggestions. https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1202: true, result->proxy_server(), net_error); shouldn't this be "false"?
https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... File net/http/http_network_layer_unittest.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:546: "Server: good-proxy\r\n" On 2014/05/29 16:41:38, mef wrote: > Is this the data that is expected from server? Why did it change to > 'good-proxy'? "Not proxy" was incorrect. The test falls from a bad proxy to a good one. https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:594: ProxyRetryInfoMap::const_iterator i = On 2014/05/29 16:41:38, mef wrote: > Given that this is a map I think it is too brittle to expect particular order of > proxies. > > I'd suggest EXPECT_EQ(1u, > proxy_service_->proxy_retry_info().count(data_reduction_proxy_fallback)); Good suggestion. Thanks. Done. https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... net/http/http_network_layer_unittest.cc:710: // when the data reduction proxy via header is absent on a 4xx. On 2014/05/29 16:41:38, mef wrote: > Is this a valid comment? I see 'Via' header in mock reads... Done. https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... net/http/http_response_headers.cc:1517: return ProxyService::PROXY_4XX_BYPASS; On 2014/05/29 16:41:38, mef wrote: > I'm still uneasy about hardcoded "GFE/2.0", and I'm looking for suggestions. OK https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service.cc File net/proxy/proxy_service.cc (right): https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service... net/proxy/proxy_service.cc:1202: true, result->proxy_server(), net_error); On 2014/05/29 16:41:38, mef wrote: > shouldn't this be "false"? Egad. It should. Done. Thanks!!!
On 2014/05/29 18:46:18, bengr1 wrote: > https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... > File net/http/http_network_layer_unittest.cc (right): > > https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... > net/http/http_network_layer_unittest.cc:546: "Server: good-proxy\r\n" > On 2014/05/29 16:41:38, mef wrote: > > Is this the data that is expected from server? Why did it change to > > 'good-proxy'? > > "Not proxy" was incorrect. The test falls from a bad proxy to a good one. > > https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... > net/http/http_network_layer_unittest.cc:594: ProxyRetryInfoMap::const_iterator i > = > On 2014/05/29 16:41:38, mef wrote: > > Given that this is a map I think it is too brittle to expect particular order > of > > proxies. > > > > I'd suggest EXPECT_EQ(1u, > > proxy_service_->proxy_retry_info().count(data_reduction_proxy_fallback)); > > Good suggestion. Thanks. Done. > > https://codereview.chromium.org/298883011/diff/180001/net/http/http_network_l... > net/http/http_network_layer_unittest.cc:710: // when the data reduction proxy > via header is absent on a 4xx. > On 2014/05/29 16:41:38, mef wrote: > > Is this a valid comment? I see 'Via' header in mock reads... > > Done. > > https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... > File net/http/http_response_headers.cc (right): > > https://codereview.chromium.org/298883011/diff/180001/net/http/http_response_... > net/http/http_response_headers.cc:1517: return ProxyService::PROXY_4XX_BYPASS; > On 2014/05/29 16:41:38, mef wrote: > > I'm still uneasy about hardcoded "GFE/2.0", and I'm looking for suggestions. > > OK > > https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service.cc > File net/proxy/proxy_service.cc (right): > > https://codereview.chromium.org/298883011/diff/180001/net/proxy/proxy_service... > net/proxy/proxy_service.cc:1202: true, result->proxy_server(), net_error); > On 2014/05/29 16:41:38, mef wrote: > > shouldn't this be "false"? > > Egad. It should. Done. Thanks!!! I have some real reservations about introducing the GFE detection (and 4xx bypass) in this CL given that there is a cleaner long-term solution, and it's unclear to me if there is an urgent need to get a fix in for M36 since the issue has existed for a while. The rest of the UMA changes seem fine.
https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... net/http/http_response_headers.cc:1516: if (HasHeaderValue("Server", "GFE/2.0")) Actually, I'm confused about why we even need this check here and why we can't just return PROXY_4XX_BYPASS. At the point this is called, I think that: a) We expect that the request was directed to the proxy. b) The proxy was accessed via SPDY (and TLS more importantly). c) There is not a "Via 1.1 Chrome Compression Proxy" header. d) There is a 4xx response code. In that case, we should be able to expect that this response is coming from the proxy itself and not from an origin server, and also not due to headers being stripped by intermediaries. If there are cases where the Via headers are being removed from the proxy even though it is forwarding a 4xx response from the origin servers then these are probably bugs in the proxy itself and should be resolved there. I also don't know if the behavior of responding with 4xx response codes is technically correct for the proxy in this case, and will try to see if there is a better way to signal a proxy-internal issue.
https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... net/http/http_response_headers.cc:1516: if (HasHeaderValue("Server", "GFE/2.0")) I think the reason for the GFE/2.0 header check is to distinguish the case where we *think* we're talking to the proxy (via HTTP) but we're actually talking to some other middlebox which is sending us a 4xx. Example would be a middlebox that sends a 404 for some request. The main rationale for this change is that the GFE can send 4xx responses on things like too-large URLs and the like, and we don't want to turn off the proxy in this case. That said we don't have UMA data telling us how often this happens. So one proposal: (1) Revert this change and continue bypassing 4xx responses that lack a Via header; (2) Make sure that the UMA reporting on bypass reasons includes 4xx bypass so we can track when it happens. On 2014/05/29 20:04:41, cbentzel wrote: > Actually, I'm confused about why we even need this check here and why we can't > just return PROXY_4XX_BYPASS. > > At the point this is called, I think that: > a) We expect that the request was directed to the proxy. > b) The proxy was accessed via SPDY (and TLS more importantly). > c) There is not a "Via 1.1 Chrome Compression Proxy" header. > d) There is a 4xx response code. > > In that case, we should be able to expect that this response is coming from the > proxy itself and not from an origin server, and also not due to headers being > stripped by intermediaries. If there are cases where the Via headers are being > removed from the proxy even though it is forwarding a 4xx response from the > origin servers then these are probably bugs in the proxy itself and should be > resolved there. > > I also don't know if the behavior of responding with 4xx response codes is > technically correct for the proxy in this case, and will try to see if there is > a better way to signal a proxy-internal issue. >
On 2014/05/29 20:49:17, Matt Welsh wrote: > https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... > File net/http/http_response_headers.cc (right): > > https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... > net/http/http_response_headers.cc:1516: if (HasHeaderValue("Server", "GFE/2.0")) > I think the reason for the GFE/2.0 header check is to distinguish the case where > we *think* we're talking to the proxy (via HTTP) but we're actually talking to > some other middlebox which is sending us a 4xx. Example would be a middlebox > that sends a 404 for some request. If I understand correctly, this code is only when speaking over SPDY. So it would need to be an SSL MITM. Not inconceivable, but rare. > > The main rationale for this change is that the GFE can send 4xx responses on > things like too-large URLs and the like, and we don't want to turn off the proxy > in this case. Yes, ideally we would just reroute the specific request instead of disabling other requests. It sounds like that is in the plans. I also took a look and there is unfortunately not a canonical HTTP response code that a proxy can use to signal problems. For example, http://www.squid-cache.org/Doc/config/deny_info/ shows that SQUID is set up to 403 by default, but it can be configured to do lots of other behavior. > > That said we don't have UMA data telling us how often this happens. So one > proposal: > (1) Revert this change and continue bypassing 4xx responses that lack a Via > header; > (2) Make sure that the UMA reporting on bypass reasons includes 4xx bypass so > we can track when it happens. > Sure. > > On 2014/05/29 20:04:41, cbentzel wrote: > > Actually, I'm confused about why we even need this check here and why we can't > > just return PROXY_4XX_BYPASS. > > > > At the point this is called, I think that: > > a) We expect that the request was directed to the proxy. > > b) The proxy was accessed via SPDY (and TLS more importantly). > > c) There is not a "Via 1.1 Chrome Compression Proxy" header. > > d) There is a 4xx response code. > > > > In that case, we should be able to expect that this response is coming from > the > > proxy itself and not from an origin server, and also not due to headers being > > stripped by intermediaries. If there are cases where the Via headers are being > > removed from the proxy even though it is forwarding a 4xx response from the > > origin servers then these are probably bugs in the proxy itself and should be > > resolved there. > > > > I also don't know if the behavior of responding with 4xx response codes is > > technically correct for the proxy in this case, and will try to see if there > is > > a better way to signal a proxy-internal issue. > >
https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... File net/http/http_response_headers.cc (right): https://codereview.chromium.org/298883011/diff/200001/net/http/http_response_... net/http/http_response_headers.cc:1516: if (HasHeaderValue("Server", "GFE/2.0")) On 2014/05/29 20:49:18, Matt Welsh wrote: > I think the reason for the GFE/2.0 header check is to distinguish the case where > we *think* we're talking to the proxy (via HTTP) but we're actually talking to > some other middlebox which is sending us a 4xx. Example would be a middlebox > that sends a 404 for some request. > > The main rationale for this change is that the GFE can send 4xx responses on > things like too-large URLs and the like, and we don't want to turn off the proxy > in this case. > > That said we don't have UMA data telling us how often this happens. So one > proposal: > (1) Revert this change and continue bypassing 4xx responses that lack a Via > header; > (2) Make sure that the UMA reporting on bypass reasons includes 4xx bypass so > we can track when it happens. > > > On 2014/05/29 20:04:41, cbentzel wrote: > > Actually, I'm confused about why we even need this check here and why we can't > > just return PROXY_4XX_BYPASS. > > > > At the point this is called, I think that: > > a) We expect that the request was directed to the proxy. > > b) The proxy was accessed via SPDY (and TLS more importantly). > > c) There is not a "Via 1.1 Chrome Compression Proxy" header. > > d) There is a 4xx response code. > > > > In that case, we should be able to expect that this response is coming from > the > > proxy itself and not from an origin server, and also not due to headers being > > stripped by intermediaries. If there are cases where the Via headers are being > > removed from the proxy even though it is forwarding a 4xx response from the > > origin servers then these are probably bugs in the proxy itself and should be > > resolved there. > > > > I also don't know if the behavior of responding with 4xx response codes is > > technically correct for the proxy in this case, and will try to see if there > is > > a better way to signal a proxy-internal issue. > > > Done.
lgtm
The CQ bit was checked by bengr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bengr@chromium.org/298883011/260001
Message was sent while issue was closed.
Change committed as 273839
Message was sent while issue was closed.
Thanks for making the changes to this CL |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
