Closed Bug 1196897 Opened 4 years ago Closed 4 years ago

Integrate Switchboard with onboarding for A/B testing

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: liuche, Assigned: mfinkle)

References

Details

Attachments

(4 files, 2 obsolete files)

This should be behind a NIGHTLY flag.

We'll switch between two onboarding experiences - Import vs (current) Set up Sync.
Blocks: ab-v1
Depends on: 1186037
Switchboard is an open source SDK for doing A/B testing and staged rollouts [1]. It connects to a server component, which maintains a list of active experiments.

The SDK does create a UUID, which is stored on the device. The UUID is sent to the server, which uses it to "bucket" the client, but the UUID is never stored on the server. The server we are using is being hosted from Mozilla's Heroku instance [3].

[1] https://github.com/KeepSafe/Switchboard
[2] https://github.com/mfinkle/switchboard-server
[3] http://mozilla-switchboard.herokuapp.com
Attached patch switchboard WIP (obsolete) — Splinter Review
This patch adds the Switchboard SDK file into our thirdparty JAR.

We should think about whether we want to add a config/build flag to control pulling this in. Given the PITA we'd have with Java imports, I almost want to not worry about build flags, unless someone wants to talk me out of it.
Attached patch init-switchboard WIP (obsolete) — Splinter Review
This patch initializes Switchboard in BrowserApp.onCreate(). I am currently using the sample code verbatim, which is sync. The code comments suggest moving to async loading methods, but I wonder if we'll have race conditions during firstrun.
Comment on attachment 8650677 [details] [diff] [review]
init-switchboard WIP

