New issue
Advanced search Search tips

Issue 851045 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Randomize variations setting and record UMA

Project Member Reported by paulmiller@chromium.org, Jun 8 2018

Issue description

With https://crrev.com/c/1050931, variations is enabled 100% in m68. We should break off some small percentage into a study with enabled/disabled groups. We can't use variations itself to enabled/disable variations, so we should hard-code the random assignment and record a UMA histogram with the result. We can then split other metrics by the new histogram as described under go/uma-dremel, "Histogram Filtered by a Secondary Histogram".
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jun 12 2018

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: M-68
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/074de24df77eee54efbb6fc3646528b008bdb513

commit 074de24df77eee54efbb6fc3646528b008bdb513
Author: Paul Miller <paulmiller@google.com>
Date: Sat Jun 16 00:26:37 2018

WebView: Add study for variations rollout

With https://crrev.com/c/1050931, variations is enabled 100% in m68. We want to
keep some small percentage disabled in order to compare metrics, but can't use
variations to do so. So hard-code the random group assignment instead, and add a
UMA histogram, Android.WebView.VariationsEnableState, with the result.

The logic about whether variations is enabled is now pretty complex, but we'll
remove most of it after 100% rollout. The current variables are:

- mEnabledByCmd
    Controlled by the --enable-webview-variations switch.
- mEnabledByExperiment
    Controlled by AGSA's experiment.
- sVariationsAlwaysEnabled
    Hard-coded to true in m68, intended for easy revert in case we decided to
    completely disable variations in m68.
- sEnableState
    The newly-added tri-state: default enabled, control-group enabled, and
    experiment-group enabled.

Before this change, variations would be enabled if any 1 of the first 3
variables were true. The new sEnableState can disable variations, overriding
sVariationsAlwaysEnabled.

This means that "sVariationsAlwaysEnabled" is no longer an accurate name, since
it can be overridden, but I'm keeping it as-is to allow easy reverts.

Bug:  851045 
Change-Id: I5b96d5636d1a1d2b0d662a121cd8b285cdb37a48
Reviewed-on: https://chromium-review.googlesource.com/1094139
Commit-Queue: Paul Miller <paulmiller@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567838}
[modify] https://crrev.com/074de24df77eee54efbb6fc3646528b008bdb513/android_webview/java/src/org/chromium/android_webview/VariationsSeedLoader.java
[modify] https://crrev.com/074de24df77eee54efbb6fc3646528b008bdb513/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/074de24df77eee54efbb6fc3646528b008bdb513/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-68
Project Member

Comment 5 by sheriffbot@chromium.org, Jun 16 2018

Labels: -Merge-Request-68 Merge-Reject-68 Hotlist-Merge-Reject
The bug is marked as P3 or Feature. It should not be merged as M68 is in beta. 
Please contact the approriate milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Type-Feature -Hotlist-Merge-Reject -Merge-Reject-68 Merge-Request-68 Type-Bug
^ yeah, well, ya know that's just like, uh, your opinion, man
Project Member

Comment 8 by sheriffbot@chromium.org, Jun 16 2018

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: M68 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), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 9 by cma...@chromium.org, Jun 18 2018

is it possible to verify this in canary?
We discussed this in the meeting this morning. Testing manually would be difficult, because you'll get the existing behavior (variations is enabled) exactly 99.9% of the time. The only thing to test would be that variations is disabled .1% of the time.

I tested this manually myself by changing the probabilities to 1/3 default, 1/3 control, 1/3 experiment (as opposed to 99.8% default, .1% control, .1% experiment), and verified that all 3 states worked. But we can't change the probabilities in canary.

Variations, the feature which this commit is disables .1% of the time, has already gotten manual testing: https://crbug.com/848910.
Hmm... I agree that it is hard to manually test this. How about automated testing, then?

Could you try increasing the probability to 100% to 'disabled' on trybot? Does this break anything?

Also, we may be able to verify this through UMAs after some time: does this increase crash rates on experiment?

> Could you try increasing the probability to 100% to 'disabled' on trybot?

Like so? https://chromium-review.googlesource.com/c/chromium/src/+/1105282

> Also, we may be able to verify this through UMAs after some time: does this increase crash rates on experiment?

