Closed Bug 1431412 Opened 6 years ago Closed 6 years ago

Analytics Requirements for SUMO

Categories

(support.mozilla.org :: General, enhancement)

Product:
support.mozilla.org
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: pgerman, Assigned: jpetto, Mentored)

Details

Attachments

(1 file)

Screen Shot 2018-02-16 at 2.14.55 PM.png
65.97 KB, image/png
Details 
I've attached the analytics requirements that AP put together as a result of discussions late last year.

https://drive.google.com/open?id=0B-vqNw0Uhi8Rb056dlpmbWdlNjhIU0VjekstUDYzcFZEamRJ
Hi Ben,

Has this been assigned to a developer? Please NI wes contreras and garethc if you have any questions.

Thanks,

PG
Flags: needinfo?(bsternthal)
Yep, I sent you the details via email, but thanks for pinging me here to add info.

Jon Petto is working on it.
Assignee: nobody → jon
Flags: needinfo?(bsternthal)
Wes -

A couple questions on core tracking implementation:

1) Can we leverage the same DNT-abiding loading technique used on mozilla.org[1]? If so, can the exact same JS be used (with a unique GTM_CONTAINER_ID), or do we need to make changes? (I see the googletagmanager.com URL in the PDF is slightly different than used on mozilla.org.)

2) Do any changes need to be made the the existing GA scripts/calls made on the site?

3) In terms of logging 'product_name', my intent is to simply send the value in the URL after /products/ and before the next /, e.g. 'firefox-rocket', 'firefox-fire-tv', 'mobile', etc. Any page without /products/ in the URL (or the root /products/ URL) would result in an empty string being sent for 'product_name'. Do you see any issues with this approach? It would be simple to remove the dashes if necessary - e.g. 'firefox-rocket' becomes 'firefox rocket'.


For the click handler implementation...

1) The gtag function specified on page 7 of the PDF is the same function defined in the core tracking code sample, correct? In other words, this function is defined in the core tracking code and is then used throughout the site for click handlers.

2) Would we (Mozilla devs) be responsible for adding data-* attributes throughout the site, or would the GTM JS add these attributes? (I'm guessing the former, but one can hope. :)


Thanks!

[1] https://github.com/mozilla/bedrock/blob/master/media/js/base/gtm-snippet.js#L13-L17
Flags: needinfo?(wes.contreras)
Core tracking:

1) I'm not familiar with the specific technique you're using on mozilla.org, but the basic technique will probably work just fine. It is definitely a different JS call, though, so we do not want to use the exact same code. SUMO is using gtag.js as a parent tag, but is not using the full GTM product, so the implementation will be noticeably different than www.

2) This will entirely replace existing GA code on the SUMO site. 

3) That seems entirely reasonable to me.


Click handler:

1) Yes, same function, different arguments. The goal is to give it a wrapper function, though, so that only the wrapper function gets called throughout the site, and gtag() only gets called from within the wrapper function. That way, if we decide to change the implementation later (say, adding a universal custom dimension or even implementing full GTM), we can update only the wrapper function rather than changing many different touchpoints throughout the site. 

2) Mozilla devs will need to add the data-* attributes throughout the site, since GTM will not be in use. It's a bit more manual, but hopefully the data-* attributes are at least easier than custom JS everywhere.
Flags: needinfo?(wes.contreras)
(In reply to Wes Contreras from comment #4)
> Click handler:
> 
> 1) Yes, same function, different arguments. The goal is to give it a wrapper
> function, though, so that only the wrapper function gets called throughout
> the site, and gtag() only gets called from within the wrapper function. That
> way, if we decide to change the implementation later (say, adding a
> universal custom dimension or even implementing full GTM), we can update
> only the wrapper function rather than changing many different touchpoints
> throughout the site.

Can you explain what you mean by 'wrapper function' above? I'm guessing we should create a function (by any name?) that will be called by all click handlers on the site. Initially (and potentially always), this function would simply be a proxy to the gtag function. Something like:

