Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

b1ink0 · 2024-12-19T17:59:46Z

Summary

Relevant technical choices

Adds a site health check to verify the availability and status of the /optimization-detective/v1/url-metrics:store REST API endpoint. The process will short-circuit if the endpoint is inaccessible.
Based on the status, a global admin notice on plugin activation and meta row notice on subsequent admin pages will be shown.

Scenarios:

When the health check passes
When the REST API endpoint returns a forbidden error
When the REST API endpoint returns an unauthorized error
When other errors are encountered

westonruter

Should there be a plugin activation hook added as well which does add_option() for the new option and then also kicks off (or schedules) a REST API check? Ideally there would be a warning shown immediately after activating the plugin (e.g. on the plugins list table screen) whether the REST API is working so that the user doesn't have to discover it later via Site Health.

westonruter · 2024-12-19T18:26:02Z

plugins/optimization-detective/includes/site-health/load.php

Maybe put site-health in the root directory instead of inside includes since there are no other directories in there?

I thought in future if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing then it would be better to add that feature in includes/admin. But we can just refactor things later when we need it so moving site-health to root makes sence.

Yeah, let's put it in the root for now since all other directories are there.

if we added something like admin dashboard for managing URL metrics or any other admin dashboard related thing

Aside: I did put together a rough utility plugin for this: https://github.com/westonruter/od-admin-ui

