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

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

Open
wants to merge 20 commits into
base: trunk
Choose a base branch
from

Conversation

b1ink0
Copy link
Contributor

@b1ink0 b1ink0 commented Dec 19, 2024

Summary

Fixes #1731

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.
  • A site health check will run weekly and update the status.
  • Based on the status, a notice will be displayed on the plugins page.

Scenarios:

  1. When the health check passes
    Screenshot 2024-12-20 at 7 10 55 PM

  2. When the REST API endpoint returns a forbidden error
    Screenshot 2024-12-20 at 7 12 27 PM
    Screenshot 2024-12-24 at 7 28 55 PM

  3. When the REST API endpoint returns an unauthorized error
    Screenshot 2024-12-20 at 7 11 41 PM
    Screenshot 2024-12-24 at 7 28 23 PM

  4. When other errors are encountered
    Screenshot 2024-12-20 at 7 14 18 PM
    Screenshot 2024-12-24 at 7 29 50 PM

@b1ink0 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
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

'<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(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines 229 to 234
// 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'];
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

update_option(
'od_rest_api_info',
array(
'status' => 'error',
Copy link
Member

Choose a reason for hiding this comment

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

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

update_option(
'od_rest_api_info',
array(
'status' => 'forbidden',
Copy link
Member

Choose a reason for hiding this comment

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

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

'available' => false,
)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

@westonruter 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 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
wp_schedule_event( time(), 'hourly', 'od_rest_api_health_check_event' );
}
}
add_action( 'wp', 'od_schedule_rest_api_health_check' );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

*/
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' );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@b1ink0 b1ink0 marked this pull request as ready for review December 24, 2024 14:12
@b1ink0 b1ink0 requested a review from felixarntz as a code owner December 24, 2024 14:12
Copy link

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: b1ink0 <[email protected]>
Co-authored-by: westonruter <[email protected]>

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

@b1ink0 b1ink0 requested a review from westonruter December 24, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimization Detective can be blocked from working due to sites disabling the REST API
2 participants