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 for source code/tag status #5695

Closed
bethanylang opened this issue Aug 15, 2022 · 36 comments
Closed

Add Site Health check for source code/tag status #5695

bethanylang opened this issue Aug 15, 2022 · 36 comments
Labels
Module: AdSense Google AdSense module related issues Module: Analytics Google Analytics module related issues Module: Tag Manager Google Tag Manager module related issues P1 Medium priority PHP Type: Enhancement Improvement of an existing feature Type: Support Support request

Comments

@bethanylang
Copy link
Collaborator

bethanylang commented Aug 15, 2022

Feature Description

The support team regularly gets topics from users who are confused about why a particular module in Site Kit isn't working. One of the first things that they often ask is if Site Kit is properly "connected," e.g. if the necessary code snippet was properly added. As a result, the support team spends a lot of time verifying source code placement on each site and letting the user know that the code was properly placed (it almost always is) and that the issue must be elsewhere.

We do have a guide about what code Site Kit places and where, but many of our users have difficulty using the inspector to verify that the code has been added to their site.

Per our discussion in the August 15 meeting, it would be helpful to have a Site Health check that confirms that code snippets have been properly added and placed for the Site Kit modules that are connected on a site. Ideally, this would appear in both the Status and Info screens so that this information is an easy check for users in Status but is also included in their Site Health Info, as many users share this information with support for troubleshooting purposes.

cc @jamesozzie @adamdunnage @marrrmarrr @felixarntz


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

Acceptance criteria

  • Site Health information should display whether a tag is placed for a connected module that supports tags.
  • The tag placement should appear in Site Health in WordPress. It should always reflect the current state of the site, with no caching of the frontend request.
  • The request should check for any compatible tag for the relevant Site Kit module, eg. use the tag-matchers.js file for the relevant module.
  • The request should also check for the HTML comments inserted by Site Kit when Site Kit places a tag. (See:
    $snippet_comment_begin = sprintf( "\n<!-- %s -->\n", esc_html__( 'Google Analytics snippet added by Site Kit', 'google-site-kit' ) );
    $snippet_comment_end = sprintf( "\n<!-- %s -->\n", esc_html__( 'End Google Analytics snippet added by Site Kit', 'google-site-kit' ) );
    for an example of the HTML tags output by Site Kit).
  • The request should always be made unauthenticated, to ensure the tag appears as it would to a non-authenticated user.
  • If no tag is detected for a module, we should output: "No tag detected."
    • Optional: If the "Site Kit should place snippet/tag" setting is true for that module, we should surface a warning in the Site Health info tab. (If this is a lot of effort/better left to a separate issue, please flag in the IB/Execution and we can create a follow-up)
  • If a tag is detected but we could not detect the "placed by Site Kit" HTML, we should output: "Tag detected but could not verify that Site Kit placed the tag."
  • If a tag is detected with the Site Kit HTML comment, we should output: "Tag detected and placed by Site Kit."
  • If a module is not connected, it should not display the tag status.

For example: if Analytics and Tag Manager are connected, then Site Health should display the status of whether the Analytics and Tag Manager tag is placed on the frontend.

Implementation Brief

  • Update includes/Core/Util/Debug_Data.php:
    • In register method:
      • Hook into googlesitekit_rest_routes filter, to register a new GET REST route, say core/site/data/tags-placement-test. You can use this example as starting point
      • Hook into site_status_tests filter to add tests. You can follow the example here. Include a callback method, say tag_tests.
    • Add tag_tests method:
      • Use the passed $tests variable, to include custom Site Kit test to the array. Check for WordPress version, if it is higher than 5.6, put it into async tests group instead of direct, to leverage asynchronous handling without adding new javascript, as WordPress core handles the response and updates the UI. But if WP version is less than 5.6, than keep it as direct, like in the example.
    • Add tags_placement_test method:
      • If it is WP version bellow 5.6, return the description This feature requires WordPress version 5.6 or higher
      • Otherwise fetch the homepage, you can see an example here
      • Add new variable $active_modules, and assign it a value of active modules, e.g $this->modules->get_active_modules(), and filter only for AdSense, Tag Manager, and Analytics modules
      • You can add new variable that would dynamically store the string output, based on the match results. It can be formatted like $description .= '<li>{MODULE_NAME}: Tag detected and placed by Site Kit.</li>', etc based on the messaging provided in AC.
      • If there is no active module (from the mentioned 3 above), then $description can output Tag status not available: AdSense, Tag Manager, and Analytics modules are not connected.
      • Loop through $active_modules and using the module slug, search through fetched response body of the homepage, for Google $module_name snippet added by Site Kit. Note that modules slug needs to be modified for module name, as Tag Manager slug is tagmanager, and snippet comment will be Google Tag Manager snippet added by Site Kit
        • If match is found, you can join the text for the module to the $description
        • If match is not found, search for the module specific tags, you can adapt the js tag-matchers regex to a php regex, example, for the modules (AdSense, Tag Manager and Analytics).
          • If tag is found, then add text defined in AC (when no Site Kit comment is found, but there is a tag match) to the $description.
        • If neither Site Kit inline comment, or tag are found, add text defined in AC to the $description.
      • Format the test result data
        • For label under badge use Site Kit
        • For color under badge use blue
        • For status, use recommended, so that tab always stays visible, and does not go under Passed Tests dropdown, so users can always spot it right away.
        • For description, use the final $description variable output
      • You can check this doc for more info on parameters, and for more info, this is the core method for getting the tests array. And this core class for one of the REST endpoint callbacks (pointed out above), as well as update announcement where site health switched to REST callback instead of AJAX callback