There is already some data for Android.WebView.VariationsEnableState on the dashboard, though not enough to make any conclusions. But I don't know how to split crashes by UMA data.
Labels: -Hotlist-Merge-Review -Merge-Review-68 Merge-Approved-68
All good! Thanks!
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 19 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55a2f913aa0eb3668bdef455b5ed3b71d230fb31

commit 55a2f913aa0eb3668bdef455b5ed3b71d230fb31
Author: Paul Miller <paulmiller@google.com>
Date: Tue Jun 19 21:07:57 2018

WebView: Add study for variations rollout

With https://crrev.com/c/1050931, variations is enabled 100% in m68. We want to
keep some small percentage disabled in order to compare metrics, but can't use
variations to do so. So hard-code the random group assignment instead, and add a
UMA histogram, Android.WebView.VariationsEnableState, with the result.

The logic about whether variations is enabled is now pretty complex, but we'll
remove most of it after 100% rollout. The current variables are:

- mEnabledByCmd
    Controlled by the --enable-webview-variations switch.
- mEnabledByExperiment
    Controlled by AGSA's experiment.
- sVariationsAlwaysEnabled
    Hard-coded to true in m68, intended for easy revert in case we decided to
    completely disable variations in m68.
- sEnableState
    The newly-added tri-state: default enabled, control-group enabled, and
    experiment-group enabled.

Before this change, variations would be enabled if any 1 of the first 3
variables were true. The new sEnableState can disable variations, overriding
sVariationsAlwaysEnabled.

This means that "sVariationsAlwaysEnabled" is no longer an accurate name, since
it can be overridden, but I'm keeping it as-is to allow easy reverts.

Bug:  851045 
Change-Id: I5b96d5636d1a1d2b0d662a121cd8b285cdb37a48
Reviewed-on: https://chromium-review.googlesource.com/1094139
Commit-Queue: Paul Miller <paulmiller@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#567838}(cherry picked from commit 074de24df77eee54efbb6fc3646528b008bdb513)
Reviewed-on: https://chromium-review.googlesource.com/1106917
Reviewed-by: Paul Miller <paulmiller@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#450}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/55a2f913aa0eb3668bdef455b5ed3b71d230fb31/android_webview/java/src/org/chromium/android_webview/VariationsSeedLoader.java
[modify] https://crrev.com/55a2f913aa0eb3668bdef455b5ed3b71d230fb31/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/55a2f913aa0eb3668bdef455b5ed3b71d230fb31/tools/metrics/histograms/histograms.xml

Please close if this is fixed and verified.
Status: Fixed (was: Assigned)
Please mark them verified if possible as we cannot do manual verification as per #10 
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/57e6c106954b12f71a6af72811eaa70ac81455b7

commit 57e6c106954b12f71a6af72811eaa70ac81455b7
Author: Paul Miller <paulmiller@google.com>
Date: Thu Jul 26 00:08:32 2018

WebView: Remove variations experiment code

There were 4 mechanisms for enabling/disabling variations in WebView:
- a command-line flag (mEnabledByCmd)
- the AGSA experiment in m67 (mEnabledByExperiment)
- the hard-coded setting (sVariationsAlwaysEnabled)
- the hold-back experiment in m68 & m69 (sEnableState)

Remove all of them, so that variations is always enabled.

Set the hold-back experiment histogram, VariationsEnableState, to expire
after m69. Don't mark it as obsolete yet, because we'll want to watch it
for the lifetime of m68 & m69.

BUG= 851045 

Change-Id: I44bd36789019074f422ec0c25be2afadba95b8c4
Reviewed-on: https://chromium-review.googlesource.com/1150772
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Paul Miller <paulmiller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578136}
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/browser/aw_browser_main_parts.cc
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/browser/aw_metrics_service_client.cc
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/common/aw_switches.cc
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/common/aw_switches.h
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/java/src/org/chromium/android_webview/AwSwitches.java
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/java/src/org/chromium/android_webview/VariationsSeedLoader.java
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/android_webview/javatests/src/org/chromium/android_webview/test/VariationsSeedLoaderTest.java
[modify] https://crrev.com/57e6c106954b12f71a6af72811eaa70ac81455b7/tools/metrics/histograms/histograms.xml

No manual verification required.

Sign in to add a comment