// gtag function as defined on page 5 of the PDF
function gtag() {
    dataLayer.push(arguments);
}

// function to be called by all click handlers
function gtag_wrapper() {
    window.gtag(arguments);
}

Is this correct?
Flags: needinfo?(wes.contreras)
I misspoke a bit. In this case, since the site dev piece is only data-* elements, it's not really a wrapper function - just a single click handler in one place. Same intent, though - gtag() should be called only from the central code (once for the base tag, once for the single click handler), while all the many touchpoints throughout the site are just data-* attributes instead of JS at all. 

Apologies for the confusion.
Flags: needinfo?(wes.contreras)
Wes, Jon finished the initial js and I am going to help with the tagging and implementing. A couple questions came up I need some advice on.

1. "Read" Tracking. There is some code that exists to fire events after a set time period.
 
Should this be preserved or removed. If it is preserved can you advise on the JS we should use.

Examples:
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/wiki.metrics.js#L47
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/analytics.js#L88

2. Convenience methods trackEvent & trackPageview
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/analytics.js#L109

There are two convenience methods that are used in a lot of places...

https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/questions.metrics.js


Should this collection be removed or do we need to re-implement. If these need to be re-implemented can you provide the code or an example? Perhaps we just need an updated format for:

'_gaq.push(['_trackEvent', category, action, value]);'

Feel free to reach out if you have additional questions or need clarification outside of bugzilla.
Flags: needinfo?(wes.contreras)
1. The time delay events are not part of the current requirements. Maybe keep the code handy in case they want to add it back in later, but it can be removed for now. 

2. Can you point me to where the current trackEvent function is defined? With that, I should be able to provide a code sample.

If there are cases where trackEvent is being called on a simple click event, we can replace that with the new data- attribute driven mechanism. In cases where it's being called inside of another event handler, like this one, we can probably update the trackEvent definition to work with the new tagging schema.
Flags: needinfo?(wes.contreras)
Wes, thanks for the help here:

Location of trackEvent & trackPageview 
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/analytics.js#L109

Examples of implementation
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/users.js#L34
https://github.com/mozilla/kitsune/blob/master/kitsune/sumo/static/sumo/js/questions.metrics.js#L42

There is a lot of JS for tracking events throughout the code.. a LOT more than I expected :)
This would be a good replacement function (extends the argument signature by one value, but stays compatible with legacy usage):

function trackEvent(category, action, value, label) {
  if (gtag) {
    gtag('event', action, {
      'event_category': category,
      'value': value,
      'event_label': label
    });
  }
}

Then we could implement a basic click handler like:

jQuery('[data-event-category]').on('mousedown', function(evt) {
  trackEvent(
    evt.target.attr('data-event-category'), 
    evt.target.attr('data-event-action'), 
    evt.target.attr('data-event-value'), 
    evt.target.attr('data-event-value')
  );
});

Then we'd run through the spec document and make sure that all of the events noted there get the appropriate data- attributes added, and that any trackEvent call gets removed (to avoid double-tagging). 

If there are some extra trackEvent calls on other events, not covered by the spec, that should just result in some extra event coverage.
Thanks... what about trackPageview?
Flags: needinfo?(wes.contreras)
I'm not aware of any components on the site that should warrant using it. If it's being called anywhere, let me know and we can address it - otherwise, we should be able to do away with it.
Flags: needinfo?(wes.contreras)
Good point... looks like it is just being used in instant search so I can remove...

Lots of cleanup but I think we should be good to go.

Aiming to complete this week.
Ok one last question.

It looks like a few custom vars are set for the purposes of site dashboards.

The data for this lives in a body attribute, for example:

data-ga-push="[["_setCustomVar", 1,"User Type", "Registered", 1]]"

