diff --git a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigController.kt b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigController.kt index b467a52c7a..8b0b96c36e 100644 --- a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigController.kt +++ b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigController.kt @@ -67,7 +67,7 @@ internal class RemoteConfigController( private val inFlight = AtomicBoolean(false) - /** Whether the entries of app versions this device no longer runs have been cleared yet. */ + /** Whether the entries this device no longer reads have been cleared yet. */ private val swept = AtomicBoolean(false) private var failedAttempts = 0 private var pendingRetry: ScheduledFuture<*>? = null diff --git a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStore.kt b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStore.kt index f4b3a5a2f5..e5a1dbd651 100644 --- a/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStore.kt +++ b/features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStore.kt @@ -174,12 +174,13 @@ internal class RemoteConfigStore( } /** - * Removes the entries of app versions this device is no longer running. + * Removes the entries this device no longer reads: those of an endpoint, application, service + * or environment the app has stopped using, and those written by releases that still keyed the + * entry by app version, which no current key can reach. * - * The key covers the app version, so every release the device installs leaves one behind, and - * nothing read or removed them again — they accumulated for good inside a preferences file - * that is parsed in full at every launch. Age is what separates an abandoned entry from a live - * one: a store that is still being read is also being written, at the latest by [touch] when + * Nothing else reads or removes them, so they would accumulate for good inside a preferences + * file that is parsed in full at every launch. Age is what separates an abandoned entry from a + * live one: a store that is still being read is also being written, at the latest by [touch] when * its answer comes back unchanged. * * This store's own entry is never a candidate, whatever its age says. It is the one entry that @@ -212,7 +213,7 @@ internal class RemoteConfigStore( /** * The entry a stored key belongs to, or null when the key is not one of ours. Splitting on the * known suffixes rather than on the last separator is what keeps a store key free to contain - * one: a service name and an app version both routinely do. + * one: a service name and an environment name both routinely do. */ private fun entryKeyOf(key: String): String? { if (!key.startsWith(STORE_KEY_PREFIX)) return null @@ -325,15 +326,23 @@ internal class RemoteConfigStore( "Unable to open the remote configuration store; the values passed to init will apply." /** - * Identifies whose configuration this is. It covers everything that can change the answer — - * which endpoint the app asks, which application, in which environment, at which app - * version — so an app that ships a new version, or two applications sharing a device, never - * read each other's values. + * Identifies whose configuration this is: which endpoint the app asks, which application, + * which service, in which environment — so two applications or two environments sharing a + * device never read each other's values. * - * It deliberately leaves out the SDK version: including it would discard the stored values - * on every SDK upgrade and put the first session after an upgrade back on the init values. - * The storage format version lives in [STORE_KEY_PREFIX] instead, so only a real format - * change orphans the cache. + * It deliberately leaves out the app version. An installed app runs one version at a time, + * and keying by version would send the first session after every upgrade to the init values + * with no settings version to report, even though the previous release's settings are a + * better answer than that until the new fetch is stored. The console does not target by app + * version either; if it ever did, the response stored here is replaced by the first fetch + * of the new version anyway. + * + * It also leaves out the SDK version, for the same reason. The storage format version lives + * in [STORE_KEY_PREFIX] instead, so only a real format change orphans the cache. + * + * Entries written under the earlier layout, which ended in the app version, are not reached + * by this key and are left for [sweepAbandoned]; the format did not change, so there is + * nothing for new code to misparse and no reason to bump the prefix. */ fun buildStoreKey(context: DatadogContext, intakeUrl: String, applicationId: String): String { val host = try { @@ -348,8 +357,7 @@ internal class RemoteConfigStore( host, applicationId, context.service, - context.env, - context.version + context.env ).joinToString("|") } } diff --git a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStoreTest.kt b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStoreTest.kt index b4c6a45a26..491d52a7cb 100644 --- a/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStoreTest.kt +++ b/features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/remoteconfig/RemoteConfigStoreTest.kt @@ -50,7 +50,53 @@ internal class RemoteConfigStoreTest { assertThat(key).contains("app-1") assertThat(key).contains(SERVICE) assertThat(key).contains(ENV) - assertThat(key).contains(APP_VERSION) + } + + @Test + fun `M leave the app version out of the key W buildStoreKey()`() { + // Including it would send the first session after every app upgrade to the init values. + val key = RemoteConfigStore.buildStoreKey( + context = datadogContext(), + intakeUrl = "https://rum.example.com/api/v2/rum", + applicationId = "app-1" + ) + + assertThat(key).doesNotContain(APP_VERSION) + } + + @Test + fun `M produce the same key W buildStoreKey() { app version changed }`() { + val intakeUrl = "https://rum.example.com/api/v2/rum" + + val before = RemoteConfigStore.buildStoreKey(datadogContext(version = "1.2.3"), intakeUrl, "app-1") + val after = RemoteConfigStore.buildStoreKey(datadogContext(version = "1.3.0"), intakeUrl, "app-1") + + assertThat(after).isEqualTo(before) + } + + @Test + fun `M produce different keys W buildStoreKey() { application, environment or service differs }`() { + val intakeUrl = "https://rum.example.com/api/v2/rum" + val base = RemoteConfigStore.buildStoreKey(datadogContext(), intakeUrl, "app-1") + + assertThat(RemoteConfigStore.buildStoreKey(datadogContext(), intakeUrl, "app-2")).isNotEqualTo(base) + assertThat(RemoteConfigStore.buildStoreKey(datadogContext(env = "prod"), intakeUrl, "app-1")) + .isNotEqualTo(base) + assertThat(RemoteConfigStore.buildStoreKey(datadogContext(service = "other"), intakeUrl, "app-1")) + .isNotEqualTo(base) + } + + @Test + fun `M read a stored value W snapshot() { app version changed between launches }`() { + val intakeUrl = "https://rum.example.com/api/v2/rum" + val oldKey = RemoteConfigStore.buildStoreKey(datadogContext(version = "1.2.3"), intakeUrl, "app-1") + val newKey = RemoteConfigStore.buildStoreKey(datadogContext(version = "1.3.0"), intakeUrl, "app-1") + val values = RemoteConfigValues(25f, 4, etag = "\"v4\"") + RemoteConfigStore(appContext, oldKey, mock()).store(values) + + val snapshot = RemoteConfigStore(appContext, newKey, mock()).snapshot() + + assertThat(snapshot).isEqualTo(values) } @Test @@ -165,39 +211,39 @@ internal class RemoteConfigStoreTest { // endregion - // region sweeping the entries of versions this device no longer runs + // region sweeping the entries this device no longer reads @Test fun `M remove an entry nothing has refreshed for longer than the threshold W sweepAbandoned()`() { - // The key covers the app version, so every release the device installs leaves one behind. - storeUnder(OTHER_VERSION_KEY, atMs = NOW - THREE_DAYS_MS) + // An entry written under the earlier layout, which ended in the app version. + storeUnder(LEGACY_VERSIONED_KEY, atMs = NOW - THREE_DAYS_MS) val store = testedStore(nowMs = NOW) store.sweepAbandoned() - assertThat(preferences.all.keys.filter { it.startsWith(OTHER_VERSION_KEY) }).isEmpty() + assertThat(preferences.all.keys.filter { it.startsWith(LEGACY_VERSIONED_KEY) }).isEmpty() } @Test fun `M keep an entry something refreshed recently W sweepAbandoned()`() { - storeUnder(OTHER_VERSION_KEY, atMs = NOW - ONE_DAY_MS) + storeUnder(LEGACY_VERSIONED_KEY, atMs = NOW - ONE_DAY_MS) val store = testedStore(nowMs = NOW) store.sweepAbandoned() - assertThat(preferences.all.keys.filter { it.startsWith(OTHER_VERSION_KEY) }).isNotEmpty() + assertThat(preferences.all.keys.filter { it.startsWith(LEGACY_VERSIONED_KEY) }).isNotEmpty() } @Test fun `M remove every key of an abandoned entry W sweepAbandoned()`() { // An entry is spread over several keys, and leaving any of them behind leaks just as well. - storeUnder(OTHER_VERSION_KEY, atMs = NOW - THREE_DAYS_MS) - val before = preferences.all.keys.count { it.startsWith(OTHER_VERSION_KEY) } + storeUnder(LEGACY_VERSIONED_KEY, atMs = NOW - THREE_DAYS_MS) + val before = preferences.all.keys.count { it.startsWith(LEGACY_VERSIONED_KEY) } testedStore(nowMs = NOW).sweepAbandoned() assertThat(before).isGreaterThan(1) - assertThat(preferences.all.keys.none { it.startsWith(OTHER_VERSION_KEY) }).isTrue() + assertThat(preferences.all.keys.none { it.startsWith(LEGACY_VERSIONED_KEY) }).isTrue() } @Test @@ -215,7 +261,7 @@ internal class RemoteConfigStoreTest { @Test fun `M leave alone every key it did not write W sweepAbandoned()`() { preferences.edit().putString("a-key-the-host-app-owns", "not ours").apply() - storeUnder(OTHER_VERSION_KEY, atMs = NOW - THREE_DAYS_MS) + storeUnder(LEGACY_VERSIONED_KEY, atMs = NOW - THREE_DAYS_MS) testedStore(nowMs = NOW).sweepAbandoned() @@ -224,9 +270,9 @@ internal class RemoteConfigStoreTest { @Test fun `M take a store key containing separators apart correctly W sweepAbandoned()`() { - // A service name and an app version both routinely contain dots, so the suffix cannot be + // A service name and an environment name both routinely contain dots, so the suffix cannot be // found by splitting on the last one. - val dotted = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|com.example.shop|prod|1.2.3" + val dotted = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|com.example.shop|prod.eu" storeUnder(dotted, atMs = NOW - THREE_DAYS_MS) testedStore(nowMs = NOW).sweepAbandoned() @@ -273,11 +319,15 @@ internal class RemoteConfigStoreTest { RemoteConfigStore(appContext, STORE_KEY, mock()) { nowMs } } - private fun datadogContext(): DatadogContext { + private fun datadogContext( + service: String = SERVICE, + env: String = ENV, + version: String = APP_VERSION + ): DatadogContext { val context = mock() - whenever(context.service).thenReturn(SERVICE) - whenever(context.env).thenReturn(ENV) - whenever(context.version).thenReturn(APP_VERSION) + whenever(context.service).thenReturn(service) + whenever(context.env).thenReturn(env) + whenever(context.version).thenReturn(version) whenever(context.sdkVersion).thenReturn(SDK_VERSION) return context } @@ -346,8 +396,8 @@ internal class RemoteConfigStoreTest { // Shaped like a real store key, and carrying the prefix, so the sweep considers it at all: // a key it skips for the wrong reason would let the "never remove its own entry" test pass // with the guard gone. - private val STORE_KEY = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|shop|prod|1.0.0" - private val OTHER_VERSION_KEY = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|shop|prod|0.9.0" + private val STORE_KEY = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|shop|prod" + private val LEGACY_VERSIONED_KEY = "${RemoteConfigStore.STORE_KEY_PREFIX}host|app|shop|prod|0.9.0" private const val NOW = 1_800_000_000_000L private const val ONE_DAY_MS = 24L * 60 * 60 * 1000