Skip to content
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

Mark existing URL Metrics as stale when a new tag visitor is registered #1705

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
f7295d8
Store the ETag along with the URL metric
ShyamGadde Nov 26, 2024
57b79e1
Factor in ETag of the URL metric for determining if it is stale
ShyamGadde Nov 26, 2024
7b48f26
Update test case to allow ETag to be optional in JSON Schema
ShyamGadde Nov 26, 2024
9999eea
Fix filter name typo in URL Metric schema extension
ShyamGadde Nov 26, 2024
3b01ea2
Factor in ETag when computing HMAC
ShyamGadde Nov 26, 2024
c801a65
Merge branch 'trunk' into add/mark-url-metrics-stale-on-tag-visitor-c…
ShyamGadde Nov 26, 2024
b84f62a
Make ETag a core property instead of adding it using a filter
ShyamGadde Nov 26, 2024
ae708cf
Return empty string instead of null when an existing URL metric doesn…
ShyamGadde Nov 27, 2024
e115134
Send ETag as REST API arg instead of URL metric property
ShyamGadde Nov 27, 2024
1cd62c0
Fix Test_OD_Storage_REST_API
ShyamGadde Nov 27, 2024
cb64e1d
Store the current ETag as a property of `OD_URL_Metric_Group_Collecti…
ShyamGadde Nov 27, 2024
a65fbad
Fix test_od_optimize_template_output_buffer
ShyamGadde Nov 27, 2024
5287f79
Fix test cases for embed-optimizer
ShyamGadde Nov 27, 2024
86023eb
Fix test-cases for image-prioritizer
ShyamGadde Nov 27, 2024
2eb6d98
Add comment to clarify future intent for ETag property
ShyamGadde Nov 28, 2024
2c7aee5
Update `get_etag` method to return `null` when ETag is absent
ShyamGadde Nov 28, 2024
821b48d
Handle PHPStan error for possible null ETag in store_url_metric() method
ShyamGadde Nov 28, 2024
52c486f
Improve variable naming
ShyamGadde Nov 28, 2024
63758ef
Add missing since tags
ShyamGadde Nov 28, 2024
08f8f8a
Improve naming for URL Metric schema properties
ShyamGadde Nov 28, 2024
371e998
Improve naming for REST API endpoint args
ShyamGadde Nov 28, 2024
ba2ea93
Update test cases to use the new property and arg names
ShyamGadde Nov 28, 2024
8fe38f7
Introduce new function to compute the current ETag
ShyamGadde Nov 28, 2024
e6078a9
Add pattern validation for ETag in schema
ShyamGadde Nov 28, 2024
309f9bd
Update test cases to use the new ETag format
ShyamGadde Nov 28, 2024
36cdeca
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Nov 30, 2024
d1458d8
Refactor staleness check logic for URL Metrics
ShyamGadde Dec 1, 2024
c4eb613
Use non-empty-string as more precise type for ETag
westonruter Dec 1, 2024
695eecb
Refine ETag parameter in more places
westonruter Dec 1, 2024
5a36beb
Refactor static analysis accommodation code for when ETag is null
westonruter Dec 1, 2024
d855ae8
Fix test_add_url_metric
westonruter Dec 1, 2024
5e39581
Eliminate collection being an optional arg when constructing an OD_UR…
westonruter Dec 1, 2024
a430c56
Supply null as etag param to address static analysis complaint
westonruter Dec 1, 2024
bcc34c9
Temporarily work around errors related to non-hashes being supplied f…
westonruter Dec 1, 2024
4f0ac4c
Rename od_compute_current_etag() to od_get_current_etag() for parity …
westonruter Dec 1, 2024
ae3eab0
Use JSON-encoding instead of PHP serialization
westonruter Dec 1, 2024
a33eb28
Introduce od_current_url_metrics_etag_data filter for extensibility a…
westonruter Dec 1, 2024
d37d671
Use md5() directly when current ETag is not needed
westonruter Dec 1, 2024
92dc354
Remove extra empty line
ShyamGadde Dec 1, 2024
85540e6
Add validation for current_etag paramter in OD_URL_Metric_Group_Colle…
ShyamGadde Dec 1, 2024
731fe91
Use passed URL in test helper instead of default home_url()
ShyamGadde Dec 1, 2024
c3b7344
Update test case for group collection constructor
ShyamGadde Dec 1, 2024
9fcfb7f
Add tests for OD_URL_Metric_Group::is_complete
ShyamGadde Dec 1, 2024
b225fba
Add tests for OD_URL_Metric::get_etag
ShyamGadde Dec 1, 2024
7b01e50
Add missing `@covers ::get_url` annotation in test case
ShyamGadde Dec 1, 2024
4456563
Harden ETag regex to disregard newlines
westonruter Dec 1, 2024
034fe46
Add test for od_get_current_url_metrics_etag()
westonruter Dec 2, 2024
02e9759
Add docs for od_current_url_metrics_etag_data filter
westonruter Dec 2, 2024
54183af
Eliminate constructing etag via OD_Tag_Visitor_Registry() unless side…
westonruter Dec 2, 2024
6960a28
Fix checked logic in old_url_metric test case
westonruter Dec 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions plugins/optimization-detective/class-od-url-metric-group.php
Original file line number Diff line number Diff line change
Expand Up @@ -220,20 +220,32 @@ static function ( OD_URL_Metric $a, OD_URL_Metric $b ): int {
* A group is complete if it has the full sample size of URL Metrics
* and all of these URL Metrics are fresh.
*
* @since n.e.x.t If the current environment's generated ETag does not match the URL Metric's ETag, the URL Metric is considered stale.
*
* @global string $od_etag ETag for the current environment.
*
* @return bool Whether complete.
*/
public function is_complete(): bool {
global $od_etag;

if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
return $this->result_cache[ __FUNCTION__ ];
}

$result = ( function () {
$result = ( function () use ( $od_etag ) {
if ( count( $this->url_metrics ) < $this->sample_size ) {
return false;
}
$current_time = microtime( true );
foreach ( $this->url_metrics as $url_metric ) {
if ( $current_time > $url_metric->get_timestamp() + $this->freshness_ttl ) {
if (
$current_time > $url_metric->get_timestamp() + $this->freshness_ttl
||
// If the generated ETag does not match the URL metric's ETag, consider the URL metric as stale.
// NOTE: Since the ETag is optional for now, existing ones without it are not considered stale.
( $url_metric->get( 'eTag' ) !== null && $url_metric->get( 'eTag' ) !== $od_etag )
westonruter marked this conversation as resolved.
Show resolved Hide resolved
) {
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/optimization-detective/class-od-url-metric.php
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ public static function get_json_schema(): array {
$schema['properties']['elements']['items']['properties'] = self::extend_schema_with_optional_properties(
$schema['properties']['elements']['items']['properties'],
$additional_properties,
'od_url_metric_schema_root_additional_properties'
'od_url_metric_schema_element_item_additional_properties'
westonruter marked this conversation as resolved.
Show resolved Hide resolved
);
}

Expand Down
3 changes: 3 additions & 0 deletions plugins/optimization-detective/detect.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ function extendElementData( xpath, properties ) {
* @param {number} args.maxViewportAspectRatio Maximum aspect ratio allowed for the viewport.
* @param {boolean} args.isDebug Whether to show debug messages.
* @param {string} args.restApiEndpoint URL for where to send the detection data.
* @param {string} args.currentETag ETag for URL Metric.
* @param {string} args.currentUrl Current URL.
* @param {string} args.urlMetricSlug Slug for URL Metric.
* @param {number|null} args.cachePurgePostId Cache purge post ID.
Expand All @@ -252,6 +253,7 @@ export default async function detect( {
isDebug,
extensionModuleUrls,
restApiEndpoint,
currentETag,
currentUrl,
urlMetricSlug,
cachePurgePostId,
Expand Down Expand Up @@ -440,6 +442,7 @@ export default async function detect( {
}

urlMetric = {
eTag: currentETag,
url: currentUrl,
viewport: {
width: win.innerWidth,
Expand Down
8 changes: 7 additions & 1 deletion plugins/optimization-detective/detection.php
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,14 @@ function od_get_cache_purge_post_id(): ?int {
* @since 0.1.0
* @access private
*
* @global string $od_etag ETag for the current environment.
*
* @param string $slug URL Metrics slug.
* @param OD_URL_Metric_Group_Collection $group_collection URL Metric group collection.
*/
function od_get_detection_script( string $slug, OD_URL_Metric_Group_Collection $group_collection ): string {
global $od_etag;

$web_vitals_lib_data = require __DIR__ . '/build/web-vitals.asset.php';
$web_vitals_lib_src = add_query_arg( 'ver', $web_vitals_lib_data['version'], plugin_dir_url( __FILE__ ) . 'build/web-vitals.js' );

Expand All @@ -85,16 +89,18 @@ function od_get_detection_script( string $slug, OD_URL_Metric_Group_Collection $
$cache_purge_post_id = od_get_cache_purge_post_id();

$current_url = od_get_current_url();

$detect_args = array(
'minViewportAspectRatio' => od_get_minimum_viewport_aspect_ratio(),
'maxViewportAspectRatio' => od_get_maximum_viewport_aspect_ratio(),
'isDebug' => WP_DEBUG,
'extensionModuleUrls' => $extension_module_urls,
'restApiEndpoint' => rest_url( OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ),
'currentETag' => $od_etag,
westonruter marked this conversation as resolved.
Show resolved Hide resolved
'currentUrl' => $current_url,
'urlMetricSlug' => $slug,
'cachePurgePostId' => od_get_cache_purge_post_id(),
'urlMetricHMAC' => od_get_url_metrics_storage_hmac( $slug, $current_url, $cache_purge_post_id ),
'urlMetricHMAC' => od_get_url_metrics_storage_hmac( $slug, $od_etag, $current_url, $cache_purge_post_id ),
westonruter marked this conversation as resolved.
Show resolved Hide resolved
'urlMetricGroupStatuses' => array_map(
static function ( OD_URL_Metric_Group $group ): array {
return array(
Expand Down
18 changes: 18 additions & 0 deletions plugins/optimization-detective/helper.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,21 @@ function od_get_asset_path( string $src_path, ?string $min_path = null ): string

return $min_path;
}

/**
* Adds optional URL Metric schema root properties.
*
* @since n.e.x.t
*
* @param array<string, array{type: string}> $additional_properties Additional properties.
* @return array<string, array{type: string}> Additional properties.
*/
function od_add_optional_url_metric_schema_root_properties( array $additional_properties ): array {
// The ETag is being kept optional for now so as to avoid invalidating all current URL Metrics.
$additional_properties['eTag'] = array(
'description' => __( 'The ETag for the URL Metric.', 'optimization-detective' ),
'type' => 'string',
);

return $additional_properties;
}
westonruter marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions plugins/optimization-detective/hooks.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@
OD_URL_Metrics_Post_Type::add_hooks();
add_action( 'wp', 'od_maybe_add_template_output_buffer_filter' );
add_action( 'wp_head', 'od_render_generator_meta_tag' );
add_filter( 'od_url_metric_schema_root_additional_properties', 'od_add_optional_url_metric_schema_root_properties' );
westonruter marked this conversation as resolved.
Show resolved Hide resolved
19 changes: 16 additions & 3 deletions plugins/optimization-detective/optimization.php
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,6 @@ function od_optimize_template_output_buffer( string $buffer ): string {
od_get_url_metric_freshness_ttl()
);

// Whether we need to add the data-od-xpath attribute to elements and whether the detection script should be injected.
$needs_detection = ! $group_collection->is_every_group_complete();

$tag_visitor_registry = new OD_Tag_Visitor_Registry();

/**
Expand All @@ -217,6 +214,22 @@ function od_optimize_template_output_buffer( string $buffer ): string {
$tag_visitor_context = new OD_Tag_Visitor_Context( $processor, $group_collection, $link_collection );
$current_tag_bookmark = 'optimization_detective_current_tag';
$visitors = iterator_to_array( $tag_visitor_registry );

/**
* ETag generated for the current environment.
*
* @since n.e.x.t
*
* @global string $od_etag
*/
global $od_etag;
westonruter marked this conversation as resolved.
Show resolved Hide resolved

// Generate and store an ETag consisting of all the tag visitors' IDs in the current environment.
$od_etag = implode( ',', array_keys( $visitors ) );

// Whether we need to add the data-od-xpath attribute to elements and whether the detection script should be injected.
$needs_detection = ! $group_collection->is_every_group_complete();

do {
$tracked_in_url_metrics = false;
$processor->set_bookmark( $current_tag_bookmark ); // TODO: Should we break if this returns false?
Expand Down
11 changes: 6 additions & 5 deletions plugins/optimization-detective/storage/data.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,15 @@ function od_get_url_metrics_slug( array $query_vars ): string {
*
* @see od_verify_url_metrics_storage_hmac()
* @see od_get_url_metrics_slug()
* @todo This should also include an ETag as a parameter. See <https://github.com/WordPress/performance/issues/1466>.
*
* @param string $slug Slug (hash of normalized query vars).
* @param string $etag ETag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider $current_etag instead.

* @param string $url URL.
* @param int|null $cache_purge_post_id Cache purge post ID.
* @return string HMAC.
*/
function od_get_url_metrics_storage_hmac( string $slug, string $url, ?int $cache_purge_post_id = null ): string {
$action = "store_url_metric:$slug:$url:$cache_purge_post_id";
function od_get_url_metrics_storage_hmac( string $slug, string $etag, string $url, ?int $cache_purge_post_id = null ): string {
$action = "store_url_metric:$slug:$etag:$url:$cache_purge_post_id";
return wp_hash( $action, 'nonce' );
}

Expand All @@ -173,12 +173,13 @@ function od_get_url_metrics_storage_hmac( string $slug, string $url, ?int $cache
*
* @param string $hmac HMAC.
* @param string $slug Slug (hash of normalized query vars).
* @param string $etag ETag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto on $current_etag.

* @param String $url URL.
* @param int|null $cache_purge_post_id Cache purge post ID.
* @return bool Whether the HMAC is valid.
*/
function od_verify_url_metrics_storage_hmac( string $hmac, string $slug, string $url, ?int $cache_purge_post_id = null ): bool {
return hash_equals( od_get_url_metrics_storage_hmac( $slug, $url, $cache_purge_post_id ), $hmac );
function od_verify_url_metrics_storage_hmac( string $hmac, string $slug, string $etag, string $url, ?int $cache_purge_post_id = null ): bool {
return hash_equals( od_get_url_metrics_storage_hmac( $slug, $etag, $url, $cache_purge_post_id ), $hmac );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion plugins/optimization-detective/storage/rest-api.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function od_register_endpoint(): void {
'required' => true,
'pattern' => '^[0-9a-f]+$',
'validate_callback' => static function ( string $hmac, WP_REST_Request $request ) {
if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request['slug'], $request['url'], $request['cache_purge_post_id'] ?? null ) ) {
if ( ! od_verify_url_metrics_storage_hmac( $hmac, $request['slug'], $request['eTag'], $request['url'], $request['cache_purge_post_id'] ?? null ) ) {
return new WP_Error( 'invalid_hmac', __( 'URL Metrics HMAC verification failure.', 'optimization-detective' ) );
}
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ function ( $params ) {
'hmac' => 'not even a hash',
),
'invalid_hmac' => array(
'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), home_url( '/' ) ),
'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array( 'different' => 'query vars' ) ), '', home_url( '/' ) ),
),
'invalid_hmac_with_queried_object' => array(
'hmac' => od_get_url_metrics_storage_hmac( od_get_url_metrics_slug( array() ), home_url( '/' ), 1 ),
Expand Down Expand Up @@ -672,7 +672,7 @@ private function get_valid_params( array $extras = array() ): array {
$data = array_merge(
array(
'slug' => $slug,
'hmac' => od_get_url_metrics_storage_hmac( $slug, $data['url'] ),
'hmac' => od_get_url_metrics_storage_hmac( $slug, '', $data['url'] ),
),
$data
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ static function ( $additional_properties ) {
'assert' => function ( array $original_schema, $extended_schema ): void {
$this->assertSame( $original_schema, $extended_schema );
},
'expected_incorrect_usage' => 'Filter: &#039;od_url_metric_schema_root_additional_properties&#039;',
'expected_incorrect_usage' => 'Filter: &#039;od_url_metric_schema_element_item_additional_properties&#039;',
),

'adding_root_string' => array(
Expand Down Expand Up @@ -728,7 +728,11 @@ protected function check_schema_subset( array $schema, string $path, bool $exten
$this->assertTrue( $schema['additionalProperties'], "Path: $path" );
}
foreach ( $schema['properties'] as $key => $property_schema ) {
$this->check_schema_subset( $property_schema, "$path/$key", $extended );
if ( 'eTag' === $key ) {
$this->check_schema_subset( $property_schema, "$path/$key", true );
} else {
$this->check_schema_subset( $property_schema, "$path/$key", $extended );
}
}
} elseif ( 'array' === $schema['type'] ) {
$this->assertArrayHasKey( 'items', $schema, $path );
Expand Down
1 change: 1 addition & 0 deletions plugins/optimization-detective/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface ElementData {
export type ExtendedElementData = ExcludeProps< ElementData >;

export interface URLMetric {
eTag: string;
url: string;
viewport: {
width: number;
Expand Down
Loading