Add health check for Site Kit service connectivity

[aaemnnosttv]

Feature Description

Currently, Site Kit performs a number of compatibility checks before the user is sent along to perform the initial set up in order to flag potential problems early and point users in the right direction with the support team.

One such check is to test general connectivity with Google APIs (using the Search Console API in particular). While the user does not have credentials yet to make such a request successfully, the point is to simply test that a request can be made at all. This helps flag problems where a web firewall or even Google itself may be blocking connectivity (such as legally required geo-restrictions).

We should add a similar check to test backend connectivity with the Site Kit service as an equally, if not more critical API due to its role as an authentication proxy.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The GET:core/site/data/health-checks endpoint should perform an additional check, making an HTTP HEAD request to the base URL of the Google proxy, according to the environment
  • A new skService entry should be added to the checks object returned by this endpoint with its value as the result of this check, with the following shape

    
    pass: Boolean,
    errorMsg: String,
    
    

  • pass should be true if the request returns a status code < 400
  • errorMsg should be a relevant error message (connection_fail) if the status code is 400 or greater
    • If the request fails with a WP_Error use the message from the error
• On the client-side call to this endpoint, if that specific check has failed (!response.checks.skService.pass), a new error constant should be returned. In that case the user should see a message Looks like your site is having a technical issue with connecting to the Site Kit authentication service., followed by the same copy about reaching out to support that is currently present for the ERROR_GOOGLE_API_CONNECTION_FAIL error.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

[aaemnnosttv]

@felixarntz we can implement the client side here or in a small follow-up issue, whichever you'd prefer.

[felixarntz]

@aaemnnosttv I wonder whether the base URL is an appropriate indicator though. The base URL is technically the Site Kit website, which is static HTML that may behave differently from the actual proxy pages. It may be better to request one of the proxy pages - although most of those require certain parameters to be passed so it wouldn't be as trivial. What do you think?

[aaemnnosttv]

@felixarntz from recent related support topics, a head request to the base URL was sufficient to confirm if the site was blocked as this should really only be possible before the request reaches the application side of things on the service. If it can do that, then it can connect 😄

I initially thought about potentially adding a simple /ping endpoint to the service which could return a hardcoded pong or something trivial to indicate a successful request. Again, the problem isn't really within the realm of the application on the receiving end but in the connection between so even this should be unnecessary.

[felixarntz]

@aaemnnosttv Okay that makes sense. In that case, let's go with the ACs as they are. I've added proposed copy for the message, which is similar to the existing message Looks like your site is having a technical issue with requesting data from Google services.

@bethanylang Can you have a quick check of the message defined in the ACs whether that's good to move forward with from your perspective?

[aaemnnosttv]

Thanks @felixarntz, those LGTM 👍

[aaemnnosttv]

Moving this forward for now, we can tweak the language before it moves into execution as the IB shouldn't duplicate that anyways.

[mxbclang]

:wave: @aaemnnosttv Thanks for flagging this with me in Slack and apologies for missing it for this long. :|

I'm wondering if we have an opportunity here to use the new "get help" links to link out to a guide about this error. We have a few guides that look related to this, but wondering if we would need a new guide specific for this error. What do you think?

[aaemnnosttv]

@bethanylang this would fall under the other the same kind of warnings that are shown in the "Your site may not be ready for Site Kit" setup checks. I'm not sure if we would need a new guide or not, but we should be able to link to the error and update the guide/doc for it independently 🙂

Would you please share the related docs we have now?

[aaemnnosttv]

In the class constructor:

• Accept two new parameters:

  • $context as an instance of the Google\Site_Kit\Context class.
  • $google_proxy as an instance of the Google\Site_Kit\Core\Authentication\Google_Proxy class

@nfmohit this shouldn't be necessary as a Google_Proxy instance is available via the Authentication->get_google_proxy() method 👍

• Create a new instance of the WP_Http class and assign it to a variable named $http (or something more meaningful).
• Call the head() method of the $http variable with $service_url passed to it and assign it to a variable named $response.

