From 72f977771d0eedb94ce4097ac621c0320c147862 Mon Sep 17 00:00:00 2001 From: jhugman Date: Mon, 1 Feb 2021 23:50:07 +0000 Subject: [PATCH] Re-enable Nimbus with Off the main thread I/O and non-blocking networking (#17684) * Moved Nimbus setup from Analytics to its own file * Change experiment identifiers to fit new run of the experiment * Re-enable nimbus for debug and nightly builds * ./gradlew ktlint && detekt --- .../java/org/mozilla/fenix/FeatureFlags.kt | 12 +--- .../org/mozilla/fenix/components/Analytics.kt | 33 +-------- .../mozilla/fenix/experiments/Experiments.kt | 6 +- .../mozilla/fenix/experiments/NimbusSetup.kt | 72 +++++++++++++++++++ 4 files changed, 79 insertions(+), 44 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt diff --git a/app/src/main/java/org/mozilla/fenix/FeatureFlags.kt b/app/src/main/java/org/mozilla/fenix/FeatureFlags.kt index 2aedb9361c..35e89de46b 100644 --- a/app/src/main/java/org/mozilla/fenix/FeatureFlags.kt +++ b/app/src/main/java/org/mozilla/fenix/FeatureFlags.kt @@ -22,17 +22,9 @@ object FeatureFlags { val syncedTabsInTabsTray = Config.channel.isNightlyOrDebug /** - * Enables the Nimbus experiments library, especially the settings toggle to opt-out of - * all experiments. + * Enables the Nimbus experiments library. */ - // IMPORTANT: Only turn this back on once the following issues are resolved: - // - https://github.com/mozilla-mobile/fenix/issues/17086: Calls to - // getExperimentBranch seem to block on updateExperiments causing a - // large performance regression loading the home screen. - // - https://github.com/mozilla-mobile/fenix/issues/17143: Despite - // having wrapped getExperimentBranch/withExperiments in a catch-all - // users are still experiencing crashes. - const val nimbusExperiments = false + val nimbusExperiments = Config.channel.isNightlyOrDebug /** * Enables the new MediaSession API. diff --git a/app/src/main/java/org/mozilla/fenix/components/Analytics.kt b/app/src/main/java/org/mozilla/fenix/components/Analytics.kt index 2feadad1b7..00bfb66479 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Analytics.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Analytics.kt @@ -8,17 +8,13 @@ import android.app.Application import android.app.PendingIntent import android.content.Context import android.content.Intent -import android.net.Uri -import android.os.StrictMode import mozilla.components.lib.crash.CrashReporter import mozilla.components.lib.crash.service.CrashReporterService import mozilla.components.lib.crash.service.GleanCrashReporterService import mozilla.components.lib.crash.service.MozillaSocorroService import mozilla.components.lib.crash.service.SentryService import mozilla.components.service.nimbus.NimbusApi -import mozilla.components.service.nimbus.Nimbus as NimbusEnabled import mozilla.components.service.nimbus.NimbusDisabled -import mozilla.components.service.nimbus.NimbusServerSettings import org.mozilla.fenix.BuildConfig import org.mozilla.fenix.Config import org.mozilla.fenix.FeatureFlags @@ -29,6 +25,7 @@ import org.mozilla.fenix.components.metrics.AdjustMetricsService import org.mozilla.fenix.components.metrics.GleanMetricsService import org.mozilla.fenix.components.metrics.LeanplumMetricsService import org.mozilla.fenix.components.metrics.MetricController +import org.mozilla.fenix.experiments.createNimbus import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings import org.mozilla.fenix.perf.lazyMonitored @@ -109,33 +106,7 @@ class Analytics( val experiments: NimbusApi by lazyMonitored { if (FeatureFlags.nimbusExperiments) { - // Eventually we'll want to use `NimbusDisabled` when we have no NIMBUS_ENDPOINT. - // but we keep this here to not mix feature flags and how we configure Nimbus. - val url: String? = BuildConfig.NIMBUS_ENDPOINT - val serverSettings = if (!url.isNullOrBlank()) { - NimbusServerSettings(url = Uri.parse(url)) - } else { - null - } - - NimbusEnabled(context, serverSettings).apply { - // Global opt out state is stored in Nimbus, and shouldn't be toggled to `true` - // from the app unless the user does so from a UI control. - // However, the user may have opt-ed out of mako experiments already, so - // we should respect that setting here. - val enabled = - context.components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) { - context.settings().isExperimentationEnabled - } - if (!enabled) { - globalUserParticipation = enabled - } - - // Nimbus should look after downloading experiment definitions from remote settings - // on another thread, and making sure we don't hit the server each time we start. - @Suppress("Deprecation") - updateExperiments() - } + createNimbus(context, BuildConfig.NIMBUS_ENDPOINT) } else { NimbusDisabled() } diff --git a/app/src/main/java/org/mozilla/fenix/experiments/Experiments.kt b/app/src/main/java/org/mozilla/fenix/experiments/Experiments.kt index 5b5f67b34c..ab11ec8f49 100644 --- a/app/src/main/java/org/mozilla/fenix/experiments/Experiments.kt +++ b/app/src/main/java/org/mozilla/fenix/experiments/Experiments.kt @@ -6,7 +6,7 @@ package org.mozilla.fenix.experiments class Experiments { companion object { - const val A_A_NIMBUS_VALIDATION = "fenix-nimbus-validation" + const val A_A_NIMBUS_VALIDATION = "fenix-nimbus-validation-v2" const val BOOKMARK_ICON = "fenix-bookmark-list-icon" } } @@ -15,7 +15,7 @@ class ExperimentBranch { companion object { const val TREATMENT = "treatment" const val CONTROL = "control" - const val A1 = "A1" - const val A2 = "A2" + const val A1 = "a1" + const val A2 = "a2" } } diff --git a/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt new file mode 100644 index 0000000000..1622e79dab --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt @@ -0,0 +1,72 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.experiments + +import android.content.Context +import android.net.Uri +import android.os.StrictMode +import io.sentry.Sentry +import mozilla.components.service.nimbus.NimbusApi +import mozilla.components.service.nimbus.Nimbus +import mozilla.components.service.nimbus.NimbusDisabled +import mozilla.components.service.nimbus.NimbusServerSettings +import mozilla.components.support.base.log.logger.Logger +import org.mozilla.fenix.components.isSentryEnabled +import org.mozilla.fenix.ext.components +import org.mozilla.fenix.ext.settings + +@Suppress("TooGenericExceptionCaught") +fun createNimbus(context: Context, url: String?): NimbusApi = + try { + // Eventually we'll want to use `NimbusDisabled` when we have no NIMBUS_ENDPOINT. + // but we keep this here to not mix feature flags and how we configure Nimbus. + val serverSettings = if (!url.isNullOrBlank()) { + NimbusServerSettings(url = Uri.parse(url)) + } else { + null + } + + // Global opt out state is stored in Nimbus, and shouldn't be toggled to `true` + // from the app unless the user does so from a UI control. + // However, the user may have opt-ed out of mako experiments already, so + // we should respect that setting here. + val enabled = + context.components.strictMode.resetAfter(StrictMode.allowThreadDiskReads()) { + context.settings().isExperimentationEnabled + } + + Nimbus(context, serverSettings).apply { + // This performs the minimal amount of work required to load branch and enrolment data + // into memory. If `getExperimentBranch` is called from another thread between here + // and the next nimbus disk write (setting `globalUserParticipation` or + // `applyPendingExperiments()`) then this has you covered. + // This call does its work on the db thread. + initialize() + + if (!enabled) { + // This opts out of nimbus experiments. It involves writing to disk, so does its + // work on the db thread. + globalUserParticipation = enabled + } + + // We may have downloaded experiments on a previous run, so let's start using them + // now. We didn't do this earlier, so as to make getExperimentBranch and friends returns + // the same thing throughout the session. This call does its work on the db thread. + applyPendingExperiments() + + // Now fetch the experiments from the server. These will be available for feature + // configuration on the next run of the app. This call launches on the fetch thread. + fetchExperiments() + } + } catch (e: Throwable) { + // Something went wrong. We'd like not to, but stability of the app is more important than + // failing fast here. + if (isSentryEnabled()) { + Sentry.capture(e) + } else { + Logger.error("Failed to initialize Nimbus", e) + } + NimbusDisabled() + }