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-1060] fix failing worker detection in Firefox #1139

Merged
merged 21 commits into from
Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
84e6191
♻️ [RUMF-1060] give proper types to worker responses
BenoitZugmeyer Oct 14, 2021
c8f90bd
♻️ [RUMF-1060] add `addEventListener('error')` on the deflate worker …
BenoitZugmeyer Oct 14, 2021
4e16cf1
[RUMF-1060] add a 'init <> ready' action to the deflate worker
BenoitZugmeyer Oct 14, 2021
8537b76
🐛 [RUMF-1060] check if the worker responds before starting recording
BenoitZugmeyer Oct 14, 2021
0694b0e
✅ [RUMF-1060] add test on calling stop before worker load
BenoitZugmeyer Oct 15, 2021
63474df
✅ [RUMF-1060] add tests on deflateWorkerSingleton
BenoitZugmeyer Oct 15, 2021
0634666
✅ Fix test IE compatibility
amortemousque Oct 19, 2021
890689c
👌 Update ready state to initialized
amortemousque Oct 19, 2021
7670213
rename deflate worker singleton functions
amortemousque Oct 19, 2021
435eee0
update start deflate worker name
amortemousque Oct 19, 2021
602a407
👌 Add comment about worker creation state
amortemousque Oct 20, 2021
9868a94
✅ Fix unit test IE compatibility
amortemousque Oct 20, 2021
2d97b97
Encapsulate some worker technical test part
amortemousque Oct 20, 2021
c6df2fe
👌 Test with worker back to synchronous
amortemousque Oct 20, 2021
287d6b0
👌 Change worker error type into errored
amortemousque Oct 20, 2021
8930aca
👌 Fix last comment
amortemousque Oct 21, 2021
49acaf4
Update comment to add Firefox feedback
amortemousque Oct 21, 2021
9598ef3
Rewrite comment
amortemousque Oct 21, 2021
205b5e8
FIx test
amortemousque Oct 22, 2021
88f9be1
Merge branch 'main' into benoit/fix-failing-worker-detection-in-firefox
amortemousque Oct 22, 2021
8503b8c
Merge branch 'main' into benoit/fix-failing-worker-detection-in-firefox
amortemousque Oct 25, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/core/src/domain/internalMonitoring.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,20 @@ function startMonitoringBatch(configuration: Configuration) {
}
}

export function startFakeInternalMonitoring() {
const messages: MonitoringMessage[] = []
assign(monitoringConfiguration, {
batch: {
add(message: MonitoringMessage) {
messages.push(message)
},
},
maxMessagesPerPage: Infinity,
sentMessageCount: 0,
})
return messages
}

