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

[ShyamGadde]

Summary

Fixes #1424

Relevant technical choices

• The ETag remains optional for now, as discussed in Internal state of the page (e.g. queried posts or queried object) should factor into whether a stored URL metric is fresh #1466 (comment).
• The current ETag is also added to the list of arguments used to compute the HMAC.
• The ETag generated for the current environment is stored as a new property ($current_etag) on OD_URL_Metric_Group_Collection.
• Introduced a function od_get_current_url_metrics_etag() that generates the ETag for the current environment.
• Introduced an od_current_url_metrics_etag_data filter to allow sites to customize the internal page state used for computing the ETag. This will feed into Internal state of the page (e.g. queried posts or queried object) should factor into whether a stored URL metric is fresh #1466.

[github-actions]

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: ShyamGadde <shyamgadde@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

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

[ShyamGadde]

Code in question:

performance/tests/class-optimization-detective-test-helpers.php

Lines 64 to 116 in 69eca9a

	/**
	* Gets a sample URL metric.
	*
	* @phpstan-param array{
	* url?: string,
	* viewport_width?: int,
	* viewport_height?: int,
	* element?: ElementDataSubset,
	* elements?: array<ElementDataSubset>
	* } $params Params.
	*
	* @return OD_URL_Metric URL metric.
	*/
	public function get_sample_url_metric( array $params ): OD_URL_Metric {
	$params = array_merge(
	array(
	'url' => home_url( '/' ),
	'viewport_width' => 480,
	'elements' => array(),
	),
	$params
	);
	if ( array_key_exists( 'element', $params ) ) {
	$params['elements'][] = $params['element'];
	}
	return new OD_URL_Metric(
	array(
	'url' => home_url( '/' ),
	'viewport' => array(
	'width' => $params['viewport_width'],
	'height' => $params['viewport_height'] ?? ceil( $params['viewport_width'] / 2 ),
	),
	'timestamp' => microtime( true ),
	'elements' => array_map(
	function ( array $element ): array {
	return array_merge(
	array(
	'isLCP' => false,
	'isLCPCandidate' => $element['isLCP'] ?? false,
	'intersectionRatio' => 1,
	'intersectionRect' => $this->get_sample_dom_rect(),
	'boundingClientRect' => $this->get_sample_dom_rect(),
	),
	$element
	);
	},
	$params['elements']
	),
	)
	);
	}

@westonruter, while working on this PR, I came across this code and had a quick question. On line 93, shouldn’t it use $params['url'] instead of home_url( '/' )? It seems like if a URL is passed in the $params argument, it doesn’t get used. Is this intentional?

Just thought I’d flag it while I noticed it—thanks!

[westonruter]

You're right! That seems to be an oversight in the tests.

[westonruter]

See other comment about calling this rather current_etag.

[ShyamGadde]

Done in 371e998.

[westonruter]

It's a bit confusing that this is here rather than (also?) added as urlMetric.eTag (or urlMetric.etag). In the end they will need to be identical of course, so it does make sense. Just an observation. Maybe this should be called current_etag to better disambiguate?

[ShyamGadde]

Renamed the arg in 371e998.

[westonruter]

I wonder if we should make this just 'etag'. In HTTP it is "ETag", and a lower case "eTag" feels like it would maybe be something else, like an "electronic tag" or something.

[ShyamGadde]

Makes sense. Done in 08f8f8a.

[westonruter]

Consider $current_etag instead.

[westonruter]

Ditto on $current_etag.

[westonruter]

This is good. It makes sense that the ETag wouldn't be provided with the URL Metric object in the JSON body since it's not something that the client collects from the document. In this way it is similar to the uuid and url, although in theory the url and etag could be provided with in the urlMetric object even though they will just be ignored.

[westonruter]

An important point here: similar to the slug, i think the etag actually should be hashed:

Suggested change

	$current_etag = implode( ',', array_keys( $visitors ) );
	$current_etag = md5( implode( ',', array_keys( $visitors ) ) );

The key reason here is that when expanding the scope here to address the parent issue #1466, there will be much more data to include in the ETag. So I suggest that there be a new function created, like od_compute_current_etag() which takes $tag_visitor_registry as an argument. For now it could look like:

function od_compute_current_etag( OD_Tag_Visitor_Registry $tag_visitor_registry ) {
    $data = array(
        'tag_visitors' => array_keys( iterator_to_array( $tag_visitor_registry ) ),
    );
    return md5( serialize( $data ) );
}

Then later when working on #1466 the $data can be expanded to include things like the IDs and post_modified times for the posts in the loop, what the current queried object is, and so on.

[ShyamGadde]

Done in 8fe38f7.

[westonruter]

The test cases for Embed Optimizer and Image Prioritizer are not fixed yet, as I'm uncertain if the approach used in Optimization Detective's test cases is the best path forward.

Suggestions or feedback on the Optimization Detective test cases would be appreciated before proceeding.

@ShyamGadde I believe I've worked out a solution for the testing problem by introducing a filter for allowing extensions to customize the parameters that go into constructing an ETag. For #1466 this will be useful in case sites want to fine-tune what internal state of a page goes into constructing an ETag.

[westonruter]

Signing off for the day.

[westonruter]

Whew! This was more involved than I expected. Great work!