Run a holdback for using brotli with connection to data saver proxy |
|||||||
Issue descriptionOn HTTPS connections to data saver proxy, we add "br" to the accept-encoding header. Finally, if the page is loaded via non-data saver proxy or directly, we remove "br" from accept-encoding header. This current implementation is complicated as it requires Chrome to modify headers that pertain to network implementation. With network servicification, we would need to rewrite this implementation. Previous experiment analysis did not show any impact on page load performance metrics: https://docs.google.com/document/d/1_aWQtesB3al8l77gyh6UX0eEdPGW7eSPuDBw-JBP-lA/edit We should run a holdback experiment to see if that's still the case. If so, we should consider removing the brotli support for data saver proxy.
,
Oct 2
,
Oct 2
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d884aa86877aec7f54bee9cfa6868ee8d34c31f9 commit d884aa86877aec7f54bee9cfa6868ee8d34c31f9 Author: Tarun Bansal <tbansal@chromium.org> Date: Tue Oct 02 20:53:21 2018 Add holdback for using brotli with data saver proxies Bug: 891458 Change-Id: I261651abb0ecfec9b46fa1a6ee8b269963de9f7e Reviewed-on: https://chromium-review.googlesource.com/c/1258049 Reviewed-by: Scott Little <sclittle@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#595972} [modify] https://crrev.com/d884aa86877aec7f54bee9cfa6868ee8d34c31f9/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc [modify] https://crrev.com/d884aa86877aec7f54bee9cfa6868ee8d34c31f9/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc [modify] https://crrev.com/d884aa86877aec7f54bee9cfa6868ee8d34c31f9/components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc [modify] https://crrev.com/d884aa86877aec7f54bee9cfa6868ee8d34c31f9/components/data_reduction_proxy/core/common/data_reduction_proxy_features.h
,
Oct 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8fde0ca32f978d181544a7176bba4cdff6a22a6 commit d8fde0ca32f978d181544a7176bba4cdff6a22a6 Author: Tarun Bansal <tbansal@chromium.org> Date: Thu Oct 18 22:23:49 2018 Add DataReductionProxyBrotliHoldback to field trial testing config Bug: 891458 Change-Id: I144a1eb2c528b0e02f45a7b000628c07b1ba78c3 Reviewed-on: https://chromium-review.googlesource.com/c/1287893 Reviewed-by: Steven Holte <holte@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#600942} [modify] https://crrev.com/d8fde0ca32f978d181544a7176bba4cdff6a22a6/testing/variations/fieldtrial_testing_config.json
,
Oct 23
,
Oct 26
Assigning to Raj as per OKRs. Here is the UMA dashboard I am using to track the results: https://uma.googleplex.com/p/chrome/variations/?sid=7990de14bc975c2a196e8d5e2d1442d0
,
Oct 31
I have added some savings metrics in the dashboard https://uma.googleplex.com/p/chrome/variations/?sid=dfa0935a2888f6ec5b87c7067fc513b6 Performance: No regression observed in FCP, memory metrics. PageLoad.Clients.DataReductionProxy.PaintTiming.NavigationToFirstContentfulPaint, Memory.Browser.PrivateMemoryFootprint. Savings: * Median daily bytes via data saver has increased 1% (Net.DailyContentLength_DataReductionProxyEnabled) * Median page size has increased 7% (PageLoad.Clients.DataReductionProxy.Experimental.Bytes.*). This metric has some caveats in some video resources would not get logged. So ignoring this metric. * More pages see inflation (PageLoad.Clients.DataReductionProxy.Experimental.Bytes.Network.Inflation). However this is low compared to total pageloads.
,
Oct 31
Raj, thanks for the analysis. I think for Net.DailyContentLength_DataReductionProxyEnabled, we care about the "Sum" and not the median value. There is only a difference of 0.02% in sum. Also, it seems result is not statistically significant. If we change data interval to 14-day period, it actually decreases by 0.16% when holdback is enabled. https://screenshot.googleplex.com/WfMRLkfNJCm
,
Oct 31
Ack. None of the differences are statistically significant. So we could either bump experiment(beta and/or 1% stable), or I could start removing the code.
,
Oct 31
I think the experiment is already running on 50% beta population. http://shortn/_5IRc7arSEI. I think we can wait and run it on 1% M-71 stable before removing it.
,
Dec 12
It seems we have enough data now (https://uma.googleplex.com/p/chrome/variations/?sid=4f340ed2048786c53c23ab065e8ff6d4). I do not see any statistically significant changes across all connection types or when restricted to 2G (https://uma.googleplex.com/p/chrome/variations/?sid=5dd913df84f241423528e6c72cc7fd6c). Should we enable this for 100% users?
,
Dec 12
Is this running in stable, or still just beta? I'm not following the bug exactly -- what would enabling for 100% of users tell us? If the experiment is in stable and results are definitive, then we can remove the code. If the results aren't definitive, then we need some users in the control group so we can get more data...
,
Dec 12
The experiment has not been enabled for stable yet. It's only in Beta. I was thinking we could just use the Beta data since now we have more than 28 days of data. IIRC, we had enabled this feature based on only the Beta channel data. This is contradictory to what I said in #11 above.
,
Dec 12
Why not run a small stable experiment? I'm guessing when we enabled the feature, we looked at beta and then looked at a small stable experiment. We at least looked at stable numbers once it was launched and we would have disabled it if things looked wrong. If there is a big time crunch, then it's probably fine to make a change based on beta only, but seems better to just run a short stable experiment to be sure as it's standard protocol.
,
Jan 2
UMA dashboard when restricted to slow connections: https://uma.googleplex.com/p/chrome/variations/?sid=bcc3e78a2bcda1f57ac4da1b2047e622 All connections: https://uma.googleplex.com/p/chrome/variations/?sid=0d4c2c5586323f2482d572d7127e6dd2 The data is from 10% M-71 stable (running since Dec 13). The dashboard shows no change in metrics, so I am planning to enable this 100%.
,
Jan 2
Thanks for running the experiment. Looks like a whole-lotta no impact. Is the plan to enable for 100%, and then remove code? I can file a bug to remove server code.
,
Jan 2
> Is the plan to enable for 100%, and then remove code? First remove code, then enable for 100% for M-71+ population.
,
Jan 2
Actually the bug is assigned to Raj, so probably I won't be the one doing it.
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46 commit faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46 Author: Tarun Bansal <tbansal@chromium.org> Date: Wed Jan 09 19:10:34 2019 Data reduction proxy: Remove brotli integration code Remove addition of "br" to accept-encoding header when data reduction proxy is in use. Change-Id: I13141ef9558ad71b28271c2c73c1a6a4b379a8b8 Bug: 891458 Reviewed-on: https://chromium-review.googlesource.com/c/1400345 Reviewed-by: rajendrant <rajendrant@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#621254} [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/common/data_reduction_proxy_features.cc [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/common/data_reduction_proxy_features.h [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/common/data_reduction_proxy_params.h [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc [modify] https://crrev.com/faf5db2a1ba1501ef6fd24b55e5e7a8afa5f0a46/testing/variations/fieldtrial_testing_config.json
,
Jan 9
,
Jan 10
Marking as fixed. "br" would not be used for M-71 or later versions. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by tbansal@chromium.org
, Oct 2