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

Create Proxy Event Tracking Function to Add Site Kit as Source for All ICE Events #8641

Closed
4 tasks
10upsimon opened this issue Apr 30, 2024 · 15 comments
Closed
4 tasks
Labels
javascript Pull requests that update Javascript code P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Apr 30, 2024

Feature Description

As part of the initial event conversion infrastructure work, the need to add an event parameter citing Site Kit as the source of events tracked as part of this epic has been identified.

Given the decoupled nature of the event tracking infrastructure - where supported plugins have their own JS files - it has been discussed that the simplest way to achieve this would be via a proxy style function that is defined in a script that in turn is a dependency to individual plugin script enqueues, much like Script_Data functions. Note, this is not an actual proxy function, merely a helper function abstracting our calls to gtag when we track events.

This body of work requires the need to define said function in a self contained script asset, set it as a dependency for all singular plugin JS scripts and update all individual plugin JS scripts to make use of this new helper function. This helper function should be define against a globally available Site Kit specific var, such as _googlesitekit, so that all plugins requiring it can access it easily from said global.

Said function should accept an event name and an event data object. Said event data object will then be merged so that it always contains the a _source property with a value of 'site-kit'.

A proof-of-concept function was shared as follows:

window._googlesitekit.trackEvent(name, data) {
  dataLayer = dataLayer || [];
  gtag = function () { dataLayer.push(arguments) }
  gtag('event', name, {...data, _source: 'site-kit' })
}

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

Acceptance criteria

  • A new helper function (acting as an abstraction/proxy to event tracking) should be created as a new asset for enqueue, and be set as a script dependency for all standalone plugin JS scripts supported in the Initial Conversion Events (ICE) epic
    • Said function should be loaded async using the relatively new async/defer support of WP_Scripts
    • Said function should track the intended event against gtag and ensure that site-kit is added as a _source event parameter value for all events tracked via this proxy/helper function
    • Said function should be attached to the global _googlesitekit var
    • All existing plugin scripts of the listed supported plugins should be updated to be loaded async and include this new script as a dependency
    • All future plugin scripts should utilise this new script as a dependency, and also be loaded in an async manner

Implementation Brief

  • Add assets/js/track-conversion-event.js
    • Define new function, say trackConversionEvent and attach it to the _googlesitekit global, example global._googlesitekit.trackConversionEvent
    • It should accept name parameter - representing a conversion event name, and a data parameter which holds event data object with any additional details conversion provider might include.
    • Track the event against gtag function, using passed parameters, including _source: 'site-kit' to the event data (it should be merged with data parameter if present). You can check example POC in the issue description.
  • Update includes/Core/Conversion_Tracking/Conversion_Tracking.php
    • Add register_event_tracking_script
      • Instantiate new Script including the above JS file and return the instance. It should include 'execution' => 'async' in the arguments.
    • In register method under existing wp_enqueue_scripts hook, before the scripts from active providers are enqueued:
      • If conversionInfra feature flag is enabled retrieve the tracking script instance with register_event_tracking_script method and invoke register_script() and enqueue() on it.
  • Update existing conversion providers, individual provider JS file should adapt the usage of global._googlesitekit.trackConversionEvent and individual Google\Site_Kit\Core\Conversion_Tracking\Conversion_Events_Provider::register_script method should include 'execution' => 'async' in the arguments of Script, and have assets/js/track-conversion-event.js as dependency.
  • Update webpack/basicModules.config.js webpack partial
    • Include assets/js/track-conversion-event.js to the entry

Test Coverage

  • No updates needed

QA Brief

Changelog entry

  • Update conversion tracking events to include source of event tracking as Site Kit.
@10upsimon 10upsimon added javascript Pull requests that update Javascript code P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature labels Apr 30, 2024
@10upsimon 10upsimon assigned 10upsimon and unassigned 10upsimon Apr 30, 2024
@eclarke1 eclarke1 added the Next Up Issues to prioritize for definition label Apr 30, 2024
@tofumatt tofumatt self-assigned this Apr 30, 2024
@tofumatt
Copy link
Collaborator

From the context/ACs here, would there be a separate, inline script if the user was using two ICE scripts (eg. using two ICE-supported plugins)?

Why output the script inline instead of having a single, common script file we build and include whenever any ICE JS script is used?

@tofumatt tofumatt assigned 10upsimon and unassigned tofumatt Apr 30, 2024
@10upsimon
Copy link
Collaborator Author

@tofumatt as per out comms offline, I think it makes sense to:

  • Have this function be contained within it's own script that we will enqueue
  • Register it as a dep of all singular plugin scripts
  • Load it async
  • Load all singular plugin tracking scripts async too

I'll update the AC accordingly.

@10upsimon 10upsimon removed their assignment May 2, 2024
@tofumatt tofumatt self-assigned this May 2, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented May 2, 2024

Just one question really, when you say a “proxy function”, do you mean an actual JS Proxy? Or do you just mean that our scripts should include/reference the new trackEvent function and use it to send events?

If it’s the latter, the AC should specify that it should be available (on a Site Kit JS global I’d think).

If the former: why? 😅 Proxies when it’s just for our code seems odd. Maybe it’s just a wording thing that’s tripping me up here 😆

@tofumatt tofumatt assigned 10upsimon and unassigned tofumatt May 2, 2024
@10upsimon
Copy link
Collaborator Author

Just one question really, when you say a “proxy function”, do you mean an actual JS Proxy?