export function resetInternalMonitoring() {
monitoringConfiguration.batch = undefined
}
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ export {
callMonitored,
addMonitoringMessage,
addErrorToMonitoringBatch,
startFakeInternalMonitoring,
resetInternalMonitoring,
setDebugMode,
} from './domain/internalMonitoring'
export { Observable, Subscription } from './tools/observable'
Expand Down
41 changes: 36 additions & 5 deletions packages/rum/src/boot/recorderApi.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Configuration } from '@datadog/browser-core'
import { Configuration, noop } from '@datadog/browser-core'
import {
RecorderApi,
ParentContexts,
Expand All @@ -10,6 +10,7 @@ import { createNewEvent } from '@datadog/browser-core/test/specHelper'
import { createRumSessionMock, RumSessionMock } from '../../../rum-core/test/mockRumSession'
import { setup, TestSetupBuilder } from '../../../rum-core/test/specHelper'
import { DeflateWorker } from '../domain/segmentCollection/deflateWorker'
import { startDeflateWorker } from '../domain/segmentCollection/startDeflateWorker'
import { makeRecorderApi, StartRecording } from './recorderApi'

const DEFAULT_INIT_CONFIGURATION = { applicationId: 'xxx', clientToken: 'xxx' }
Expand All @@ -19,7 +20,23 @@ describe('makeRecorderApi', () => {
let recorderApi: RecorderApi
let startRecordingSpy: jasmine.Spy<StartRecording>
let stopRecordingSpy: jasmine.Spy<() => void>
let getDeflateWorkerSingletonSpy: jasmine.Spy<() => DeflateWorker | undefined>
let startDeflateWorkerSpy: jasmine.Spy<typeof startDeflateWorker>
const FAKE_WORKER = {} as DeflateWorker

function startDeflateWorkerWith(worker?: DeflateWorker) {
if (!startDeflateWorkerSpy) {
startDeflateWorkerSpy = jasmine.createSpy<typeof startDeflateWorker>('startDeflateWorker')
}
startDeflateWorkerSpy.and.callFake((callback) => callback(worker))
}

function callLastRegisteredInitialisationCallback() {
startDeflateWorkerSpy.calls.mostRecent().args[0](FAKE_WORKER)
}

function stopDeflateWorker() {
startDeflateWorkerSpy.and.callFake(noop)
}

let rumInit: (initConfiguration: RumInitConfiguration) => void

Expand All @@ -29,8 +46,9 @@ describe('makeRecorderApi', () => {
startRecordingSpy = jasmine.createSpy('startRecording').and.callFake(() => ({
stop: stopRecordingSpy,
}))
getDeflateWorkerSingletonSpy = jasmine.createSpy('getDeflateWorkerSingleton').and.returnValue({})
recorderApi = makeRecorderApi(startRecordingSpy, getDeflateWorkerSingletonSpy)

startDeflateWorkerWith(FAKE_WORKER)
recorderApi = makeRecorderApi(startRecordingSpy, startDeflateWorkerSpy)
rumInit = (initConfiguration) => {
recorderApi.onRumStart(lifeCycle, initConfiguration, {} as Configuration, session, {} as ParentContexts)
}
Expand Down Expand Up @@ -104,10 +122,23 @@ describe('makeRecorderApi', () => {
it('do not start recording if worker fails to be instantiated', () => {
setupBuilder.build()
rumInit(DEFAULT_INIT_CONFIGURATION)
getDeflateWorkerSingletonSpy.and.returnValue(undefined)
startDeflateWorkerWith(undefined)
recorderApi.start()
expect(startRecordingSpy).not.toHaveBeenCalled()
})

it('does not start recording multiple times if restarted before worker is initialized', () => {
setupBuilder.build()
rumInit(DEFAULT_INIT_CONFIGURATION)
stopDeflateWorker()
recorderApi.start()
recorderApi.stop()

callLastRegisteredInitialisationCallback()
recorderApi.start()
callLastRegisteredInitialisationCallback()
expect(startRecordingSpy).toHaveBeenCalledTimes(1)
})
})

describe('stopSessionReplayRecording()', () => {
Expand Down
45 changes: 25 additions & 20 deletions packages/rum/src/boot/recorderApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
RecorderApi,
} from '@datadog/browser-rum-core'
import { getReplayStats } from '../domain/replayStats'
import { getDeflateWorkerSingleton } from '../domain/segmentCollection/deflateWorkerSingleton'
import { startDeflateWorker } from '../domain/segmentCollection/startDeflateWorker'

import { startRecording } from './startRecording'

Expand Down Expand Up @@ -42,7 +42,7 @@ type RecorderState =

export function makeRecorderApi(
startRecordingImpl: StartRecording,
getDeflateWorkerSingletonImpl = getDeflateWorkerSingleton
startDeflateWorkerImpl = startDeflateWorker
): RecorderApi {
let state: RecorderState = {
status: RecorderStatus.Stopped,
Expand Down Expand Up @@ -96,26 +96,31 @@ export function makeRecorderApi(
return
}

const worker = getDeflateWorkerSingletonImpl()
if (!worker) {
state = {
status: RecorderStatus.Stopped,
startDeflateWorkerImpl((worker) => {
if (state.status !== RecorderStatus.Starting) {
return
}
return
}

const { stop: stopRecording } = startRecordingImpl(
lifeCycle,
initConfiguration.applicationId,
configuration,
session,
parentContexts,
worker
)
state = {
status: RecorderStatus.Started,
stopRecording,
}
if (!worker) {
state = {
status: RecorderStatus.Stopped,
}
return
}

const { stop: stopRecording } = startRecordingImpl(
lifeCycle,
initConfiguration.applicationId,
configuration,
session,
parentContexts,
worker
)
state = {
status: RecorderStatus.Started,
stopRecording,
}
})
})
}

Expand Down
16 changes: 8 additions & 8 deletions packages/rum/src/boot/startRecording.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { collectAsyncCalls } from '../../test/utils'
import { setMaxSegmentSize } from '../domain/segmentCollection/segmentCollection'

import { Segment, RecordType } from '../types'
import { getDeflateWorkerSingleton } from '../domain/segmentCollection/deflateWorkerSingleton'
import { doStartDeflateWorker } from '../domain/segmentCollection/startDeflateWorker'
import { startRecording } from './startRecording'

describe('startRecording', () => {
Expand Down Expand Up @@ -37,6 +37,12 @@ describe('startRecording', () => {
textField = document.createElement('input')
sandbox.appendChild(textField)

const requestSendSpy = spyOn(HttpRequest.prototype, 'send')
;({
waitAsyncCalls: waitRequestSendCalls,
expectNoExtraAsyncCall: expectNoExtraRequestSendCalls,
} = collectAsyncCalls(requestSendSpy))

setupBuilder = setup()
.withParentContexts({
findView() {
Expand All @@ -61,17 +67,11 @@ describe('startRecording', () => {
configuration,
session,
parentContexts,
getDeflateWorkerSingleton()!
doStartDeflateWorker()!
)
stopRecording = recording ? recording.stop : noop
return { stop: stopRecording }
})

const requestSendSpy = spyOn(HttpRequest.prototype, 'send')
;({
waitAsyncCalls: waitRequestSendCalls,
expectNoExtraAsyncCall: expectNoExtraRequestSendCalls,
} = collectAsyncCalls(requestSendSpy))
})

afterEach(() => {
Expand Down
31 changes: 28 additions & 3 deletions packages/rum/src/domain/segmentCollection/deflateWorker.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,60 @@ export function createDeflateWorker(): DeflateWorker

export interface DeflateWorker {
addEventListener(name: 'message', listener: DeflateWorkerListener): void
addEventListener(name: 'error', listener: (error: ErrorEvent) => void): void
removeEventListener(name: 'message', listener: DeflateWorkerListener): void
removeEventListener(name: 'error', listener: (error: ErrorEvent) => void): void

postMessage(message: DeflateWorkerAction): void
terminate(): void
}

export type DeflateWorkerListener = (event: { data: DeflateWorkerResponse }) => void

export type DeflateWorkerAction =
// Action to send when creating the worker to check if the communication is working correctly.
// The worker should respond with a 'initialized' response.
| {
action: 'init'
}
// Action to send when writing some unfinished data. The worker will respond with a 'wrote'
// response, with the same id and measurements of the wrote data size.
| {
id: number
action: 'write'
id: number
data: string
}
// Action to send when finishing to write some data. The worker will respond with a 'flushed'
// response, with the same id, measurements of the wrote data size and the complete deflate
// data.
| {
id: number
action: 'flush'
id: number
data?: string
}

export type DeflateWorkerResponse =
// Response to 'init' action
| {
type: 'initialized'
}
// Response to 'write' action
| {
type: 'wrote'
id: number
compressedSize: number
additionalRawSize: number
}
// Response to 'flush' action
| {
type: 'flushed'
id: number
result: Uint8Array
additionalRawSize: number
rawSize: number
}
| { error: Error | string }
// Could happen at any time when something goes wrong in the worker
| {
type: 'errored'
error: Error | string
}
17 changes: 15 additions & 2 deletions packages/rum/src/domain/segmentCollection/deflateWorker.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,15 @@ function workerCodeFn() {
monitor((event) => {
const data = event.data
switch (data.action) {
case 'init':
self.postMessage({
type: 'initialized',
})
break
case 'write':
const additionalRawSize = pushData(data.data)
self.postMessage({
type: 'wrote',
id: data.id,
compressedSize: deflate.chunks.reduce((total, chunk) => total + chunk.length, 0),
additionalRawSize,
Expand All @@ -32,6 +38,7 @@ function workerCodeFn() {
const additionalRawSize = data.data ? pushData(data.data) : 0
deflate.push('', constants.Z_FINISH)
self.postMessage({
type: 'flushed',
id: data.id,
result: deflate.result,
additionalRawSize,
Expand All @@ -58,10 +65,16 @@ function workerCodeFn() {
return fn.apply(this, arguments)
} catch (e) {
try {
self.postMessage({ error: e })
self.postMessage({
type: 'errored',
error: e,
})
} catch (_) {
// DATA_CLONE_ERR, cf https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
self.postMessage({ error: '' + e })
self.postMessage({
type: 'errored',
error: '' + e,
})
}
}
}
Expand Down
14 changes: 10 additions & 4 deletions packages/rum/src/domain/segmentCollection/deflateWorker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ describe('deflateWorker', () => {
const deflateWorker = createDeflateWorker()
listen(deflateWorker, 3, (events) => {
expect(events).toEqual([
{ id: 0, compressedSize: 11, additionalRawSize: 3 },
{ id: 1, compressedSize: 20, additionalRawSize: 3 },
{ id: 2, compressedSize: 29, additionalRawSize: 3 },
{ type: 'wrote', id: 0, compressedSize: 11, additionalRawSize: 3 },
{ type: 'wrote', id: 1, compressedSize: 20, additionalRawSize: 3 },
{ type: 'wrote', id: 2, compressedSize: 29, additionalRawSize: 3 },
])
done()
})
Expand All @@ -26,8 +26,9 @@ describe('deflateWorker', () => {
const deflateWorker = createDeflateWorker()
listen(deflateWorker, 2, (events) => {
expect(events).toEqual([
{ id: 0, compressedSize: 11, additionalRawSize: 3 },
{ type: 'wrote', id: 0, compressedSize: 11, additionalRawSize: 3 },
{
type: 'flushed',
id: 1,
result: new Uint8Array([120, 156, 74, 203, 207, 7, 0, 0, 0, 255, 255, 3, 0, 2, 130, 1, 69]),
additionalRawSize: 0,
Expand All @@ -45,6 +46,7 @@ describe('deflateWorker', () => {
listen(deflateWorker, 1, (events) => {
expect(events).toEqual([
{
type: 'flushed',
id: 0,
result: new Uint8Array([120, 156, 74, 203, 207, 7, 0, 0, 0, 255, 255, 3, 0, 2, 130, 1, 69]),
additionalRawSize: 3,
Expand All @@ -61,22 +63,26 @@ describe('deflateWorker', () => {
listen(deflateWorker, 4, (events) => {
expect(events).toEqual([
{
type: 'wrote',
id: 0,
compressedSize: 11,
additionalRawSize: 3,
},
{
type: 'flushed',
id: 1,
result: new Uint8Array([120, 156, 74, 203, 207, 7, 0, 0, 0, 255, 255, 3, 0, 2, 130, 1, 69]),
additionalRawSize: 0,
rawSize: 3,
},
{
type: 'wrote',
id: 2,
compressedSize: 11,
additionalRawSize: 3,
},
{
type: 'flushed',
id: 3,
result: new Uint8Array([120, 156, 74, 74, 44, 2, 0, 0, 0, 255, 255, 3, 0, 2, 93, 1, 54]),
additionalRawSize: 0,
Expand Down
Loading