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

Prevent analytics-4 from outputting tags when GTM takes over #7991

Closed
1 task
nfmohit opened this issue Dec 15, 2023 · 4 comments
Closed
1 task

Prevent analytics-4 from outputting tags when GTM takes over #7991

nfmohit opened this issue Dec 15, 2023 · 4 comments
Labels
Module: Tag Manager Google Tag Manager module related issues P2 Low priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Dec 15, 2023

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. A key part of this integration is that it used to prevent the analytics module from placing its own tags if a GTM tag was placed that had the relevant GA propertyID in its container. This was made possible by using the tagmanager gaPropertyID and analytics canUseSnippet module settings.

Even though placing both GA4 and GTM tags does not cause a duplicate tracking issue, this behaviour should be adapted to work with GA4 so that an unnecessary tag isn't placed on the page.

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.

#7923 already adds the canUseSnppet module setting to analytics-4.

Briefly, the following should be done:

  1. The tagmanager gaPropertyID module setting should be changed to gaMeasurementID.
  2. Existing logic to populate this setting (e.g. the useGAPropertyIDEffect hook) should be renamed and updated to populate the new setting with the single GA4 measurement ID (datastore infrastructure already being implemented in #7989).
  3. analytics hooks used in the Tag_Manager class should be copied over from the analytics module to the analytics-4 module (if not done already in previous issues), with the same hook name as they should function in the same way.
  4. Callback functions for analytics hooks in the Tag_Manager class should use the new gaMeasurementID module setting instead of gaPropertyID.
  5. Any other part of the codebase that refers to the tagmanager gaPropertyID module setting should refer to gaMeasurementID instead.

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

Acceptance criteria

  • The Google Analytics snippet should not be added by Site Kit:
    • If GA4 is connected with a measurement ID AND
    • If Tag Manager is connected and the current container contains a tag that matches the measurement ID set above.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature Module: Tag Manager Google Tag Manager module related issues labels Dec 15, 2023
@jimmymadon jimmymadon self-assigned this Dec 18, 2023
@jimmymadon jimmymadon removed their assignment Jan 3, 2024
@mxbclang mxbclang added the P1 Medium priority label Jan 3, 2024
@techanvil techanvil self-assigned this Jan 4, 2024
@techanvil
Copy link
Collaborator

AC ✅

@techanvil techanvil removed their assignment Jan 4, 2024
@jimmymadon jimmymadon self-assigned this Jan 4, 2024
@mxbclang mxbclang added the Next Up Issues to prioritize for definition label Jan 5, 2024
@aaemnnosttv
Copy link
Collaborator

@nfmohit @jimmymadon I'm not sure that this issue is necessary, so please hold until we confirm.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 12, 2024

@nfmohit @jimmymadon I'm not sure that this issue is necessary, so please hold until we confirm.

Thank you for pointing that out, @aaemnnosttv. Jimmy and I are aware that this issue may not be needed at all as #7990 should already implement this behaviour. For clarity, I have updated this issue to be dependent on #7990 as that will dictate if this issue is necessary at all, but this will most likely be closed.

Let me know what you think, thanks!

CC: @bethanylang @ivonac4 (for visibility as the dependency has been updated - this now depends on #7990 instead of #7989).

@mxbclang mxbclang added P2 Low priority and removed P1 Medium priority labels Jan 12, 2024
@ivonac4 ivonac4 added the QA: Eng Requires specialized QA by an engineer label Jan 16, 2024
@mxbclang mxbclang removed the Next Up Issues to prioritize for definition label Jan 19, 2024
@jimmymadon
Copy link
Collaborator

As per Point number 2 in this comment, it is no longer necessary (and potentially detrimental to our new custom dimension tracking) to disable outputting the GA4 snippet when GTM takes over. So closing this issue and created #8196 which will carefully remove this legacy functionality which worked for UA.

@jimmymadon jimmymadon closed this as not planned Won't fix, can't repro, duplicate, stale Jan 29, 2024
@jimmymadon jimmymadon removed their assignment Jan 29, 2024
@mxbclang mxbclang changed the title [GTM + GA4] Prevent analytics-4 from outputting tags when GTM takes over Prevent analytics-4 from outputting tags when GTM takes over Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Tag Manager Google Tag Manager module related issues P2 Low priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants