SB2.Delay doesn't measure exactly the same thing with/without S13nSafeBrowsingParallelUrlCheck |
|||||||||
Issue descriptionWith S13nSafeBrowsingParallelUrlCheck: - At most one value is reported for each resource load. - Report for all resource types. - It doesn't exclude the cases where the interstitial page is showed and user action is involved. Without S13nSafeBrowsingParallelUrlCheck: - If a resource load is delayed multiple times (before starting request, before following redirects), there will be multiple values reported. - Some resource types are skipped on Android. - It excludes the cases where the interstitial page is showed and user action is involved. The most reasonable solution is to introduce a new histogram, so that we avoid changing the actual meaning of SB2.Delay, while recording useful data for an apple-to-apple comparison.
,
Nov 10 2017
,
Nov 10 2017
,
Nov 10 2017
Hi, Release managers. I wish I could merge this change to M63. This CL is a very safe change: it only logs a new UMA histogram, but doesn't change any actual behavior. I have looked at the UMA numbers from Canary and verified that the CL worked as expected. Background: S13nSafeBrowsingParallelUrlCheck is a finch experiment running on M63. And we found out that the histogram SB2.Delay is not an apple-to-apple comparison for the enable/control groups. Therefore, crosreview.com/756962 introduced a new histogram to better measure the impact of the experiment. Thanks!
,
Nov 10 2017
Pls apply appropriate OSs label. Thank you.
,
Nov 11 2017
Thanks, Krishna! Done.
,
Nov 11 2017
This bug requires manual review: M63 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 11 2017
Approving merge to M63 branch 3239 based on comment #4. Please merge ASAP. Thank you.
,
Nov 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8 commit 4c074246a2e5ad2d2d5c45b495d53f87e802e0d8 Author: Yuzhu Shen <yzshen@chromium.org> Date: Sat Nov 11 01:11:10 2017 Introduce a new histogram: SB2.NoUserActionResourceLoadingDelay It reports the total delay, in milliseconds, caused by SafeBrowsing for a resource load, if the SafeBrowsing interstitial page is not showed and therefore no user action is involved. At most one value is reported for each resource load. If SafeBrowsing causes delays at different stages of a load, the sum of all the delays will be reported. BUG= 782418 TBR=yzshen@chromium.org (cherry picked from commit 91a2dca5bef0a9851f4c853c834539240bc14356) Change-Id: Ie467d181c4f535dbb4e5fcc67a8e23c284a4b4f7 Reviewed-on: https://chromium-review.googlesource.com/756962 Commit-Queue: Yuzhu Shen <yzshen@chromium.org> Reviewed-by: Varun Khaneja <vakh@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#515379} Reviewed-on: https://chromium-review.googlesource.com/765099 Reviewed-by: Yuzhu Shen <yzshen@chromium.org> Cr-Commit-Position: refs/branch-heads/3239@{#450} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/base_resource_throttle.cc [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/base_resource_throttle.h [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/browser/browser_url_loader_throttle.cc [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/browser/browser_url_loader_throttle.h [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/common/utils.cc [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/common/utils.h [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/renderer/renderer_url_loader_throttle.cc [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/components/safe_browsing/renderer/renderer_url_loader_throttle.h [modify] https://crrev.com/4c074246a2e5ad2d2d5c45b495d53f87e802e0d8/tools/metrics/histograms/histograms.xml
,
Nov 15 2017
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 10 2017