We don't need to touch WP_Http directly, instead we can use the wp_remote_head function.

• If $response is a WP_Error (check with is_wp_error), return an array with the following properties:

  • pass with the value of false.
  • errorMsg with the error message of WP_Error as the value. The error message can be accessed by calling the get_error_message() method of the $response variable.

• If not, access the code nested property from the response property of the $response array (i.e. $response[ 'response' ][ 'code' ]). Assign it to a new variable named $status_code.

We can use wp_remote_retrieve_response_code for getting the response code from the response in a way which is WP_Error-safe. If called with a WP_Error, it would return an empty string.

Also, while this isn't part of the AC, we should make sure this change is coordinated with the support team so they can publish new documentation for this error before it's released. This could even be started now I think. cc: @bethanylang

[nfmohit]

I have updated the IB according to the above suggestions, thank you @aaemnnosttv! I have a small follow-up question if that's okay:

We can use wp_remote_retrieve_response_code for getting the response code from the response in a way which is WP_Error-safe. If called with a WP_Error, it would return an empty string.

In a case where the response is a WP_Error, do we not want to pass the error message from it to the errorMsg property?

Thank you!

[aaemnnosttv]

In a case where the response is a WP_Error, do we not want to pass the error message from it to the errorMsg property?

@nfmohit that would be consistent with what we currently do, so yes let's do that. We don't actually use this value anywhere it seems but could perhaps be useful by support. The user-facing error details are all based on the check's pass value. Let's pass the actual error message through in the response from the check though as you said. I'll update the AC to clarify.

[nfmohit]

Updated the IB, thank you, @aaemnnosttv!

[aaemnnosttv]

IB ✅

[mohitwp]

QA Update ⚠️

@nfmohit

• Verified this on dev environment following the steps mentioned under QAB.
• Verified that On Site Kit connection splash screen an error with "Your site may not be ready for Site Kit shows up", including the message mentioned in the ACs.
• Same copy as 'ERROR_GOOGLE_API_CONNECTION_FAIL error is showing.
• Support link navigating user to https://wordpress.org/support/plugin/google-site-kit/ same as google_api_connection_fail.
• Verified - As per AC a new skService entry added to the checks object returned by this endpoint with its value as the result of this check, with the following shape

{
	pass: Boolean,
	errorMsg: String,
}

Question - In AC it is mentioned that pass should be true if the request returns a status code < 400. I verified response status code is 200 which is <400 and I'm getting ``pass: false` . Please clarify on this one. Am I considering status code valid for this condition mentioned under AC ?

cc @wpdarren

[nfmohit]

Question - In AC it is mentioned that pass should be true if the request returns a status code < 400. I verified response status code is 200 which is <400 and I'm getting ``pass: false` . Please clarify on this one. Am I considering status code valid for this condition mentioned under AC ?

Thank you for your question, @mohitwp. The AC doesn't actually ask to check the status code for this request. The request that the AC speaks about is an internal HEAD request that can't be seen on the client side. The status code that you have pointed in the screenshot above is valid and correct.

[mohitwp]

QA Update ✅

Note : I'm not able to verify AC 2 below points because that can't be seen on the client side. Confirmed with Nahid here.

- pass should be true if the request returns a status code < 400
- errorMsg should be the WP_Error message if the response is a WP_Error, otherwise (connection_fail) if the status code is 400 or greater
- If the request fails with a WP_Error use the message from the error.

• Verified this on dev environment following the steps mentioned under QAB.
• Verified that On Site Kit connection splash screen an error with "Your site may not be ready for Site Kit shows up", including the message mentioned in the ACs.
• Same copy as 'ERROR_GOOGLE_API_CONNECTION_FAIL error is showing.
• Support link navigating user to https://wordpress.org/support/plugin/google-site-kit/ same as google_api_connection_fail.
• Verified - As per AC a new skService entry added to the checks object returned by this endpoint with its value as the result of this check, with the following shape

{
	pass: Boolean,
	errorMsg: String,
}