From 399c85c929c790b5efc2e7bd7057dc2352b85419 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 25 Nov 2022 13:30:44 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20[RUMF-1421]=20move=20e?= =?UTF-8?q?vent=20counters=20outside=20of=20trackViewMetrics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../view/trackViewEventCounts.spec.ts | 51 +++++++++++++++++++ .../view/trackViewEventCounts.ts | 12 +++++ .../view/trackViewMetrics.ts | 23 +-------- .../rumEventsCollection/view/trackViews.ts | 15 +++--- .../src/domain/trackEventCounts.spec.ts | 7 +-- .../rum-core/src/domain/trackEventCounts.ts | 12 ++--- 6 files changed, 78 insertions(+), 42 deletions(-) create mode 100644 packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts create mode 100644 packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts new file mode 100644 index 0000000000..898b8e0997 --- /dev/null +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts @@ -0,0 +1,51 @@ +import type { Context } from '@datadog/browser-core' +import { noop } from '@datadog/browser-core' +import type { RumResourceEvent } from '../../../rumEvent.types' +import { RumEventType } from '../../../rawRumEvent.types' +import { LifeCycle, LifeCycleEventType } from '../../lifeCycle' +import { trackViewEventCounts } from './trackViewEventCounts' + +describe('trackViewEventCounts', () => { + const VIEW_ID = 'a' + const OTHER_VIEW_ID = 'b' + let lifeCycle: LifeCycle + + beforeEach(() => { + lifeCycle = new LifeCycle() + }) + + it('initializes eventCounts to 0', () => { + const { eventCounts } = trackViewEventCounts(lifeCycle, VIEW_ID, noop) + + expect(eventCounts).toEqual({ + actionCount: 0, + errorCount: 0, + longTaskCount: 0, + frustrationCount: 0, + resourceCount: 0, + }) + }) + + it('increments counters', () => { + const { eventCounts } = trackViewEventCounts(lifeCycle, VIEW_ID, noop) + + notifyResourceEvent() + + expect(eventCounts.resourceCount).toBe(1) + }) + + it('does not increment counters related to other views', () => { + const { eventCounts } = trackViewEventCounts(lifeCycle, VIEW_ID, noop) + + notifyResourceEvent(OTHER_VIEW_ID) + + expect(eventCounts.resourceCount).toBe(0) + }) + + function notifyResourceEvent(viewId = VIEW_ID) { + lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, { + type: RumEventType.RESOURCE, + view: { id: viewId }, + } as unknown as RumResourceEvent & Context) + } +}) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts new file mode 100644 index 0000000000..e21613f385 --- /dev/null +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts @@ -0,0 +1,12 @@ +import type { LifeCycle } from '../../lifeCycle' +import { trackEventCounts } from '../../trackEventCounts' + +export function trackViewEventCounts(lifeCycle: LifeCycle, viewId: string, onChange: () => void) { + const { stop, eventCounts } = trackEventCounts({ + lifeCycle, + isChildEvent: (event) => event.view.id === viewId, + onChange, + }) + + return { stop, eventCounts } +} diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts index 03c1f54b96..5d580a5283 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts @@ -6,12 +6,9 @@ import { ViewLoadingType } from '../../../rawRumEvent.types' import type { RumConfiguration } from '../../configuration' import type { LifeCycle } from '../../lifeCycle' import { LifeCycleEventType } from '../../lifeCycle' -import type { EventCounts } from '../../trackEventCounts' -import { trackEventCounts } from '../../trackEventCounts' import { waitPageActivityEnd } from '../../waitPageActivityEnd' export interface ViewMetrics { - eventCounts: EventCounts loadingTime?: Duration cumulativeLayoutShift?: number } @@ -21,27 +18,10 @@ export function trackViewMetrics( domMutationObservable: Observable, configuration: RumConfiguration, scheduleViewUpdate: () => void, - viewId: string, loadingType: ViewLoadingType, viewStart: ClocksState ) { - const viewMetrics: ViewMetrics = { - eventCounts: { - errorCount: 0, - longTaskCount: 0, - resourceCount: 0, - actionCount: 0, - frustrationCount: 0, - }, - } - const { stop: stopEventCountsTracking } = trackEventCounts({ - lifeCycle, - isChildEvent: (event) => event.view.id === viewId, - callback: (newEventCounts) => { - viewMetrics.eventCounts = newEventCounts - scheduleViewUpdate() - }, - }) + const viewMetrics: ViewMetrics = {} const { stop: stopLoadingTimeTracking, setLoadEvent } = trackLoadingTime( lifeCycle, @@ -67,7 +47,6 @@ export function trackViewMetrics( } return { stop: () => { - stopEventCountsTracking() stopLoadingTimeTracking() stopCLSTracking() }, diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts index f355b46ace..f69f5878cc 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts @@ -26,6 +26,7 @@ import type { RumConfiguration } from '../../configuration' import type { Timings } from './trackInitialViewTimings' import { trackInitialViewTimings } from './trackInitialViewTimings' import { trackViewMetrics } from './trackViewMetrics' +import { trackViewEventCounts } from './trackViewEventCounts' export interface ViewEvent { id: string @@ -226,15 +227,9 @@ function newView( setLoadEvent, stop: stopViewMetricsTracking, viewMetrics, - } = trackViewMetrics( - lifeCycle, - domMutationObservable, - configuration, - scheduleViewUpdate, - id, - loadingType, - startClocks - ) + } = trackViewMetrics(lifeCycle, domMutationObservable, configuration, scheduleViewUpdate, loadingType, startClocks) + + const { stop: stopEventCountsTracking, eventCounts } = trackViewEventCounts(lifeCycle, id, scheduleViewUpdate) // Initial view update triggerViewUpdate() @@ -258,6 +253,7 @@ function newView( timings, duration: elapsed(startClocks.timeStamp, currentEnd), isActive: endClocks === undefined, + eventCounts, }, viewMetrics ) @@ -273,6 +269,7 @@ function newView( endClocks = clocks lifeCycle.notify(LifeCycleEventType.VIEW_ENDED, { endClocks }) stopViewMetricsTracking() + stopEventCountsTracking() }, triggerUpdate() { // cancel any pending view updates execution diff --git a/packages/rum-core/src/domain/trackEventCounts.spec.ts b/packages/rum-core/src/domain/trackEventCounts.spec.ts index c93c7a226b..3ab1d23f96 100644 --- a/packages/rum-core/src/domain/trackEventCounts.spec.ts +++ b/packages/rum-core/src/domain/trackEventCounts.spec.ts @@ -3,7 +3,6 @@ import { objectValues } from '@datadog/browser-core' import type { RumEvent } from '../rumEvent.types' import { FrustrationType, RumEventType } from '../rawRumEvent.types' import { LifeCycle, LifeCycleEventType } from './lifeCycle' -import type { EventCounts } from './trackEventCounts' import { trackEventCounts } from './trackEventCounts' describe('trackEventCounts', () => { @@ -71,16 +70,14 @@ describe('trackEventCounts', () => { }) it('invokes a potential callback when a count is increased', () => { - const spy = jasmine.createSpy<(eventCounts: EventCounts) => void>() - trackEventCounts({ lifeCycle, isChildEvent: () => true, callback: spy }) + const spy = jasmine.createSpy<() => void>() + trackEventCounts({ lifeCycle, isChildEvent: () => true, onChange: spy }) notifyCollectedRawRumEvent({ type: RumEventType.RESOURCE }) expect(spy).toHaveBeenCalledTimes(1) - expect(spy.calls.mostRecent().args[0].resourceCount).toBe(1) notifyCollectedRawRumEvent({ type: RumEventType.RESOURCE }) expect(spy).toHaveBeenCalledTimes(2) - expect(spy.calls.mostRecent().args[0].resourceCount).toBe(2) }) it('does not take into account events that are not child events', () => { diff --git a/packages/rum-core/src/domain/trackEventCounts.ts b/packages/rum-core/src/domain/trackEventCounts.ts index 85259921ba..0c511cb52f 100644 --- a/packages/rum-core/src/domain/trackEventCounts.ts +++ b/packages/rum-core/src/domain/trackEventCounts.ts @@ -15,11 +15,11 @@ export interface EventCounts { export function trackEventCounts({ lifeCycle, isChildEvent, - callback = noop, + onChange: callback = noop, }: { lifeCycle: LifeCycle isChildEvent: (event: RumActionEvent | RumErrorEvent | RumLongTaskEvent | RumResourceEvent) => boolean - callback?: (eventCounts: EventCounts) => void + onChange?: () => void }) { const eventCounts: EventCounts = { errorCount: 0, @@ -36,22 +36,22 @@ export function trackEventCounts({ switch (event.type) { case RumEventType.ERROR: eventCounts.errorCount += 1 - callback(eventCounts) + callback() break case RumEventType.ACTION: eventCounts.actionCount += 1 if (event.action.frustration) { eventCounts.frustrationCount += event.action.frustration.type.length } - callback(eventCounts) + callback() break case RumEventType.LONG_TASK: eventCounts.longTaskCount += 1 - callback(eventCounts) + callback() break case RumEventType.RESOURCE: eventCounts.resourceCount += 1 - callback(eventCounts) + callback() break } }) From 060a6ddf24bb67d4b14f65cb1574fd0bb6d6bb94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 9 Dec 2022 15:46:21 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1421]=20keep=20updatin?= =?UTF-8?q?g=20the=20view=20event=20counters=20after=20view=20end?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../view/trackViewEventCounts.spec.ts | 26 ++++++++++++++++++- .../view/trackViewEventCounts.ts | 22 +++++++++++++++- .../rumEventsCollection/view/trackViews.ts | 8 ++++-- 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts index 898b8e0997..8f7266e759 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.spec.ts @@ -2,18 +2,25 @@ import type { Context } from '@datadog/browser-core' import { noop } from '@datadog/browser-core' import type { RumResourceEvent } from '../../../rumEvent.types' import { RumEventType } from '../../../rawRumEvent.types' +import type { Clock } from '../../../../../core/test/specHelper' +import { mockClock } from '../../../../../core/test/specHelper' import { LifeCycle, LifeCycleEventType } from '../../lifeCycle' -import { trackViewEventCounts } from './trackViewEventCounts' +import { KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY, trackViewEventCounts } from './trackViewEventCounts' describe('trackViewEventCounts', () => { const VIEW_ID = 'a' const OTHER_VIEW_ID = 'b' let lifeCycle: LifeCycle + let clock: Clock | undefined beforeEach(() => { lifeCycle = new LifeCycle() }) + afterEach(() => { + if (clock) clock.cleanup() + }) + it('initializes eventCounts to 0', () => { const { eventCounts } = trackViewEventCounts(lifeCycle, VIEW_ID, noop) @@ -42,6 +49,23 @@ describe('trackViewEventCounts', () => { expect(eventCounts.resourceCount).toBe(0) }) + it('when calling scheduleStop, it keeps counting events for a bit of time', () => { + clock = mockClock() + const { scheduleStop, eventCounts } = trackViewEventCounts(lifeCycle, VIEW_ID, noop) + + scheduleStop() + + clock.tick(KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY - 1) + notifyResourceEvent() + + expect(eventCounts.resourceCount).toBe(1) + + clock.tick(1) + notifyResourceEvent() + + expect(eventCounts.resourceCount).toBe(1) + }) + function notifyResourceEvent(viewId = VIEW_ID) { lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, { type: RumEventType.RESOURCE, diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts index e21613f385..e5023c7cf5 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts @@ -1,6 +1,21 @@ +import { ONE_MINUTE } from '@datadog/browser-core' import type { LifeCycle } from '../../lifeCycle' import { trackEventCounts } from '../../trackEventCounts' +// Arbitrary delay for stopping event counting after the view ends. Ideally, we would not stop and +// keep counting events until the end of the session. But this might have a small performance impact +// if there are many many views: we would need to go through each event to see if the related view +// matches. So let's have a fairly short delay to avoid impacting performances too much. +// +// In the future, we could have views stored in a data structure similar to ContextHistory. Whenever +// a child event is collected, we could look into this history to find the matching view and +// increase the associated and increase its counter. Having a centralized data structure for it +// would allow us to look for views more efficiently. +// +// For now, having a small cleanup delay will already improve the situation in most cases. + +export const KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY = 5 * ONE_MINUTE + export function trackViewEventCounts(lifeCycle: LifeCycle, viewId: string, onChange: () => void) { const { stop, eventCounts } = trackEventCounts({ lifeCycle, @@ -8,5 +23,10 @@ export function trackViewEventCounts(lifeCycle: LifeCycle, viewId: string, onCha onChange, }) - return { stop, eventCounts } + return { + scheduleStop: () => { + setTimeout(stop, KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY) + }, + eventCounts, + } } diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts index f69f5878cc..033e3a1680 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViews.ts @@ -229,7 +229,11 @@ function newView( viewMetrics, } = trackViewMetrics(lifeCycle, domMutationObservable, configuration, scheduleViewUpdate, loadingType, startClocks) - const { stop: stopEventCountsTracking, eventCounts } = trackViewEventCounts(lifeCycle, id, scheduleViewUpdate) + const { scheduleStop: scheduleStopEventCountsTracking, eventCounts } = trackViewEventCounts( + lifeCycle, + id, + scheduleViewUpdate + ) // Initial view update triggerViewUpdate() @@ -269,7 +273,7 @@ function newView( endClocks = clocks lifeCycle.notify(LifeCycleEventType.VIEW_ENDED, { endClocks }) stopViewMetricsTracking() - stopEventCountsTracking() + scheduleStopEventCountsTracking() }, triggerUpdate() { // cancel any pending view updates execution From dd9b5e877534adfe82205b2a65d0de1f61d0a94f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt?= Date: Tue, 13 Dec 2022 14:31:52 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=91=8C=20clearer=20documentation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: William Lacroix --- .../rumEventsCollection/view/trackViewEventCounts.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts index e5023c7cf5..65e97c40f6 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts @@ -2,10 +2,13 @@ import { ONE_MINUTE } from '@datadog/browser-core' import type { LifeCycle } from '../../lifeCycle' import { trackEventCounts } from '../../trackEventCounts' -// Arbitrary delay for stopping event counting after the view ends. Ideally, we would not stop and -// keep counting events until the end of the session. But this might have a small performance impact -// if there are many many views: we would need to go through each event to see if the related view -// matches. So let's have a fairly short delay to avoid impacting performances too much. +// Some events are not being counted as they transcend views. To reduce the occurrence; +// an arbitrary delay is added for stopping event counting after the view ends. +// +// Ideally, we would not stop and keep counting events until the end of the session. +// But this might have a small performance impact if there are many many views: +// we would need to go through each event to see if the related view matches. +// So let's have a fairly short delay to avoid impacting performances too much. // // In the future, we could have views stored in a data structure similar to ContextHistory. Whenever // a child event is collected, we could look into this history to find the matching view and From 1265bbc5f407062080179a31f713081c8ecc6091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 13 Dec 2022 14:32:54 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=91=8C=20monitor=20stop=20method?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../rumEventsCollection/view/trackViewEventCounts.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts index 65e97c40f6..1d87c2165a 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewEventCounts.ts @@ -1,12 +1,12 @@ -import { ONE_MINUTE } from '@datadog/browser-core' +import { monitor, ONE_MINUTE } from '@datadog/browser-core' import type { LifeCycle } from '../../lifeCycle' import { trackEventCounts } from '../../trackEventCounts' -// Some events are not being counted as they transcend views. To reduce the occurrence; -// an arbitrary delay is added for stopping event counting after the view ends. -// +// Some events are not being counted as they transcend views. To reduce the occurrence; +// an arbitrary delay is added for stopping event counting after the view ends. +// // Ideally, we would not stop and keep counting events until the end of the session. -// But this might have a small performance impact if there are many many views: +// But this might have a small performance impact if there are many many views: // we would need to go through each event to see if the related view matches. // So let's have a fairly short delay to avoid impacting performances too much. // @@ -28,7 +28,7 @@ export function trackViewEventCounts(lifeCycle: LifeCycle, viewId: string, onCha return { scheduleStop: () => { - setTimeout(stop, KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY) + setTimeout(monitor(stop), KEEP_TRACKING_EVENT_COUNTS_AFTER_VIEW_DELAY) }, eventCounts, }