Skip to content

Commit

Permalink
🐛 [RUMF-997] dont take a FullSnapshot on view creation during session…
Browse files Browse the repository at this point in the history 
… renew (#1011)

* [RUMF-997] add a SESSION_EXPIRED lifecycle event

* 🐛 [RUMF-997] stop recording before renewing the session

A full snapshot was incorrectly taken when renewing the session because
`startRecording` is called before initializing the recorder. So when a
session is renewed, we first create a new view then we stop recording:

```
notify SESSION_RENEWED
  -> notify VIEW_CREATED   (trackViews)
    -> takeFullSnapshot()  (startRecording)
  -> stopRecording()       (recorderApi)
  -> startRecording()      (recorderApi)
```

This commit uses the newly introduced lifecycle event triggered before
SESSION_RENEWED to stop recording when the session is expired:

```
notify SESSION_EXPIRED
  -> stopRecording()
notify SESSION_RENEWED
  -> notify VIEW_CREATED
    -> (nothing happens since recording is stopped)
  -> startRecording()
```

* ✅ [RUMF-997] add a e2e test

to ensure a single fullsnapshot is taken when renewing the session

* ✅ adjust test for new lifecycle event
  • Loading branch information
@BenoitZugmeyer
BenoitZugmeyer authored Aug 26, 2021
1 parent 89bf272 commit 636989e
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 6 deletions.
15 changes: 15 additions & 0 deletions packages/rum-core/src/domain/lifeCycle.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,19 @@ export enum LifeCycleEventType {
VIEW_ENDED,
REQUEST_STARTED,
REQUEST_COMPLETED,

// The SESSION_EXPIRED lifecycle event has been introduced to represent when a session has expired
// and trigger cleanup tasks related to this, prior to renewing the session. Its implementation is
// slightly naive: it is not triggered as soon as the session is expired, but rather just before
// notifying that the session is renewed. Thus, the session id is already set to the newly renewed
// session.
//
// This implementation is "good enough" for our use-cases. Improving this is not trivial,
// primarily because multiple instances of the SDK may be managing the same session cookie at
// the same time, for example when using Logs and RUM on the same page, or opening multiple tabs
// on the same domain.
SESSION_EXPIRED,

SESSION_RENEWED,
BEFORE_UNLOAD,
RAW_RUM_EVENT_COLLECTED,
Expand All @@ -41,6 +54,7 @@ export class LifeCycle {
): void
notify(
eventType:
| LifeCycleEventType.SESSION_EXPIRED
| LifeCycleEventType.SESSION_RENEWED
| LifeCycleEventType.BEFORE_UNLOAD
| LifeCycleEventType.AUTO_ACTION_DISCARDED
Expand Down Expand Up @@ -77,6 +91,7 @@ export class LifeCycle {
): Subscription
subscribe(
eventType:
| LifeCycleEventType.SESSION_EXPIRED
| LifeCycleEventType.SESSION_RENEWED
| LifeCycleEventType.BEFORE_UNLOAD
| LifeCycleEventType.AUTO_ACTION_DISCARDED,
Expand Down
10 changes: 10 additions & 0 deletions packages/rum-core/src/domain/rumSession.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe('rum session', () => {
replaySampleRate: 50,
}
let lifeCycle: LifeCycle
let expireSessionSpy: jasmine.Spy
let renewSessionSpy: jasmine.Spy
let clock: Clock

Expand All @@ -33,8 +34,10 @@ describe('rum session', () => {
pending('no full rum support')
}
clock = mockClock()
expireSessionSpy = jasmine.createSpy('expireSessionSpy')
renewSessionSpy = jasmine.createSpy('renewSessionSpy')
lifeCycle = new LifeCycle()
lifeCycle.subscribe(LifeCycleEventType.SESSION_EXPIRED, expireSessionSpy)
lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, renewSessionSpy)
})

Expand All @@ -52,6 +55,7 @@ describe('rum session', () => {

startRumSession(configuration as Configuration, lifeCycle)

expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.TRACKED_REPLAY}`)
expect(getCookie(SESSION_COOKIE_NAME)).toMatch(/id=[a-f0-9-]/)
Expand All @@ -62,6 +66,7 @@ describe('rum session', () => {

startRumSession(configuration as Configuration, lifeCycle)

expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.TRACKED_LITE}`)
expect(getCookie(SESSION_COOKIE_NAME)).toMatch(/id=[a-f0-9-]/)
Expand All @@ -72,6 +77,7 @@ describe('rum session', () => {

startRumSession(configuration as Configuration, lifeCycle)

expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.NOT_TRACKED}`)
expect(getCookie(SESSION_COOKIE_NAME)).not.toContain('id=')
Expand All @@ -82,6 +88,7 @@ describe('rum session', () => {

startRumSession(configuration as Configuration, lifeCycle)

expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.TRACKED_REPLAY}`)
expect(getCookie(SESSION_COOKIE_NAME)).toContain('id=abcdef')
Expand All @@ -92,6 +99,7 @@ describe('rum session', () => {

startRumSession(configuration as Configuration, lifeCycle)

expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.NOT_TRACKED}`)
})
Expand All @@ -101,12 +109,14 @@ describe('rum session', () => {

setCookie(SESSION_COOKIE_NAME, '', DURATION)
expect(getCookie(SESSION_COOKIE_NAME)).toBeUndefined()
expect(expireSessionSpy).not.toHaveBeenCalled()
expect(renewSessionSpy).not.toHaveBeenCalled()
clock.tick(COOKIE_ACCESS_DELAY)

setupDraws({ tracked: true, trackedWithReplay: true })
document.dispatchEvent(new CustomEvent('click'))

expect(expireSessionSpy).toHaveBeenCalled()
expect(renewSessionSpy).toHaveBeenCalled()
expect(getCookie(SESSION_COOKIE_NAME)).toContain(`${RUM_SESSION_KEY}=${RumTrackingType.TRACKED_REPLAY}`)
expect(getCookie(SESSION_COOKIE_NAME)).toMatch(/id=[a-f0-9-]/)
Expand Down
1 change: 1 addition & 0 deletions packages/rum-core/src/domain/rumSession.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export function startRumSession(configuration: Configuration, lifeCycle: LifeCyc
)

session.renewObservable.subscribe(() => {
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
})

Expand Down
28 changes: 25 additions & 3 deletions packages/rum/src/boot/recorderApi.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ describe('makeRecorderApi', () => {
it('starts recording if startSessionReplayRecording was called', () => {
recorderApi.start()
session.setReplayPlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
expect(startRecordingSpy).not.toHaveBeenCalled()
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
Expand All @@ -164,6 +166,7 @@ describe('makeRecorderApi', () => {
recorderApi.start()
recorderApi.stop()
session.setReplayPlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
})
Expand All @@ -177,6 +180,7 @@ describe('makeRecorderApi', () => {
it('keeps not recording if startSessionReplayRecording was called', () => {
recorderApi.start()
session.setNotTracked()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
Expand All @@ -190,6 +194,7 @@ describe('makeRecorderApi', () => {

it('keeps not recording if startSessionReplayRecording was called', () => {
recorderApi.start()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
Expand All @@ -203,10 +208,12 @@ describe('makeRecorderApi', () => {

it('stops recording if startSessionReplayRecording was called', () => {
recorderApi.start()
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
session.setLitePlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
expect(stopRecordingSpy).toHaveBeenCalled()
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
expect(stopRecordingSpy).toHaveBeenCalled()
})

it('prevents session recording to start if the session is renewed before onload', () => {
Expand All @@ -215,6 +222,7 @@ describe('makeRecorderApi', () => {
rumInit(DEFAULT_INIT_CONFIGURATION)
recorderApi.start()
session.setLitePlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
triggerOnLoad()
expect(startRecordingSpy).not.toHaveBeenCalled()
Expand All @@ -228,10 +236,12 @@ describe('makeRecorderApi', () => {

it('stops recording if startSessionReplayRecording was called', () => {
recorderApi.start()
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
session.setNotTracked()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
expect(stopRecordingSpy).toHaveBeenCalled()
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
expect(stopRecordingSpy).toHaveBeenCalled()
})
})

Expand All @@ -242,14 +252,19 @@ describe('makeRecorderApi', () => {

it('keeps recording if startSessionReplayRecording was called', () => {
recorderApi.start()
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
expect(stopRecordingSpy).toHaveBeenCalled()
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalledTimes(2)
})

it('does not starts recording if stopSessionReplayRecording was called', () => {
recorderApi.start()
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
recorderApi.stop()
expect(stopRecordingSpy).toHaveBeenCalledTimes(1)
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
expect(stopRecordingSpy).toHaveBeenCalledTimes(1)
Expand All @@ -264,6 +279,7 @@ describe('makeRecorderApi', () => {
it('starts recording if startSessionReplayRecording was called', () => {
recorderApi.start()
session.setReplayPlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
Expand All @@ -273,8 +289,10 @@ describe('makeRecorderApi', () => {
recorderApi.start()
recorderApi.stop()
session.setReplayPlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
})
})

Expand All @@ -286,8 +304,10 @@ describe('makeRecorderApi', () => {
it('keeps not recording if startSessionReplayRecording was called', () => {
recorderApi.start()
session.setLitePlan()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
})
})

Expand All @@ -298,8 +318,10 @@ describe('makeRecorderApi', () => {

it('keeps not recording if startSessionReplayRecording was called', () => {
recorderApi.start()
lifeCycle.notify(LifeCycleEventType.SESSION_EXPIRED)
lifeCycle.notify(LifeCycleEventType.SESSION_RENEWED)
expect(startRecordingSpy).not.toHaveBeenCalled()
expect(stopRecordingSpy).not.toHaveBeenCalled()
})
})
})
Expand Down
4 changes: 3 additions & 1 deletion packages/rum/src/boot/recorderApi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,14 @@ export function makeRecorderApi(startRecordingImpl: StartRecording): RecorderApi
session: RumSession,
parentContexts: ParentContexts
) => {
lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => {
lifeCycle.subscribe(LifeCycleEventType.SESSION_EXPIRED, () => {
if (state.status === RecorderStatus.Starting || state.status === RecorderStatus.Started) {
stopStrategy()
state = { status: RecorderStatus.IntentToStart }
}
})

lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => {
if (state.status === RecorderStatus.IntentToStart) {
startStrategy()
}
Expand Down
29 changes: 27 additions & 2 deletions test/e2e/scenario/recorder.scenario.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { CreationReason, IncrementalSource, Segment } from '@datadog/browser-rum/cjs/types'
import { CreationReason, IncrementalSource, RecordType, Segment } from '@datadog/browser-rum/cjs/types'
import { InputData, StyleSheetRuleData, NodeType } from '@datadog/browser-rum/cjs/domain/record/types'
import { RumInitConfiguration } from '@datadog/browser-rum-core'

import { createTest, bundleSetup, html, EventRegistry } from '../lib/framework'
import { browserExecute } from '../lib/helpers/browser'
import { flushEvents } from '../lib/helpers/sdk'
import { flushEvents, renewSession } from '../lib/helpers/sdk'
import {
findElement,
findElementWithIdAttribute,
Expand Down Expand Up @@ -628,12 +628,37 @@ describe('recorder', () => {
expect(styleSheetRules[1].data.adds).toEqual([{ rule: '.added {}', index: 0 }])
})
})

describe('session renewal', () => {
createTest('a single fullSnapshot is taken when the session is renewed')
.withRum()
.withRumInit(initRumAndStartRecording)
.withSetup(bundleSetup)
.run(async ({ events }) => {
await renewSession()

await flushEvents()

expect(events.sessionReplay.length).toBe(2)

const segment = getLastSegment(events)
expect(segment.creation_reason).toBe('init')
expect(segment.records[0].type).toBe(RecordType.Meta)
expect(segment.records[1].type).toBe(RecordType.Focus)
expect(segment.records[2].type).toBe(RecordType.FullSnapshot)
expect(segment.records.slice(3).every((record) => record.type !== RecordType.FullSnapshot)).toBe(true)
})
})
})

function getFirstSegment(events: EventRegistry) {
return events.sessionReplay[0].segment.data
}

function getLastSegment(events: EventRegistry) {
return events.sessionReplay[events.sessionReplay.length - 1].segment.data
}

function initRumAndStartRecording(initConfiguration: RumInitConfiguration) {
window.DD_RUM!.init(initConfiguration)
window.DD_RUM!.startSessionReplayRecording()
Expand Down

0 comments on commit 636989e

Please sign in to comment.