Test Coverage

  • You can introduce new tests for Google\Site_Kit\Tests\Core\Util\Debug_DataTest that would check for new changes

QA Brief

  • Setup Site Kit
  • Visit Tools -> Site Health. Under the first Status tab, within 5-10sec, a new accordion will Appear on the list which will show the status messages as described in AC. Wihtout any of the three modules active - Analytics, AdSense and Tag Manager, or any of them being active, etc. You can play with options, for example in Analytics module settings, disable option to place tag automatically, and check the health status, it should report that no tag is detected, etc

Changelog entry

  • Add information about code/tag placement statuses to Site Health information.
@bethanylang bethanylang added Type: Support Support request Type: Enhancement Improvement of an existing feature labels Aug 15, 2022
@felixarntz felixarntz self-assigned this Aug 16, 2022
@felixarntz felixarntz added the P1 Medium priority label Aug 16, 2022
@bethanylang
Copy link
Collaborator Author

@jamesozzie This is an issue that I filed that last time that we discussed this issue back in August. When I mentioned this on our call earlier this week, you said that we already have something in the plugin like this. Can you please take a look at this issue and let me know if this is something that we already have, and if so, if it should be tweaked in any way to make this easier for us/users? Thanks!

@jamesozzie
Copy link
Collaborator

jamesozzie commented Oct 27, 2022

@bethanylang We have the snippet placement status printed to a users Site Health information, within the Site Kit section. We also have it output on the AdSense settings page. Screenshots below of each.

