-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add ability to prime URL metrics #1850
base: trunk
Are you sure you want to change the base?
Changes from all commits
57f9958
71cd706
da1d275
bfed250
6ec82e3
af0172e
bff9973
c155fc1
7640ef7
f05f6c0
0631daf
5d93fdc
3118fb3
d8cfa02
8879f60
72b18fd
f5bba48
b0422eb
db1fd81
63564c4
4fd36b6
82a16b5
a50f23e
9201c86
163d24e
57d77ea
2bc9f49
d17dede
97b888f
931861b
0a7c1f1
969f6b5
c1e245d
b7eeef7
41f5085
42bfb98
7487ae8
49f9eab
fed3012
591a013
17e24c4
32f1e9e
53cc462
f9fd604
8de014a
ce69218
7908b89
3df2f36
14f8be3
fdcb05f
a2ebf17
3a795e9
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 | ||||
---|---|---|---|---|---|---|
|
@@ -65,6 +65,14 @@ const storageLockTimeSessionKey = 'odStorageLockTime'; | |||||
*/ | ||||||
const compressionDebounceWaitDuration = 1000; | ||||||
|
||||||
/** | ||||||
* Verification token for skipping the storage lock check while priming URL Metrics. | ||||||
* | ||||||
* @see {detect} | ||||||
* @type {?string} | ||||||
*/ | ||||||
let odPrimeUrlMetricsVerificationToken = null; | ||||||
|
||||||
/** | ||||||
* Checks whether storage is locked. | ||||||
* | ||||||
|
@@ -73,6 +81,10 @@ const compressionDebounceWaitDuration = 1000; | |||||
* @return {boolean} Whether storage is locked. | ||||||
*/ | ||||||
function isStorageLocked( currentTime, storageLockTTL ) { | ||||||
if ( odPrimeUrlMetricsVerificationToken ) { | ||||||
return false; | ||||||
} | ||||||
|
||||||
if ( storageLockTTL === 0 ) { | ||||||
return false; | ||||||
} | ||||||
|
@@ -503,6 +515,38 @@ function debounceCompressUrlMetric() { | |||||
}, compressionDebounceWaitDuration ); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Notifies about the URL Metric request status. | ||||||
* | ||||||
* @param {Object} status - The status details. | ||||||
* @param {boolean} status.success - Indicates if the request succeeded. | ||||||
* @param {string} [status.error] - An error message if the request failed. | ||||||
* @param {Object} [options] - Options for where to dispatch the message. | ||||||
* @param {boolean} [options.toParent=true] - Whether to send the message to the parent window. | ||||||
* @param {boolean} [options.toLocal=true] - Whether to dispatch a custom event locally. | ||||||
*/ | ||||||
function notifyStatus( status, options = { toParent: true, toLocal: true } ) { | ||||||
const message = { | ||||||
type: 'OD_PRIME_URL_METRICS_REQUEST_STATUS', | ||||||
success: status.success, | ||||||
...( status.error && { error: status.error } ), | ||||||
}; | ||||||
|
||||||
// This will be used when URL metrics are primed using a IFRAME. | ||||||
if ( options.toParent && window.parent && window.parent !== window ) { | ||||||
window.parent.postMessage( message, '*' ); | ||||||
} | ||||||
|
||||||
// This will be used when URL metrics are primed using Puppeteer script. | ||||||
if ( options.toLocal ) { | ||||||
document.dispatchEvent( | ||||||
new CustomEvent( message.type, { | ||||||
detail: { ...status }, | ||||||
} ) | ||||||
); | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* @typedef {{timestamp: number, creationDate: Date}} UrlMetricDebugData | ||||||
* @typedef {{groups: Array<{url_metrics: Array<UrlMetricDebugData>}>}} CollectionDebugData | ||||||
|
@@ -595,6 +639,34 @@ export default async function detect( { | |||||
return; | ||||||
} | ||||||
|
||||||
try { | ||||||
if ( | ||||||
win.parent && | ||||||
win.location.origin === win.parent.location.origin | ||||||
) { | ||||||
/** @type {HTMLIFrameElement|null} */ | ||||||
const urlPrimeIframeElement = win.parent.document.querySelector( | ||||||
'iframe#od-prime-url-metrics-iframe' | ||||||
); | ||||||
if ( | ||||||
urlPrimeIframeElement && | ||||||
urlPrimeIframeElement.dataset.odPrimeUrlMetricsVerificationToken | ||||||
) { | ||||||
odPrimeUrlMetricsVerificationToken = | ||||||
urlPrimeIframeElement.dataset | ||||||
.odPrimeUrlMetricsVerificationToken; | ||||||
} | ||||||
} | ||||||
} catch ( e ) { | ||||||
// Ignoring error caused possibly due to cross-origin iframe access. | ||||||
} | ||||||
|
||||||
// Only available when page is loaded by Puppeteer script. | ||||||
if ( win.__odPrimeUrlMetricsVerificationToken ) { | ||||||
odPrimeUrlMetricsVerificationToken = | ||||||
win.__odPrimeUrlMetricsVerificationToken; | ||||||
} | ||||||
|
||||||
// Abort if the client already submitted a URL Metric for this URL and viewport group. | ||||||
const alreadySubmittedSessionStorageKey = | ||||||
await getAlreadySubmittedSessionStorageKey( | ||||||
|
@@ -604,6 +676,7 @@ export default async function detect( { | |||||
logger | ||||||
); | ||||||
if ( | ||||||
! odPrimeUrlMetricsVerificationToken && | ||||||
null !== alreadySubmittedSessionStorageKey && | ||||||
alreadySubmittedSessionStorageKey in sessionStorage | ||||||
) { | ||||||
|
@@ -922,6 +995,10 @@ export default async function detect( { | |||||
|
||||||
// Wait for the page to be hidden. | ||||||
await new Promise( ( resolve ) => { | ||||||
if ( odPrimeUrlMetricsVerificationToken ) { | ||||||
resolve(); | ||||||
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. Parent and If the promise isn't resolved immediately, navigating to a new URL causes the code following the promise to never execute. This is because changing the performance/plugins/optimization-detective/detect.js Lines 568 to 569 in 6ca5c4b
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 wonder. Do we even need to post a message here? As soon as the 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. The problem is we need to signal the parent that we can move to next URL or breakpoint using Will it makes sense to send the 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. Yes, I think it makes sense to send the message after the beacon is sent, definitely. |
||||||
} | ||||||
|
||||||
win.addEventListener( 'pagehide', resolve, { once: true } ); | ||||||
win.addEventListener( 'pageswap', resolve, { once: true } ); | ||||||
doc.addEventListener( | ||||||
|
@@ -1092,6 +1169,12 @@ export default async function detect( { | |||||
); | ||||||
} | ||||||
url.searchParams.set( 'hmac', urlMetricHMAC ); | ||||||
if ( odPrimeUrlMetricsVerificationToken ) { | ||||||
url.searchParams.set( | ||||||
'prime_url_metrics_verification_token', | ||||||
odPrimeUrlMetricsVerificationToken | ||||||
); | ||||||
} | ||||||
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. Authentication for REST API
In #1835 PR, WP nonces are introduced for REST API requests for logged-in users. This may allow us to eliminate the custom token authentication if URL metrics are collected exclusively from logged-in users. |
||||||
|
||||||
const headers = { | ||||||
'Content-Type': 'application/json', | ||||||
|
@@ -1104,7 +1187,22 @@ export default async function detect( { | |||||
method: 'POST', | ||||||
body: payloadBlob, | ||||||
headers, | ||||||
keepalive: true, // This makes fetch() behave the same as navigator.sendBeacon(). | ||||||
keepalive: odPrimeUrlMetricsVerificationToken ? false : true, // Setting keepalive to true makes fetch() behave the same as navigator.sendBeacon(). | ||||||
} ); | ||||||
await fetch( request ); | ||||||
|
||||||
if ( ! odPrimeUrlMetricsVerificationToken ) { | ||||||
await fetch( request ); | ||||||
} else { | ||||||
try { | ||||||
const response = await fetch( request ); | ||||||
if ( ! response.ok ) { | ||||||
throw new Error( | ||||||
`Failed to send URL Metric. Status: ${ response.status }` | ||||||
); | ||||||
} | ||||||
notifyStatus( { success: true } ); | ||||||
} catch ( err ) { | ||||||
notifyStatus( { success: false, error: err.message } ); | ||||||
} | ||||||
} | ||||||
} |
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 suggest the query parameter be supplied consistently by adding a URL query parameter instead. The cross-domain issue could be a common one, where wp-admin is on a different origin than the frontend. Then the token would be supplied in the same way for Puppeteer as for non-Puppeteer.
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.
Oh, and as opposed to passing the parameter as a query parameter it could also be supplied in the URL fragment instead. The benefit here would be it wouldn't get bust any page caches, although caching would be unlikely since the user is already logged-in. But another reason is that sometimes I've noticed that requests with unrecognized query parameters get redirected with those stripped out. For example:
https://joost.blog/wordpress-leadership/?odPrimeUrlMetricsVerificationToken=abc123
Notice how that redirects with the query parameter removed. Compare with:
https://joost.blog/wordpress-leadership/#odPrimeUrlMetricsVerificationToken=abc123
The URL target here now remains.
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.
The URL fragment approach seems very good. I had previously thought of using a query parameter, but I was also worried about the same problem you described. Additionally, we would have had to remove the query parameter from the
currentUrl
. I was also considering usingpostMessage
to send the verification token, as cross-origin access can't reach the dataset of the iframe. Using a URL fragment will eliminate the complex logic required forpostMessage
.