-
Notifications
You must be signed in to change notification settings - Fork 286
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 tagmanager
datastore partial to determine connected GA4 entities
#7989
Comments
@jimmymadon I wasn't aware of this, but it's not entirely surprising since GA4 support for AMP was added sometime around June, right before the start of the UA sunset. I think we'll need to ask internally if support will not be coming for it, otherwise I think we should keep that part but we'd only check for the tag in web containers, does that make sense? Essentially trying to keep it the same as today.
I believe this was possible with UA as well but we only check for one, correct? This is similar to existing tags where we only "capture"/report about the first one we find. Having multiple GA tags in GTM should be an edge case and trying to account for it would likely be a large lift that probably isn't worth it.
We should only be concerned with the Google Tag. The event tag is for custom events that are set up, and managed in GTM. |
Hey @jimmymadon, the AC here LGTM. ✅ The IB also looks good; there's just one aspect that could use clarification. Considering Providing full support for variable use would clearly be out of scope, but it might be worth adding support for constant variables which seem to be the most straightforward case. Alternatively, we could defer variable support and explore it more fully in a separate issue. Either way, it would be useful to make it clear what approach to take in the IB. What do you think? Additionally, a minor point - there are a couple of typos in the IB with |
@techanvil My apologies - I had just assumed that fetching the We can set Google Tag configuration settings but these do not allow to set a So should we drop support for variables all together? (UPDATE: I have updated the AC and IB to reflect this decision for now.) |
Hey @jimmymadon, while we can't use So, I was thinking we might want to provide support for constant variables and then review other potential cases later... Or, as mentioned simply defer the whole topic. But considering we do currently provide support for variables, it does feel like it would be nice to retain some support in the initial migration. |
@techanvil I did try this out too but was just wondering how we would "support" this. There is no indication that the constant variable above would be a measurement ID (even if we can make out from the format of the string). I don't think we can simply assume that this string, even if it is a measurement ID, is one that needs to be tracked within this GTM container. I don't think that using variables to store the measurement ID is prescribed anywhere by GTM and is good practise (we have a Google Tag for this precise reason). The reason we already have variable support is purely because the "Google Analytics Settings" variable would always have a property ID set and we would be certain about its existence there. So IMO, dropping variable support here fully is potentially the right way to proceed unless we want to capture any other data from variables in the future. |
Thanks @jimmymadon. I do think that it would be straightforward to support in the simple case where the variable reference is specified as the I don't think there's necessarily a question of good practice here, the user-defined variables are there to support specific user requirements - I think using a variable to share a measurement ID across multiple tags would be a pretty legitimate use case, as, for example both Google Tag and GA4 Event tags have a measurement ID field. However, I do take your point that there can be more complex uses e.g. multiple variables, or the variable itself can point to other variables. Given it's a bit of an arbitrary isolated case to support, let's go ahead and drop the variable support for now, we can take a more comprehensive look for a future iteration. It is, though, not correct to say that it's no longer possible to set the measurement ID via a variable - as mentioned I tested out setting it via a constant and it worked fine, correctly tracking events, so please can you tweak the AC accordingly? I'll send it back to AC for the purpose but the IB looks good to go. |
Thanks for the update @jimmymadon! As discussed on Slack we've gone ahead with adding support for variables of type constant. AC ✅ |
Hi @jimmymadon, apologies but while reviewing #7990 I had the realisation that we haven't quite got the IB right for the implementation of Where I've steered us slightly wrong is talking about the Here's an updated set of screenshots to illustrate this: So, the implication here is that we actually need to do an additional container lookup in order to determine the measurement ID from the Google tag ID. We probably want to determine the measurement ID from the container using similar logic to that used in Note that I've screenshotted the using-a-variable scenario, but even without a variable we'd still need to do a container lookup. Please can you update the IB accordingly, and drop me a line if you want to discuss this further? |
@jimmymadon This is going to be scheduled for Sprint 120 – can you please prioritize making these IB updates? Thanks! |
As summarised in Slack Thread 1 and Slack Thread 2, it is not possible to have an exact 1:1 mapping between selectors mentioned in the AC of this issue. The selectors being adapted here were originally planned to be used in #7990 which attempted to adapt the following two UA-GTM integration screens that were shown to the user when setting up Google Tag Manager:
So in summary:
|
tagmanager
datastore partial to determine connected GA4 entitiestagmanager
datastore partial to determine connected GA4 entities
Feature Description
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. As an effort to adapt this integration to work with GA4, a new partial should be introduced in the
tagmanager
datastore that contains GA4 versions of thetagmanager
selectors that are relevant to this integration.One important point to note is that while the legacy integration checks for the UA property ID as the in the GTM containers, in case of GA4, the container actually keeps track of the GA4 measurement ID as form of a
googtag
. Some of the selectors that should be copied and adapted from the existing integration:getAnalyticsPropertyIDs
->getAnalyticsMeasurementIDs
getLiveContainerAnalyticsPropertyID
->getLiveContainerAnalyticsMeasurementID
getLiveContainerAnalyticsTag
-> renamed to whatever is appropriate according to the implementation.getSingleAnalyticsPropertyID
->getSingleAnalyticsMeasurementID
hasAnyAnalyticsPropertyID
->hasAnyAnalyticsMeasurementID
hasMultipleAnalyticsPropertyIDs
->hasMultipleAnalyticsMeasurementIDs
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
getLiveContainerAnalyticsTag
->getLiveContainerAnalytics4Tag
: Adapt this selector so that the tag with typegoogtag
is returned (instead of returningua
orua_amp
tags).getLiveContainerAnalyticsPropertyID
->getLiveContainerAnalytics4MeasurementID
: Adapt this selector so that the measurement ID is returned from the linked Google Tag (GA4 Tag). For UA, apropertyID
(tracking ID) could be set as agaSettings
variable but it is no longer possible to set themeasurementID
via a "Google Tag: Configuration Settings" variable. However, we should still support setting the Google Tag ID via a "Constant" variable as per this comment.getAnalyticsPropertyIDs
->getAnalytics4MeasurementIDs
: Adapt this selector to return an array of measurement IDs. However, since AMP is not supported yet, this selector will only ever return at most one measurement ID for the web container only. However, let us keep the logic to fetch theinternalAMPContainerID
and the corresponding measurement ID intact for future compatibility purposes as per this comment.getSingleAnalyticsPropertyID
->getSingleAnalytics4MeasurementID
: Adapt this selector to return the sole measurement ID thatgetAnalytics4MeasurementIDs
above will return.hasAnyAnalyticsPropertyID
->hasAnyAnalytics4MeasurementID
: Adapt this selector to return if a container has a measurement ID.hasMultipleAnalyticsPropertyIDs
->hasMultipleAnalytics4MeasurementIDs
: Adapt this selector to return if multiple measurement IDs are returned for a container. This would always be false (or undefined) since we will not be supporting AMP containers for now.Implementation Brief
Within
assets/js/modules/tagmanager/datastore/versions.js
, create new selectors named as per the AC and implement them as follows:getLiveContainerAnalytics4Tag
: Similar togetLiveContainerAnalyticsTag
, use thegetLiveContainerVersion
selector and check for theliveContainerVersion?.tag
property. If it exists, return the tag with a type ofgoogtag
.getLiveContainerAnalytics4MeasurementID
:getLiveContainerAnalyticsPropertyID
, 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.isValidMeasurementID()
.getAnalytics4MeasurementIDs
: Similar togetAnalyticsPropertyIDs
, keep all logic the same but usegetLiveContainerAnalytics4MeasurementID
instead ofgetLiveContainerAnalyticsPropertyID
. The second part of this selector which checks for AMP container won't ever return ameasurementID
since there isn't a possibility to create a GA4googtag
for containers that have theamp
context as yet. However, for future compatibility purposes, let us keep this logic intact as per this comment.getAnalytics4MeasurementIDs
selector instead ofgetAnalyticsPropertyIDs
.Test Coverage
versions.test.js
.QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: