Google Analytics 4 #570

gpsamson · 2021-03-09T06:53:51Z

What does this PR do?
This pull request introduces a new Google Analytics 4 to service the new GA4 property type which varies greatly from Universal property types. It includes functionality up to P2 in the work breakdown.

The complete spec can be found in the GA4 A.js Destination SDD.

Are there breaking changes in this PR?
No

Testing

Testing completed successfully in stage by manually sending events.
Unit tests complete

Detailed testing can be found here: https://paper.dropbox.com/doc/GA4-A.js-Test-Plan--BHKEytO0krCgb7Rq0ETLY6TBAg-hIeXHKehkOvlhqONgOGdV

Does this require a new integration setting? If so, please explain how the new setting works
A completely new definition can be found in Partner Portal for google-analytics-4

Links to helpful docs and other external resources
https://developers.google.com/analytics/devguides/collection/ga4/tag-guide is heavily referenced throughout the code.

briemcnally · 2021-03-10T23:30:47Z

integrations/google-analytics-4/lib/index.js

+  for (var i = 0; i < measurementIds.length; i++) {
+    config.push(['config', measurementIds[i]]);
+  }
+
+  // Disable Page View Measurement
+  // https://developers.google.com/analytics/devguides/collection/ga4/disable-page-view
+  if (opts.disablePageViewMeasurement) {
+    for (var i = 0; i < measurementIds.length; i++) {
+      config.push([
+        'config',
+        measurementIds[i],
+        {
+          send_page_view: false
+        }
+      ]);
+    }
+  }


It looks like we add the GA properties then if they have another setting enabled ie disablePageViewMeasurement push the same properties to the config with with send_page_view: false Curious if this causes any dupes? Looks like config is just an array. I.e. should we have

config.push([ 'config', measurementIds[i], { allParamtersHere} ])

It could need to be separate for a reason I am not aware, just checking 😄

Not sure if this would cause dupes. I think it would be cleaner to push the properties once, so I can make that change.

Thank you so much for calling this out! Seems when we call config multiple times it may overwrite others 🤔.

integrations/google-analytics-4/lib/index.js

brennan · 2021-03-11T00:18:37Z

integrations/google-analytics-4/lib/index.js

+  var props = page.properties();
+  var name = page.fullName();
+
+  // var language = ;


Not sure if we have or are working on a plan for language? I see in the SDD that Segment doesn't have it specced for a.js events.

I don't think it is part of the Segment spec which is why I left it commented. It is mapped in the SDD but I don't think it's crucial for the pilot version. I can remove for now and re-add if there are asks for it!

briemcnally · 2021-03-18T15:53:35Z

integrations/google-analytics-4/lib/index.js

+    cookie_expires: opts.cookieExpiration,
+  };
+
+  var sets = [


Should this be an array of objects? A little bit confused by the different format of Cookie Flags and the rest of them. Could be missing something

gtag.js has a set command and it's really confusing. Sometimes it accepts a single object argument (L1) and sometimes it accepts multiple arguments (L2):

window.gtag('set', { cookie_flags: opts.cookieFlags }) window.gtag('set', 'allow_google_signals', !opts.disableAllAdvertisingFeatures)

The sets var we have is an array of arrays to represent a list of set commands and the arguments to pass.

Gotcha makes sense. Thanks for clarifying

briemcnally · 2021-03-18T15:54:43Z

integrations/google-analytics-4/lib/index.js

+     * can be appended as part of the user_properties object instead of being configured by
+     * an explicit command.
+     * https://developers.google.com/analytics/devguides/collection/ga4/cookies-user-id#set_user_id
+     */


Small nit: should we move the comment to L to be aligned

integrations/google-analytics-4/lib/index.js

gpsamson · 2021-03-22T17:22:47Z

@brennan @briemcnally - Jen and I went through the test plans on Friday and all functionality is working as expected. There may be changes to the setting names today, but not a drastic change in behavior.

Thanks for the reviews so far!

briemcnally

LGTM! Nice work @gpsamson

brennan · 2021-03-24T18:05:30Z

integrations/google-analytics-4/lib/index.js

+    return;
+  }
+
+  // Track categorized pages setting needed here?


