fix(rum): keep remote settings across releases of the application

CHANGELOG.md

@@ -18,6 +18,15 @@
 
 ---
 
+## Unreleased
+
+- 🐛 With `remoteConfigurationEnabled`, settings stored by one release of a site are now read by
+  the next. The stored settings used to be keyed by application version as well, so the first
+  session a visitor started on a newly deployed release found nothing, was drawn on the init
+  settings, and reported no remote configuration version. Entries left by earlier builds are no
+  longer read and are swept after two days; the first session after upgrading the SDK uses the
+  init settings once.
+
 ## v0.3.0
 
 - 💥 **Breaking**: `startSessionReplayRecordingManually` no longer defaults to `true` on an init

packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts

@@ -777,10 +777,10 @@ describe('remoteConfiguration', () => {
     })
   })
 
-  describe('sweeping the entries of releases nobody runs', () => {
-    // The key carries the application version, so every release leaves one behind. Without a sweep
-    // they accumulate for good in a quota the host application shares.
-    const otherReleaseKey = buildRemoteConfigSetup({ ...INIT_CONFIGURATION, version: '0.9.0' })!.storeKey
+  describe('sweeping the entries nothing reads', () => {
+    // Builds that keyed the entry by application version left one behind per release. Without a
+    // sweep they accumulate for good in a quota the host application shares.
+    const otherReleaseKey = `${buildRemoteConfigSetup(INIT_CONFIGURATION)!.storeKey}|0.9.0`
     const drawKey = buildDrawStoreKey(INIT_CONFIGURATION)
     const foreignKey = 'a-key-the-host-application-owns'
 
@@ -874,13 +874,33 @@ describe('remoteConfiguration', () => {
   })
 
   describe('the storage key', () => {
-    it('separates applications, environments and versions', () => {
+    it('separates applications and environments', () => {
       const keyOf = (partial: Partial<RumInitConfiguration>) =>
         buildRemoteConfigSetup({ ...INIT_CONFIGURATION, ...partial })!.storeKey
 
       expect(keyOf({})).not.toEqual(keyOf({ applicationId: 'other' }))
       expect(keyOf({})).not.toEqual(keyOf({ env: 'production' }))
-      expect(keyOf({})).not.toEqual(keyOf({ version: '1.2.4' }))
+    })
+
+    it('is shared by every release of the application', () => {
+      // Keyed by version, the first session after every deploy found no entry and was drawn on the
+      // init values. The settings the previous release stored are the better answer.
+      const keyOf = (partial: Partial<RumInitConfiguration>) =>
+        buildRemoteConfigSetup({ ...INIT_CONFIGURATION, ...partial })!.storeKey
+
+      expect(keyOf({ version: '1.2.4' })).toEqual(keyOf({}))
+      expect(keyOf({ version: undefined })).toEqual(keyOf({}))
+    })
+
+    it('lets a new release read the settings the previous one stored', (done) => {
+      const previousRelease = buildRemoteConfigSetup({ ...INIT_CONFIGURATION, version: '1.2.2' })!
+      interceptor.withMockXhr((xhr) => {
+        xhr.complete(200, body({ rum: { sessionSampleRate: 42 }, version: 3 }))
+
+        expect(readRemoteConfig(setup)).toEqual({ sessionSampleRate: 42, version: 3 })
+        done()
+      })
+      start(configurationWith({ remoteConfig: previousRelease }))
     })
 
     it('cannot be spelled the same way by two different identities', () => {
@@ -889,7 +909,7 @@ describe('remoteConfiguration', () => {
       const keyOf = (partial: Partial<RumInitConfiguration>) =>
         buildRemoteConfigSetup({ ...INIT_CONFIGURATION, ...partial })!.storeKey
 
-      expect(keyOf({ env: 'prod', version: '1_0' })).not.toEqual(keyOf({ env: 'prod_1', version: '0' }))
+      expect(keyOf({ applicationId: 'app', env: '1_0' })).not.toEqual(keyOf({ applicationId: 'app_1', env: '0' }))
     })
 
     it('separates whatever the server is able to tell apart', () => {
@@ -906,15 +926,15 @@ describe('remoteConfiguration', () => {
       // part. `service` is on it while it is still inert: nothing reports it today, so the check
       // skips it, and the day it starts being reported this turns into a real assertion. Fields
       // that identify the application rather than target it are left off — `clientToken` travels
-      // on the request but the key separates the same clients through `applicationId`.
+      // on the request but the key separates the same clients through `applicationId`. So is the
+      // application version, deliberately: see `buildStoreKey`, and the test below that pins it.
       const urlOf = (partial: Partial<RumInitConfiguration>) =>
         buildRemoteConfigSetup({ ...INIT_CONFIGURATION, ...partial })!.buildUrl(undefined)
       const keyOf = (partial: Partial<RumInitConfiguration>) =>
         buildRemoteConfigSetup({ ...INIT_CONFIGURATION, ...partial })!.storeKey
 
       const candidates: Array<Partial<RumInitConfiguration>> = [
         { env: 'production' },
-        { version: '9.9.9' },
         { service: 'checkout' },
         { applicationId: 'other-app' },
       ]

packages/rum-core/src/domain/configuration/remoteConfiguration.ts

@@ -60,15 +60,15 @@ const DRAW_STORE_KEY_PREFIX = '_fc_draw_1_'
 const DEFAULT_FETCH_TIMEOUT = 3 * ONE_SECOND
 
 /**
- * How long an entry may go unrefreshed before `sweepAbandonedEntries` treats it as belonging to a
- * release nobody is running any more.
+ * How long an entry may go unrefreshed before `sweepAbandonedEntries` treats it as one nothing reads
+ * any more.
  *
  * An entry written by this SDK that is still being read is also being rewritten: the page reading
  * it refetches at every session renewal and stores the answer. So the threshold only has to clear
  * the longest a live entry can legitimately stay silent, which is the longest session (four hours,
  * after which a renewal refetches) plus the longest endpoint outage we are willing to survive
  * without dropping anyone — a failed fetch stores nothing. Two days leaves better than a day and a
- * half of outage, and still bounds the leak at the entries of two days of releases.
+ * half of outage.
  *
  * Erring long is deliberate. Deleting an entry too early costs the page reading it one session on
  * its init values; keeping a dead one costs a few hundred bytes.
@@ -346,7 +346,7 @@ function keepConfigFresh(configuration: RumConfiguration, setup: RemoteConfigSet
 
   const renewSubscription = lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, onTrigger)
 
-  // Before the first request, so the room the entries of dead releases are holding is free by the
+  // Before the first request, so the room abandoned entries are holding is free by the
   // time there is an answer to store. See `sweepAbandonedEntries`.
   sweepAbandonedEntries(setup.storeKey)
 
@@ -553,13 +553,12 @@ function writeEntry(setup: RemoteConfigSetup, values: RemoteConfigValues) {
 }
 
 /**
- * Delete the entries of releases nobody is running any more.
+ * Delete the entries nothing reads any more.
  *
- * The store key carries the application version, because two releases live at the same time are
- * entitled to different rates and one entry between them would have each overwrite the other's at
- * every fetch. The cost of that is an entry per release, and nothing ever read or removed them
- * again — on a site that deploys daily they accumulate for good, in a quota the host application
- * shares.
+ * Builds before this one keyed the entry by application version as well, which left an entry per
+ * release that nothing ever read or removed again — on a site that deploys daily they accumulate for
+ * good, in a quota the host application shares. Those entries, and the entries of an application or
+ * environment a visitor no longer uses, are what this clears.
  *
  * Run once per initialisation rather than at every write. Sweeping on write was the shape tried
  * first and it is the wrong one: `localStorage` is synchronous, a session renewal is a hot path,
@@ -660,35 +659,26 @@ function validFetchTimeout(timeout: number | undefined) {
 }
 
 /**
- * The key covers everything that can change the answer — which application, on which host, in which
- * environment, at which version — so a visitor moving between two of them does not read the other's
- * rates. It deliberately leaves out the SDK version: including it would throw the stored rates away
- * on every SDK upgrade and put the first session after an upgrade back on the local settings. The
- * storage format version lives in `STORE_KEY_PREFIX` instead, so only a real format change orphans
- * the cache.
+ * The key covers which application, on which host, in which environment, so a visitor moving
+ * between two of them does not read the other's rates.
  *
- * The application version is in it for a reason the SDK version does not have: settings can be
- * targeted at a release, so two releases of one site that are live at the same time are entitled to
- * different rates. One entry between them would have each overwrite the other's on every fetch, for
- * as long as both are being served — so the version has to stay, and cannot be dropped to make the
- * limitation below go away.
- *
- * That costs an entry per release — the first session after one reads the local settings, and the
- * entry the release before it used is never read again — so the entries are swept by age rather
- * than left to accumulate in a quota the host application shares. See `sweepAbandonedEntries` for
- * why age is the only thing that can tell an abandoned entry from the entry of a tab still open on
- * yesterday's release.
+ * It deliberately leaves out both versions. The SDK version, because including it would throw the
+ * stored rates away on every SDK upgrade; the storage format version lives in `STORE_KEY_PREFIX`
+ * instead, so only a real format change orphans the cache. The application version, because the
+ * settings stored by the release a visitor used last are the best answer the first session on a new
+ * release can get: keyed by version, that session found nothing and was drawn on the init values,
+ * after every deploy. Releases of one site live at the same time share the entry, which is right as
+ * long as they are given the same settings, and is still the latest decision on record if they ever
+ * are not — the fetch every page makes stores its own release's answer for its next draw.
  */
 function buildStoreKey(initConfiguration: RumInitConfiguration) {
-  return buildKey(STORE_KEY_PREFIX, identityParts(initConfiguration).concat(initConfiguration.version ?? ''))
+  return buildKey(STORE_KEY_PREFIX, identityParts(initConfiguration))
 }
 
 /**
- * The key of the draw record the session manager writes. It shares the identity of the settings
- * cache but deliberately not its application version: the record belongs to the session, and a
- * session outlives a deploy. Keying it by version would lose the decision the moment a visitor with
- * a live session navigates onto a newly deployed page, putting that session's events and its
- * tracer back on the init values — the mid-session flip the record exists to prevent.
+ * The key of the draw record the session manager writes. It has the same identity as the settings
+ * cache — the record belongs to the session, and a session outlives a deploy, so it is not keyed by
+ * application version either — and its own prefix, so the two never collide.
  *
  * Built for every site, not only the ones that opted in: `beforeSampling` and `setForcedSession()`
  * move a draw off the init values with remote configuration switched off.