Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RUMF-867] implement stop recording #771

Merged
merged 7 commits into from
Mar 31, 2021
12 changes: 11 additions & 1 deletion packages/rum-core/src/domain/assembly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,10 +367,20 @@ describe('rum assembly', () => {
commonContext.hasReplay = true

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.VIEW),
rawRumEvent: createRawRumEvent(RumEventType.ERROR),
startTime: 0 as RelativeTime,
})
expect(serverRumEvents[0].session.has_replay).toBe(true)
})

it('should not use commonContext.hasReplay on view events', () => {
commonContext.hasReplay = true

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.VIEW),
startTime: 0 as RelativeTime,
})
expect(serverRumEvents[0].session.has_replay).toBe(undefined)
})
})
})
5 changes: 4 additions & 1 deletion packages/rum-core/src/domain/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ export function startRumAssembly(
date: timeStampNow(),
service: configuration.service,
session: {
has_replay: commonContext.hasReplay,
// must be computed on each event because synthetics instrumentation can be done after sdk execution
// cf https://github.com/puppeteer/puppeteer/issues/3667
type: getSessionType(),
Expand All @@ -73,6 +72,10 @@ export function startRumAssembly(
serverRumEvent.context = context
}

if (!('has_replay' in serverRumEvent.session)) {
;(serverRumEvent.session as { has_replay?: boolean }).has_replay = commonContext.hasReplay
BenoitZugmeyer marked this conversation as resolved.
Show resolved Hide resolved
}

if (!isEmptyObject(commonContext.user)) {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
;(serverRumEvent.usr as RumEvent['usr']) = commonContext.user as User & Context
Expand Down
8 changes: 7 additions & 1 deletion packages/rum-core/src/domain/lifeCycle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export enum LifeCycleEventType {
BEFORE_UNLOAD,
RAW_RUM_EVENT_COLLECTED,
RUM_EVENT_COLLECTED,
RECORD_STARTED,
RECORD_STOPPED,
}

export interface Subscription {
Expand All @@ -44,6 +46,8 @@ export class LifeCycle {
| LifeCycleEventType.BEFORE_UNLOAD
| LifeCycleEventType.AUTO_ACTION_DISCARDED
| LifeCycleEventType.VIEW_ENDED
| LifeCycleEventType.RECORD_STARTED
| LifeCycleEventType.RECORD_STOPPED
): void
notify(
eventType: LifeCycleEventType.RAW_RUM_EVENT_COLLECTED,
Expand Down Expand Up @@ -84,7 +88,9 @@ export class LifeCycle {
| LifeCycleEventType.DOM_MUTATED
| LifeCycleEventType.BEFORE_UNLOAD
| LifeCycleEventType.AUTO_ACTION_DISCARDED
| LifeCycleEventType.VIEW_ENDED,
| LifeCycleEventType.VIEW_ENDED
| LifeCycleEventType.RECORD_STARTED
| LifeCycleEventType.RECORD_STOPPED,
callback: () => void
): Subscription
subscribe(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1061,3 +1061,72 @@ describe('rum track custom timings', () => {
expect(warnSpy).toHaveBeenCalled()
})
})

describe('track hasReplay', () => {
let setupBuilder: TestSetupBuilder
let handler: jasmine.Spy
let getViewEvent: (index: number) => View

beforeEach(() => {
;({ handler, getViewEvent } = spyOnViews())
BenoitZugmeyer marked this conversation as resolved.
Show resolved Hide resolved

setupBuilder = setup()
.withFakeLocation('/foo')
.withFakeClock()
.beforeBuild(({ location, lifeCycle }) => {
lifeCycle.subscribe(LifeCycleEventType.VIEW_UPDATED, handler)
return trackViews(location, lifeCycle)
})
})

afterEach(() => {
setupBuilder.cleanup()
})

it('sets hasReplay to false by default', () => {
setupBuilder.build()
expect(getViewEvent(0).hasReplay).toBe(false)
})

it('sets hasReplay to true when the recording starts', () => {
const { lifeCycle } = setupBuilder.build()

lifeCycle.notify(LifeCycleEventType.RECORD_STARTED)

history.pushState({}, '', '/bar')

expect(getViewEvent(1).hasReplay).toBe(true)
})

it('keeps hasReplay to true when the recording stops', () => {
const { lifeCycle } = setupBuilder.build()

lifeCycle.notify(LifeCycleEventType.RECORD_STARTED)
lifeCycle.notify(LifeCycleEventType.RECORD_STOPPED)

history.pushState({}, '', '/bar')

expect(getViewEvent(1).hasReplay).toBe(true)
})

it('sets hasReplay to true when a new view is created after the recording starts', () => {
const { lifeCycle } = setupBuilder.build()

lifeCycle.notify(LifeCycleEventType.RECORD_STARTED)

history.pushState({}, '', '/bar')

expect(getViewEvent(2).hasReplay).toBe(true)
})

it('sets hasReplay to false when a new view is created after the recording stops', () => {
const { lifeCycle } = setupBuilder.build()

lifeCycle.notify(LifeCycleEventType.RECORD_STARTED)
lifeCycle.notify(LifeCycleEventType.RECORD_STOPPED)

history.pushState({}, '', '/bar')

expect(getViewEvent(2).hasReplay).toBe(false)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export interface View {
loadingTime?: Duration
loadingType: ViewLoadingType
cumulativeLayoutShift?: number
hasReplay: boolean
}

export interface ViewCreatedEvent {
Expand All @@ -55,9 +56,11 @@ export function trackViews(
onNewLocation: NewLocationListener = () => undefined
) {
const startOrigin = 0 as RelativeTime
let hasReplay = false
const initialView = newView(
lifeCycle,
location,
hasReplay,
ViewLoadingType.INITIAL_LOAD,
document.referrer,
startOrigin,
Expand All @@ -79,7 +82,15 @@ export function trackViews(
// Renew view on location changes
currentView.end()
currentView.triggerUpdate()
currentView = newView(lifeCycle, location, ViewLoadingType.ROUTE_CHANGE, currentView.url, undefined, viewName)
currentView = newView(
lifeCycle,
location,
hasReplay,
ViewLoadingType.ROUTE_CHANGE,
currentView.url,
undefined,
viewName
)
return
}
currentView.updateLocation(location)
Expand All @@ -90,7 +101,7 @@ export function trackViews(
lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, () => {
// do not trigger view update to avoid wrong data
currentView.end()
currentView = newView(lifeCycle, location, ViewLoadingType.ROUTE_CHANGE, currentView.url)
currentView = newView(lifeCycle, location, hasReplay, ViewLoadingType.ROUTE_CHANGE, currentView.url)
})

// End the current view on page unload
Expand All @@ -99,6 +110,15 @@ export function trackViews(
currentView.triggerUpdate()
})

lifeCycle.subscribe(LifeCycleEventType.RECORD_STARTED, () => {
hasReplay = true
currentView.updateHasReplay(true)
})

lifeCycle.subscribe(LifeCycleEventType.RECORD_STOPPED, () => {
hasReplay = false
})

// Session keep alive
const keepAliveInterval = window.setInterval(
monitor(() => {
Expand All @@ -125,6 +145,7 @@ export function trackViews(
function newView(
lifeCycle: LifeCycle,
initialLocation: Location,
initialHasReplay: boolean,
loadingType: ViewLoadingType,
referrer: string,
startTime = relativeNow(),
Expand All @@ -145,6 +166,7 @@ function newView(
let loadingTime: Duration | undefined
let endTime: RelativeTime | undefined
let location: Location = { ...initialLocation }
let hasReplay = initialHasReplay

lifeCycle.notify(LifeCycleEventType.VIEW_CREATED, { id, startTime, location, referrer })

Expand Down Expand Up @@ -195,6 +217,7 @@ function newView(
loadingTime,
loadingType,
location,
hasReplay,
referrer,
startTime,
timings,
Expand Down Expand Up @@ -238,6 +261,9 @@ function newView(
updateLocation(newLocation: Location) {
location = { ...newLocation }
},
updateHasReplay(newHasReplay: boolean) {
hasReplay = newHasReplay
},
get url() {
return location.href
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Duration, RelativeTime, ServerDuration } from '@datadog/browser-core'
import { setup, TestSetupBuilder } from '../../../../test/specHelper'
import { RumEventType, ViewLoadingType } from '../../../rawRumEvent.types'
import { LifeCycleEventType } from '../../lifeCycle'
import { View } from './trackViews'
import { startViewCollection } from './viewCollection'

describe('viewCollection', () => {
Expand All @@ -24,7 +25,7 @@ describe('viewCollection', () => {
it('should create view from view update', () => {
const { lifeCycle, rawRumEvents } = setupBuilder.build()
const location: Partial<Location> = {}
const view = {
const view: View = {
cumulativeLayoutShift: 1,
customTimings: {
bar: 20 as Duration,
Expand All @@ -41,6 +42,7 @@ describe('viewCollection', () => {
id: 'xxx',
name: undefined,
isActive: false,
hasReplay: false,
loadingTime: 20 as Duration,
loadingType: ViewLoadingType.INITIAL_LOAD,
location: location as Location,
Expand Down Expand Up @@ -98,6 +100,9 @@ describe('viewCollection', () => {
},
time_spent: (100 * 1e6) as ServerDuration,
},
session: {
has_replay: undefined,
},
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ function processViewUpdate(view: View) {
},
time_spent: toServerDuration(view.duration),
},
session: {
has_replay: view.hasReplay || undefined,
},
}
if (!isEmptyObject(view.customTimings)) {
viewEvent.view.custom_timings = mapValues(
Expand Down
3 changes: 3 additions & 0 deletions packages/rum-core/src/rawRumEvent.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ export interface RawRumViewEvent {
long_task: Count
resource: Count
}
session: {
has_replay: true | undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantically it would be better to have a boolean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but in practice, this field should not exist for backward and other SDK compatibility. If I want to select all events that don't have replay, using @session.has_replay:false would lead to incorrect results since not all SDKs implement this flag. Using [email protected]_replay:true is the way to go.

Using this weird typing forces us to ensure has_replay is never false.

}
_dd: {
document_version: number
}
Expand Down
3 changes: 3 additions & 0 deletions packages/rum-core/test/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ export function createRawRumEvent(type: RumEventType, overrides?: Context): RawR
resource: { count: 0 },
time_spent: 0 as ServerDuration,
},
session: {
has_replay: undefined,
},
},
overrides
)
Expand Down
2 changes: 1 addition & 1 deletion packages/rum-recorder/src/boot/recorder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function startRecording(
trackViewEndRecord(lifeCycle, (record) => addRawRecord(record))

return {
stop() {
stop: () => {
stopRecording()
stopSegmentCollection()
stopTrackingFocusRecords()
Expand Down
6 changes: 4 additions & 2 deletions packages/rum-recorder/src/boot/rumRecorderPublicApi.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable @typescript-eslint/unbound-method */
import { Configuration } from '@datadog/browser-core'
import { Configuration, noop } from '@datadog/browser-core'
import { RumPublicApi, RumUserConfiguration, StartRum } from '@datadog/browser-rum-core'
import { makeRumRecorderPublicApi, StartRecording } from './rumRecorderPublicApi'

Expand All @@ -18,7 +18,9 @@ describe('makeRumRecorderPublicApi', () => {

beforeEach(() => {
enabledFlags = []
startRecordingSpy = jasmine.createSpy()
startRecordingSpy = jasmine.createSpy().and.callFake(() => ({
stop: noop,
}))
startRumSpy = jasmine.createSpy().and.callFake(() => {
const configuration: Partial<Configuration> = {
isEnabled(flag: string) {
Expand Down
Loading