The old code used to set this is:

    var extraPush = $('body').data('ga-push');
    if (extraPush && extraPush.length) {
      for (var i = 0, l = extraPush.length; i < l; i++) {
        _gaq.push(extraPush[i]);
      }
    }

I am thinking the new code will look like 

if (window.gtag) {
    var extraPush = $('body').data('ga-push');
    if (extraPush && extraPush.length) {
      for (var i = 0, l = extraPush.length; i < l; i++) {
        window.gtag(extraPush[i]);

      }
    }
  }

Can you advise what format the params to gtag need to be (and if the psuedo code is wrong).. right now the params look like the image I am attaching.
Flags: needinfo?(wes.contreras)
Wes ok this is the last thing I need to complete.

Adding some more info. There are two circumstances where the extra page data is used:

1. [['_setCustomVar', 1, 'User Type', 'Registered', 1]]
2. trackEvent

For #2 I know how this works but I am unsure of the syntax to set customvars using the datalayer. I can not find documentation on that anywhere.

If you can provide that we can get this into testing.
I'm having trouble finding anywhere that the custom data is being used - it's not documented anywhere I can find, and the Google Analytics configuration doesn't account for it. Though user type does seem like a good data point to capture, and it's easy to add in from my end.

As for the code, if the attribute is available at initial page load, the easy way is to incorporate it into the existing tag based on slide 5 in the spec:
https://docs.google.com/presentation/d/15cIgl0TKA0C7G18fxCHHilVrwN0ozI-nZIcDj1nQyOQ/edit#slide=id.g2b4864f04c_2_52

We'll take this code:


<!-- Global Site Tag (gtag.js) - Google Analytics →
<script async src="https://www.googletagmanager.com/gtag/js?id=UA-36116321-2"></script>
<script>
  window.dataLayer = window.dataLayer || [];
  function gtag(){dataLayer.push(arguments)};
  gtag('js', new Date());
  gtag('config', 'UA-36116321-2', {
      'dimension1': product_name
    }
  });
</script>


And update it to this:

<!-- Global Site Tag (gtag.js) - Google Analytics →
<script async src="https://www.googletagmanager.com/gtag/js?id=UA-36116321-2"></script>
<script>
  window.dataLayer = window.dataLayer || [];
  function gtag(){dataLayer.push(arguments)};
  gtag('js', new Date());
  gtag('config', 'UA-36116321-2', {
      'dimension1': product_name,
      'dimension2': window.user_type()  // The new dimension for user type
    }
  });
</script>


Where window.user_type() would be a function defined somewhere to look into that attribute and get the value for the user_type key specifically - if that's the only attribute ever included in that array, and it's always in that format, it could be something like this:

window.user_type = function() {
  var arr = $('body').data('ga-push');
  if (arr.length > 0 && arr[0][3] === 'User Type') {
    return arr[0][5];
  }
  return 'Unknown';
}

Unfortunately, the old "_setCustomVar" syntax is from a couple versions of the GA tag ago, and isn't supported at all any more.
Flags: needinfo?(wes.contreras)
Wes thanks.

My concern is that this info was being used to drive some of the dashboards on the site. SUMO uses the ga api to power a bunch of charts.

However.. I am pretty convinced that the data here is not being used.. im just double checking that's true. If so I can rip this code out.

Thanks for the above super helpful.
Yeah, it's not part of our approved requirements so we can get away with dropping it, but if it's easier to add it in now than later, that might be easier than opening a new ticket if they decide they want to keep the dashboards working with the update. I'll leave that up to you.
Wes can you double check this code. I am able to see everything populated in GA except "value". Should this be 'event_value' or is there something additional that needs to be setup?


function trackEvent(category, action, value, label) {
  if (window.gtag) {
    window.gtag('event', action, {
      'event_category': category,
      'value': value,
      'event_label': label
    });
  }
}
Flags: needinfo?(wes.contreras)
That looks like the right code. For reference:
https://developers.google.com/analytics/devguides/collection/gtagjs/events

