964893 - Create reusable module for doing GA Events in Webmaker apps

Status: RESOLVED FIXED

(Webmaker Graveyard :: Metrics, defect)

Description

[David Humphrey (:humph)]

Attachment: https://github.com/mozilla/webmaker-analytics/pull/1

Adam and I discussed what he wants in the way of custom event tracking for GA in our apps, see https://etherpad.mozilla.org/webmaker-analytics.

We will want to do this all over the place, so we opted for a separate front-end module people can reuse.  We also want to hide the details of how GA works, since they are going to be switching their API soon.

I've taken a stab at implementing what we discussed.

Comment 1

[Jon Buckley [:jbuck]]

Here's a question, if the idea is to make life easier on us to migrate to the new Universal Analytics API, why not make this a shim that adds something like the following:

if (!window.ga) {
  window.ga = function() {
    _gaq.push(arguments); // won't work e
  };
}

Then once we upgrade to the new UA API the shim won't be loaded anymore and it's like we were never using it!

Thoughts?

Comment 2

[Adam Lofting (:adamlofting)]

:humph, awesome work and thanks for spotting the non-interaction bits I'd overlooked in the spec.

Feedback:

1) Rather than skipping email addresses, I'd like to filter out the value but keep the event so we can see in the data if this is happening often and check we're not losing interaction stats. 

Can we replace these values with: "REDACTED (Potential Email Address)"? 

That should be clear in the reporting interface what has happened.

2) :jbuck this is a great idea, and copes with multiple sites using either version of GA as we migrate.

FWIW, there is a potential use case where people run both versions of the GA API simultaneously during the migration (this has been common while universal analytics is in beta and upgrading has broken backwards compatibility with historic data).

I think the best solution is to fire the event at both the new (ga) and old (_gaq) versions simultaneously, but I'm not clear on the syntax to push events to the ga function when it possible this code might get called before the ga function is loaded?

P.s. As a relative amateur, this code is beautiful and I'll learn what I can from the module structure and test setup. I want to get better at this.

Comment 3

[David Humphrey (:humph)]

I've done another pass at this based on the feedback above, including:

* I now redact instead of skip email-like strings in the action and label args

* I've added dual reporting to both the old style _gaq ga.js API, and also to the new ga() analytics stuff, see https://developers.google.com/analytics/devguides/collection/analyticsjs/events.  I've added new tests as well.

Based on my reading of the new analytics stuff, the ga() function should be available, as it's placed in the head now.  However, my script won't fail if it's not loaded yet.  I couldn't find a proper event to listen to in order to know if it's safe to call or not--there might be one?

This should allow us to migrate our GA from the old to new stuff painlessly, so great suggestion.

Thanks for the review, Adam.  If you could take a look at my PR again for the changes, and then in the details for the attachment, you can mark the attachment r+ (review passed, looks good) or r- (review failed, please fix issues x, y, z) depending on what you want me to do.

Comment 4

[Jon Buckley [:jbuck]]

Comment on attachment 8366784 [details] [review]
https://github.com/mozilla/webmaker-analytics/pull/1

Looks good to me, my only nit would be you could add qunit to bower devDependencies.

Comment 5

[David Humphrey (:humph)]

I've switched to using bower for qunit, updates in PR.

Comment 6

[Adam Lofting (:adamlofting)]

Looks great to me, and I think I've correctly r+'d this now.

Comment 7

[David Humphrey (:humph)]

I've registered this as a bower component, fixed up some small install ordering for the travis build, and tagged version 0.0.1.

Closing.  My next task is to use this in Thimble as a test.