-
Notifications
You must be signed in to change notification settings - Fork 85
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
Introduce ILO_URL_Metric to encapsulate core data structure #988
Changes from 22 commits
0e652fe
3c6b153
0530a69
d8228c2
e87e3ac
3f91818
0963753
cc9e956
eab177c
87f518f
56346fd
6cafe3a
db11827
af95769
6073274
4f3119c
adf37d0
fcbfd42
c5765e2
c19c371
afc13ff
949642d
e148091
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 |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
/** | ||
* Image Loading Optimization: ILO_Data_Validation_Exception class | ||
* | ||
* @package performance-lab | ||
* @since n.e.x.t | ||
*/ | ||
|
||
/** | ||
* Exception thrown when failing to validate URL metrics data. | ||
* | ||
* @since n.e.x.t | ||
* @access private | ||
*/ | ||
final class ILO_Data_Validation_Exception extends Exception {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,181 @@ | ||
<?php | ||
/** | ||
* Image Loading Optimization: ILO_URL_Metric class | ||
* | ||
* @package performance-lab | ||
* @since n.e.x.t | ||
*/ | ||
|
||
/** | ||
* Representation of the measurements taken from a single client's visit to a specific URL. | ||
* | ||
* @since n.e.x.t | ||
* @access private | ||
*/ | ||
final class ILO_URL_Metric implements JsonSerializable { | ||
|
||
/** | ||
* Data. | ||
* | ||
* @var array{ | ||
* timestamp: int, | ||
* viewport: array{ width: int, height: int }, | ||
* elements: array<array{ | ||
* isLCP: bool, | ||
* isLCPCandidate: bool, | ||
* xpath: string, | ||
* intersectionRatio: float, | ||
* intersectionRect: array{ width: int, height: int }, | ||
* boundingClientRect: array{ width: int, height: int }, | ||
* }> | ||
* } | ||
*/ | ||
private $data; | ||
|
||
/** | ||
* Constructor. | ||
* | ||
* @param array $data URL metric data. | ||
westonruter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @throws ILO_Data_Validation_Exception When the input is invalid. | ||
*/ | ||
public function __construct( array $data ) { | ||
$valid = rest_validate_object_value_from_schema( $data, self::get_json_schema(), self::class ); | ||
if ( is_wp_error( $valid ) ) { | ||
throw new ILO_Data_Validation_Exception( esc_html( $valid->get_error_message() ) ); | ||
} | ||
$this->data = $data; | ||
} | ||
|
||
/** | ||
* Gets JSON schema for URL Metric. | ||
* | ||
* @return array Schema. | ||
*/ | ||
public static function get_json_schema(): array { | ||
$dom_rect_schema = array( | ||
'type' => 'object', | ||
'properties' => array( | ||
'width' => array( | ||
'type' => 'number', | ||
'minimum' => 0, | ||
), | ||
'height' => array( | ||
'type' => 'number', | ||
'minimum' => 0, | ||
), | ||
), | ||
// TODO: There are other properties to define if we need them: x, y, top, right, bottom, left. | ||
'additionalProperties' => true, | ||
); | ||
|
||
return array( | ||
'$schema' => 'http://json-schema.org/draft-04/schema#', | ||
'title' => 'ilo-url-metric', | ||
'type' => 'object', | ||
'properties' => array( | ||
'viewport' => array( | ||
'description' => __( 'Viewport dimensions', 'performance-lab' ), | ||
'type' => 'object', | ||
'required' => true, | ||
'properties' => array( | ||
'width' => array( | ||
'type' => 'integer', | ||
'required' => true, | ||
'minimum' => 0, | ||
), | ||
'height' => array( | ||
'type' => 'integer', | ||
'required' => true, | ||
'minimum' => 0, | ||
), | ||
), | ||
), | ||
'timestamp' => array( | ||
'description' => __( 'Timestamp at which the URL metric was captured.', 'performance-lab' ), | ||
'type' => 'number', | ||
'required' => true, | ||
'readonly' => true, // Omit from REST API. | ||
'default' => microtime( true ), // Value provided when instantiating ILO_URL_Metric in REST API. | ||
'minimum' => 0, | ||
), | ||
'elements' => array( | ||
'description' => __( 'Element metrics', 'performance-lab' ), | ||
'type' => 'array', | ||
'required' => true, | ||
'items' => array( | ||
// See the ElementMetrics in detect.js. | ||
'type' => 'object', | ||
'properties' => array( | ||
'isLCP' => array( | ||
'type' => 'boolean', | ||
'required' => true, | ||
), | ||
'isLCPCandidate' => array( | ||
'type' => 'boolean', | ||
), | ||
'xpath' => array( | ||
'type' => 'string', | ||
'required' => true, | ||
'pattern' => ILO_HTML_Tag_Processor::XPATH_PATTERN, | ||
), | ||
'intersectionRatio' => array( | ||
'type' => 'number', | ||
'required' => true, | ||
'minimum' => 0.0, | ||
'maximum' => 1.0, | ||
), | ||
'intersectionRect' => $dom_rect_schema, | ||
'boundingClientRect' => $dom_rect_schema, | ||
), | ||
'additionalProperties' => false, | ||
), | ||
), | ||
), | ||
'additionalProperties' => false, | ||
); | ||
} | ||
|
||
/** | ||
* Gets viewport width. | ||
* | ||
* @return array{ width: int, height: int } | ||
*/ | ||
public function get_viewport(): array { | ||
return $this->data['viewport']; | ||
} | ||
|
||
/** | ||
* Gets timestamp. | ||
* | ||
* @return float | ||
*/ | ||
public function get_timestamp(): float { | ||
return $this->data['timestamp']; | ||
} | ||
|
||
/** | ||
* Gets elements. | ||
* | ||
* @return array<array{ | ||
* isLCP: bool, | ||
* isLCPCandidate: bool, | ||
* xpath: string, | ||
* intersectionRatio: float, | ||
* intersectionRect: array{ width: int, height: int }, | ||
* boundingClientRect: array{ width: int, height: int }, | ||
* }> | ||
*/ | ||
public function get_elements(): array { | ||
return $this->data['elements']; | ||
} | ||
|
||
/** | ||
* Gets the JSON representation of the object. | ||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @return array | ||
*/ | ||
public function jsonSerialize(): array { | ||
return $this->data; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,15 +121,15 @@ function ilo_verify_url_metrics_storage_nonce( string $nonce, string $slug ): bo | |
* @since n.e.x.t | ||
* @access private | ||
* | ||
* @param array $url_metrics URL metrics. Each URL metric is expected to have a timestamp key. | ||
* @param array $validated_url_metric Validated URL metric. See JSON Schema defined in ilo_register_endpoint(). | ||
* @param int[] $breakpoints Breakpoint max widths. | ||
* @param int $sample_size Sample size for URL metrics at a given breakpoint. | ||
* @return array Updated URL metrics, with timestamp key added. | ||
* @param ILO_URL_Metric[] $url_metrics Existing URL metrics. Each URL metric is expected to have a timestamp key. | ||
* @param ILO_URL_Metric $new_url_metric New URL metric. | ||
* @param int[] $breakpoints Breakpoint max widths. | ||
* @param int $sample_size Sample size for URL metrics at a given breakpoint. | ||
* | ||
* @return ILO_URL_Metric[] Updated URL metrics. | ||
*/ | ||
function ilo_unshift_url_metrics( array $url_metrics, array $validated_url_metric, array $breakpoints, int $sample_size ): array { | ||
$validated_url_metric['timestamp'] = microtime( true ); | ||
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. This is now set more naturally in |
||
array_unshift( $url_metrics, $validated_url_metric ); | ||
function ilo_unshift_url_metrics( array $url_metrics, ILO_URL_Metric $new_url_metric, array $breakpoints, int $sample_size ): array { | ||
array_unshift( $url_metrics, $new_url_metric ); | ||
$grouped_url_metrics = ilo_group_url_metrics_by_breakpoint( $url_metrics, $breakpoints ); | ||
|
||
// Make sure there is at most $sample_size number of URL metrics for each breakpoint. | ||
|
@@ -140,11 +140,8 @@ static function ( $breakpoint_url_metrics ) use ( $sample_size ) { | |
// Sort URL metrics in descending order by timestamp. | ||
usort( | ||
$breakpoint_url_metrics, | ||
static function ( $a, $b ) { | ||
if ( ! isset( $a['timestamp'] ) || ! isset( $b['timestamp'] ) ) { | ||
return 0; | ||
} | ||
Comment on lines
-144
to
-146
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. No need for this now that the timestamp is guaranteed to be set. |
||
return $b['timestamp'] <=> $a['timestamp']; | ||
static function ( ILO_URL_Metric $a, ILO_URL_Metric $b ): int { | ||
return $b->get_timestamp() <=> $a->get_timestamp(); | ||
} | ||
); | ||
|
||
|
@@ -234,12 +231,12 @@ function ilo_get_url_metrics_breakpoint_sample_size(): int { | |
* @since n.e.x.t | ||
* @access private | ||
* | ||
* @param array $url_metrics URL metrics. | ||
* @param int[] $breakpoints Viewport breakpoint max widths, sorted in ascending order. | ||
* @return array URL metrics grouped by breakpoint. The array keys are the minimum widths for a viewport to lie within | ||
* the breakpoint. The returned array is always one larger than the provided array of breakpoints, since | ||
* the breakpoints reflect the max inclusive boundaries whereas the return value is the groups of page | ||
* metrics with viewports on either side of the breakpoint boundaries. | ||
* @param ILO_URL_Metric[] $url_metrics URL metrics. | ||
* @param int[] $breakpoints Viewport breakpoint max widths, sorted in ascending order. | ||
* @return array<int, ILO_URL_Metric[]> URL metrics grouped by breakpoint. The array keys are the minimum widths for a viewport to lie within | ||
* the breakpoint. The returned array is always one larger than the provided array of breakpoints, since | ||
* the breakpoints reflect the max inclusive boundaries whereas the return value is the groups of page | ||
* metrics with viewports on either side of the breakpoint boundaries. | ||
*/ | ||
function ilo_group_url_metrics_by_breakpoint( array $url_metrics, array $breakpoints ): array { | ||
|
||
|
@@ -254,13 +251,9 @@ static function ( $breakpoint ) { | |
$grouped = array_fill_keys( array_merge( array( 0 ), $minimum_viewport_widths ), array() ); | ||
|
||
foreach ( $url_metrics as $url_metric ) { | ||
if ( ! isset( $url_metric['viewport']['width'] ) ) { | ||
continue; | ||
} | ||
|
||
Comment on lines
-257
to
-260
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. No need for this anymore since the property is guaranteed to exist. |
||
$current_minimum_viewport = 0; | ||
foreach ( $minimum_viewport_widths as $viewport_minimum_width ) { | ||
if ( $url_metric['viewport']['width'] > $viewport_minimum_width ) { | ||
if ( $url_metric->get_viewport()['width'] > $viewport_minimum_width ) { | ||
$current_minimum_viewport = $viewport_minimum_width; | ||
} else { | ||
break; | ||
|
@@ -284,7 +277,7 @@ static function ( $breakpoint ) { | |
* @since n.e.x.t | ||
* @access private | ||
* | ||
* @param array $grouped_url_metrics URL metrics grouped by breakpoint. See `ilo_group_url_metrics_by_breakpoint()`. | ||
* @param array<int, ILO_URL_Metric[]> $grouped_url_metrics URL metrics grouped by breakpoint. See `ilo_group_url_metrics_by_breakpoint()`. | ||
* @return array LCP elements keyed by its minimum viewport width. If there is no supported LCP element at a breakpoint, then `false` is used. | ||
*/ | ||
function ilo_get_lcp_elements_by_minimum_viewport_widths( array $grouped_url_metrics ): array { | ||
|
@@ -298,7 +291,7 @@ function ilo_get_lcp_elements_by_minimum_viewport_widths( array $grouped_url_met | |
$breadcrumb_element = array(); | ||
|
||
foreach ( $breakpoint_url_metrics as $breakpoint_url_metric ) { | ||
foreach ( $breakpoint_url_metric['elements'] as $element ) { | ||
foreach ( $breakpoint_url_metric->get_elements() as $element ) { | ||
if ( ! $element['isLCP'] ) { | ||
continue; | ||
} | ||
|
@@ -360,11 +353,11 @@ static function ( $lcp_element ) use ( &$prev_lcp_element ) { | |
* @since n.e.x.t | ||
* @access private | ||
* | ||
* @param array $url_metrics URL metrics. | ||
* @param float $current_time Current time as returned by `microtime(true)`. | ||
* @param int[] $breakpoint_max_widths Breakpoint max widths. | ||
* @param int $sample_size Sample size for viewports in a breakpoint. | ||
* @param int $freshness_ttl Freshness TTL for a URL metric. | ||
* @param ILO_URL_Metric[] $url_metrics URL metrics. | ||
* @param float $current_time Current time as returned by `microtime(true)`. | ||
* @param int[] $breakpoint_max_widths Breakpoint max widths. | ||
* @param int $sample_size Sample size for viewports in a breakpoint. | ||
* @param int $freshness_ttl Freshness TTL for a URL metric. | ||
* @return array<int, array{int, bool}> Array of tuples mapping minimum viewport width to whether URL metric(s) are needed. | ||
*/ | ||
function ilo_get_needed_minimum_viewport_widths( array $url_metrics, float $current_time, array $breakpoint_max_widths, int $sample_size, int $freshness_ttl ): array { | ||
|
@@ -376,7 +369,7 @@ function ilo_get_needed_minimum_viewport_widths( array $url_metrics, float $curr | |
$needs_url_metrics = true; | ||
} else { | ||
foreach ( $viewport_url_metrics as $url_metric ) { | ||
if ( isset( $url_metric['timestamp'] ) && $url_metric['timestamp'] + $freshness_ttl < $current_time ) { | ||
if ( $url_metric->get_timestamp() + $freshness_ttl < $current_time ) { | ||
$needs_url_metrics = true; | ||
break; | ||
} | ||
|
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.
Wouldn't it be valuable to store the
$url
and$slug
in this class as well? Maybe they could be separate properties from$data
, but I find it a bit odd to have a class for a URL metric exclude those identifying variables.I think it could simplify some things by putting the responsibility for this information purely into the class representing the entity. We could still use associative arrays keyed by URL or slug if we need it, but I think it would be cleaner to have it covered by this class in its entirety - unless I'm missing something.
Alternatively, if there are scenarios where URL and slug should be decoupled from the actual metric data, maybe we could separately introduce a class that is a value object representing URL and slug, and it could also provide the methods to create and validate the nonces etc. Just an idea.
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.
Well, the thing is that there are multiple URL metrics stored for a given response. The post type stores this list of URL metrics, based on the number of breakpoints and the sample size. So if we store the URL and slug with each
ILO_URL_Metric
we're storing multiple copies of the same data. The URL/slug is more higher level metadata which is only needed for the post type. We don't need the URL or slug except to get/set the post. I'd prefer to not include the URL or slug in this class.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.
Well, let me think about this some more.
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.
Maybe we merge this as-is and I propose the change in another PR? I want to revisit how the URL is handled because in reality it is the request args really that matter, not the end URL. The slug is computed from the request args, not the URL.
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've added this as a todo item on #869
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.
Ultimately, I think it would actually make sense to store a
url
with each URL Metric because in actuality the URL can vary while the request args change. This could be important for debugging, for example if it turns out that a template is modified by direct access to$_GET
and not for a registered query var.But I want to address this in a subsequent PR.
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 I see you added more comments in the meantime. Let me know what you think about the above proposal.
But in any case, this would be okay to do in a follow up PR.
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.
Yeah, let's do in a separate PR. It would actually be useful to duplicate the URL and even the request args for each URL metric since it reveals the underlying data for the response that created the URL metric. The
slug
used in the post type is computed from a normalized set of request args, so having the original request args would still be useful to have on hand.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.
Sounds great!
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.
See #1043