One tweak I think would be useful would be to display the snippet status when viewing the connected services, and not just display the status when editing the AdSense settings. There is a GitHub issue for this (#1292). An enhancement to this would also be to display not just "Snippet inserted via Site Kit" but a "Standard" or "Modified" suffix, to help users to determine a third party modification (#2817). I say this as consent, optimization and other plugins can impact the standard snippet.

Site Health information:

image

AdSense settings page:

image

@jamesozzie
Copy link
Collaborator

@bethanylang I've proposed some UX improvements over on another issue (#5693 (comment)) which I feel would would address user confusion for when their site is not yet approved on the AdSense side.

When it comes to further improvements I think that displaying the snippet status next to the connected status (as per #1292) would cover this particular issue. Should we revisit that one? Alternatively we can add this for internal discussion next week?

@eugene-manuilov eugene-manuilov added Module: Analytics Google Analytics module related issues Module: AdSense Google AdSense module related issues Module: Tag Manager Google Tag Manager module related issues PHP labels Dec 14, 2023
@tofumatt tofumatt self-assigned this Dec 14, 2023
@tofumatt
Copy link
Collaborator

ACs here are good, moving to IB 👍🏻

@tofumatt tofumatt removed their assignment Dec 14, 2023
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Dec 15, 2023
@tofumatt tofumatt self-assigned this Dec 19, 2023
@tofumatt
Copy link
Collaborator

search through fetched response body for placed tags using module slug

Do you mean just search through the HTML for adsense? I'm not sure what this means exactly—just searching for the module slug is pretty broad and would lead to false positives. For instance, we have tag matcher code here:

, we might want to be more specific to ensure the code is output.

tag is found push it to the $tags array, using module slug as a key, and true for value

This isn't in the ACs, but it could be nice to distinguish between a tag output by Site Kit (which would have HTML tags like this:

function tagBodyOpenHTML( containerID ) {
return `<!-- Google Tag Manager (noscript) -->
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=${ containerID }"
height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
<!-- End Google Tag Manager (noscript) -->
`;
}
) and just any Tag Manager/other tag on the page. Maybe we could differentiate with:

  • No tag found (nothing output)
  • Google $module_name snippet found on website
  • Google $module_name snippet added by Site Kit

Of course, for some users with HTML optimisation plugins, our HTML comments might be stripped. 🤔

Hook into admin_init, and check if parse_homepage_tags_transient exists, and if it doesn't invoke the parse_homepage_tags method

This would trigger the homepage HTTP request when the user accesses any Admin page. Given the slowdown this would cause I think that's excessive; it'd be better to add a hook that triggers this behaviour when loading the Site Health page or similar.

I'd also caution against saving this debug information as a 24 hour transient, because it means if the user changes/removes the tag on the frontend somehow, the debug information will be out-of-date for 24 hours. That would be a highly confusing debugging experience; we'd be showing the user very technical information on a very technical page that was out of date. We're better off either:

  • making the request via PHP every time, regardless of the slowdown, but only on the Site Health page
  • making the request in the background after loading Site Health and triggering an update of the page after the request is complete

@tofumatt tofumatt assigned zutigrm and unassigned tofumatt Dec 19, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Dec 20, 2023

@tofumatt Thanks for the feedback.

Do you mean just search through the HTML for adsense? I'm not sure what this means exactly—just searching for the module slug is pretty broad and would lead to false positives

It is meant to be used as $module_name in the string that will be searched for, mentioned in the IB: Google $module_name snippet added by Site Kit. This is to ensure we only look for output made by Site Kit. The AdSense and Tag Manager are mentioned in the context of their module slugs needing modified assignment to $module_name variable that will be part of the searched inline comment.

For instance, we have tag matcher code here:

  • Tag matching patterns.
    we might want to be more specific to ensure the code is output

Thanks for pointing this out to me, this can be helpful. But detecting just that snippet is present as you also mentioned it, can't tell us if it was done from Site Kit, by user, or any other plugin. That's why I suggested searching for comment (see above) in the IB. Although comments can be stripped by optimisation plugins, this is still the safest way to confirm if snippet is added by Site Kit.

Hook into admin_init, and check if parse_homepage_tags_transient exists, and if it doesn't invoke the parse_homepage_tags method

This would trigger the homepage HTTP request when the user accesses any Admin page. Given the slowdown this would cause I think that's excessive; it'd be better to add a hook that triggers this behaviour when loading the Site Health page or similar.

I'd also caution against saving this debug information as a 24 hour transient, because it means if the user changes/removes the tag on the frontend somehow, the debug information will be out-of-date for 24 hours. That would be a highly confusing debugging experience; we'd be showing the user very technical information on a very technical page that was out of date. We're better off either:

  • making the request via PHP every time, regardless of the slowdown, but only on the Site Health page
  • making the request in the background after loading Site Health and triggering an update of the page after the request is complete

admin_init is the earliest hook for site health also, I will update IB to include additional check if we are on Site Health page to make a request. But I would still suggest using transient vs making request every time. User can change snippet usage, by activating/deactivating the module that inserts the snippet, but that's why I included that transient should be deleted on activation and deactivation method in the modules. I can also include to reset this transient when useSnippet setting change?

Let me know what you think?

@tofumatt
Copy link
Collaborator

Sounds good regarding searching for the Site Kit HTML snippet. Let's make sure we include the HTML comments as well, but that sounds good. 👍🏻


User can change snippet usage, by activating/deactivating the module that inserts the snippet

That's true, but they can also change a lot of other things, like a plugin's optimisation to strip HTML comments, that would show/hide the Site Kit comments in HTML output. Also: if they weren't outputting the snippet via Site Kit, went to the admin to set the transient, and then set Site Kit to output the snippet, with this approach, Site Health would still claim that the snippet is not output.

If you want to go the transient route, it would be good to at least offer the user two things:

  1. when the site was last checked
  2. a button to force a refresh of the tag

Given this feature is for a subset of Site Kit users who need to see its status in Site Health, we shouldn't be slowing down the entire WordPress admin by a minimum of a few hundred ms (though I'd guess it might take a second or two to make the request in total). Site Kit sometimes gets mischaracterised as slowing down user's sites, which we really don't do, but adding this kind of request on every admin page for something a subset of users will use, and running it once a day, would actually give the idea that we slow down users' sites some weight 😆. So let's not do that.

