Update to Google Analytics 4 #1839
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1839 +/- ##
==========================================
+ Coverage 67.81% 67.82% +0.01%
==========================================
Files 406 407 +1
Lines 24434 24459 +25
Branches 3681 3685 +4
==========================================
+ Hits 16569 16589 +20
- Misses 7304 7309 +5
Partials 561 561
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I assume you've also made the required adjustments inside Google Tag Manager to pass the data through to GA4?
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Nateowami)
|
@nigel-wells I have never understood what Google Tag Manager is, despite seeing it referenced everywhere in relation to Google Analytics. I'm pretty sure it's configured correctly. Can we look at it together and make sure before merging? |
There was a problem hiding this comment.
I've talked this over with Mieke, she says the code you have will send the data directly to Google Analytics without the need to setup anything specific inside of Google Tag Manager. You could proceed with what you have in this PR and it will work. What you would lose is the ability to compare data going into both Google Analytics 4 and Google Universal Analytics which you could do if you were using Google Tag Manager containers.
At some point it would be good to get a Google Tag Manager container setup as then other events and triggers could be setup for tracking specific things in the app. Did you want to go through the process of getting the base of that setup now as part of this change or do later? Mieke could certainly help with the setup if we can get permission on your account to manage it.
Happy to chat things through.
Reviewable status:
Nateowami
force-pushed
the
update/ga-4
branch
6 times, most recently
from
7c3f77b to
bd0b2b4
Compare
|
I somehow missed that we configure Google Analytics in a number of places. I've updated the other locations, and added logic to sanitize URLs before they're sent, as we've already been doing with Bugsnag. |
Nateowami
dismissed
nigel-wells’s stale review
PR has changed a ton, so dismissing review
There was a problem hiding this comment.
Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/index.html line 4 at r2 (raw file):
<html lang="en"> <head> <script async src="https://www.googletagmanager.com/gtag/js?id=G-SVKBDV7K3Q"></script>
You'll want to remove the tag from here - I understand it can be set later using the config command.
src/SIL.XForge.Scripture/ClientApp/src/index.html line 10 at r2 (raw file):
dataLayer.push(arguments); } gtag("js", new Date());
Apparently this may also trigger a page view and should be reserved for being set when you're ready to trigger something along with the config.
src/SIL.XForge.Scripture/ClientApp/src/index.html line 12 at r2 (raw file):
gtag("js", new Date()); gtag("config", "G-SVKBDV7K3Q");
You will want to remove this as this as the ID needs to come from the environment config plus I "think" it will trigger a page view. Essentially you want this specific config command to only be called once but I suspect, with the minimal tag manager configuration, it will also automatically trigger a page view.Bas
src/SIL.XForge.Scripture/ClientApp/src/app/app.component.ts line 150 at r2 (raw file):
); this.subscribe(navEndEvent$, e => { if (this.isAppOnline) {
This check is already part of the new AnalyticsService so no need to have it here
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.ts line 18 at r2 (raw file):
}) export class AnalyticsService { constructor(private readonly pwaService: PwaService) {}
Based on comments for index.html you may need to change part of this to ensure the config is set (and may the date?) but only on first logging. I'm not 100% (due to GTM setup) if setting that will in itself trigger a page view meaning you may not then need to trigger the alternative page_view event.
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.ts line 27 at r2 (raw file):
logNavigation(url: string): void { const sanitizedUrl = sanitizeUrl(url); this.logEvent('page_view', { page_path: sanitizedUrl });
The command is event and the type is page_view. You can then set params as seen here: https://developers.google.com/tag-platform/gtagjs/reference/events#page_view
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.ts line 30 at r2 (raw file):
} private logEvent(eventName: string, eventParams: EventParams): void {
I'd stay away from event unless you're dealing specifically with a Google Analytics "event". Google's docs refers to the first param as a "command" followed up with the tag and additional parameters. We may want to specifically trigger a real "event" at some point in the future - so I'd prefer we reserved wording for that situation.
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.ts line 40 at r2 (raw file):
// redact access token from the hash function redactAccessToken(url: string): string {
Perhaps we should put all these methods in its own utils file. Perhaps have a service we can call with one method and multiple ENUM options to pass with a string as to what needs to be redacted? This would save us having to remember all the different method names. It may not be obvious to look for bugsnag redact code in the analytics service.
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.spec.ts line 10 at r2 (raw file):
it('should redact the join key from URL', () => { const url = 'https://example.com/join/123';
Also run a test (or change to) for URLs that have the language code at the end i.e. /join/123/en
src/SIL.XForge.Scripture/Pages/Shared/_Layout.cshtml line 18 at r2 (raw file):
gtag('js', new Date()); gtag('config', 'G-SVKBDV7K3Q');
Can this come from an environment variable as well?
There was a problem hiding this comment.
Did you want to pick this one back up again @Nateowami ? It would be good to get it across as GA4 is well underway now and I'm not sure how much longer before UA is dropped.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Nateowami)
There was a problem hiding this comment.
That would be great. Thanks.
And coming back to Reviewable, I see I had a draft response that may still be relevant:
I think I finally figured out where the gtag documentation is: https://developers.google.com/analytics/devguides/collection/ga4?hl=en
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @nigel-wells)
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/analytics.service.ts line 30 at r2 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
I'd stay away from
eventunless you're dealing specifically with a Google Analytics "event". Google's docs refers to the first param as a "command" followed up with the tag and additional parameters. We may want to specifically trigger a real "event" at some point in the future - so I'd prefer we reserved wording for that situation.
Ah, didn't know that. So maybe use the "command" terminology?
src/SIL.XForge.Scripture/Pages/Shared/_Layout.cshtml line 18 at r2 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
Can this come from an environment variable as well?
Probably, but it didn't look like it would be trivial.
There was a problem hiding this comment.
Would you be happy for me to run through making these updates as you're going to be out of action for a bit? You can then review. I'll just stick to what we have here rather than introducing any other special events or tracking as that can be added later on.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Nateowami)
|
Yes, that would be great. |
|
I have this mostly working now with data flowing through to GA4. It is only triggering, however, on first page load. I suspect this is a setting that needs to be tweaked in the Google Tag Manager instance to work better with SPA instances. Am I able to get permission to view the settings in there as well please? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1839 +/- ##
=======================================
Coverage 78.31% 78.31%
=======================================
Files 517 518 +1
Lines 29710 29735 +25
Branches 4849 4831 -18
=======================================
+ Hits 23268 23288 +20
- Misses 5704 5709 +5
Partials 738 738 ☔ View full report in Codecov by Sentry. |
|
I've taken another look at this and I see I can access the Google Tag Manager screen for the account which I didn't think I had access to. I'll take another look at this setup and will come back to you with next steps. |
This change was copied verbatim from the Google Analytics interface.
This change is