DataReductionProxy Holdback Experiment is not holding back |
|||||||||
Issue description
When a user is in a holdback experiment (e.g. chrome --force-fieldtrials=DataCompressionProxyHoldback/Enabled/) they should not have traffic navigated through the data reduction proxy.
Expected:
WasEligibleWithoutHoldback should not DCHECK
Actual:
[101620:101656:0526/104900.500544:FATAL:data_reduction_proxy_network_delegate.cc(591)] Check failed: proxy_info.is_empty() || proxy_info.is_direct() || !data_reduction_proxy_config_->IsDataReductionProxy( proxy_info.proxy_server(), nullptr).
I started to investigate:
Added
LOG(WARNING)<< "Calling WasEligibleWithoutHoldback";
LOG(WARNING) << proxy_info.is_empty();
LOG(WARNING) << proxy_info.is_direct();
LOG(WARNING) << data_reduction_proxy_config_->IsDataReductionProxy(
proxy_info.proxy_server(), nullptr);
LOG(WARNING)<<request.url().spec();
before the DCHECK
[101620:101656:0526/104900.500353:WARNING:data_reduction_proxy_network_delegate.cc(583)] Calling WasEligibleWithoutHoldback
[101620:101656:0526/104900.500423:WARNING:data_reduction_proxy_network_delegate.cc(584)] 0
[101620:101656:0526/104900.500451:WARNING:data_reduction_proxy_network_delegate.cc(585)] 0
[101620:101656:0526/104900.500477:WARNING:data_reduction_proxy_network_delegate.cc(586)] 1
[101620:101656:0526/104900.500512:WARNING:data_reduction_proxy_network_delegate.cc(588)] http://aws1.mdw.la/fw/
[101620:101656:0526/104900.500544:FATAL:data_reduction_proxy_network_delegate.cc(591)] Check failed: proxy_info.is_empty() || proxy_info.is_direct() || !data_reduction_proxy_config_->IsDataReductionProxy( proxy_info.proxy_server(), nullptr).
This indicates that the DRP was used for the URL http://aws1.mdw.la/fw/ despite being in the holdback experiment.
Based on UMA this appears to be broken starting in M58:
https://uma.googleplex.com/p/chrome/variations/?sid=76f3f7615ca5350d0e9bf98ce1e56795
,
May 26 2017
From the dashboard (https://uma.googleplex.com/p/chrome/variations/?sid=6c276f613806a82cd50fac306951806f), it seems that Enabled group has ~7.5k (almost negligible) navigations while Control has ~5.4M. Seems like it is working? It is possible that there is a DCHECK failure. If yes, we should update the DCHECK.
,
May 30 2017
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93fb0767a75b443bf25bc99b1b97f48f7b074ccd commit 93fb0767a75b443bf25bc99b1b97f48f7b074ccd Author: tbansal <tbansal@chromium.org> Date: Wed May 31 01:30:53 2017 Do not attempt to use data reduction proxy when holdback is enabled BUG= 726773 Review-Url: https://codereview.chromium.org/2905423002 Cr-Commit-Position: refs/heads/master@{#475723} [modify] https://crrev.com/93fb0767a75b443bf25bc99b1b97f48f7b074ccd/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc [modify] https://crrev.com/93fb0767a75b443bf25bc99b1b97f48f7b074ccd/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc [modify] https://crrev.com/93fb0767a75b443bf25bc99b1b97f48f7b074ccd/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
,
May 31 2017
,
May 31 2017
Please tag with appropriate OSs. Thanks.
,
May 31 2017
,
Jun 1 2017
Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/025bdf115218a3ac729d033b665a093901d541e6 commit 025bdf115218a3ac729d033b665a093901d541e6 Author: Tarun Bansal <tbansal@google.com> Date: Thu Jun 01 16:44:48 2017 Do not attempt to use data reduction proxy when holdback is enabled BUG= 726773 Review-Url: https://codereview.chromium.org/2905423002 Cr-Original-Commit-Position: refs/heads/master@{#475723} Review-Url: https://codereview.chromium.org/2915103002 . Cr-Commit-Position: refs/branch-heads/3112@{#93} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/025bdf115218a3ac729d033b665a093901d541e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc [modify] https://crrev.com/025bdf115218a3ac729d033b665a093901d541e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate_unittest.cc [modify] https://crrev.com/025bdf115218a3ac729d033b665a093901d541e6/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
,
Jun 1 2017
,
Jun 1 2017
This bug requires manual review: We are only 4 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 1 2017
We are targeting M59 to stable promotion for next week. Can you please tell why this is urgent? Has this been tested and verified in Canary, and is this a safe merge? My recommendation is to wait until M60, unless this is absolutely critical.
,
Jun 1 2017
OK, if it is less than a week out, then I will skip M-59. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by ryansturm@chromium.org
, May 26 2017