westonruter · 2024-12-19T18:27:41Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+			'<p>%s</p>',
+			esc_html__( 'The Optimization Detective endpoint could not be reached. This might mean the REST API is disabled or blocked.', 'optimization-detective' )
+		);
+		update_option(


This is the first PR that adds an option to to Optimization Detective. We'll need to make sure that the relevant delete_option() calls get added to the plugin's uninstall.php.

westonruter · 2024-12-19T18:29:24Z

plugins/optimization-detective/optimization.php

+	// Disable detection if the REST API is disabled.
+	$od_rest_api_info = get_option( 'od_rest_api_info' );
+	if ( is_array( $od_rest_api_info ) && isset( $od_rest_api_info['available'] ) ) {
+		$needs_detection = (bool) $od_rest_api_info['available'];
+	}
+


Actually, this check wouldn't make sense here. It should rather be done in od_maybe_add_template_output_buffer_filter() to short-circuit if the REST API it is not available.

westonruter · 2024-12-19T18:32:01Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		update_option(
+			'od_rest_api_info',
+			array(
+				'status'    => 'error',


Should the $error->get_message() and maybe $error->get_code() be stored here?

westonruter · 2024-12-19T18:34:40Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		update_option(
+			'od_rest_api_info',
+			array(
+				'status'    => 'forbidden',


Instead of storing the string, what about storing the $status_code instead?

westonruter · 2024-12-19T18:35:07Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+				'available' => false,
+			)
+		);
+	}


The else condition should be added as an error result as well. Here especially the $status_code could be used.

westonruter · 2024-12-19T18:38:03Z

plugins/optimization-detective/includes/site-health/rest-api/helper.php

+		&& count( $expected_params ) === count( array_intersect( $data['data']['params'], $expected_params ) )
+	) {
+		// The REST API endpoint is available.
+		update_option(


Instead of having update_option() appearing in multiple places, each condition could populate an $info variable which is then sent into update_option() once at the end of the function.

…ndition

westonruter · 2024-12-20T21:13:28Z

plugins/optimization-detective/site-health/rest-api/helper.php

+		wp_schedule_event( time(), 'hourly', 'od_rest_api_health_check_event' );
+	}
+}
+add_action( 'wp', 'od_schedule_rest_api_health_check' );


Is this a best practice? Should it rather go in admin_init to avoid frontend writes? I'm not sure what others do here.

I think scheduling on plugin activation hook will be better than admin_init.

The problem is that the plugin activation hook doesn't work when network-activating a plugin in multisite.

In looking at WP_Site_Health, it goes ahead and schedules an event even for frontend requests, since it calls its maybe_create_scheduled_event method in the constructor. And the instance is loaded in wp-settings.php. Nevertheless, since a database write is involved, it is preferable if event scheduling happens via an admin request and not unauthenticated frontend requests.

Please look at this comment regarding which hook should be used.

westonruter · 2024-12-20T21:13:42Z

plugins/optimization-detective/site-health/rest-api/helper.php

+ */
+function od_schedule_rest_api_health_check(): void {
+	if ( ! (bool) wp_next_scheduled( 'od_rest_api_health_check_event' ) ) {
+		wp_schedule_event( time(), 'hourly', 'od_rest_api_health_check_event' );


I think hourly is too much. Maybe weekly would make sense.

I currently have it set to run weekly, but that might be too infrequent. If a configuration change disables the REST API, it could take an entire week for user to detect the issue. I believe running it daily would be a better choice.

I think weekly is fine. It's not likely that a user would be changing the availability of the REST API. If we check at the moment that a plugin is activated, and then check weekly thereafter, then this should be good. Note that Site Health's own checks run on a weekly basis via the wp_site_health_scheduled_check action.

…ck fails

…tion

westonruter · 2025-01-15T19:40:10Z

plugins/optimization-detective/helper.php

+	// Omit the generator tag if the REST API is unavailable since in this case Optimization Detective is not generating anything on the page.
+	if ( od_is_rest_api_unavailable() ) {
+		return;
+	}


@felixarntz What do you think of adding this? This will avoid us querying pages from HTTP Archive using Optimization Detective which don't actually have it doing anything. This will improve our checks for efficacy rates.

I don't think we should omit the generator tag, as it's about indicating which sites use this plugin. If we want to mark sites which have or don't have the REST API available, we should do that with a separate mechanism.

OK, what about appending this additional information at the end of the content, like 2a132ca

This is what the AMP plugin does: https://github.com/ampproject/amp-wp/blob/232df1cdf86bb76e1d7f885fbcf76f3ae6a67ebc/includes/amp-helper-functions.php#L886-L909

I think that would work!

westonruter · 2025-01-15T22:12:44Z

As of 523daaa, the styling of the admin notices have been improved based on their context. Initially after activating the plugin:

Subsequent page loads when only the inline admin notice is displayed after the user has expanded it:

So the admin notice displayed at admin_notices does not use a DETAILS element whereas the notice inline with the plugin row does.

…ailable

… add/site-health-check-for-od-rest-api

…tests

…dmins

…ove formatting of error message

westonruter · 2025-01-16T01:17:20Z

I realized that the Site Health test was getting buried among other recommended tests so I've bumped it to critical. I've also included the raw response so the user can have additional context:

westonruter · 2025-01-16T01:24:19Z

Example I've seen with Nginx blocking the REST API shows up as this:

…or example

westonruter

Looking forward to getting this out there!

b1ink0 added 7 commits December 19, 2024 18:34

Add REST API health check for Optimization Detective

88952a5

Improve error message of site health check

4bff9fe

Add scheduled health check for Optimization Detective REST API

56d769a

Save site health check status for Optimization Detective REST API

ba911e1

Add test for when REST API is available

12a229c

Add tests for REST API unauthorized error handling

a0f460d

Add test for REST API forbidden error handling

c29cb7c

b1ink0 changed the title ~~Add/site health check for od rest api~~ Optimization Detective can be blocked from working due to sites disabling the REST API Dec 19, 2024

westonruter reviewed Dec 19, 2024

View reviewed changes

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

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Dec 19, 2024

b1ink0 added 6 commits December 20, 2024 18:04

Refactor to move site-health directory to plugin root directory

8d8ae3e

Clear site health check data during plugin uninstallation

422a5bf

Move REST API availability check to appropriate function

0bc74f7

Add error message and error code to option

36042fd

Refactor to store status code instead of string, Add fallback else co…

ad21df8

…ndition

Refactor to only use update_option once

0bba468

b1ink0 changed the title ~~Optimization Detective can be blocked from working due to sites disabling the REST API~~ Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible Dec 20, 2024

westonruter reviewed Dec 20, 2024

View reviewed changes

b1ink0 added 7 commits December 23, 2024 19:07

Add activation hook to initialize option value

e1b9004

Move scheduling to activation hook

dbede65

Change health check scheduling from hourly to weekly

2b2ca3e

Run REST API health check on plugin activation, Display notice if che…

e7c1001

…ck fails

Move activation logic to separate function

7e4e057

Refactor activation logic to directly call REST API health check func…

a2baa65

…tion

Merge branch 'trunk' into add/site-health-check-for-od-rest-api

e1ec10c

b1ink0 marked this pull request as ready for review December 24, 2024 14:12

b1ink0 requested a review from felixarntz as a code owner December 24, 2024 14:12

b1ink0 and others added 4 commits January 16, 2025 00:42

Remove redundant unscheduling of cron event

22dde1b

Refactor test to use data provider

9869150

Use 'unavailable' instead of 'inaccessible'

55e3b55

Omit generator tag when REST API is unavailable

85b13f4

westonruter reviewed Jan 15, 2025

View reviewed changes

westonruter added 6 commits January 15, 2025 13:39

Fix checking array shape

51a56ba

Amend generator with REST API availability

2a132ca

Rename functions to better reflect purpose

e22bb84

Add missing private tags to helper functions

7eda72c

Pass through original HTTP message

cc16600

Improve styling of admin notice

523daaa

westonruter added 11 commits January 15, 2025 14:15

Improve function names

a1335e0

Account for another site_status_tests filter not returning an array

e5d3f72

Add od_render_generator_meta_tag() test for when the REST API is unav…

c6b4417

…ailable

Add test for od_get_cache_purge_post_id()

3212dae

Merge branch 'trunk' of https://github.com/WordPress/performance into…

19ff8e4

… add/site-health-check-for-od-rest-api

Update check in od_maybe_add_template_output_buffer_filter() and add …

546217f

…tests

Add test coverage for Site Health logic

3d5e8a8

Account for Site Health not being available in multisite to regular a…

c3b682f

…dmins

Add test coverage for HTTP request returning WP_Error

c085ff0

Fix absent site health test after failed function rename

50a1898

Further account for multisite and Site Health

c7d31a7

westonruter requested a review from felixarntz January 16, 2025 00:48

westonruter added 2 commits January 15, 2025 16:55

Make Optimization Detective REST API access a critical error and impr…

85da162

…ove formatting of error message

Increase specificity of what the expected error response looks like

0772ebe

Show REST API error message in blockquote if available; add Nginx err…

dbff1f7

…or example

westonruter approved these changes Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

b1ink0 commented Dec 19, 2024 •

edited

Loading

westonruter left a comment

westonruter Dec 19, 2024

b1ink0 Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 19, 2024

westonruter Dec 20, 2024

b1ink0 Dec 23, 2024

westonruter Jan 6, 2025

westonruter Jan 7, 2025

b1ink0 Jan 8, 2025

westonruter Dec 20, 2024

b1ink0 Dec 24, 2024

westonruter Jan 7, 2025

westonruter Jan 15, 2025

felixarntz Jan 15, 2025

westonruter Jan 15, 2025

felixarntz Jan 15, 2025

westonruter commented Jan 15, 2025

westonruter commented Jan 16, 2025 •

edited

Loading

westonruter commented Jan 16, 2025

westonruter left a comment

Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

Are you sure you want to change the base?

Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762

Conversation

b1ink0 commented Dec 19, 2024 • edited Loading

Summary

Relevant technical choices

Scenarios:

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jan 15, 2025

westonruter commented Jan 16, 2025 • edited Loading

westonruter commented Jan 16, 2025

westonruter left a comment

Choose a reason for hiding this comment

b1ink0 commented Dec 19, 2024 •

edited

Loading

westonruter commented Jan 16, 2025 •

edited

Loading