feat(rum): end the session when new settings decide its fate

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

@@ -56,6 +56,13 @@ export interface RumInitConfiguration extends InitConfiguration {
    * collects, 0 never does — or nothing to leave the incoming rates alone. Runs inside session
    * creation, so it must be fast and synchronous; a thrown error or an out-of-range value is
    * ignored. A session already under way is never re-decided.
+   *
+   * It must be free of side effects, and must answer the same way for the same input. The SDK
+   * calls it outside a draw as well — when new settings arrive it asks which rate would apply now,
+   * to decide whether the running session has to end for them to take effect — so anything the
+   * callback does besides returning a rate (a metric, a log, a counter) happens more often than
+   * there are sessions, and a callback that answers differently each time can keep ending the
+   * session it was just asked about.
    */
   beforeSampling?: BeforeSamplingCallback | undefined
   /**
@@ -77,9 +84,17 @@ export interface RumInitConfiguration extends InitConfiguration {
    * Take the sampling rates from the application's settings in the console instead of only from the
    * values passed here, so they can be changed without releasing a new version of this site.
    *
-   * A change applies to sessions started after it arrives; a session already under way keeps the
-   * decision it was created with. The values below stay in use until the first settings arrive, and
-   * whenever the settings cannot be reached.
+   * A change applies to sessions started after it arrives, and a session already under way is never
+   * re-decided in place. Three changes do not wait for that session to end on its own, because
+   * their effect on it can be told without drawing again: a session sample rate of 0 while the
+   * visitor is being collected, a rate of 100 while they are not, and a stricter
+   * `defaultPrivacyLevel`. Each of those ends the current session, and the visitor's next action
+   * starts a new one under the new settings — the old session is collected to its end as it was
+   * begun, so no recording is left masked in one half and plain in the other. Every other change,
+   * a loosening privacy level included, waits for the next session.
+   *
+   * The values below stay in use until the first settings arrive, and whenever the settings cannot
+   * be reached.
    *
    * @default false
    */

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

@@ -196,6 +196,67 @@ describe('remoteConfiguration', () => {
     })
   })
 
