-
Notifications
You must be signed in to change notification settings - Fork 142
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
⚡️ Process buffered performance entries in an idle callback #1337
The head ref may contain hidden characters: "aymeric/process-buffered-resources-in-an\u2014idle-callback"
Changes from 2 commits
ebcead7
2d3bb9a
e42a65e
d94be5a
0528e72
e6bd54b
5dc0e14
b9464c0
d23c607
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { | |
monitor, | ||
relativeNow, | ||
runOnReadyState, | ||
requestIdleCallback, | ||
} from '@datadog/browser-core' | ||
import type { RumConfiguration } from '../domain/configuration' | ||
import type { LifeCycle } from '../domain/lifeCycle' | ||
|
@@ -109,8 +110,14 @@ export function startPerformanceCollection(lifeCycle: LifeCycle, configuration: | |
}) | ||
|
||
if (supportPerformanceObject()) { | ||
handleRumPerformanceEntries(lifeCycle, configuration, performance.getEntries()) | ||
const performanceEntries = performance.getEntries() | ||
// Because the performance entry list can be quite large | ||
// delay the computation to prevent the SDK from blocking the main thread on page load | ||
amortemousque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
waitForRequestIdleOrUnload(lifeCycle, () => { | ||
handleRumPerformanceEntries(lifeCycle, configuration, performanceEntries) | ||
}) | ||
} | ||
|
||
if (window.PerformanceObserver) { | ||
const handlePerformanceEntryList = monitor((entries: PerformanceObserverEntryList) => | ||
handleRumPerformanceEntries(lifeCycle, configuration, entries.getEntries()) | ||
|
@@ -154,6 +161,17 @@ export function startPerformanceCollection(lifeCycle: LifeCycle, configuration: | |
} | ||
} | ||
|
||
function waitForRequestIdleOrUnload(lifeCycle: LifeCycle, callback: () => void) { | ||
let hasBeenCalled = false | ||
const callOnce = () => { | ||
if (hasBeenCalled) return | ||
hasBeenCalled = true | ||
callback() | ||
} | ||
requestIdleCallback(callOnce) | ||
amortemousque marked this conversation as resolved.
Show resolved
Hide resolved
|
||
lifeCycle.subscribe(LifeCycleEventType.BEFORE_UNLOAD, callOnce) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ question: what is the reasoning for calling it on unload? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know the detail behind the idle period detection but, FMU we can have cases where they are no idle periods before the unload. This is why the requestIdleCallback API provides a timeout option. Btw the documentation states: "A timeout option is strongly recommended for required work, as otherwise it's possible multiple seconds will elapse before the callback is fired." but in our case BEFORE_UNLOAD seems more appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, these kind of behaviors should be explained in the code and tested. On the other hand, if we are worried about the requestIdleCallback not being called due to too much activity, let's just use a setTimeout, it should at least split this processing from the init. wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other concern about the async call: |
||
} | ||
|
||
export function retrieveInitialDocumentResourceTiming(callback: (timing: RumPerformanceResourceTiming) => void) { | ||
runOnReadyState('interactive', () => { | ||
let timing: RumPerformanceResourceTiming | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could still let this function here as it could be valuable at some point in other packages but let me know.