@tofumatt no no, not an actual proxy :) I simply meant a helper function that abstracts our calls to gtag when we track events. Yes, this should be defined against the global _googlesitekit or similar var, but that seems more like IB territory. I'll update the AC to reflect the above.

@10upsimon 10upsimon assigned tofumatt and unassigned 10upsimon May 2, 2024
@aaemnnosttv
Copy link
Collaborator

FWIW, I think we should avoid using trackEvent as the name to avoid confusion with our internal function by the same name.

@tofumatt
Copy link
Collaborator

tofumatt commented May 2, 2024

Sounds good, moving to IB 👍🏻

@tofumatt tofumatt removed their assignment May 2, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm May 2, 2024
@10upsimon 10upsimon self-assigned this May 3, 2024
@10upsimon
Copy link
Collaborator Author

@zutigrm

  • Update includes/Modules/Ads.php
    • In setup_assets method:
      • If conversionInfra feature flag is enabled instantiate new Script registering the above JS file and pushing it to the $assets array.
      • It should include 'execution' => 'async' in the arguments.

I do not think this is correct, as Conversion Tracking is not specific to the Ads module, and can be turned on from GA4 without the ads module active, and vice-versa.

I think it may be better to register this script from within the Conversion_Tracking class. WDYT?

@10upsimon 10upsimon assigned zutigrm and unassigned 10upsimon May 3, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented May 3, 2024

@10upsimon Yes that makes more sense. Thanks. IB updated

@zutigrm zutigrm assigned 10upsimon and unassigned zutigrm May 3, 2024
@10upsimon
Copy link
Collaborator Author

Thanks @zutigrm IB ✅

@10upsimon 10upsimon removed their assignment May 3, 2024
@eugene-manuilov eugene-manuilov self-assigned this May 6, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label May 6, 2024
@eugene-manuilov eugene-manuilov removed their assignment May 13, 2024
@benbowler benbowler assigned benbowler and unassigned benbowler May 13, 2024
@benbowler
Copy link
Collaborator

Flagging here that @eugene-manuilov doesn't update the tracking for MailChimp or Popup Maker as these weren't merged at the time this ticket was started.

Should we hold back this ticket and address that here when these two are merged or update those separately?

@tofumatt tofumatt self-assigned this May 16, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented May 16, 2024

@benbowler I think it makes sense to add those here. I'll do that in the PR, it's not a big deal to include 🙂

EDIT: Never mind, they were included later 🙂

@tofumatt tofumatt removed their assignment May 16, 2024
@mohitwp mohitwp self-assigned this May 20, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented May 29, 2024

QA Update ⚠️

  • Tested on dev environment.
  • Verified Optin Monster, WP Forms, MailChimp, Popup Maker and WooCommerce plugin events.
  • Verified that events tracked for WPForms OptIn Monster, Popup Maker, Mailchimp, and WooCommerce providers now tracked with _source: "site-kit".
  • Verified when conversionInfra feature flag is enabled.
  • Verified when Analytics is connected.
  • Verified when both Analytics and ads module are connected.
  • Verified when only Ads module is connected.

Note : As mentioned on #8553 (comment) for WooCommerce only 'add to cart' event is getting tracked.

@wpdarren I'm not able to test Contact Form 7 event on InstaWP and TasteWP due to same error. Can you please retest this on your live site ?

Optin monster

Submit lead form event tracked when only analytics is connected

image

When only Ad module is connected

image

WP Forms

When only Ads module is connected

image

When only Analytics is connected

image

MailChimp

When only Analytics is connected

image

When only Ads module is connected

image

Popup Maker

When only ads module is connected

image

When only Analytics is connected

image

WooCommerce -

Add to cart event tracked

image

@wpdarren
Copy link
Collaborator

@mohitwp yes, I can confirm that the source appears for the contact conversion event.

image

@wpdarren wpdarren removed their assignment May 29, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented May 29, 2024

QA Update ✅

Thanks @wpdarren !

  • Tested on dev environment.
  • Verified Optin Monster, WP Forms, MailChimp, Popup Maker, Contact Form 7 and WooCommerce plugin events.
  • Verified that events tracked for WPForms OptIn Monster, Popup Maker, Mailchimp, and WooCommerce providers now tracked with _source: "site-kit".
  • Verified when conversionInfra feature flag is enabled.
  • Verified when Analytics is connected.
  • Verified when both Analytics and ads module are connected.
  • Verified when only Ads module is connected.

Note : As mentioned on #8553 (comment) for WooCommerce only 'add to cart' event is getting tracked.

Optin monster

Submit lead form event tracked when only analytics is connected

image

When only Ad module is connected

image

WP Forms

When only Ads module is connected

image

When only Analytics is connected

image

MailChimp

When only Analytics is connected

image

When only Ads module is connected

image

Popup Maker

When only ads module is connected

image

When only Analytics is connected

image

WooCommerce -

Add to cart event tracked

image

@mohitwp mohitwp removed their assignment May 29, 2024
@aaemnnosttv
Copy link
Collaborator

Noting that the source parameter may change but it is currently implemented as defined.

One thing from the AC

All existing plugin scripts of the listed supported plugins should be updated to be loaded async and include this new script as a dependency

All the provider scripts should be loaded using deferasync scripts can't have dependencies since the load order isn't guaranteed. There are still a few scripts loading async so these will need to be updated in a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

10 participants