From 9be78e77fb24d5c50b2bd76d88e46a8918050d18 Mon Sep 17 00:00:00 2001 From: Fiona Date: Tue, 1 Sep 2026 23:51:53 -0700 Subject: [PATCH] refactor(rum): stop acting on a rate that rises to a hundred Of the three changes that did not wait for the running session to end, this was the one with the weakest claim to a place. `setForcedSession()` already exists for "collect this visitor now" and is precise where a global rate is blunt; it was the only one of the three that raises volume, and does so the same day nobody asked for it; and nothing about "let us see more" is urgent enough that waiting for the next session costs anything that cannot be had later. The other two both undo something that cannot be undone later -- a second of plaintext already uploaded, an event already ingested. It was also what made the remaining rules hard to state. Both survivors are about a session that is being collected, so that precondition rises to the top of the function: the nesting around the privacy comparison goes, the rate check loses its conjunction, and the guard for a forced session goes back to being simply true -- ending a collected forced session on a rate really would only produce the same session again. The rule is nineteen lines with no nesting. The motivation is corrected everywhere it was stated, in the option's own documentation and in the changelog. It said this was for visitors who never go idle. It is not: settings are fetched at page load and at each new session and never on a timer, so a single tab that is never reloaded hears nothing until the four-hour cap -- an always-on screen is the case this does least for. What it actually changes is the ordinary visit, where the client downloads the new settings on the next page load and, until now, went on under the old decision for the rest of that visit. --- CHANGELOG.md | 60 +++++----- .../src/domain/configuration/configuration.ts | 22 ++-- .../src/domain/rumSessionManager.spec.ts | 56 ++++----- .../rum-core/src/domain/rumSessionManager.ts | 107 +++++++++--------- 4 files changed, 116 insertions(+), 129 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b97349e43d..a224436f0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,40 +20,36 @@ ## Unreleased -- ✨ Three changes published from the console now end the running session, so they reach the - visitor at their next interaction instead of waiting for that session to end on its own: a - session sample rate of 0 while the visitor is being collected — the emergency stop — a rate of - 100 while they are not, and a stricter Session Replay privacy level while they are being - collected. The session that ends is collected to its end as it began, so no recording is left - masked in one half and plain in the other. Every other change — any rate between 0 and 100, a - loosening privacy level, the replay and trace rates — still waits for the next session. Custom - values wait on their own too, but not once `beforeSampling` turns them into one of the three: a - callback answering 0 for the values just published ends the session exactly as a published 0 - would. Nothing here happens without `remoteConfigurationEnabled: true`. -- 📝 What you will see on the day you publish one of those three: session counts rise and average - session length drops, because each affected visitor's running session is split at that moment; a - replay in progress ends at the split, and the session that follows draws again, so it carries a - new recording only if that draw keeps one; a rate of 100 makes previously invisible visitors - appear within hours rather than the next day, so collected volume climbs the same day. That is - the change taking effect, not a defect. -- 📝 "At once" means "as soon as this client hears of the change". Settings are fetched at page load - and at each new session, never on a timer, so a page nobody reloads hears of a publish at its next - session boundary — at most four hours away, the cap on a session's life. Opening a tab or - reloading any page fetches immediately and ends the session every tab shares, which is why a - visitor who touches the site converges in seconds. A change that is not one of the three still - takes effect one session after that. -- 📝 The three act on what actually changed, not on the activation mode recorded with the publish: - a change the console files as "next session" still ends the running session if it is one of them. +- ✨ Two changes published from the console now end the running session, so they reach the visitor + at their next interaction instead of waiting for that session to end on its own: a stricter + Session Replay privacy level, and a session sample rate of 0 — the emergency stop, which took up + to four hours to stop anything before this. Both apply only while the visitor is being collected; + one who is not records nothing and sends nothing, so neither change has anything to act on there. + The session that ends is collected to its end as it began, so no recording is left masked in one + half and plain in the other. Every other change still waits for the next session, including a + loosening privacy level and a rate rising to 100 — for "collect this visitor now" there is + `setForcedSession()`. Custom values wait on their own too, but not once `beforeSampling` turns + them into a rate of 0. Nothing here happens without `remoteConfigurationEnabled: true`. +- 📝 How soon "does not wait" is depends on when this client next hears of the change, and it hears + only at page load and at each new session — there is no timer. A visitor who keeps loading pages + hears within seconds of the publish and their session ends there. A single tab that is never + reloaded hears nothing until its session reaches the four-hour cap, so an always-on screen is the + case this does least for; any other tab the same visitor loads ends the session they share. +- 📝 What you will see on the day you publish one of the two: session counts rise and average + session length drops, because each affected visitor's running session is split at that moment, + and a replay in progress ends at the split — the session that follows draws again, so it carries + a new recording only if that draw keeps one. That is the change taking effect, not a defect. +- 📝 The two act on what actually changed, not on the activation mode recorded with the publish: a + change the console files as "next session" still ends the running session if it is one of them. - 📝 `beforeSampling` is now also consulted when settings arrive, away from any draw, to work out which rate would apply. It must stay free of side effects and answer the same way for the same - input: a callback that draws its own lottery — answering 0 or 100 at random — can end a session - that a steady answer would have left running. -- 📝 A session forced with `setForcedSession()` is not ended by a rate while it is being collected: - forcing decides whether this visitor is collected, and every draw the page makes is collected - whatever the console says, so ending it would only produce the same session again. A - stricter privacy level still ends it, because forcing says nothing about how much of the page may - be uploaded in the clear. The page forces the next session on its own, so the visit continues as - two sessions. + input: a callback that draws its own lottery — answering 0 at random — can end a session that a + steady answer would have left running. +- 📝 A session forced with `setForcedSession()` is not ended by a rate: forcing decides whether this + visitor is collected, and every draw the page makes is collected whatever the console says, so + ending it would only produce the same session again. A stricter privacy level still ends it, + because forcing says nothing about how much of the page may be uploaded in the clear. The page + forces the next session on its own, so the visit continues as two sessions. - 📝 Turning remote configuration off is itself a change: the rates go back to the ones passed to `init`. On a site whose init rate is 0, switching it off stops collection at once rather than at the next session. diff --git a/packages/rum-core/src/domain/configuration/configuration.ts b/packages/rum-core/src/domain/configuration/configuration.ts index 62cc56db14..9ea4ca9ea7 100644 --- a/packages/rum-core/src/domain/configuration/configuration.ts +++ b/packages/rum-core/src/domain/configuration/configuration.ts @@ -93,15 +93,19 @@ export interface RumInitConfiguration extends InitConfiguration { * 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, 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` while they are being collected — a visitor who is not being collected - * records nothing, so a stricter level has no plaintext to catch there. 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. + * re-decided in place. Two changes do not wait for that session to end on its own, because their + * effect on it can be told without drawing again: a stricter `defaultPrivacyLevel`, and a session + * sample rate of 0. Both apply only while the visitor is being collected — one who is not records + * nothing and sends nothing, so neither has anything to act on there. Either 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 waits for the next session, a loosening privacy level and a + * rate rising to 100 included — for "collect this visitor now" there is `setForcedSession()`. + * + * How soon "does not wait" is depends on when this client next hears of the change, and it hears + * only at page load and at each new session. A visitor who keeps loading pages hears within + * seconds; a single tab that is never reloaded hears nothing until its session reaches the + * four-hour cap. * * The values below stay in use until the first settings arrive, and whenever the settings cannot * be reached. diff --git a/packages/rum-core/src/domain/rumSessionManager.spec.ts b/packages/rum-core/src/domain/rumSessionManager.spec.ts index cfa6debf94..40878e2db0 100644 --- a/packages/rum-core/src/domain/rumSessionManager.spec.ts +++ b/packages/rum-core/src/domain/rumSessionManager.spec.ts @@ -853,7 +853,7 @@ describe('rum session manager', () => { return getSessionState(SESSION_STORE_KEY).isExpired === '1' } - describe('the three changes it can decide on its own', () => { + describe('the two changes it can decide on its own', () => { it('ends a session being collected when the rate goes to zero', () => { storeRemote({ version: 1, sessionSampleRate: 100, sessionReplaySampleRate: 100 }) startWith({ sessionSampleRate: 100 }) @@ -864,16 +864,6 @@ describe('rum session manager', () => { expect(isSessionEnded()).toBeTrue() }) - it('ends a session that is not being collected when the rate goes to a hundred', () => { - storeRemote({ version: 1, sessionSampleRate: 0 }) - startWith({ sessionSampleRate: 0 }) - expect(getSessionState(SESSION_STORE_KEY)[RUM_SESSION_KEY]).toBe(RumTrackingType.NOT_TRACKED) - - deliver({ version: 2, sessionSampleRate: 100, sessionReplaySampleRate: 100 }) - - expect(isSessionEnded()).toBeTrue() - }) - it('ends the session when the privacy level tightens', () => { storeRemote({ version: 1, sessionSampleRate: 100, defaultPrivacyLevel: 'allow' }) startWith({ sessionSampleRate: 100, defaultPrivacyLevel: 'allow' }) @@ -934,14 +924,14 @@ describe('rum session manager', () => { }) it('draws the session that follows on the settings that have just landed', () => { - storeRemote({ version: 1, sessionSampleRate: 0 }) - startWith({ sessionSampleRate: 0 }) + storeRemote({ version: 1, sessionSampleRate: 100, sessionReplaySampleRate: 100 }) + startWith({ sessionSampleRate: 100 }) - deliver({ version: 2, sessionSampleRate: 100, sessionReplaySampleRate: 100 }) + deliver({ version: 2, sessionSampleRate: 0 }) clock.tick(STORAGE_POLL_DELAY) document.dispatchEvent(createNewEvent(DOM_EVENT.CLICK)) - expect(getSessionState(SESSION_STORE_KEY)[RUM_SESSION_KEY]).toBe(RumTrackingType.TRACKED_WITH_SESSION_REPLAY) + expect(getSessionState(SESSION_STORE_KEY)[RUM_SESSION_KEY]).toBe(RumTrackingType.NOT_TRACKED) }) }) @@ -956,6 +946,20 @@ describe('rum session manager', () => { expect(isSessionEnded()).toBeFalse() }) + it('leaves a session that is not being collected alone when the rate goes to a hundred', () => { + storeRemote({ version: 1, sessionSampleRate: 0 }) + startWith({ sessionSampleRate: 0 }) + expect(getSessionState(SESSION_STORE_KEY)[RUM_SESSION_KEY]).toBe(RumTrackingType.NOT_TRACKED) + + // The one rate whose outcome could be asserted and deliberately is not: `setForcedSession` + // already covers "collect this visitor now", raising volume unannounced is the one + // direction that surprises, and nothing about it is urgent. + deliver({ version: 2, sessionSampleRate: 100, sessionReplaySampleRate: 100 }) + + expect(expireSessionSpy).not.toHaveBeenCalled() + expect(isSessionEnded()).toBeFalse() + }) + it('leaves a session that is not collected alone when the rate merely rises', () => { storeRemote({ version: 1, sessionSampleRate: 0 }) startWith({ sessionSampleRate: 0 }) @@ -1166,28 +1170,6 @@ describe('rum session manager', () => { return rumSessionManager } - it('is still ended by a rate of a hundred when the session it adopted collects nothing', () => { - storeRemote({ version: 1, sessionSampleRate: 0 }) - setCookie(SESSION_STORE_KEY, `rum=0&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) - const rumSessionManager = startWith({ sessionSampleRate: 0 }) - - // Forcing ends a session that collects nothing, so that the next draw can be the forced - // one. Before that draw happens, a tab that never forced anything starts a session of its - // own, and this page adopts it: the page is forced while the session it holds is not. - rumSessionManager.setForcedSession() - setCookie(SESSION_STORE_KEY, `rum=0&created=${Date.now()}&expire=${Date.now() + DURATION}`, DURATION) - clock.tick(STORAGE_POLL_DELAY) - document.dispatchEvent(createNewEvent(DOM_EVENT.CLICK)) - expect(getSessionState(SESSION_STORE_KEY)[RUM_SESSION_KEY]).toBe(RumTrackingType.NOT_TRACKED) - expireSessionSpy.calls.reset() - - // Here the rate has something to change, so the exemption does not apply: ending the - // session is what lets the next draw be the forced one this page asked for. - deliver({ version: 2, sessionSampleRate: 100 }) - - expect(isSessionEnded()).toBeTrue() - }) - it('is not ended by a rate, since every draw it makes is collected anyway', () => { storeRemote({ version: 1, sessionSampleRate: 0 }) startForced() diff --git a/packages/rum-core/src/domain/rumSessionManager.ts b/packages/rum-core/src/domain/rumSessionManager.ts index dc82b415c9..9e742043ee 100644 --- a/packages/rum-core/src/domain/rumSessionManager.ts +++ b/packages/rum-core/src/domain/rumSessionManager.ts @@ -207,79 +207,84 @@ export function startRumSessionManager( lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED) }) - // FLASHCAT FORK - a change published mid-session normally waits for that session to end on its - // own, which for a visitor who never goes idle is hours away. Three changes cannot afford the - // wait, and what makes exactly those three special is that their outcome for the running session - // can be asserted without drawing again: + // FLASHCAT FORK - by the time this runs the client has already downloaded the new settings and + // filed them away, and without this it would then do nothing with them until the running session + // ends on its own — up to four hours. That wait is the whole problem: a visitor who keeps loading + // pages fetches the change within seconds and then carries on under the old decision for the rest + // of their visit. // - // - a session sample rate of 0 while this session is being collected: nothing is meant to be - // collected any more, and this is the emergency stop the console offers; - // - a session sample rate of 100 while this session is not: everything is meant to be - // collected, and this visitor is the exception; - // - a stricter default privacy level while this session is being collected: every further - // second recorded is a second of plaintext uploaded, and masking cannot reach back for it. + // Two changes are not made to wait, and what makes exactly those two special is that their + // outcome for the running session can be asserted without drawing again: // - // No other rate says anything about whether THIS session should have been kept — only a second - // draw could, and drawing twice silently turns a rate p into p². So everything else waits for - // the next session, a loosening privacy level included. Loosening waits on purpose: the delay - // is what leaves an operator room to undo a mistake, and what it costs meanwhile is more of the - // data already being collected. + // - a stricter default privacy level: every further second recorded is a second of plaintext + // uploaded, and masking cannot reach back for it. This is the one whose cost is not + // recoverable, and the reason the rest of this exists; + // - a session sample rate of 0: nothing is meant to be collected any more, and this is the + // emergency stop the console offers — one that took four hours would not be one. // - // The action is always to end the session and let the next activity start a new one — never to - // flip the running one, which would leave a replay masked in its first half and plain in its - // second, or invent a session that begins in the middle of a visit. + // Both are about a session that is being collected, which is why that is the first thing checked. + // A visitor who is not being collected records nothing and uploads nothing, so neither rule has + // anything to act on for them. + // + // No rate other than 0 says anything about whether THIS session should have been kept — only a + // second draw could, and drawing twice silently turns a rate p into p². A rate of 100 could be + // asserted about a session that is not collected, and deliberately is not acted on: `setForcedSession` + // already exists for "collect this visitor now", it is the one direction that raises volume + // unannounced, and nothing about it is urgent. So everything else waits for the next session, a + // loosening privacy level included. Loosening waits on purpose: the delay is what leaves an + // operator room to undo a mistake, and what it costs meanwhile is more of the data already being + // collected. + // + // The action is to end the session and let the next activity start a new one — never to flip the + // running one, which would leave a replay masked in its first half and plain in its second, or + // invent a session that begins in the middle of a visit. // // It stays idempotent with no bookkeeping at all: it compares what this session was drawn under // against what a draw would use now, and ending the session is exactly what makes that // difference disappear. The same response arriving again — another tab, a retry, a reload — // finds nothing left to act on. + // + // What it cannot reach: settings are fetched at start-up and on session renewal only, so a page + // that is never reloaded never hears of the change. A single always-visible tab is exactly that + // page — the visibility timer keeps renewing it, so it fetches nothing until the four-hour cap. + // Any other tab of the same visitor that does load a page ends the session they share. function endSessionIfSettingsAreDecisive() { const session = sessionManager.findSession() - if (!session) { - // Nothing to end. Whatever starts the next session draws on the settings just stored, which - // is the ordinary path and already gives them their effect. + if (!session || !isTypeTracked(session.trackingType)) { + // Nothing here that ending would change. Whatever starts this visitor's next session draws + // on the settings just stored, which is the ordinary path and already gives them effect. + // + // It also could not be decided if we wanted to: a session that is not collected is given no + // id, so no record is kept of what it was drawn under. The comparison below would fall + // through to the init value on every announcement and keep answering "tighter", ending one + // empty session after another for as long as the visitor stayed. return } const remote = readRemoteConfig(configuration.remoteConfig) - // Whether this session is collected is read off the session itself rather than reconstructed - // by comparing rates: the session IS the outcome its draw produced, and an outcome is the only - // thing 0 and 100 let us assert anything about. - const isCollected = isTypeTracked(session.trackingType) - - // Only a session that is being collected can be recording, and only a recording can be too - // plain. A sampled-out visitor uploads nothing, so a stricter level has nothing to protect - // there — and nothing to compare against either: a session that is not collected is given no - // id, so no draw is recorded for it and what it was drawn under cannot be read back here. The - // comparison would fall through to the init value on every announcement and keep answering - // "tighter", ending one empty session after another for as long as the visitor stays. - if (isCollected) { - // What this session is masking pages with right now, which is not the previously stored - // settings: settings are stored while a session runs, and the session was drawn under - // whatever was stored before that. No record means the draw used the init value, and so does - // the recorder — see `startRecording`, which falls back the same way. - const drawnPrivacyLevel = drawnHistory.find()?.defaultPrivacyLevel ?? configuration.defaultPrivacyLevel - const nextPrivacyLevel = remote.defaultPrivacyLevel ?? configuration.defaultPrivacyLevel - if (PRIVACY_LEVEL_STRICTNESS[nextPrivacyLevel] > PRIVACY_LEVEL_STRICTNESS[drawnPrivacyLevel]) { - sessionManager.expire() - return - } + // What this session is masking pages with right now, which is not the previously stored + // settings: settings are stored while a session runs, and the session was drawn under whatever + // was stored before that. No record means the draw used the init value, and so does the + // recorder — see `startRecording`, which falls back the same way. + const drawnPrivacyLevel = drawnHistory.find()?.defaultPrivacyLevel ?? configuration.defaultPrivacyLevel + const nextPrivacyLevel = remote.defaultPrivacyLevel ?? configuration.defaultPrivacyLevel + if (PRIVACY_LEVEL_STRICTNESS[nextPrivacyLevel] > PRIVACY_LEVEL_STRICTNESS[drawnPrivacyLevel]) { + sessionManager.expire() + return } - if (forcedSession && isCollected) { + if (forcedSession) { // The host application has taken this page off the rates deliberately, and every draw it - // makes from now on is collected whatever the console says. Ending a collected session on a - // rate would only replace it with another collected one — the same difference, forever. That - // reasoning runs out when the session is not collected: this page can adopt one an unforced - // tab drew, and there a rate of 100 has something to change, so it is left to the rule - // below. The flag is this page's either way — another tab that never called - // `setForcedSession` reads the shared session as an ordinary one. + // makes from now on is collected whatever the console says. Ending it on a rate would only + // replace it with an identical forced session — the same difference, forever. The flag is + // this page's: another tab of the same visitor that never called `setForcedSession` reads + // the shared session as an ordinary one and may end it on a rate. return } const { sessionSampleRate } = resolveSampleRates(configuration, remote) - if ((sessionSampleRate === 0 && isCollected) || (sessionSampleRate === 100 && !isCollected)) { + if (sessionSampleRate === 0) { sessionManager.expire() } }