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

Enable Prefetch DNS Requests on external Google services used by Site Kit #2203

Closed
glanglois opened this issue Oct 16, 2020 · 7 comments
Closed
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Milestone

Comments

@glanglois
Copy link

glanglois commented Oct 16, 2020

Feature Description

Several WP Chacing Plugins offer the possibility to connect to external sites (such as Google Fonts, Analytics, ...) to reduce DNS Lookups and improve user experience by improving loading time.

For instance, see WP Rocket documentation on their implementation: https://docs.wp-rocket.me/article/1302-prefetch-dns-requests

I would be useful if SiteKit offer directly this option as part of the plugin settings, especially to embed in one unique plugin, everything related to Google services.


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

Acceptance criteria

  • The frontend should include a dns-prefetch directive for //www.googletagmanager.com when the Analytics tag or Tag Manager tag is placed.
  • The frontend should include a dns-prefetch directive for //pagead2.googlesyndication.com when the AdSense tag is placed.
  • Note: The wp_resource_hints filter should be used.

Implementation Brief

  • Using includes/Modules/AdSense/Web_Tag.php,
    • Use the wp_resource_hints filter to add //pagead2.googlesyndication.com to the list of urls if the relation type is dns-prefetch.
  • Using includes/Modules/Tag_Manager/Web_Tag.php,
    • Use the wp_resource_hints filter to add //www.googletagmanager.com to the list of urls if the relation type is dns-prefetch.

Test Coverage

  • N/A

Visual Regression Changes

  • N/A

QA Brief

  • Connect and activate the Analytics module.
  • Inspect the markup on the front end and the following line should be present: <link rel='dns-prefetch' href='//www.googletagmanager.com' />
  • Deactivate the Analytics module, connect and activate the Tag Manager module.
  • Inspect the markup on the front end and the following line should be present: <link rel='dns-prefetch' href='//www.googletagmanager.com' />
  • Connect and activate the AdSense module.
  • Inspect the markup on the front end and the following line should be present: <link rel='dns-prefetch' href='//pagead2.googlesyndication.com' />

Changelog entry

  • Enable Prefetch DNS Requests on external Google services used by Site Kit
@felixarntz felixarntz self-assigned this Oct 20, 2020
@felixarntz
Copy link
Member

@glanglois Thanks for the suggestion (and sorry for the super long delay 🤦‍♂️ )!

We're going to look into adding DNS prefetch directives for the external services that Site Kit uses (e.g. Analytics, AdSense, Tag Manager snippets), to improve performance. Regarding any external Google services, that is rather out of scope for this plugin and IMO is more suitable in a performance plugin, for example since we are not actually loading Google fonts in the frontend in Site Kit.

As such, I'd like to focus this issue on adding DNS prefetch for the services actually used by Site Kit in the frontend.

@felixarntz felixarntz changed the title Enable Prefetch DNS Requests on Google External Services Enable Prefetch DNS Requests on external Google services used by Site Kit Mar 18, 2021
@felixarntz felixarntz added P1 Medium priority Type: Enhancement Improvement of an existing feature Next Up labels Mar 18, 2021
@felixarntz felixarntz removed their assignment Mar 18, 2021
@glanglois
Copy link
Author

@felixarntz Thank you for your reply.

I was only thinking that Google Site Kit would perform a prefetch of connected Google services (analytics, tag manager, etc.). It's great to know that this could go in your roadmap.

For any other prefetch, I agree that it's out of scope and should be handled by performance plugins.

Have a great day!

@eclarke1 eclarke1 removed the Next Up label Mar 29, 2021
@asvinb asvinb self-assigned this Apr 21, 2021
@asvinb
Copy link
Collaborator

asvinb commented Apr 21, 2021

@aaemnnosttv @felixarntz I noticed that after setting up Tag Manager, we already have the dns-prefetch directive set for //www.googletagmanager.com I'am on WordPress 5.7.1 and using the Twenty Twenty-One theme. Any idea why this is the case?
I've added the IB which caters only for the AdSense tag.

@felixarntz
Copy link
Member

@asvinb I think that is actually due to Analytics, not Tag Manager. WordPress automatically includes dns-prefetch directives for assets that are enqueued, and we enqueue https://www.googletagmanager.com/gtag/js... via Analytics.

So for Tag Manager, we still need to do something about it just for the scenario that the user has only Tag Manager enabled (in the Tag Manager Web_Tag, the asset is not enqueued, but just added via the script on the page. So this part needs to be added to the IB. (Make sure to only add the hostname if it is not already present, e.g. via Analytics.)

@felixarntz felixarntz assigned asvinb and unassigned felixarntz Apr 21, 2021
@asvinb
Copy link
Collaborator

asvinb commented Apr 22, 2021

Thanks @felixarntz . IB updated to cater for Tag Manager.

@asvinb asvinb assigned felixarntz and unassigned asvinb Apr 22, 2021
@felixarntz
Copy link
Member

IB ✅

@felixarntz felixarntz removed their assignment Apr 26, 2021
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Apr 29, 2021
@Hazlitte Hazlitte assigned Hazlitte and unassigned Hazlitte May 10, 2021
@fhollis fhollis added this to the Sprint 51 milestone Jun 2, 2021
@asvinb asvinb self-assigned this Jun 8, 2021
@asvinb asvinb removed their assignment Jun 8, 2021
@asvinb asvinb assigned eugene-manuilov and unassigned asvinb Jun 14, 2021
@eugene-manuilov eugene-manuilov removed their assignment Jun 14, 2021
@wpdarren wpdarren assigned wpdarren and unassigned wpdarren Jun 14, 2021
@wpdarren
Copy link
Collaborator

QA Update: Pass ✅

Verified:

  • For Analytics: Inspected the markup on the front end and the following line is present: - Screenshot
  • For Tag Manager: Inspected the markup on the front end and the following line is present: -
    Screenshot
  • For Adsense: Inspected the markup on the front end and the following line is present: - Screenshot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

10 participants