I don't like the transient because it thoroughly decreases the accuracy and thus usefulness of this feature. The whole point is that this should save time and clarify whether Site Kit is successfully outputting tags, in part for Support. If we add the feature but make it laggy, that might just confuse the support flow further because it might say that Site Kit it outputting the tag, but then a user changes a setting in an already activated plugin, and then the output is stripped. But the Site Health would still claim it's there.

I think the info needs to be refreshed every time. I'd be fine if it were done asynchronously, but I think that's the only way this feature meets its intended use case.

@tofumatt tofumatt assigned tofumatt and zutigrm and unassigned tofumatt and zutigrm Dec 20, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Jan 25, 2024

@mohitwp I see what's the issue, we are outputting the comment for AdSense above meta tags as well, not only tag snippet. I will consult with others if I can change the comment content for meta tags. You can return ticket back to me

@mohitwp
Copy link
Collaborator

mohitwp commented Jan 25, 2024

@zutigrm I'm using oi.ie site to test this.

Case 1 : When AdSense code placed on site.

image

image

image

Case 2 : When AdSense code not placed on site.

No change in status.

image

@mohitwp mohitwp removed their assignment Jan 25, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Jan 25, 2024

@mohitwp Yes, currently AdSense meta tags have the same comment, which leads to false positive, when AdSense tag is disabled, since meta tags are still added, including the same comment that "snippet" is placed, even it is not.

@zutigrm zutigrm removed their assignment Jan 25, 2024
@techanvil techanvil self-assigned this Jan 25, 2024
@zutigrm zutigrm mentioned this issue Jan 25, 2024
18 tasks
@techanvil techanvil assigned mohitwp and unassigned techanvil Jan 25, 2024
@techanvil
Copy link
Collaborator

Back to you for another pass, @mohitwp.

@mohitwp
Copy link
Collaborator

mohitwp commented Jan 26, 2024

QA Update ⚠️

  • Tested on main environment.
  • Verified Tag placement status under site health when modules are connected or disconnected.
  • Verified when Analytics 4, Tag manager and AdSense Set up is not complete.
  • Verified when Analytics 4, Tag manager and AdSense Set up code is not placed in site using option available under settings.
  • Verified Tag placement status is appearing as per AC.

@zutigrm when we cancel the setup of module or do not complete the module setup. Then in this case Tag Placement Status of AdSense is not same as Analytics 4 and Tag manager module. Notice at set up screen display that _"Site Kit has placed AdSense code on your site to connect your site to AdSense and help you get the most out of ads" so even if user do not configure AdSense still code is placed on the site so tag is detected. But do not want to assume and want to confirm that current tag placement status for AdSense in this case is expected or we want to display different status in this case ?

image

image

image

Pass Cases Screenshots

Analytics connected and code placed

image

No modules are connected

image

Analytics connected but code is not placed.

image

Tag manager connected and code placed

image

Tag manager connected but code is not placed

image

AdSense connected and code is placed.

image

AdSense connected but code is not placed

image

@zutigrm
Copy link
Collaborator

zutigrm commented Jan 26, 2024

@mohitwp Thanks for your observation. The tag placement check does a single purpose task - checking if tag is present or not. In this case, I just checked, we do place a tag when connecting the AdSense, even we do not configure the next step. It is correct for tag placement test to report that tag is placed, since it is present.

Now, it is a different question if this is originally intended behaviour for AdSense to place snippet at this point in the setup. Let's see if @aaemnnosttv has some input regarding that part

@techanvil
Copy link
Collaborator

techanvil commented Jan 26, 2024

@mohitwp, it's an interesting observation. The current behaviour is intentional, but as you've pointed out it's inconsistent and I think there is room for improvement.

Essentially, having clicked "Set up Analytics", gone through the OAuth flow and landed on this screen, Site Kit has retrieved the AdSense clientID which is what's needed to render the tag.

However, AdSense is still not connected at this point, because its connected status depends on accountSetupComplete and siteSetupComplete being set to a value other than false. These values are only set to true upon clicking the Configure Analytics CTA. So until the CTA has been successfully actioned, the tag is placed but the module is still not connected.

