-
Notifications
You must be signed in to change notification settings - Fork 291
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
Adapt GTM integration with GA to support GA4 #7990
Comments
Hey @nfmohit, the AC is most of the way there, however, as noted in a related comment, the If not, we'll determine the most relevant measurement ID for the Google tag following similar logic to that used in GoogleTagIDMismatchNotification, although falling back to the first valid measurement ID if none of them exist in the available Analytics accounts (TBC as per the linked comment). |
Hi @techanvil! I'm handing this over to @jimmymadon as Jimmy actually added the ACs for this one. @jimmymadon Let's sync if needed to discuss the ACR feedback. Thank you folks! |
Oh, sorry @nfmohit, I should have realised that. Thanks for pointing this in the right direction. |
@aaemnnosttv I've modified the AC here based on our discussions for your approval. On a side note, I just wanted to confirm if your reply to my comment regarding supporting AMP is still valid? If yes, then these selectors will assume that an AMP container could contain a c.c. @techanvil @nfmohit |
@bethanylang @ivonac4 As an update, we are waiting to hear back from Mariya and possibly the GTM team on this one. |
This is still in progress, although if it stalls out, I agree with @jimmymadon that we can essentially prune the unused/unreachable AMP code and re-add it later if/when it becomes necessary, but let's hold on this a bit longer. |
@bethanylang I’ve checked the comment you were referring to in Asana and we just want to wait a little bit more before we can finalise the AC for 7990. I believe we are want to wait for the GTM team to liaise with the GA team on whether they've planned to allow adding GA4 tags to a GTM Amp container. c.c. @aaemnnosttv @marrrmarrr |
@jimmymadon Should this be assigned back to you to update the AC, since we've determined we're not going to support AMP? |
I've set this as a 19 as the IB doesn't cover all removal of code that is unreachable and allowing for additional testing of anything missed in the IB. This will have to be done carefully. |
@jimmymadon while GTM doesn't support configuring GA4, removing GTM support for AMP entirely is out of scope for this issue but also isn't something on our foreseeable roadmap. Folks should still be able to set up GTM using AMP as today, they just can't implement GA4 with it, which is fine since Analytics supports it and makes things a bit easier for us actually. This means we'd always check the web container for a Google tag and suggest setting up Analytics regardless of whether or not the site was using AMP or not. I'm not sure if this significantly impacts the rest of the IB (which LGTM), but anything which was based on this point about removing AMP support for GTM would need to be reverted/revised. |
QA Update: ✅Verified:
|
Feature Description
UPDATE 29 Jan 2024: Refer to this comment for decisions taken regarding this issue.
The existing integration between GTM and GA modules in Site Kit only supports UA and since UA has been removed, this integration is no longer functional. #7989 already started an effort to adapt this integration to support GA4.
This issue should be focused on using the datastore infrastructure introduced in #7989 in Tag Manager module components/datastore replacing any usage of the legacy
analytics
module/datastore.To summarise, across the Tag Manager module components/datastore, this should work to:
tagmanager
(get|has)AnalyticsPropertyID*()
selectors with GA4 counterparts.analytics-4
module instead ofanalytics
.analytics-4
measurementID
instead ofanalytics
propertyID
.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
googtag
) which is a valid Google Tag ID, then display the following screen:/wp-admin/admin.php?page=googlesitekit-dashboard&slug=analytics&reAuth=true
).Implementation Brief
Within
assets/js/modules/tagmanager/datastore/versions.js
, create the following new selectors and implement them as below:getLiveContainerGoogleTag
: Similar togetLiveContainerAnalyticsTag
, use thegetLiveContainerVersion
selector and check for theliveContainerVersion?.tag
property. If it exists, return the first tag with a type ofgoogtag
.getLiveContainerGoogleTagID
: Similar togetLiveContainerAnalyticsPropertyID
, check for theparameter
property having a key oftagId
withinanalyticsTag
. If it does, this should be the Google tag ID. If this value is wrapped within{{ }}
brackets, then find a variable (usinggetLiveContainerVariable()
) within the container with this value as the name. Iff this variable is of typec
(constant), then the value of the first parameter should be the Google tag ID. This comment shows the shape of the container data returned. Ignore logic that tried to find the property ID within agaSettings
variable. Validate any of the IDs found usingisValidMeasurementID()
.getCurrentGTMGoogleTagID
: This selector should first fetch theaccountID
andinternalContainerID
saved in Tag Manager settings as done ingetAnalyticsPropertyIDs()
. Pass these values to thegetLiveContainerGoogleTagID
selector created above to fetch the current container's associated Google Tag ID (if any).useGAPropertyIDEffect
). This functionality will be removed as part of Remove functionality preventing GA4 from outputting tags when GTM takes over #8196 and removing this here is out of scope for this issue.Within
assets/js/modules/tagmanager/components/common/FormInstructions.js
:if
blocks that use their values:analyticsModuleAvailable
andanalyticsModuleActive
constants to check for theanalytics-4
module instead of theanalytics
module.getCurrentGTMGoogleTagID
selector saving its result in a constant, saycurrentGTMGoogleTagID
. Modify the legacyif
block whereanalyticsModuleAvailable
is true ANDanalyticsModuleActive
is false. Check if thecurrentGTMGoogleTagID
has a value (instead of checking forsingleAnalyticsPropertyID
). Modify the message as per the AC.isSetup
block which needs no modification.Within
assets/js/modules/tagmanager/components/setup/SetupForm.js
:singleAnalyticsPropertyID
, select thecurrentGTMGoogleTagID
usinggetCurrentGTMGoogleTagID
. Replace its usage when defining theisSetupWithAnalytics
constant.Test Coverage
versions.test.js
.QA Brief
G-ABC12DE34F
in the "Google Tag ID" field. Click "Triggering" and choose any trigger (say All Pages). After adding this new tag, click on "Submit" in the upper right corner, give the version a name and description and click "Publish".{{My Google Tag ID}}
. After saving this tag, go to the "Variables" tab, click on "New" within the "User defined variables" list. Then click "Variable Configuration" -> Constant (under Utlities). The value here should be a valid Google Tag ID likeG-ABC12DE34F
. Now click on "Submit" and publish a new live version. Verify that the messageChangelog entry
The text was updated successfully, but these errors were encountered: