From 03c4010c5f3b89383cc8d47fefeb17ff4cadbfb2 Mon Sep 17 00:00:00 2001 From: Fiona Date: Thu, 15 Oct 2026 06:17:57 -0700 Subject: [PATCH] fix(rum): keep remote settings across app upgrades The remote configuration store key included the application version, so the first session after every app upgrade found no stored entry and was drawn on the init values, without a settings version to report. An installed app runs one version at a time, and the previous release's stored settings are a better answer than the init values until the new fetch is stored. The key is now built from endpoint host, application id, service and environment only. Entries written under the old layout (key ending in the app version) are no longer reachable. The storage format is unchanged, so the key prefix is not bumped; those entries are removed by the existing age-based sweep once they have gone unrefreshed. The sweep documentation now describes this. --- .../remoteconfig/RemoteConfigController.kt | 2 +- .../remoteconfig/RemoteConfigStore.kt | 40 +++++---- .../remoteconfig/RemoteConfigStoreTest.kt | 88 +++++++++++++++---- 3 files changed, 94 insertions(+), 36 deletions(-) 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