+  describe('announcing that new settings are in storage', () => {
+    function watchStoredNotifications() {
+      const notified = jasmine.createSpy('remoteConfigurationStored')
+      lifeCycle.subscribe(LifeCycleEventType.REMOTE_CONFIGURATION_STORED, notified)
+      return notified
+    }
+
+    it('announces settings that reached storage, so a subscriber can act on them', (done) => {
+      const notified = watchStoredNotifications()
+
+      interceptor.withMockXhr((xhr) => {
+        xhr.complete(200, body({ rum: { sessionSampleRate: 0 } }))
+
+        expect(notified).toHaveBeenCalledTimes(1)
+        done()
+      })
+      start(configurationWith())
+    })
+
+    it('stays silent about settings it refused as older than the ones it holds', (done) => {
+      localStorage.setItem(setup!.storeKey, JSON.stringify({ sessionSampleRate: 42, version: 8 }))
+      const notified = watchStoredNotifications()
+
+      interceptor.withMockXhr((xhr) => {
+        xhr.complete(200, body({ version: 7, rum: { sessionSampleRate: 0 } }))
+
+        // Nothing changed in storage, so nothing downstream may behave as though it had.
+        expect(notified).not.toHaveBeenCalled()
+        done()
+      })
+      start(configurationWith())
+    })
+
+    it('stays silent when the answer never reached storage', (done) => {
+      const notified = watchStoredNotifications()
+      spyOn(Storage.prototype, 'setItem').and.throwError('storage is full')
+
+      interceptor.withMockXhr((xhr) => {
+        xhr.complete(200, body({ rum: { sessionSampleRate: 0 } }))
+
+        // The next draw will not find these settings, so ending a session for their sake would end
+        // it for nothing.
+        expect(notified).not.toHaveBeenCalled()
+        done()
+      })
+      start(configurationWith())
+    })
+
+    it('stays silent about an answer that never made it', (done) => {
+      const notified = watchStoredNotifications()
+
+      interceptor.withMockXhr((xhr) => {
+        xhr.complete(500)
+
+        expect(notified).not.toHaveBeenCalled()
+        done()
+      })
+      start(configurationWith())
+    })
+  })
+
   describe('refusing a payload it cannot read', () => {
     const STORED = { sessionSampleRate: 42, version: 2 }
 

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

@@ -19,11 +19,17 @@ declare const __BUILD_ENV__SDK_VERSION__: string
  * masks a page by default.
  *
  * A change only affects sessions created after it arrives, so a visitor is never dropped halfway
- * through and never starts being recorded halfway through. Fetching follows the same rhythm: once
- * at start-up and once whenever a new session begins — a change can only matter at the next draw,
- * so asking more often than sessions are drawn would be requests for nothing. There is no timer
- * between sessions; the server's `ttl` field is accepted and ignored, reserved for a future
- * polling mode.
+ * through and never starts being recorded halfway through. What "immediately" means for the
+ * handful of changes that cannot wait is therefore not a flip of the running session but its end:
+ * see `endSessionIfSettingsAreDecisive` in the session manager, which subscribes to the event this
+ * module emits once new settings are in storage.
+ *
+ * Fetching happens once at start-up and once whenever a new session begins — a change can only
+ * matter at a draw, and every draw is a new session — so asking more often than sessions are drawn
+ * would be requests for nothing. There is no timer between sessions; the server's `ttl` field is
+ * accepted and ignored, reserved for a future polling mode. The cost of that rhythm is that a
+ * visitor who never goes idle stays on one session, and so on one set of settings, for as long as
+ * they keep using the site.
  *
  * Nothing here runs unless `remoteConfigurationEnabled: true`. Left off — the default — the SDK makes no
  * extra request and behaves exactly as it did before this existed.
@@ -100,6 +106,9 @@ export interface BeforeSamplingContext {
  * The application's last word on the sampling of the session about to be drawn — see the
  * `beforeSampling` init option. Returning nothing, or an out-of-range rate, leaves the incoming
  * value in place.
+ *
+ * Must be free of side effects and answer the same way for the same input: it is also called away
+ * from a draw, to work out which rate newly delivered settings would actually apply.
  */
 export type BeforeSamplingCallback = (
   context: BeforeSamplingContext
@@ -248,7 +257,12 @@ function keepConfigFresh(configuration: RumConfiguration, setup: RemoteConfigSet
       }
       if (response) {
         failedAttempts = 0
-        store(setup, response)
+        if (store(setup, response)) {
+          // Announced only once the settings are in storage, because that is where the next draw
+          // reads them: a subscriber that ends the running session so the new values can take
+          // effect immediately has to be sure the draw that follows will find them.
+          lifeCycle.notify(LifeCycleEventType.REMOTE_CONFIGURATION_STORED)
+        }
         return
       }
       if (failedAttempts < RETRY_DELAYS.length) {
@@ -323,6 +337,11 @@ function fetchRemoteConfiguration(
   xhr.send()
 }
 
+/**
+ * Writes the response to storage, and answers whether it actually landed there. A refused or
+ * unwritable response answers `false`: nothing changed for the next draw, so nothing downstream
+ * should act as if it had.
+ */
 function store(setup: RemoteConfigSetup, response: RemoteConfigurationResponse) {
   // Settings are published under a number that only ever goes up — rolling back republishes the
   // old settings under a new, higher one — so a response numbered below what is already stored can
@@ -335,7 +354,7 @@ function store(setup: RemoteConfigSetup, response: RemoteConfigurationResponse)
   // requests that can cross are two pages, and storage is the only thing they share.
   const storedVersion = readRemoteConfig(setup).version
   if (storedVersion !== undefined && response.version < storedVersion) {
-    return
+    return false
   }
 
   const values: RemoteConfigValues = { version: response.version }
@@ -369,8 +388,10 @@ function store(setup: RemoteConfigSetup, response: RemoteConfigurationResponse)
     // settings" looks like — so that the version is kept either way and the console can still see
     // that this client is up to date with the change that turned it off.
     localStorage.setItem(setup.storeKey, JSON.stringify(values))
+    return true
   } catch {
     // Storage unavailable: the values simply do not survive this page load.
+    return false
   }
 }
 

packages/rum-core/src/domain/lifeCycle.ts

@@ -37,6 +37,17 @@ export const enum LifeCycleEventType {
   RAW_RUM_EVENT_COLLECTED,
   RUM_EVENT_COLLECTED,
   RAW_ERROR_COLLECTED,
+
+  // FLASHCAT FORK - a remote configuration response has just been written to storage. Emitted only
+  // when the write actually happened, so a response refused as stale and a storage failure both
+  // stay silent: a subscriber acting on settings that are not in storage would act on values the
+  // next draw is not going to read.
+  //
+  // Added last on purpose. The values of a const enum are inlined at build time and shift when an
+  // entry is inserted, and everything above this line is upstream's — keeping the fork's own entry
+  // at the end leaves upstream's numbering alone and keeps this file out of the way of the next
+  // upstream merge.
+  REMOTE_CONFIGURATION_STORED,
 }
 
 // This is a workaround for an issue occurring when the Browser SDK is included in a TypeScript
@@ -69,6 +80,7 @@ declare const LifeCycleEventTypeAsConst: {
   RAW_RUM_EVENT_COLLECTED: LifeCycleEventType.RAW_RUM_EVENT_COLLECTED
   RUM_EVENT_COLLECTED: LifeCycleEventType.RUM_EVENT_COLLECTED
   RAW_ERROR_COLLECTED: LifeCycleEventType.RAW_ERROR_COLLECTED
+  REMOTE_CONFIGURATION_STORED: LifeCycleEventType.REMOTE_CONFIGURATION_STORED
 }
 
 // Note: this interface needs to be exported even if it is not used outside of this module, else TS
@@ -93,6 +105,7 @@ export interface LifeCycleEventMap {
     error: RawError
     customerContext?: Context
   }
+  [LifeCycleEventTypeAsConst.REMOTE_CONFIGURATION_STORED]: void
 }
 
 export interface RawRumEventCollectedData<E extends RawRumEvent = RawRumEvent> {