Introduce viewport aspect ratio validation for URL Metrics #1494

westonruter · 2024-08-22T01:00:37Z

As mentioned in #1490, I was debugging URL Metrics on my own blog because I found that commenter avatars weren't getting loading=lazy even though they are far outside the initial viewport. When looking the URL Metrics for the URL, I was surprised to find a URL Metric with this viewport:

"viewport": {
    "width": 1024,
    "height": 12140
}

This is an aspect ratio of 0.084 which seems incredibly unusual. It turns out that Googlebot actually crawls pages with a viewport set so extremely high like this (cf. GoogleBot Crawls & Renders Tall, With 9000px High Viewport?). The result is that when computing the all_element_max_intersection_ratios, since this URL Metric from Googlebot always has the entire page in the viewport, they will always be 1. This means that loading=lazy would get removed from all images whenever there is a URL Metric stored from Googlebot for a page. Additionally, it could be that a large image outside of the viewport for normal visitors could end up getting identified as the LCP image due to it being in the viewport for Googlebot.

This is not just an issue related to Googlebot. It's also an issue with users visiting a site with unusual window sizes (e.g. split to the top half the monitor) which can skew the URL Metrics.

This PR introduces viewport aspect ratio validation when storing a URL Metric. The validation is also done at read time, so any existing URL Metrics with bad viewport aspect ratios will be immediately discarded.

The min/max aspect ratios I picked are to accommodate the device (Sony Xperia 1 ) with the most extreme aspect ratio I'm familiar with: 21:9. When in horizontal orientation, the aspect ratio is 2.33 and when in vertical orientation it is 0.43. So I picked 0.4 as the minimum and 2.5 as the maximum.

There are two new filters introduced to customize the aspect ratio range:

od_minimum_viewport_aspect_ratio
od_maximum_viewport_aspect_ratio

The od_maximum_viewport_aspect_ratio filter is particularly useful during development for example when you have DevTools open on the bottom of the screen.

When an error occurs when parsing URL Metrics out of the od_url_metrics post type, it now includes the UUID in the error message. Additionally, if the error is due to a JSON validation issue, the severity is now reduced from warning to notice, since JSON Schema changes are to be expected.

github-actions · 2024-08-22T01:00:56Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

westonruter · 2024-08-22T01:01:24Z

plugins/optimization-detective/storage/class-od-url-metrics-post-type.php

