1196897 - Integrate Switchboard with onboarding for A/B testing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

This should be behind a NIGHTLY flag.

We'll switch between two onboarding experiences - Import vs (current) Set up Sync.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: init-switchboard WIP

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 4

[Nick Alexander :nalexander [he/him]]

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?

Comment 5

[Nick Alexander :nalexander [he/him]]

(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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: switchboard v0.1

This patch adds Switchboard to the project as a thirdparty library.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: flag-switchboard v0.1

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

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.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 10

[Nick Alexander :nalexander [he/him]]

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.

Comment 11

[Nick Alexander :nalexander [he/him]]

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.

Comment 12

[Chenxia Liu [:liuche]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ff5c42e6a1a2
https://hg.mozilla.org/integration/fx-team/rev/0cb4a5fbd6b7
https://hg.mozilla.org/integration/fx-team/rev/dd8158e7fd32

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 16

[Richard Newman [:rnewman]]

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.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/637b4f9227e5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff5c42e6a1a2
https://hg.mozilla.org/mozilla-central/rev/0cb4a5fbd6b7
https://hg.mozilla.org/mozilla-central/rev/dd8158e7fd32
https://hg.mozilla.org/mozilla-central/rev/637b4f9227e5

Comment 19

[Nick Alexander :nalexander [he/him]]

(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.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 21

[Richard Newman [:rnewman]]

(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.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Mark, can you file a new bug for the perf regression? Thanks.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.