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

[DISCUSS] Begin migration from classic to universal Google Analytics #548

Closed
wants to merge 21 commits into from

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 20, 2015

For discussion, do not merge

  • This PR puts a facade in front of the Google Analytics classic and universal APIs (GOVUK.analytics), so that we can track events, pageviews and dimensions identically in both (eg GOVUK.analytics.trackEvent, GOVUK.analytics.trackPageview, GOVUK.analytics.setDimension).
  • It manages the loading of analytics libraries and configuration of the tracker profiles from a file, rather than a template, avoiding repetition of this code on every page and allowing it to be minified and cached.
  • Slimmer is inserting custom variables into a script tag, while this isn't ideal a choice has been made to leave this as-is for now and tackle separately. To account for Slimmer using only the classic API a shim has been put in place.
  • Any use of the analytics API within apps that hasn't been updated to use GOVUK.analytics should be unaffected, they just won't be tracking to Universal yet. There are stories to update them.
  • Updates to the user satisfaction survey and scroll tracking show how the API can be used.

I'd love to have feedback on all of the above as well as some advice on a couple of the TODOs still left:

fofr added 14 commits Feb 17, 2015
* Create classic and universal analytics trackers that can load their
tracking code, track custom events, track page views – virtual and
otherwise, and set custom dimensions/variables
* Create a GOVUKTracker that let’s us track to both classic and
universal simultaneously
Cleanup method declaration and improve readability.
Avoid pushing to an undefined array by creating that array when the
tracker is created.
When starting a tracker, set custom dimensions before the initial
pageview is tracked.
 A cookie is sometimes set by apps to declare the GA parameters that
should run on the subsequent page. These declare the actual methods to
call in classic analytics. This is a temporary shim to ensure these are
applied to both classic and universal before updating apps.

* Pull cookie code from current analytics snippet
* Modify to go through setDimension rather than directly to GA
* Parse index and scope variables as integers
Analytics and the loading of the libraries is now within a cacheable,
compressible JS file. Avoid repeating this javascript on every page.

* Include a comment indicating that slimmer is still inserting custom
variables
Slimmer inserts classic custom variables into the ga-params script tag,
we need to intercept these and track them in both universal and
classic. (Slimmer is being left alone for the time being)

* Intercept the classic analytics queue _gaq before initialising
analytics
* Use this queue after initialisation to setup custom variables before
the initial pageview
* Move the google_analytics include to load before the application
javascript so the slimmer variables can be distinguished from those
added later
* Compress the javascript within ga-params element
Load analytics libraries, then configure tracker and track first
pageview
Allow events to mark themselves as non-interactive so they do not
affect the page bounce rate.

Currently being used by the scroll tracker.
* Put events through tracker.trackEvent rather than “sendtoAnalytics”
The global ga function might not be available if the analytics script
couldn’t be loaded, or if the user is blocking the script from loading
(eg Adblocking)

* Proxy calls to ga via a `sendToGa` function which tests for the
presence of the global function first.
Rename the tracker object to make usage simpler and easier. Calling
GOVUK.analytics.trackEvent is easier to remember and use.
Switch to using GOVUK.analytics.trackEvent so that events go to both
classic and universal analytics profiles.
sendToAnalytics was compatible with only the classic tracker. Its uses
have already been replaced with GOVUK.analytics.trackEvent (user
satisfaction survey and scroll tracker).
@edds
Copy link
Contributor

@edds edds commented Feb 20, 2015

This is a good move and I generally approve of the approach 👍 Some general comments though:

As mentioned elsewhere it would be really good to change this to use GOVUK.analytics as the base object over GOVUK.Analytics. This would fit better within our naming conventions as you can't create a new instance of GOVUK.Analytics. You can still leave the creation of GOVUK.Analytics = {} in the ga-params script tag till slimmer has been updated to remove its usage of it.

Also rather than using self = this would it be possible to use bind (with the polyfill) to manage your this if this would otherwise get changed.

I might leave some other comments inline.

@alext
Copy link
Contributor

@alext alext commented Feb 20, 2015

Classic analytics formerly set the domain name (for cookies) to .www.gov.uk or document.domain using _setDomainName

I believe this was to force the cookies to be set on www.gov.uk. Without this, I think they get set on gov.uk, and therefore leak across to other service domains etc.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 20, 2015

@alext: Setting the .www.gov.uk bit has been kept, it's the switch that happens if the domain happens to not be www.gov.uk (where it sets the cookie domain as document.domain) that I'm confused about.

@alext
Copy link
Contributor

@alext alext commented Feb 20, 2015

Ah, that'll be so that it works correctly on preview/staging etc.

`Analytics` suggests that it’s a constructor, it’s not.
Objects are typically specific enough to not require a namespace.

* Rather than renaming `Analytics`, which is being used in Slimmer,
remove the namespacing altogether.
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 20, 2015

Spoke to @edds about Analytics namespace. Rather than renaming Analytics I've opted to remove the namespace from the new objects. See b359ad1

GOVUK.Analytics itself can be removed later when Slimmer has been updated. (It should probably become the constructor for GOVUK.analytics)

@tombye
Copy link
Contributor

@tombye tombye commented Feb 20, 2015

This is really nice and feels a lot cleaner already. All I have is a suggestion for the future, that once this has seen some use in live environments we add some deprecation warnings to the existing objects to give the switchover a push. (Something like using console.assert against the old objects).

Switch to GOVUK.analytics.trackEvent.
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 20, 2015

Speaking with @tombye and @edds, there isn't much between having a separate .js file (to prevent something like a parse error from causing tracking to fail) and bundling analytics into application.js. Considering only the users, one less http hit is preferable.

fofr added 3 commits Feb 20, 2015
Allow tracking to work in dev, preview and staging by customising the
cookie domain when it doesn’t match www.gov.uk.
Modules may depend on GOVUK.analytics being present. To ensure this the
analytics module should be loaded first. (eg user satisfaction survey)

This will also trigger the analytics libraries to load sooner, and
reduce the time to initial pageview.
@fofr fofr force-pushed the extracted-ga branch to 7fd17c6 Feb 23, 2015
fofr added 2 commits Feb 23, 2015
There is no equivalent of `setAutoLink` in Universal. The stubbed
functions, added to ensure equivalence with classic, can be removed.

* Remove stub function
@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 23, 2015

@edds I've updated the tracker to use bind rather than self, and the code is more readable for it.

It has been agreed with the analytics team that we don't need an equivalent to setAutoLink within Universal. This was the last TODO that needed work. I'm closing this discussion and opening a new PR.

@fofr fofr closed this Feb 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.