🐛 [RUMF-670] wait for the DOM to be ready before getting the trace id #525
Conversation
Because the RUM SDK can be initialized synchronously when the document is loading, the DOM may not be fully accessible when getting the trace id set by APM. In particular, if meta tags are written after the SDK snippet, the SDK will not have access to them during init.
Codecov Report
@@ Coverage Diff @@
## master #525 +/- ##
==========================================
- Coverage 87.97% 87.79% -0.19%
==========================================
Files 36 36
Lines 2262 2269 +7
Branches 466 467 +1
==========================================
+ Hits 1990 1992 +2
- Misses 272 277 +5
Continue to review full report at Codecov.
|
| export function retrieveInitialDocumentResourceTiming(callback: (timing: RumPerformanceResourceTiming) => void) { | ||
| runOnReadyState('interactive', () => { |
There was a problem hiding this comment.
Other function name is retrieveNavigationTimingWhenLoaded, so I think it could be nice to also reflect in the function name that it is waiting for a specific page state.
However, it could be even clearer to move the runOnReadyState outside of these functions:
runOnReadyState('interactive', () => {
retrieveInitialDocumentResourceTiming((timing) => {
handleRumPerformanceEntry(lifeCycle, timing)
})
}}
runOnReadyState('complete', () => {
retrieveNavigationTiming((timing) => {
handleRumPerformanceEntry(lifeCycle, timing)
})
}}
There was a problem hiding this comment.
Waiting for the DOM to be ready or document to be loaded is an implementation detail and I think it's better to keep it in the function. In particular, retrieveNavigationTiming is waiting for the document load + a bit of time for the loadEventEnd to be defined: it would be hard to understand what's going on if those tasks were in two different places.
I'm fine with renaming the function to retrieveInitialDocumentResourceTiming to retrieveInitialDocumentResourceTimingWhenDomReady even if I don't see much value in it.
There was a problem hiding this comment.
ok, so maybe rename retrieveNavigationTimingWhenLoaded to retrieveNavigationTiming to be consistent in considering it as an implementation detail
BenoitZugmeyer
deleted the
benoit/wait-for-dom-ready-before-getting-document-trace-id
branch
Motivation
Because the RUM SDK can be initialized synchronously when the document is loading, the DOM may not be fully accessible when getting the trace id set by APM. In particular, if meta tags are written after the SDK snippet, the SDK will not have access to them during init.
Changes
Wait for DOM to be loaded before getting the trace id and generating the initial document resource.
Testing
Tests have been written in the integration test POC PR #524
I have gone over the contributing documentation.