What methodology are you using to test?
Flags: needinfo?(wes.contreras)
Wes:

To test I have been clicking each item I tag, then looking in Google Analytics real time and the non-real time reports.

I think you have access to our GA account and if you look at the "localhost" profile you can see data I have generated.
Event value isn't supported in realtime reporting. Once it does the full analysis, we should see value show up in the regular reports.
Wes, I am not seeing this data. Can you poke around in the GA "Local host" profile and verify if things are where they should be?

For example I do not see "Event Label" set for {link click} -> {product}.

The code that should set this, an example can be seen here...


https://github.com/mozilla/kitsune/pull/3076/files#diff-5549248a764f1144b74b74929c05074b
Flags: needinfo?(wes.contreras)
I think I see the issue - the "data-event-value" in those examples should be "data-event-label" instead. Every event has a category, action, and label that are text strings with increasing granularity. The 'value' is a separate, optional data point that assigns a numeric dollar value to the action in those cases where we want to report on monetization. We have any cases where we're using 'value' in the new spec - it's only included in my code samples for backwards compatibility. 

Event label, however, is important, and in the spec for every event, but it looks like what ought to be assigned to 'label' got assigned to 'value' instead.
Flags: needinfo?(wes.contreras)
Got it, will fix.
All sorted. Fixes made on friday.
Any other issues come up? Is there a staging site where we might be able to run through a quick round of QA as well?
Flags: needinfo?(bsternthal)
No other issues, I can let you know when this is on stage. Just fixing a few more things prior to merging (there were a lot of code changes here).

You can follow along in the PR. https://github.com/mozilla/kitsune/pull/3076
Flags: needinfo?(bsternthal)
Sounds good. Thanks!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Wes:

The new code is deployed here https://support.allizom.org/ I poked around a bit and saw the right things being collected in GA real time. I would ask you do a quick review and give us the go-ahead to push it live.

Note the one area I had to diverge from your spec was page 11 & 12. In this PR I am not capturing success or failure of the login. Due to how this is currently implemented we do not know if the login was a success until after the form has been submitted. Implementing what you have in the spec is possible but requires a rework of how login currently works and we should wait on that until the redesign.
Flags: needinfo?(wes.contreras)
Understood. Impact of not being able to capture success/fail for logins is low, so putting that off until redesign to save a good chunk of dev effort seems like a good idea. 

We'll run through a round of QA on staging and confirm that everything looks good on our side.
Commit pushed to master at https://github.com/mozilla/kitsune

https://github.com/mozilla/kitsune/commit/990a8b4a7482ff78b692aa4a3c2fcc1b541dd04b
Revert GTM changes.

The changes are OK but they haven't made their way to SCL3 prod yet. Reverting
to keep the codebases close and so that we can track using GA on both.

Revert "Preserved legacy trackEvent function, added analytics.js back to bundle"

This reverts commit 1331ca19fa8149d21e8031c5676545b13d32a9f2.

Revert "[fix bug 1431412] Replace GA script with GTM."

This reverts commit 33be297922ad64b5dcc10808752d6cd1896348d6.
Wes, can you give us a go or no/go on this one?

This is a substantial change and having it not merged for an extended time is not a good idea.

Much thanks!
Sorry for the slow response - got caught up in another urgent issue that took some time to resolve. 

You mentioned that we're not going to capture success/fail for the login event just yet. Does that also apply to the create account event?

We also noticed that the drop-down menu for contributor tools doesn't have the data attributes on those menu items and thus don't fire any events when people choose a contributor tool option.

Otherwise, everything looks good and passes QA.
Flags: needinfo?(wes.contreras)
OK great!

Correct create account success/fail is also not captured. I suspect I have missed a few data attributes. I will file a bug for these and do this in a subsequent PR post AWS migration.

Thanks again for the help qa'ing.
This is now in prod
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: