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.

Sign up for GitHub

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

✨ Collect long animation frames as long task events #3272

Merged
merged 3 commits into from
Jan 13, 2025
Merged

✨ Collect long animation frames as long task events #3272

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3dbf48
✨ Collect long animation frames as long task events
thomas-lebeau Jan 9, 2025
e9d3e83
✅ ignore loaf console warnings
thomas-lebeau Jan 10, 2025
923877d
👌 fallback to long task when loAF is not supported
thomas-lebeau Jan 13, 2025
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
1 change: 0 additions & 1 deletion packages/core/src/tools/experimentalFeatures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { objectHasValue } from './utils/objectUtils'
export enum ExperimentalFeature {
WRITABLE_RESOURCE_GRAPHQL = 'writable_resource_graphql',
REMOTE_CONFIGURATION = 'remote_configuration',
LONG_ANIMATION_FRAME = 'long_animation_frame',
ACTION_NAME_MASKING = 'action_name_masking',
CONSISTENT_TRACE_SAMPLING = 'consistent_trace_sampling',
DELAY_VIEWPORT_COLLECTION = 'delay_viewport_collection',
Expand Down
6 changes: 3 additions & 3 deletions packages/rum-core/src/boot/startRum.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { SESSION_KEEP_ALIVE_INTERVAL, THROTTLE_VIEW_UPDATE_PERIOD } from '../dom
import { startViewCollection } from '../domain/view/viewCollection'
import type { RumEvent, RumViewEvent } from '../rumEvent.types'
import type { LocationChange } from '../browser/locationChangeObservable'
import { startLongTaskCollection } from '../domain/longTask/longTaskCollection'
import { startLongAnimationFrameCollection } from '../domain/longAnimationFrame/longAnimationFrameCollection'
import type { RumSessionManager } from '..'
import type { RumConfiguration } from '../domain/configuration'
import { RumEventType } from '../rawRumEvent.types'
Expand Down Expand Up @@ -94,7 +94,7 @@ function startRumStub(
noopRecorderApi
)

startLongTaskCollection(lifeCycle, configuration)
startLongAnimationFrameCollection(lifeCycle, configuration)
return {
stop: () => {
rumEventCollectionStop()
Expand Down Expand Up @@ -279,7 +279,7 @@ describe('rum events url', () => {
changeLocation('http://foo.com/?bar=qux')

notifyPerformanceEntries([
createPerformanceEntry(RumPerformanceEntryType.LONG_TASK, {
createPerformanceEntry(RumPerformanceEntryType.LONG_ANIMATION_FRAME, {
startTime: (relativeNow() - 5) as RelativeTime,
}),
])
Expand Down
13 changes: 3 additions & 10 deletions packages/rum-core/src/boot/startRum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ import {
addTelemetryDebug,
CustomerDataType,
drainPreStartTelemetry,
isExperimentalFeatureEnabled,
ExperimentalFeature,
} from '@datadog/browser-core'
import { createDOMMutationObservable } from '../browser/domMutationObservable'
import { createWindowOpenObservable } from '../browser/windowOpenObservable'
Expand All @@ -29,7 +27,6 @@ import { startViewHistory } from '../domain/contexts/viewHistory'
import { startRequestCollection } from '../domain/requestCollection'
import { startActionCollection } from '../domain/action/actionCollection'
import { startErrorCollection } from '../domain/error/errorCollection'
import { startLongTaskCollection } from '../domain/longTask/longTaskCollection'
import { startResourceCollection } from '../domain/resource/resourceCollection'
import { startViewCollection } from '../domain/view/viewCollection'
import type { RumSessionManager } from '../domain/rumSessionManager'
Expand Down Expand Up @@ -177,13 +174,9 @@ export function startRum(
const { stop: stopResourceCollection } = startResourceCollection(lifeCycle, configuration, pageStateHistory)
cleanupTasks.push(stopResourceCollection)

if (isExperimentalFeatureEnabled(ExperimentalFeature.LONG_ANIMATION_FRAME)) {
if (configuration.trackLongTasks) {
const { stop: stopLongAnimationFrameCollection } = startLongAnimationFrameCollection(lifeCycle, configuration)
cleanupTasks.push(stopLongAnimationFrameCollection)
}
} else {
startLongTaskCollection(lifeCycle, configuration)
if (configuration.trackLongTasks) {
const { stop: stopLongAnimationFrameCollection } = startLongAnimationFrameCollection(lifeCycle, configuration)
cleanupTasks.push(stopLongAnimationFrameCollection)
}

const { addError } = startErrorCollection(lifeCycle, configuration, pageStateHistory, featureFlagContexts)
Expand Down
12 changes: 0 additions & 12 deletions packages/rum-core/src/browser/performanceObservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,12 @@ export enum RumPerformanceEntryType {
FIRST_INPUT = 'first-input',
LARGEST_CONTENTFUL_PAINT = 'largest-contentful-paint',
LAYOUT_SHIFT = 'layout-shift',
LONG_TASK = 'longtask',
LONG_ANIMATION_FRAME = 'long-animation-frame',
NAVIGATION = 'navigation',
PAINT = 'paint',
RESOURCE = 'resource',
}

export interface RumPerformanceLongTaskTiming {
name: string
entryType: RumPerformanceEntryType.LONG_TASK
startTime: RelativeTime
duration: Duration
toJSON(): Omit<PerformanceEntry, 'toJSON'>
}

export interface RumPerformanceResourceTiming {
entryType: RumPerformanceEntryType.RESOURCE
initiatorType: string
Expand Down Expand Up @@ -164,7 +155,6 @@ export interface RumPerformanceLongAnimationFrameTiming {

export type RumPerformanceEntry =
| RumPerformanceResourceTiming
| RumPerformanceLongTaskTiming
| RumPerformanceLongAnimationFrameTiming
| RumPerformancePaintTiming
| RumPerformanceNavigationTiming
Expand All @@ -179,7 +169,6 @@ export type EntryTypeToReturnType = {
[RumPerformanceEntryType.LARGEST_CONTENTFUL_PAINT]: RumLargestContentfulPaintTiming
[RumPerformanceEntryType.LAYOUT_SHIFT]: RumLayoutShiftTiming
[RumPerformanceEntryType.PAINT]: RumPerformancePaintTiming
[RumPerformanceEntryType.LONG_TASK]: RumPerformanceLongTaskTiming
[RumPerformanceEntryType.LONG_ANIMATION_FRAME]: RumPerformanceLongAnimationFrameTiming
[RumPerformanceEntryType.NAVIGATION]: RumPerformanceNavigationTiming
[RumPerformanceEntryType.RESOURCE]: RumPerformanceResourceTiming
Expand Down Expand Up @@ -223,7 +212,6 @@ export function createPerformanceObservable<T extends RumPerformanceEntryType>(
const fallbackSupportedEntryTypes = [
RumPerformanceEntryType.RESOURCE,
RumPerformanceEntryType.NAVIGATION,
RumPerformanceEntryType.LONG_TASK,
RumPerformanceEntryType.PAINT,
]
if (fallbackSupportedEntryTypes.includes(options.type)) {
Expand Down
84 changes: 0 additions & 84 deletions packages/rum-core/src/domain/longTask/longTaskCollection.spec.ts
View file
Open in desktop

This file was deleted.

46 changes: 0 additions & 46 deletions packages/rum-core/src/domain/longTask/longTaskCollection.ts
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions packages/rum-core/test/fixtures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,18 +171,6 @@ export function createPerformanceEntry<T extends RumPerformanceEntryType>(
loadEventEnd: 567 as RelativeTime,
...overrides,
} as EntryTypeToReturnType[T]

case RumPerformanceEntryType.LONG_TASK: {
const entry = {
name: 'self',
duration: 100 as Duration,
entryType: RumPerformanceEntryType.LONG_TASK,
startTime: 1234 as RelativeTime,
...overrides,
} as EntryTypeToReturnType[T]

return { ...entry, toJSON: () => entry }
}
case RumPerformanceEntryType.LONG_ANIMATION_FRAME: {
const entry = {
name: 'long-animation-frame',
Expand Down
8 changes: 7 additions & 1 deletion test/e2e/lib/helpers/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,13 @@ export async function withBrowserLogs(fn: (logs: BrowserLog[]) => void) {
// https://github.com/webdriverio/webdriverio/issues/4275
if (browser.getLogs) {
const logs = (await browser.getLogs('browser')) as BrowserLog[]
fn(logs)

const filterdLogs = logs.filter(
// Ignore long-animation-frame warning (only happens on Edge)
(log) => !log.message.includes("The entry type 'long-animation-frame' does not exist or isn't supported")
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A bunch of e2e are verifying the number browser logs (example)

Another solution would be to makes these test for the console log they are actually expecting. But at the same time it is nice to ensure there is no unexpected console logs.

Copy link
Member

@BenoitZugmeyer BenoitZugmeyer Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💬 suggestion: ‏
We should probably adjust performanceObservable.ts to skip observing unsupported entries to avoid spamming the console on customer websites. Something like:

if (!supportPerformanceTimingEvent(options.type)) {
  return
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Performance.supportedEntryTypes is not supported everywhere. For example,
Long Task are supported since chrome 58, but Performance.supportedEntryTypes only since Chrome 73, while we supposedly support chrome 63+

Copy link
Member

Choose a reason for hiding this comment

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

Well still, I think we should find a way to avoid displaying the warning in browsers without loaf support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, as finally we're falling back to Long tasks when LoAF is not supported, I'm starting either Loaf collection or Long task collection in startRum depending on Performance.supportedEntryTypes.

It should work to get rid of the warning because everywhere LoAF are supported Performance.supportedEntryTypes is also supported.


fn(filterdLogs)
}
}

Expand Down
Loading