Review of attachment 8650677 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +740,5 @@
>  
>          final Context appContext = getApplicationContext();
>  
> +        if (AppConstants.NIGHTLY_BUILD) {
> +            //Initializes the default URLs the first time. 

nit: space after //, delete trailing whitespace throughout.

@@ +749,5 @@
> +             *
> +             * In production you should be loaded asynchronous with AsyncConfigLoader.
> +             * new AsyncConfigLoader(this, AsyncConfigLoader.UPDATE_SERVER);
> +             */
> +            Log.d(LOGTAG, "update server urls from remote");

Can we include "switchboard" or something helpful in these messages?
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Created attachment 8650675 [details] [diff] [review]
> switchboard WIP
> 
> This patch adds the Switchboard SDK file into our thirdparty JAR.
> 
> We should think about whether we want to add a config/build flag to control
> pulling this in. Given the PITA we'd have with Java imports, I almost want
> to not worry about build flags, unless someone wants to talk me out of it.

I agree.  I would like to see a separate build flag for the /feature/, but I agree that the pain of stub implementations, etc is too great to deal with conditional compilation.
Attached patch switchboard v0.1Splinter Review
This patch adds Switchboard to the project as a thirdparty library.
Assignee: nobody → mark.finkle
Attachment #8650675 - Attachment is obsolete: true
Attachment #8652342 - Flags: review?(nalexander)
This patch adds a flag, enabled by default, via confvars.sh to wrap A/B experiment code, but is not used to add/remove switchboard from the project.

We need to be careful when wrapping code with this flag. The application should be able to follow the default (non-experiment) code paths with or without the flag being enabled. Putting the default code path exclusively inside a flag-check block would mean the application may have unexpected behavior at runtime when the flag is disabled.
Attachment #8652346 - Flags: review?(nalexander)
This patch imports Switchboard into BrowserApp and initializes the system in BrowserApp.onCreate(). The code is behind MOZ_SWITCHBOARD and NIGHTLY_BUILD for now.

The code uses the sync routines to initialize and we should look into using the async versions before removing the NIGHTLY_BUILD conditional. We need to make sure the SDK gets it's test values soon enough to affect the firstrun experience.
Attachment #8650677 - Attachment is obsolete: true
Attachment #8652347 - Flags: review?(liuche)
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Created attachment 8652347 [details] [diff] [review]
> init-switchboard v0.1
> 
> This patch imports Switchboard into BrowserApp and initializes the system in
> BrowserApp.onCreate(). The code is behind MOZ_SWITCHBOARD and NIGHTLY_BUILD
> for now.

Note: The Log code is purely debug. I would remove before landing, or maybe before removing the NIGHTLY_BUILD conditional.
Comment on attachment 8652342 [details] [diff] [review]
switchboard v0.1

Review of attachment 8652342 [details] [diff] [review]:
-----------------------------------------------------------------

I leave it up to you if you want to make any changes to the style.  It is helpful for future finkle to include the repo and hash that you imported in the commit message so that when you come to update you have it waiting.
Attachment #8652342 - Flags: review?(nalexander) → review+
Comment on attachment 8652346 [details] [diff] [review]
flag-switchboard v0.1

Review of attachment 8652346 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: configure.in
@@ +4913,5 @@
>  
>  dnl ========================================================
> +dnl = Include Switchboard A/B framework on Android
> +dnl ========================================================
> +if test -n "$MOZ_SWITCHBOARD"; then

If this will /only/ be Android, consider MOZ_ANDROID_SWITCHBOARD.  We didn't do this for Adjust since we envisioned possible usage outside of Fennec.

::: mobile/android/base/moz.build
@@ +778,5 @@
>  # appropriate in Makefile.in.
>  for var in ('MOZ_ANDROID_ANR_REPORTER', 'MOZ_LINKER_EXTRACT', 'MOZILLA_OFFICIAL', 'MOZ_DEBUG',
>              'MOZ_ANDROID_SEARCH_ACTIVITY', 'MOZ_NATIVE_DEVICES', 'MOZ_ANDROID_MLS_STUMBLER',
>              'MOZ_ANDROID_SHARE_OVERLAY', 'MOZ_ANDROID_DOWNLOADS_INTEGRATION', 'MOZ_INSTALL_TRACKING',
> +            'MOZ_ANDROID_TAB_QUEUE', 'MOZ_ANDROID_FIREFOX_ACCOUNT_PROFILES', 'MOZ_SWITCHBOARD'):

Since you AC_DEFINE in configure.in, you don't need this line.  (I just learned this from glandium a day ago.)

::: mobile/android/confvars.sh
@@ +113,5 @@
> +
> +# Enable the Switchboard A/B framework code.
> +# Note: The framework is always included in the app. This flag controls
> +# usage of the framework.
> +MOZ_SWITCHBOARD=1

Consider NIGHTLY_BUILD; it's not clear this should automatically ride the trains.
Attachment #8652346 - Flags: review?(nalexander) → review+
Comment on attachment 8652347 [details] [diff] [review]
init-switchboard v0.1

Review of attachment 8652347 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +740,5 @@
>  
>          final Context appContext = getApplicationContext();
>  
> +        if (AppConstants.MOZ_SWITCHBOARD && AppConstants.NIGHTLY_BUILD) {
> +            //Initializes the default URLs the first time. 

Nit: space after // and remove trailing space. (Hey, linter!)

Maybe I should make a pull request :P
Attachment #8652347 - Flags: review?(liuche) → review+
OK. I see an almost 3x regression on autophone. Let's switch to async and figure out the first-run situation next.
Attachment #8653830 - Flags: review?(nalexander)
(In reply to Mark Finkle (:mfinkle) from comment #14)
> Created attachment 8653830 [details] [diff] [review]
> async-switchboard v0.1
> 
> OK. I see an almost 3x regression on autophone. Let's switch to async and
> figure out the first-run situation next.

Looks like the initial 3x spike is because the server is currently on the "Free" mode which is not always running and sleeps a lot. Subsequent runs are only 50% slower. We still need to move away from the sync mode, but thankfully we can make the latency lower when we switch to "Performance" mode on the server.
Comment on attachment 8653830 [details] [diff] [review]
async-switchboard v0.1

Review of attachment 8653830 [details] [diff] [review]:
-----------------------------------------------------------------

This looks safe. Reviewing this led to me reading the SwitchBoard code. There's some stuff to improve via PRs (e.g., proper stream closing), and some stuff we might need to work into our own lifecycles, like locale loading. Another time.
Attachment #8653830 - Flags: review?(nalexander) → review+
(In reply to Richard Newman [:rnewman] from comment #16)
> Comment on attachment 8653830 [details] [diff] [review]
> async-switchboard v0.1
> 
> Review of attachment 8653830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks safe. Reviewing this led to me reading the SwitchBoard code.
> There's some stuff to improve via PRs (e.g., proper stream closing), and
> some stuff we might need to work into our own lifecycles, like locale
> loading. Another time.

rnewman: thank'ee for stealing this.
Just a note: Looks like the added background work did cause a noticeable regression in total throbber time for older, single core devices:
http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-twitter/norejected/2015-08-27/2015-08-29/notcached/noerrorbars/standarderror/notry

I am not too concerned. Options are:
1. Reduce the two background network calls into a single call.
2. Decide if we could delay the Switchboard init if we are loading an external URL. Currently, we need the Switchboard code very early for Firstrun UI, but that UI is not shown when loading external URLs.
3. Remove Switchboard (A/B testing) from GB builds. Not a complete fix since some single core devices do run newer versions of Android.
(In reply to Mark Finkle (:mfinkle) from comment #20)

> 1. Reduce the two background network calls into a single call.

That's probably a good idea regardless.

> 3. Remove Switchboard (A/B testing) from GB builds. Not a complete fix since
> some single core devices do run newer versions of Android.

4. We're already reading /proc for how much memory the device has. We could spot single-core devices in the same way.
Mark, can you file a new bug for the perf regression? Thanks.
Flags: needinfo?(mark.finkle)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> Mark, can you file a new bug for the perf regression? Thanks.

It was fixed by patch in comment 14
No longer depends on: 1186037
Flags: needinfo?(mark.finkle)
Depends on: 1207719
Depends on: 1210088
Depends on: 1210089
Mark, do you have plans to test this out once 43 is in beta? Is this something softvision should take a look at? If so can you help them out in what to expect? I know that sometimes we tweak stuff like this (even the nightly/release flag logic) and it doesn't always work perfectly.
Flags: needinfo?(mark.finkle)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Mark, do you have plans to test this out once 43 is in beta? Is this
> something softvision should take a look at? If so can you help them out in
> what to expect? I know that sometimes we tweak stuff like this (even the
> nightly/release flag logic) and it doesn't always work perfectly.

Yes indeed. In fact, we will use it in Fx43 Beta to drive the A/B testing of the new Onboarding UX (bug 1199859). Softvision is familiar with the Onboarding A/B testing while on Nightly, so this should be fairly straight forward.
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.