image

public function is_connected() {
$settings = $this->get_settings()->get();
if ( empty( $settings['accountSetupComplete'] ) || empty( $settings['siteSetupComplete'] ) ) {
return false;
}
return parent::is_connected();
}

This is in contrast to modules like Analytics and Tag Manager, where the connected state and the tag placement condition both depend on the same values being set. For example, Analytics won't be able to place its tag until measurementID is set, and it's not connected until measurementID is set either (as well as propertyID and webDataStreamID, but these are all set at the same time in the setup flow). So the tag placement and connected state are inherently in sync.

I would suggest we could improve things for AdSense by not placing the tag until the module is connected. We could potentially manage this by changing the copy on the above page to indicate the code is not placed yet (e.g. the mockup below), and modify the conditions for placing the tag to include a check for accountSetupComplete and siteSetupComplete being set. Or, we may take an alternative, yet to be determined approach.

image

This is of course out of scope for the current issue, so I would suggest raising a new issue where we can consider this more thoroughly.

@mohitwp
Copy link
Collaborator

mohitwp commented Jan 26, 2024

QA Update ✅

Thank you @techanvil @zutigrm .

  • For the observation reported above I created a ticket AdSense module place code even if Set up is not complete. #8187 .
  • Tested on main environment.
  • Verified Tag placement status under site health when modules are connected or disconnected.
  • Verified when Analytics 4, Tag manager and AdSense Set up is not complete.
  • Verified when Analytics 4, Tag manager and AdSense Set up code is not placed in site using option available under settings.
  • Verified Tag placement status is appearing as per AC.

Analytics connected and code placed

image

No modules are connected

image

Analytics connected but code is not placed.

image

Tag manager connected and code placed

image

Tag manager connected but code is not placed

image

AdSense connected and code is placed.

image

AdSense connected but code is not placed

image

@aaemnnosttv
Copy link
Collaborator

@tofumatt @zutigrm This seems to be working well, but I noticed that it is a bit odd if we only say "Not tag detected" when the module's snippet is turned off in the settings. This is mentioned in the AC as something we could improve in a follow-up, which seems worth improving. Also, the fact that we cannot reliably determine if the tag is placed by SK in an AMP context (due to missing comments) could also be improved with a notice for context. Nice work!

@zutigrm
Copy link
Collaborator

zutigrm commented Jan 31, 2024

@aaemnnosttv Thanks, I opened follow-up issue #8206

@aaemnnosttv
Copy link
Collaborator

This is in contrast to modules like Analytics and Tag Manager, where the connected state and the tag placement condition both depend on the same values being set. For example, Analytics won't be able to place its tag until measurementID is set, and it's not connected until measurementID is set either (as well as propertyID and webDataStreamID, but these are all set at the same time in the setup flow). So the tag placement and connected state are inherently in sync.

I would suggest we could improve things for AdSense by not placing the tag until the module is connected.

@techanvil @mohitwp I missed this conversation during approval but it didn't affect the implementation here, so there's no problem with this issue, however AdSense is unique in that the tag needs to be placed as part of the approval process for an AdSense account (see https://support.google.com/adsense/answer/7402256). Currently, the connected status requires that the account setup is completed, so we can't bypass placing the tag until connected given the current logic.

We could consider decoupling the connected status from the account and site statuses, but I'm not really sure it's necessary or would be beneficial.

@techanvil
Copy link
Collaborator

techanvil commented Feb 1, 2024

@aaemnnosttv, thanks for clarifying this.

Seeing as a few of us got confused here I wonder if we can do something to improve the UX. Maybe we can we determine if the account is approved and skip the Configure AdSense step if so. And, that being the case, we could clarify the copy on the screen with the Configure AdSense CTA to indicate that the tag is in place in order for the account to be approved. WDYT?

@aaemnnosttv
Copy link
Collaborator

Sure, there's probably room for improvement in the flow. It doesn't seem like a high priority but we could open an issue to seek input from @marrrmarrr and @sigal-teller about how to revise it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: AdSense Google AdSense module related issues Module: Analytics Google Analytics module related issues Module: Tag Manager Google Tag Manager module related issues P1 Medium priority PHP Type: Enhancement Improvement of an existing feature Type: Support Support request
Projects
None yet
Development

No branches or pull requests

10 participants