-		$this_function   = __FUNCTION__;
-		$trigger_warning = static function ( string $message ) use ( $this_function ): void {
-			wp_trigger_error( $this_function, esc_html( $message ), E_USER_WARNING );
+		$this_function = __METHOD__;


The use of __FUNCTION__ was wrong as it didn't include the class name.

plugins/optimization-detective/detect.js

plugins/optimization-detective/class-od-url-metric.php

plugins/optimization-detective/tests/test-class-od-url-metric.php

plugins/optimization-detective/readme.txt

plugins/optimization-detective/storage/data.php

adamsilverstein

Fantastic. Added some doc block suggestions

…ude UUID when available

Co-authored-by: Pascal Birchler <pascalb@google.com>

Co-authored-by: Adam Silverstein <adamjs@google.com>

westonruter · 2024-08-22T19:49:26Z

I deployed this to my site and immediately after deploying I saw the URL Metrics with the extreme aspect ratio removed.

This resulted in all_element_max_intersection_ratios being corrected from being all 1 to being the expected values, especially the commenter avatar images which now also are getting loading=lazy as expected.

Before:

"all_element_max_intersection_ratios": {
    "/*[0][self::HTML]/*[1][self::BODY]/*[3][self::MAIN]/*[0][self::ARTICLE]/*[1][self::FIGURE]/*[0][self::DIV]/*[0][self::IMG]": 1,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[2][self::FIGURE]/*[1][self::DIV]/*[1][self::IMG]": 1,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 1,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[2][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 1,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[3][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 1
}

After:

"all_element_max_intersection_ratios": {
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[2][self::FIGURE]/*[1][self::DIV]/*[1][self::IMG]": 0.9963193535804749,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 0,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[2][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 0,
    "/*[1][self::HTML]/*[2][self::BODY]/*[4][self::MAIN]/*[1][self::ARTICLE]/*[6][self::DIV]/*[1][self::DIV]/*[2][self::DIV]/*[3][self::DIV]/*[1][self::ARTICLE]/*[1][self::FOOTER]/*[1][self::DIV]/*[1][self::A]/*[1][self::IMG]": 0
}

westonruter · 2024-08-22T21:47:25Z

Aside: I'm not sure what this element is all about:

/*[0][self::HTML]/*[1][self::BODY]/*[3][self::MAIN]/*[0][self::ARTICLE]/*[1][self::FIGURE]/*[0][self::DIV]/*[0][self::IMG]

I'm currently fetching the page on my site every 5-10 minutes with WP_DEBU on and storing the results in a directory in a timestamped HTML file. I'll look at the HTML next week to see if I'm seeing any instability in the HTML structure. This is with the Twenty Twenty theme.

westonruter · 2024-08-22T22:02:26Z

Oh, I just realized what it is. It's a very old URL Metric from before the XPath indices were fixed to be 1-based instead of 0-based! See #1191. Looking at the URL Metric data, I see the timestamp is 1712354502.752218 which is for a day back in April! This URL Metric wasn't for Googlebot as it had a viewport of 412x823. It appears that URL Metrics are not being collected frequently enough, or my site is truly devoid of traffic. I think for my site it may be in part due to page caching, where the detection time window (od_detection_time_window) is too short, so it is only collecting metrics for visitors who happen to visit when there is a MISS to the page cache, and if there is a stale-while-revalidate caching strategy going on with Varnish then it is even more likely for the visitor to never be within the detection time window (currentTime - serveTime > detectionTimeWindow):

performance/plugins/optimization-detective/detect.js

Lines 179 to 187 in f8dad34

    
           // Abort running detection logic if it was served in a cached page. 
        
           if ( currentTime - serveTime > detectionTimeWindow ) { 
        
           	if ( isDebug ) { 
        
           		warn( 
        
           			'Aborted detection due to being outside detection time window.' 
        
           		); 
        
           	} 
        
           	return; 
        
           }

I think this logic needs to be revisited.

Update: Filed as #1496

westonruter added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Aug 22, 2024

westonruter added this to the optimization-detective n.e.x.t milestone Aug 22, 2024

westonruter requested a review from felixarntz as a code owner August 22, 2024 01:00

westonruter commented Aug 22, 2024

View reviewed changes

westonruter requested review from swissspidy and adamsilverstein August 22, 2024 01:02

swissspidy reviewed Aug 22, 2024

View reviewed changes

plugins/optimization-detective/detect.js Outdated Show resolved Hide resolved

swissspidy reviewed Aug 22, 2024

View reviewed changes

plugins/optimization-detective/class-od-url-metric.php Outdated Show resolved Hide resolved

swissspidy approved these changes Aug 22, 2024

View reviewed changes

westonruter commented Aug 22, 2024

View reviewed changes

plugins/optimization-detective/tests/test-class-od-url-metric.php Outdated Show resolved Hide resolved

westonruter force-pushed the update/od-allowed-viewport-aspect-ratios branch from 561c018 to aa98bcf Compare August 22, 2024 16:06

adamsilverstein reviewed Aug 22, 2024

View reviewed changes

plugins/optimization-detective/readme.txt Outdated Show resolved Hide resolved

adamsilverstein reviewed Aug 22, 2024

View reviewed changes

plugins/optimization-detective/storage/data.php Outdated Show resolved Hide resolved

adamsilverstein reviewed Aug 22, 2024

View reviewed changes

plugins/optimization-detective/storage/data.php Outdated Show resolved Hide resolved

adamsilverstein approved these changes Aug 22, 2024

View reviewed changes

westonruter and others added 7 commits August 22, 2024 12:29

Fix passing method name when failing to parse stored URL metric; incl…

285813d

…ude UUID when available

Introduce a required min/max viewport aspect ratio range

4cc2fb0

Handle OD_Data_Validation_Exception as rest_invalid_param error

f02365d

Change a URL Metric validation error from a warning to a notice

8299b06

Fix logic inversion in error message

672f2ab

Co-authored-by: Pascal Birchler <pascalb@google.com>

Improve phpdoc

df110d4

Co-authored-by: Adam Silverstein <adamjs@google.com>

Consistently use default of 2:1 height and ensure integer

161b201

westonruter force-pushed the update/od-allowed-viewport-aspect-ratios branch from 99a3b27 to 161b201 Compare August 22, 2024 19:29

westonruter merged commit f8dad34 into trunk Aug 22, 2024
16 checks passed

westonruter deleted the update/od-allowed-viewport-aspect-ratios branch August 22, 2024 19:43

westonruter mentioned this pull request Aug 22, 2024

Detection time window may not allow enough URL Metrics to be collected #1496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce viewport aspect ratio validation for URL Metrics #1494

Introduce viewport aspect ratio validation for URL Metrics #1494

westonruter commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 •

edited

Loading

westonruter Aug 22, 2024

adamsilverstein left a comment

westonruter commented Aug 22, 2024

westonruter commented Aug 22, 2024

westonruter commented Aug 22, 2024 •

edited

Loading

Introduce viewport aspect ratio validation for URL Metrics #1494

Introduce viewport aspect ratio validation for URL Metrics #1494

Conversation

westonruter commented Aug 22, 2024

github-actions bot commented Aug 22, 2024 • edited Loading

westonruter Aug 22, 2024

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

westonruter commented Aug 22, 2024

westonruter commented Aug 22, 2024

westonruter commented Aug 22, 2024 • edited Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

westonruter commented Aug 22, 2024 •

edited

Loading