Exp 2 Analytics Fix #1672

maxxcrawford · 2020-04-29T16:18:28Z

No description provided.

…removed/excluded from the experiment.

maxxcrawford · 2020-04-30T02:23:02Z

controllers/utils.js

+const mozlog = require("../log");
+const log = mozlog("controllers.utils");


Added debug logging. Information should show up in terminal when running locally.

maxxcrawford · 2020-04-30T02:27:06Z

controllers/utils.js

@@ -33,39 +35,57 @@ function hasUserSignedUpForRelay(user) {

 function getExperimentBranch(req, sorterNum) {

+  if (req.session.excludeFromExperiment && !req.query.experimentBranch) {


This is the primary fix for #1671. In summary, any time a session would return false for getExperimentBranch(), it would be forgotten when the function was called again (on another page load, refresh, etc). This applies specifically to a use who would be eligible to be in the experiment. They would have the experiment sorter run until it landed them in one of the experiment branches.

One note about this if statement is that it includes an override for a query parameter. This is for testing purposes. If you add ?experimentBranch=vb, it will place you in that cohort, regardless of getting discounted previously. One caveat is that if your language settings rule you out, this query request is denied (Again – for testing purposes).

maxxcrawford · 2020-04-30T02:28:05Z

controllers/utils.js

      return false;
    }
+    log.debug("This session has been set to the requested branch: ", req.query.experimentBranch);
+    req.session.excludeFromExperiment = false;


Because this session variable is only set during return false occasions, this reset here was necessary.

maxxcrawford · 2020-04-30T02:32:33Z

public/js/fxa-analytics.js

@@ -126,7 +126,6 @@ function sendRecommendationPings(ctaSelector) {
      hitCallback: function() {
        removeUtmsFromUrl();
      },
-      nonInteraction: true,


Because the Growth team was having issues tracking the experiment in GA, we want to minimize changes at this time. I propose we re-open #1643 to track that issue in the future.

maxxcrawford · 2020-04-30T02:33:31Z

public/js/monitor.js

@@ -62,6 +62,16 @@ function doOauth(el) {
        url.searchParams.append(key, document.body.dataset[key]);
      }
    });
+
+    if (typeof(ga) !== "undefined") {


Added back this event to mirror what was fired in the first experiment.

maxxcrawford · 2020-04-30T02:33:47Z

views/partials/analytics/experiment.hbs

@@ -1,3 +1,3 @@
 data-utm_term={{experimentBranch}}
 data-experiment={{experimentBranch}}
-data-utm_campaign="growthuserflow2"
+data-utm_campaign="growthuserflow3"


Revised this ID for a clean break in the data

Can we keep and append to growthuserflow2, e.g. growthuserflow2a so we can keep them sequentially in sync with our project docs?

groovecoder

Spot-checks look good. I disabled the experiment, ran thru scans, sign-up, and sign-ins. Enabled the experiment, ran thru scans, sign-up, and sign-ins. Re-disabled the experiment, and ran thru scans, sign-up, and sign-ins. All work.

Comments here are questions/comments - not requests for changes.

groovecoder · 2020-04-30T13:27:18Z

controllers/user.js

    req.session.experimentBranch = experimentBranch;
+    req.session.excludeFromExperiment = excludeFromExperiment;


Oh interesting. Does req.session.reset not do this implicitly?

My in-code comment is a little murky. We want to make sure that any user assigned to a specific experiment (including being excluded) is remembered past the session reset.

The req.session.reset removes both of these variables.

groovecoder · 2020-04-30T13:31:21Z

public/js/fxa-analytics.js

@@ -120,13 +120,15 @@ function sendRecommendationPings(ctaSelector) {
      // If an experiment is active, set the "Growth Experiment Version"
      // Custom Dimension to whichever branch is active.
      ga("set", "dimension7", `${document.body.dataset.experiment}`);
+      ga("set", "dimension8", `${document.body.dataset.experiment}`);
+      ga("set", "campaignName", `${document.body.dataset.utm_campaign}`);
+      ga("set", "campaignTerm", `${document.body.dataset.utm_term}`);


I don't see a campaignTerm in the Analytics.js field reference?

Great catch. Fixing this – that property doesn't exist. I was following naming conventions (and that property is the one that contains the experiment branch name). Looks like I need to set it to campaignKeyword.

/cc @zerokarmaleft

Yea, campaignKeyword is the right choice for utm_term

groovecoder · 2020-04-30T13:34:13Z

tests/controllers/home.test.js

  let experimentNumber = 0;
  let experimentBranch = getExperimentBranch(mockRequest, experimentNumber);
  expect(experimentBranch).toBe("va");

+  mockRequestSessionReset(mockRequest);


This makes my no-mutable-state spidey-sense tingle, but if you're confident in the test logic here it's okay with me. Potential bugs in test code aren't as bad as potential bugs in functional code, I guess.

Copy that. I did a session earlier this week with @gregtatum and we talked about resetting the request/env between each test. This function is a compromise – the extra step (that I did not opt for) is to break this test out at each expect to ensure it's a clean start every time.

…paignKeyword

Revert pageview nonInteraction change

4e9812b

lesleyjanenorton temporarily deployed to monitor-v2 April 29, 2020 16:29 Inactive

Updated utm_campaign for debugging

b19a56b

groovecoder temporarily deployed to monitor-v2 April 29, 2020 16:51 Inactive

Set campaign name/term before pageview send

2dc0322

groovecoder temporarily deployed to monitor-v2 April 29, 2020 17:56 Inactive

Added event trigger for sign-in button clicks

a18713d

groovecoder temporarily deployed to monitor-v2 April 29, 2020 20:46 Inactive

groovecoder temporarily deployed to monitor-v2 April 29, 2020 20:53 Inactive

Updated campaign name for additionally debugging

42244e1

maxxcrawford force-pushed the 1671-exp-2-analytics-fix branch from 7d70e84 to 42244e1 Compare April 29, 2020 20:54

groovecoder temporarily deployed to monitor-v2 April 29, 2020 20:54 Inactive

Added session dimension

c20e8e3

groovecoder temporarily deployed to monitor-v2 April 29, 2020 22:05 Inactive

Added a durable session variable to remember when a session has been …

e63ede1

…removed/excluded from the experiment.

groovecoder temporarily deployed to monitor-v2 April 30, 2020 02:14 Inactive

maxxcrawford commented Apr 30, 2020

View reviewed changes

Fixed #1671 - Updated campaign name for clean data collection

fea7d92

maxxcrawford requested a review from groovecoder April 30, 2020 02:29

groovecoder temporarily deployed to monitor-v2 April 30, 2020 02:29 Inactive

maxxcrawford requested a review from lesleyjanenorton April 30, 2020 02:29

maxxcrawford commented Apr 30, 2020

View reviewed changes

Added session reset to the experiment assignment unit test

28ad2b3

groovecoder temporarily deployed to monitor-v2 April 30, 2020 03:20 Inactive

maxxcrawford marked this pull request as ready for review April 30, 2020 03:20

groovecoder approved these changes Apr 30, 2020

View reviewed changes

Updated ga set event to use correct language to set the utm_term: cam…

675174c

…paignKeyword

groovecoder temporarily deployed to monitor-v2 April 30, 2020 15:31 Inactive

groovecoder approved these changes Apr 30, 2020

View reviewed changes

Updated campaign name to stay in-line with experiment naming conventions

37e52de

groovecoder temporarily deployed to monitor-v2 April 30, 2020 16:23 Inactive

maxxcrawford merged commit 8920513 into master Apr 30, 2020

maxxcrawford deleted the 1671-exp-2-analytics-fix branch July 13, 2020 03:29

maxxcrawford added the growth-team Growth team label Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exp 2 Analytics Fix #1672

Exp 2 Analytics Fix #1672

maxxcrawford commented Apr 29, 2020

maxxcrawford Apr 30, 2020

maxxcrawford Apr 30, 2020

maxxcrawford Apr 30, 2020

maxxcrawford Apr 30, 2020

maxxcrawford Apr 30, 2020

maxxcrawford Apr 30, 2020

zerokarmaleft Apr 30, 2020

groovecoder left a comment

groovecoder Apr 30, 2020

maxxcrawford Apr 30, 2020

groovecoder Apr 30, 2020

maxxcrawford Apr 30, 2020

zerokarmaleft Apr 30, 2020

groovecoder Apr 30, 2020

maxxcrawford Apr 30, 2020

		const mozlog = require("../log");
		const log = mozlog("controllers.utils");

		@@ -33,39 +35,57 @@ function hasUserSignedUpForRelay(user) {

		function getExperimentBranch(req, sorterNum) {

		if (req.session.excludeFromExperiment && !req.query.experimentBranch) {

		req.session.experimentBranch = experimentBranch;
		req.session.excludeFromExperiment = excludeFromExperiment;

Exp 2 Analytics Fix #1672

Exp 2 Analytics Fix #1672

Conversation

maxxcrawford commented Apr 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment