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

♻️ [RUM-8319] Use assembly hook for action #3305

Merged
merged 3 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Use assembly hook for action
  • Loading branch information
amortemousque committed Jan 29, 2025
commit 46c2b56c1b50cf98fe61d4827a76c3f75f6619ed
2 changes: 1 addition & 1 deletion packages/rum-core/src/boot/startRum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ export function startRumEventCollection(
) {
const actionCollection = startActionCollection(
lifeCycle,
hooks,
domMutationObservable,
windowOpenObservable,
configuration,
Expand All @@ -270,7 +271,6 @@ export function startRumEventCollection(
hooks,
sessionManager,
viewHistory,
actionCollection.actionContexts,
displayContext,
ciVisibilityContext,
featureFlagContexts,
Expand Down
29 changes: 29 additions & 0 deletions packages/rum-core/src/domain/action/actionCollection.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,36 @@ import { collectAndValidateRawRumEvents, mockPageStateHistory, mockRumConfigurat
import type { RawRumEvent } from '../../rawRumEvent.types'
import { RumEventType, ActionType } from '../../rawRumEvent.types'
import { LifeCycle, LifeCycleEventType } from '../lifeCycle'
import type { Hooks } from '../../hooks'
import { createHooks, HookNames } from '../../hooks'
import type { ActionContexts } from './actionCollection'
import { startActionCollection } from './actionCollection'

const basePageStateHistory = mockPageStateHistory({ wasInPageStateAt: () => true })

describe('actionCollection', () => {
const lifeCycle = new LifeCycle()
let hooks: Hooks
let addAction: ReturnType<typeof startActionCollection>['addAction']
let rawRumEvents: Array<RawRumEventCollectedData<RawRumEvent>>
let actionContexts: ActionContexts

beforeEach(() => {
const domMutationObservable = new Observable<void>()
const windowOpenObservable = new Observable<void>()
hooks = createHooks()

const actionCollection = startActionCollection(
lifeCycle,
hooks,
domMutationObservable,
windowOpenObservable,
mockRumConfiguration(),
basePageStateHistory
)
registerCleanupTask(actionCollection.stop)
addAction = actionCollection.addAction
actionContexts = actionCollection.actionContexts

rawRumEvents = collectAndValidateRawRumEvents(lifeCycle)
})
Expand Down Expand Up @@ -162,4 +170,25 @@ describe('actionCollection', () => {
handlingStack: 'Error\n at foo\n at bar',
})
})

describe('assembly hook', () => {
;[RumEventType.RESOURCE, RumEventType.LONG_TASK, RumEventType.ERROR].forEach((eventType) => {
it(`should add action properties on ${eventType} from the context`, () => {
const actionId = '1'
spyOn(actionContexts, 'findActionId').and.returnValue(actionId)
const event = hooks.triggerHook(HookNames.Assemble, { eventType, startTime: 0 as RelativeTime })

expect(event).toEqual({ type: eventType, action: { id: actionId } })
})
})
;[RumEventType.VIEW, RumEventType.VITAL].forEach((eventType) => {
it(`should not add action properties on ${eventType} from the context`, () => {
const actionId = '1'
spyOn(actionContexts, 'findActionId').and.returnValue(actionId)
const event = hooks.triggerHook(HookNames.Assemble, { eventType, startTime: 0 as RelativeTime })

expect(event).toEqual(undefined)
})
})
})
})
23 changes: 23 additions & 0 deletions packages/rum-core/src/domain/action/actionCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import type { CommonContext } from '../contexts/commonContext'
import type { PageStateHistory } from '../contexts/pageStateHistory'
import { PageState } from '../contexts/pageStateHistory'
import type { RumActionEventDomainContext } from '../../domainContext.types'
import type { PartialRumEvent, Hooks } from '../../hooks'
import { HookNames } from '../../hooks'
import type { ActionContexts, ClickAction } from './trackClickActions'
import { trackClickActions } from './trackClickActions'

Expand All @@ -34,6 +36,7 @@ export type AutoAction = ClickAction

export function startActionCollection(
lifeCycle: LifeCycle,
hooks: Hooks,
domMutationObservable: Observable<void>,
windowOpenObservable: Observable<void>,
configuration: RumConfiguration,
Expand All @@ -43,6 +46,26 @@ export function startActionCollection(
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, processAction(action, pageStateHistory))
)

hooks.register(HookNames.Assemble, ({ startTime, eventType }): PartialRumEvent | undefined => {
if (
eventType !== RumEventType.ERROR &&
eventType !== RumEventType.RESOURCE &&
eventType !== RumEventType.LONG_TASK
) {
return
}

const actionId = actionContexts.findActionId(startTime)
if (!actionId) {
return
}

return {
type: eventType,
Copy link
Contributor

Choose a reason for hiding this comment

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

The event will already have a type property, right? Assuming that's true, it may make sense to only have the hook return the properties it's adding or changing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right! Specifying the type is a bit of an ugly hack to improve type checking 😞. See

// Ensuring the `type` field is always present improves type checking, especially in conditional logic in hooks (e.g., `if (eventType === 'view')`).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to discuss a better alternative :)

action: { id: actionId },
}
})

let actionContexts: ActionContexts = { findActionId: noop as () => undefined }
let stop: () => void = noop

Expand Down
32 changes: 2 additions & 30 deletions packages/rum-core/src/domain/assembly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@ import {
createRumSessionManagerMock,
createRawRumEvent,
mockRumConfiguration,
mockActionContexts,
mockDisplayContext,
mockViewHistory,
mockFeatureFlagContexts,
} from '../../test'
import type { RumEventDomainContext } from '../domainContext.types'
import type { RawRumActionEvent, RawRumEvent } from '../rawRumEvent.types'
import type { RawRumEvent } from '../rawRumEvent.types'
import { RumEventType } from '../rawRumEvent.types'
import type { RumActionEvent, RumErrorEvent, RumEvent, RumResourceEvent } from '../rumEvent.types'
import type { RumErrorEvent, RumEvent, RumResourceEvent } from '../rumEvent.types'
import { HookNames, createHooks } from '../hooks'
import { startRumAssembly } from './assembly'
import type { RawRumEventCollectedData } from './lifeCycle'
Expand Down Expand Up @@ -507,32 +506,6 @@ describe('rum assembly', () => {
})
})

describe('action context', () => {
it('should be added on some event categories', () => {
const { lifeCycle, serverRumEvents } = setupAssemblyTestWithDefaults()
;[RumEventType.RESOURCE, RumEventType.LONG_TASK, RumEventType.ERROR].forEach((category) => {
notifyRawRumEvent(lifeCycle, {
rawRumEvent: createRawRumEvent(category),
})
expect(serverRumEvents[0].action).toEqual({ id: '7890' })
serverRumEvents.length = 0
})

notifyRawRumEvent(lifeCycle, {
rawRumEvent: createRawRumEvent(RumEventType.VIEW),
})
expect(serverRumEvents[0].action).not.toBeDefined()
serverRumEvents.length = 0

const generatedRawRumActionEvent = createRawRumEvent(RumEventType.ACTION) as RawRumActionEvent
notifyRawRumEvent(lifeCycle, {
rawRumEvent: generatedRawRumActionEvent,
})
expect((serverRumEvents[0] as RumActionEvent).action.id).toEqual(generatedRawRumActionEvent.action.id)
serverRumEvents.length = 0
})
})

describe('service and version', () => {
const extraConfigurationOptions = { service: 'default service', version: 'default version' }

Expand Down Expand Up @@ -1000,7 +973,6 @@ function setupAssemblyTestWithDefaults({
hooks,
rumSessionManager,
{ ...mockViewHistory(), findView: () => findView() },
mockActionContexts(),
mockDisplayContext(),
{ get: () => ciVisibilityContext } as CiVisibilityContext,
featureFlagContexts,
Expand Down
18 changes: 1 addition & 17 deletions packages/rum-core/src/domain/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,7 @@ import {
getConnectivity,
} from '@datadog/browser-core'
import type { RumEventDomainContext } from '../domainContext.types'
import type {
RawRumErrorEvent,
RawRumEvent,
RawRumLongTaskEvent,
RawRumResourceEvent,
RumContext,
} from '../rawRumEvent.types'
import type { RawRumEvent, RumContext } from '../rawRumEvent.types'
import { RumEventType } from '../rawRumEvent.types'
import type { RumEvent } from '../rumEvent.types'
import type { Hooks } from '../hooks'
Expand All @@ -31,7 +25,6 @@ import { LifeCycleEventType } from './lifeCycle'
import type { ViewHistory } from './contexts/viewHistory'
import { SessionReplayState, type RumSessionManager } from './rumSessionManager'
import type { RumConfiguration, FeatureFlagsForEvents } from './configuration'
import type { ActionContexts } from './action/actionCollection'
import type { DisplayContext } from './contexts/displayContext'
import type { CommonContext } from './contexts/commonContext'
import type { ModifiableFieldPaths } from './limitModification'
Expand Down Expand Up @@ -72,7 +65,6 @@ export function startRumAssembly(
hooks: Hooks,
sessionManager: RumSessionManager,
viewHistory: ViewHistory,
actionContexts: ActionContexts,
displayContext: DisplayContext,
ciVisibilityContext: CiVisibilityContext,
featureFlagContexts: FeatureFlagContexts,
Expand Down Expand Up @@ -141,7 +133,6 @@ export function startRumAssembly(

if (session && viewHistoryEntry) {
const commonContext = savedCommonContext || getCommonContext()
const actionId = actionContexts.findActionId(startTime)

const rumContext: RumContext = {
_dd: {
Expand Down Expand Up @@ -172,7 +163,6 @@ export function startRumAssembly(
configuration.trackFeatureFlagsForEvents,
featureFlagContexts
),
action: needToAssembleWithAction(rawRumEvent) && actionId ? { id: actionId } : undefined,
synthetics: syntheticsContext,
ci_test: ciVisibilityContext.get(),
display: displayContext.get(),
Expand Down Expand Up @@ -236,12 +226,6 @@ function shouldSend(
return !rateLimitReached
}

function needToAssembleWithAction(
event: RawRumEvent
): event is RawRumErrorEvent | RawRumResourceEvent | RawRumLongTaskEvent {
return [RumEventType.ERROR, RumEventType.RESOURCE, RumEventType.LONG_TASK].indexOf(event.type) !== -1
}

function findFeatureFlagsContext(
rawRumEvent: RawRumEvent,
eventStartTime: RelativeTime,
Expand Down