From 31130126e6bea4f81b67f3f07e9cc4a4d188f95a Mon Sep 17 00:00:00 2001 From: tyiuhc <137842098+tyiuhc@users.noreply.github.com> Date: Wed, 20 Sep 2023 09:41:12 -0700 Subject: [PATCH] Update Experiment Client with LoadStoreCache and Variant (#30) --- sdk/build.gradle | 1 + .../experiment/DefaultExperimentClient.kt | 38 ++++-- .../com/amplitude/experiment/Experiment.kt | 4 +- .../java/com/amplitude/experiment/Variant.kt | 2 +- .../com/amplitude/experiment/storage/Cache.kt | 49 +++++++ .../experiment/storage/InMemoryStorage.kt | 29 ---- .../util/SessionAnalyticsProvider.kt | 2 +- .../com/amplitude/experiment/util/Variant.kt | 5 +- .../experiment/ExperimentClientTest.kt | 22 +-- .../com/amplitude/experiment/StorageTest.kt | 126 ++++++++++++++++++ .../com/amplitude/experiment/VariantTest.kt | 5 +- 11 files changed, 226 insertions(+), 57 deletions(-) delete mode 100644 sdk/src/main/java/com/amplitude/experiment/storage/InMemoryStorage.kt create mode 100644 sdk/src/test/java/com/amplitude/experiment/StorageTest.kt diff --git a/sdk/build.gradle b/sdk/build.gradle index 0874b4d..ec4f823 100644 --- a/sdk/build.gradle +++ b/sdk/build.gradle @@ -46,6 +46,7 @@ dependencies { testImplementation 'junit:junit:4.13.2' testImplementation 'org.json:json:20201115' testImplementation project(path: ':sdk') + testImplementation "org.mockito:mockito-core:3.7.7" } task docs(dependsOn: dokkaHtml) { diff --git a/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt b/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt index 043b190..e0b830b 100644 --- a/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt +++ b/sdk/src/main/java/com/amplitude/experiment/DefaultExperimentClient.kt @@ -1,7 +1,9 @@ package com.amplitude.experiment -import com.amplitude.experiment.analytics.ExposureEvent as OldExposureEvent +import com.amplitude.experiment.storage.LoadStoreCache import com.amplitude.experiment.storage.Storage +import com.amplitude.experiment.analytics.ExposureEvent as OldExposureEvent +import com.amplitude.experiment.storage.getVariantStorage import com.amplitude.experiment.util.AsyncFuture import com.amplitude.experiment.util.Backoff import com.amplitude.experiment.util.BackoffConfig @@ -35,11 +37,19 @@ internal class DefaultExperimentClient internal constructor( private val apiKey: String, private val config: ExperimentConfig, private val httpClient: OkHttpClient, - private val storage: Storage, + storage: Storage, private val executorService: ScheduledExecutorService, ) : ExperimentClient { private var user: ExperimentUser? = null + private val variants: LoadStoreCache = getVariantStorage( + this.apiKey, + this.config.instanceName, + storage, + ); + init { + this.variants.load() + } private val backoffLock = Any() private var backoff: Backoff? = null @@ -131,6 +141,7 @@ internal class DefaultExperimentClient internal constructor( } return VariantAndSource(config.fallbackVariant, VariantSource.FALLBACK_CONFIG) } + Source.INITIAL_VARIANTS -> { // for source = InitialVariants, fallback order goes: // 1. InitialFlags @@ -157,7 +168,8 @@ internal class DefaultExperimentClient internal constructor( } override fun clear() { - this.storage.clear() + this.variants.clear() + this.variants.store() } override fun getUser(): ExperimentUser? { @@ -274,24 +286,26 @@ internal class DefaultExperimentClient internal constructor( return variants } - private fun storeVariants(variants: Map, options: FetchOptions?) = synchronized(storage) { - val failedFlagKeys = options?.flagKeys ?.toMutableList() ?: mutableListOf() + private fun storeVariants(variants: Map, options: FetchOptions?) = synchronized(variants) { + val failedFlagKeys = options?.flagKeys?.toMutableList() ?: mutableListOf() if (options?.flagKeys == null) { - storage.clear() + this.variants.clear() } for (entry in variants.entries) { - storage.put(entry.key, entry.value) + this.variants.put(entry.key, entry.value) failedFlagKeys.remove(entry.key) } for (key in failedFlagKeys) { - storage.remove(key) + this.variants.remove(key) } + + this.variants.store() Logger.d("Stored variants: $variants") } private fun sourceVariants(): Map { return when (config.source) { - Source.LOCAL_STORAGE -> storage.getAll() + Source.LOCAL_STORAGE -> this.variants.getAll() Source.INITIAL_VARIANTS -> config.initialVariants } } @@ -299,7 +313,7 @@ internal class DefaultExperimentClient internal constructor( private fun secondaryVariants(): Map { return when (config.source) { Source.LOCAL_STORAGE -> config.initialVariants - Source.INITIAL_VARIANTS -> storage.getAll() + Source.INITIAL_VARIANTS -> this.variants.getAll() } } @@ -345,7 +359,7 @@ enum class VariantSource(val type: String) { fun isFallback(): Boolean { return this == FALLBACK_INLINE || - this == FALLBACK_CONFIG || - this == SECONDARY_INITIAL_VARIANTS + this == FALLBACK_CONFIG || + this == SECONDARY_INITIAL_VARIANTS } } diff --git a/sdk/src/main/java/com/amplitude/experiment/Experiment.kt b/sdk/src/main/java/com/amplitude/experiment/Experiment.kt index 3163347..39877cb 100644 --- a/sdk/src/main/java/com/amplitude/experiment/Experiment.kt +++ b/sdk/src/main/java/com/amplitude/experiment/Experiment.kt @@ -53,7 +53,7 @@ object Experiment { apiKey, mergedConfig, httpClient, - SharedPrefsStorage(application, apiKey, instanceName), + SharedPrefsStorage(application), executorService, ) instances[instanceKey] = newInstance @@ -103,7 +103,7 @@ object Experiment { apiKey, configBuilder.build(), httpClient, - SharedPrefsStorage(application, apiKey, instanceName), + SharedPrefsStorage(application), executorService, ) instances[instanceKey] = newInstance diff --git a/sdk/src/main/java/com/amplitude/experiment/Variant.kt b/sdk/src/main/java/com/amplitude/experiment/Variant.kt index 8e63219..63c84e7 100644 --- a/sdk/src/main/java/com/amplitude/experiment/Variant.kt +++ b/sdk/src/main/java/com/amplitude/experiment/Variant.kt @@ -21,7 +21,7 @@ data class Variant @JvmOverloads constructor( * Flag, segment, and variant metadata produced as a result of * evaluation for the user. Used for system purposes. */ - @JvmField val metadata: Map = emptyMap() + @JvmField val metadata: Map? = null ) { /** diff --git a/sdk/src/main/java/com/amplitude/experiment/storage/Cache.kt b/sdk/src/main/java/com/amplitude/experiment/storage/Cache.kt index 706771e..b31aa00 100644 --- a/sdk/src/main/java/com/amplitude/experiment/storage/Cache.kt +++ b/sdk/src/main/java/com/amplitude/experiment/storage/Cache.kt @@ -1,5 +1,6 @@ package com.amplitude.experiment.storage +import com.amplitude.experiment.Variant import com.amplitude.experiment.util.toMap import org.json.JSONObject @@ -56,3 +57,51 @@ internal class LoadStoreCache( storage.put(namespace, JSONObject(values).toString()) } } + +internal fun getVariantStorage(deploymentKey: String, instanceName: String, storage: Storage): LoadStoreCache { + val truncatedDeployment = deploymentKey.takeLast(6) + val namespace = "amp-exp-$instanceName-$truncatedDeployment" + return LoadStoreCache(namespace, storage, ::transformVariantFromStorage) +} + +//fun getFlagStorage(deploymentKey: String, instanceName: String, storage: Storage): LoadStoreCache { +// val truncatedDeployment = deploymentKey.takeLast(6) +// val namespace = "amp-exp-$instanceName-$truncatedDeployment-flags" +// return LoadStoreCache(namespace, storage) +//} + +internal fun transformVariantFromStorage(storageValue: Any?): Variant { + return when (storageValue) { + is String -> { + // From v0 string format + Variant( + key = storageValue, + value = storageValue + ) + } + + is Map<*, *> -> { + // From v1 or v2 object format + var key = storageValue["key"] as? String + val value = storageValue["value"] as? String + val payload = storageValue["payload"] + var metadata = (storageValue["metadata"] as? Map)?.toMutableMap() + var experimentKey= storageValue["expKey"] as? String + + if (metadata != null && metadata["experimentKey"] != null) { + experimentKey = metadata["experimentKey"] as? String + } else if (experimentKey != null) { + metadata = metadata ?: HashMap() + metadata["experimentKey"] = experimentKey + } + + if (key == null && value != null) { + key = value + } + + Variant(key = key, value = value, payload = payload, expKey = experimentKey, metadata = metadata) + } + + else -> Variant() + } +} diff --git a/sdk/src/main/java/com/amplitude/experiment/storage/InMemoryStorage.kt b/sdk/src/main/java/com/amplitude/experiment/storage/InMemoryStorage.kt deleted file mode 100644 index ea73b68..0000000 --- a/sdk/src/main/java/com/amplitude/experiment/storage/InMemoryStorage.kt +++ /dev/null @@ -1,29 +0,0 @@ -package com.amplitude.experiment.storage - -import com.amplitude.experiment.Variant -import java.util.concurrent.ConcurrentHashMap - -internal class InMemoryStorage : Storage { - - private val data: MutableMap = ConcurrentHashMap() - - override fun put(key: String, variant: Variant) { - data[key] = variant - } - - override operator fun get(key: String): Variant? { - return data[key] - } - - override fun remove(key: String) { - data.remove(key) - } - - override fun getAll(): Map { - return data.toMap() - } - - override fun clear() { - data.clear() - } -} diff --git a/sdk/src/main/java/com/amplitude/experiment/util/SessionAnalyticsProvider.kt b/sdk/src/main/java/com/amplitude/experiment/util/SessionAnalyticsProvider.kt index f2eb394..005a9b2 100644 --- a/sdk/src/main/java/com/amplitude/experiment/util/SessionAnalyticsProvider.kt +++ b/sdk/src/main/java/com/amplitude/experiment/util/SessionAnalyticsProvider.kt @@ -45,4 +45,4 @@ internal class SessionAnalyticsProvider( } analyticsProvider.unsetUserProperty(event) } -} \ No newline at end of file +} diff --git a/sdk/src/main/java/com/amplitude/experiment/util/Variant.kt b/sdk/src/main/java/com/amplitude/experiment/util/Variant.kt index 1170c31..a50e0b3 100644 --- a/sdk/src/main/java/com/amplitude/experiment/util/Variant.kt +++ b/sdk/src/main/java/com/amplitude/experiment/util/Variant.kt @@ -17,6 +17,9 @@ internal fun Variant.toJson(): String { if (expKey != null) { jsonObject.put("expKey", expKey) } + if (metadata != null) { + jsonObject.put("metadata", metadata) + } } catch (e: JSONException) { Logger.w("Error converting Variant to json string", e) } @@ -53,7 +56,7 @@ internal fun JSONObject?.toVariant(): Variant? { } val metadata = when { has("metadata") -> getJSONObject("metadata").toMap() - else -> emptyMap() + else -> null } Variant(value, payload, expKey, key, metadata) } catch (e: JSONException) { diff --git a/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt b/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt index c77d16d..1b3611f 100644 --- a/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt +++ b/sdk/src/test/java/com/amplitude/experiment/ExperimentClientTest.kt @@ -2,13 +2,14 @@ package com.amplitude.experiment import com.amplitude.experiment.analytics.ExperimentAnalyticsEvent import com.amplitude.experiment.analytics.ExperimentAnalyticsProvider -import com.amplitude.experiment.storage.InMemoryStorage +import com.amplitude.experiment.storage.Storage import com.amplitude.experiment.util.Logger import com.amplitude.experiment.util.SystemLogger import okhttp3.OkHttpClient import org.junit.Assert import org.junit.Test import java.util.concurrent.ExecutionException +import org.mockito.Mockito private const val API_KEY = "client-DvWljIjiiuqLbyjqdvBaLFfEBrAvGuA3" @@ -21,6 +22,7 @@ class ExperimentClientTest { Logger.implementation = SystemLogger(true) } + private val mockStorage = Mockito.mock(Storage::class.java) private val testUser = ExperimentUser(userId = "test_user") private val serverVariant = Variant(key = "on", value = "on", payload = "payload") @@ -40,7 +42,7 @@ class ExperimentClientTest { initialVariants = initialVariants, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) @@ -53,7 +55,7 @@ class ExperimentClientTest { fetchTimeoutMillis = 1, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) @@ -65,7 +67,7 @@ class ExperimentClientTest { initialVariants = initialVariants, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) @@ -75,7 +77,7 @@ class ExperimentClientTest { debug = true, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) @@ -249,7 +251,7 @@ class ExperimentClientTest { analyticsProvider = analyticsProvider, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) analyticsProviderClient.fetch(testUser).get() @@ -292,7 +294,7 @@ class ExperimentClientTest { analyticsProvider = analyticsProvider, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) analyticsProviderClient.fetch(testUser).get() @@ -334,7 +336,7 @@ class ExperimentClientTest { analyticsProvider = analyticsProvider, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) analyticsProviderClient.fetch(testUser).get() @@ -374,7 +376,7 @@ class ExperimentClientTest { analyticsProvider = analyticsProvider, ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) analyticsProviderClient.fetch(testUser).get() @@ -403,7 +405,7 @@ class ExperimentClientTest { initialVariants = mapOf("flagKey" to Variant(key = "variant", expKey = "experimentKey")) ), OkHttpClient(), - InMemoryStorage(), + mockStorage, Experiment.executorService, ) client.variant("flagKey") diff --git a/sdk/src/test/java/com/amplitude/experiment/StorageTest.kt b/sdk/src/test/java/com/amplitude/experiment/StorageTest.kt new file mode 100644 index 0000000..c1ed1e5 --- /dev/null +++ b/sdk/src/test/java/com/amplitude/experiment/StorageTest.kt @@ -0,0 +1,126 @@ +package com.amplitude.experiment + +import com.amplitude.experiment.storage.transformVariantFromStorage +import org.junit.Assert +import org.junit.Test + +class TransformVariantFromStorageTest { + + @Test + fun `v0 variant transformation`() { + val storedVariant = "on" + Assert.assertEquals( + Variant(key = "on", value = "on"), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v1 variant transformation`() { + val storedVariant = mapOf("value" to "on") + Assert.assertEquals( + Variant(key = "on", value = "on"), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v1 variant transformation with payload`() { + val storedVariant = mapOf( + "value" to "on", + "payload" to mapOf("k" to "v") + ) + Assert.assertEquals( + Variant(key = "on", value = "on", payload = mapOf("k" to "v")), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v1 variant transformation with payload and experiment key`() { + val storedVariant = mapOf( + "value" to "on", + "payload" to mapOf("k" to "v"), + "expKey" to "exp-1" + ) + Assert.assertEquals( + Variant( + key = "on", + value = "on", + payload = mapOf("k" to "v"), + expKey = "exp-1", + metadata = mapOf("experimentKey" to "exp-1") + ), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v2 variant transformation`() { + val storedVariant = mapOf( + "key" to "treatment", + "value" to "on" + ) + Assert.assertEquals( + Variant(key = "treatment", value = "on"), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v2 variant transformation with payload`() { + val storedVariant = mapOf( + "key" to "treatment", + "value" to "on", + "payload" to mapOf("k" to "v") + ) + Assert.assertEquals( + Variant( + key = "treatment", + value = "on", + payload = mapOf("k" to "v") + ), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v2 variant transformation with payload and experiment key`() { + val storedVariant = mapOf( + "key" to "treatment", + "value" to "on", + "payload" to mapOf("k" to "v"), + "expKey" to "exp-1" + ) + Assert.assertEquals( + Variant( + key = "treatment", + value = "on", + payload = mapOf("k" to "v"), + expKey = "exp-1", + metadata = mapOf("experimentKey" to "exp-1") + ), + transformVariantFromStorage(storedVariant) + ) + } + + @Test + fun `v2 variant transformation with payload and experiment key metadata`() { + val storedVariant = mapOf( + "key" to "treatment", + "value" to "on", + "payload" to mapOf("k" to "v"), + "metadata" to mapOf("experimentKey" to "exp-1") + ) + Assert.assertEquals( + Variant( + key = "treatment", + value = "on", + payload = mapOf("k" to "v"), + expKey = "exp-1", + metadata = mapOf("experimentKey" to "exp-1") + ), + transformVariantFromStorage(storedVariant) + ) + } +} diff --git a/sdk/src/test/java/com/amplitude/experiment/VariantTest.kt b/sdk/src/test/java/com/amplitude/experiment/VariantTest.kt index 647052e..5537a4d 100644 --- a/sdk/src/test/java/com/amplitude/experiment/VariantTest.kt +++ b/sdk/src/test/java/com/amplitude/experiment/VariantTest.kt @@ -28,22 +28,25 @@ class VariantTest { jsonObject.put("value", "value") jsonObject.put("payload", "payload") jsonObject.put("expKey", "expKey") + jsonObject.put("metadata", emptyMap()) val variant = jsonObject.toVariant() Assert.assertNotNull(variant) Assert.assertEquals("key", variant!!.key) Assert.assertEquals("value", variant.value) Assert.assertEquals("payload", variant.payload) Assert.assertEquals("expKey", variant.expKey) + Assert.assertEquals(emptyMap(), variant.metadata) } @Test fun `variant to json object`() { run { - val variant = Variant("value", null, "expKey", "key") + val variant = Variant("value", null, "expKey", "key", emptyMap()) val jsonObject = JSONObject() jsonObject.put("key", "key") jsonObject.put("value", "value") jsonObject.put("expKey", "expKey") + jsonObject.put("metadata", emptyMap()) Assert.assertEquals(jsonObject.toString(), variant.toJson()) } }