Have we been able to look into this? :)

I ran it by Jen but no response yet. I think we can leave it out for now and add it if customers request it. What do you think?

Jen recommended we omit it until this functionality is requested.

brennan · 2021-03-24T18:07:14Z

integrations/google-analytics-4/lib/index.js

+  var mappings = this.options.customEventsAndParameters;
+
+  for (var i = 0; i < mappings.length; i++) {
+    var mapping = mappings[i]; // Type check for object?


Wondering per the comment if we should put a type check for object here?

brennan · 2021-03-24T18:08:47Z

integrations/google-analytics-4/lib/index.js

+      continue;
+    }
+
+    var parameterMappings = mapping.parameters || []; // Type check for array?


Also curious about this comment re: a type check for array. Is that something we should add?

brennan · 2021-03-24T18:09:48Z

integrations/google-analytics-4/lib/index.js

+    // eg; [{ key: 'properties.genre', value: 'primary_genre }]
+    //
+    for (var j = 0; j < parameterMappings.length; j++) {
+      var map = parameterMappings[j] || {}; // Type check for object?


Another type-check comment here as well.

@gpsamson Not a big deal, but looks like we still have a type-check comment here.

Oops, thx! I will remove it prior to merging.

brennan · 2021-03-24T18:11:40Z

integrations/google-analytics-4/lib/index.js

+      parameters[param] = value;
+    }
+
+    window.gtag('event', googleEvent, reject(parameters));


Wondering if we need reject here given line 292 above - if map. value is falsy, then we don't add the key:value pair to the parameters map in the first place.

Good call - it's pretty redundant!

…grations into ga4

gpsamson · 2021-03-24T21:06:56Z

Thanks for the review @brennan @briemcnally :tribe:

I downgraded the version to 0.0.1 since this will be in private beta for a few weeks and we may make changes that break existing behavior (eg; default mappings).

dobesv · 2021-09-29T18:47:37Z

Is there any process or timeline for this to be published to npm?

Gabriel P Samson added 2 commits March 8, 2021 22:53

Initial GA4 Commit

60fe963

Add tests, Fix bugs

e105d3f

gpsamson marked this pull request as ready for review March 10, 2021 21:29

gpsamson requested review from brennan, briemcnally and mericsson March 10, 2021 21:37

briemcnally reviewed Mar 10, 2021

View reviewed changes

integrations/google-analytics-4/lib/index.js Show resolved Hide resolved

brennan reviewed Mar 11, 2021

View reviewed changes

Gabriel P Samson added 3 commits March 18, 2021 00:55

Update config and set calls

a95db2b

Remove commented language var

fcd55b8

Remove reject from group handler

5b7f617

briemcnally reviewed Mar 18, 2021

View reviewed changes

integrations/google-analytics-4/lib/index.js Show resolved Hide resolved

Fix comments and add complex setting example

b957450

gpsamson requested review from brennan and briemcnally March 22, 2021 17:21

Update setting keys

1502749

briemcnally approved these changes Mar 24, 2021

View reviewed changes

brennan reviewed Mar 24, 2021

View reviewed changes

Gabriel P Samson added 2 commits March 24, 2021 12:48

Merge branch 'master' of ssh://github.com/segmentio/analytics.js-inte…

0a2b74e

…grations into ga4

Iterate on feedback

b280ceb

gpsamson requested a review from brennan March 24, 2021 20:05

brennan approved these changes Mar 24, 2021

View reviewed changes

Gabriel P Samson added 2 commits March 24, 2021 13:50

Remove stray comment

771ff7e

Update version to 0.0.1

b5cdb10

gpsamson merged commit f3acb12 into master Mar 24, 2021

Google Analytics 4 #570

Google Analytics 4 #570

Uh oh!

Conversation

gpsamson commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briemcnally Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpsamson Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gpsamson commented Mar 22, 2021

Uh oh!

briemcnally left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpsamson commented Mar 24, 2021

Uh oh!

dobesv commented Sep 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gpsamson commented Mar 9, 2021 •

edited

Loading

briemcnally Mar 10, 2021 •

edited

Loading

gpsamson Mar 18, 2021 •

edited

Loading