feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup #4150

vladikoff · 2016-09-14T20:46:05Z

~~TODO: tests~~

vladikoff

added questions

vladikoff · 2016-09-14T20:47:54Z

app/scripts/views/mixins/password-prompt-mixin.js

@@ -90,7 +90,7 @@ define(function (require, exports, module) {

    _logPromptExperimentEvent: function (eventNameSuffix) {
      const eventName = 'experiment.pw_prompt.' + eventNameSuffix.toLowerCase();
-      this.logViewEvent(eventName);
+      this.logEventOnce(eventName);


to avoid spamming pw_strength focus

vladikoff · 2016-09-14T20:47:54Z

app/scripts/views/mixins/signin-mixin.js

+    engageForm() {
+      // user has engaged with the signin or sign up form
+      // log 'flow.engage' from both forms because both forms can login.
+      this.logEventOnce('flow.engage');


only log it once per page reload, which should match new flow.begin events right?

vladikoff · 2016-09-14T20:47:54Z

server/lib/routes/post-metrics.js

      flow_id: metrics.flowId, //eslint-disable-line camelcase
      flow_time: 0, //eslint-disable-line camelcase
      time: metrics.flowBeginTime
    }, req);
-  }
+  });


do we want to differentiate flow.begin from other events?

Not sure I understand the question?

I just was not sure if all flow events should be logged the same away, seems like we should just log all of them the same way (like flow.begin already was).

vladikoff · 2016-09-15T00:03:25Z

This also fixes #4022

vladikoff · 2016-09-15T04:42:23Z

cc @rfk ^

rfk

love it, this will make for some really great analysis opportunities

rfk · 2016-09-15T04:52:42Z

app/scripts/lib/metrics.js

+     * @param {String} eventName
+     */
+    logEventOnce: function (eventName) {
+      if (eventMemory.indexOf(eventName) === -1) {


Could use a map or set here for constant-time lookup, this lookup will grow with O(number of events).

We don't have Map or Set available. However I updated this to use underscore which should be more performant hopefully.

That was bad vocabulary on my part, what I really meant was to use one of these: {}.

@rfk - You are thinking in Python terms, aren't you?

rfk · 2016-09-15T04:52:42Z

app/scripts/views/base.js

+     */
+    logEventOnce: function (eventName) {
+      this.metrics.logEventOnce(eventName);
+    },


I really like this helper, will reduce a lot of confusion when reviewing the metrics.

philbooth

Looking good @vladikoff!

Some inline comments about event names, we need to get them consistent with the spec one way or another.

philbooth · 2016-09-15T06:45:14Z

app/scripts/views/mixins/signin-mixin.js

+    engageForm() {
+      // user has engaged with the signin or sign up form
+      // log 'flow.engage' from both forms because both forms can login.
+      this.logEventOnce('flow.engage');


Re: the name of this event, it is currently specced differently to this. Now the spec can change, of course, but also I want to make sure that the event we emit is the one that is most useful for analysis purposes.

For flows that end at this point, where the user never submits the form, it might be useful to know which form the user was engaged with when they abandoned the flow. How tricky is it to change this event name so that it is signup.engage or signin.engage? What about force_auth.engage?

Yea makes sense, I think my main issue is the word login, we don't use that anywhere. might be a good idea to stay away from that

philbooth · 2016-09-15T06:45:14Z

app/scripts/views/mixins/signup-mixin.js

@@ -31,6 +31,8 @@ define(function (require, exports, module) {
     */
    signUp: function (account, password) {
      var self = this;
+      self.logEvent('flow.attempt_signup');


Does this correspond to signup.submit from the spec? Is there a reason to prefer this name over that one?

philbooth · 2016-09-15T06:45:14Z

server/lib/routes/post-metrics.js

  if (DISABLE_CLIENT_METRICS_STDERR) {
    return;
  }

  var events = metrics.events || [];
-  var hasFlowBeginEvent = events.some(function (event) {
+  var flowEvents = _.filter(events, function(event) {
+    return event.type.indexOf('flow.') === 0;


...ahhh, and here I get my answer. The event names are changed because we use the flow. prefix as an indicator that it is a flow event.

Makes sense, but we still need to take into account the analysis of abandoned flows. Could we maybe change:

flow.engage to flow.signup.engage and flow.signin.engage?

flow.attempt_signup to flow.signup.submit?

flow.attempt_signin to flow.signin.submit?

philbooth · 2016-09-15T06:45:14Z

server/lib/routes/post-metrics.js

@@ -77,20 +82,14 @@ function optionallyLogFlowBeginEvent (req, metrics, requestReceivedTime) {
          /*eslint-enable sorting/sort-object-props*/
        });
      }
-
-      return true;


Weird, I wonder why I did that?

philbooth · 2016-09-15T06:45:14Z

server/lib/routes/post-metrics.js

      flow_id: metrics.flowId, //eslint-disable-line camelcase
      flow_time: 0, //eslint-disable-line camelcase
      time: metrics.flowBeginTime
    }, req);
-  }
+  });


Not sure I understand the question?

shane-tomlinson · 2016-09-16T09:11:57Z

app/scripts/views/mixins/signin-mixin.js

@@ -24,13 +31,15 @@ define(function (require, exports, module) {
     * @return {object} promise
     */
    signIn: function (account, password) {
+      var self = this;
+      self.logEvent(`flow.${this.signInSubmitContext}.submit`);


Is there a reason to use signInSubmitContext instead of this.viewName? Seems like the already existing name should suffice?

btw, all views have a viewName based off of their URL, if one is not already specified on the View's prototype.

When will client side created flow events be used instead of server side created events? Up to this point, I thought all flow events were server side created.

When will client side created flow events be used instead of server side created events?

Whenever the thing represented by the event occurs client-side.

The point of these events is to help us identify where users might be dropping off during the sign-in flow. Having extra events on the client, in between the existing flow.begin (client) and account.login (server) flow events provides a finer granularity for analysing those drop-offs. For instance, if some imaginary future UI change caused people to stop submitting the form for some reason, these events would help us identify that.

shane-tomlinson · 2016-09-16T09:25:33Z

app/scripts/lib/metrics.js

@@ -121,6 +121,7 @@ define(function (require, exports, module) {

    this._marketingImpressions = {};
    this._activeExperiments = {};
+    this._eventMemory = [];


Like @rfk suggested, lookups could be a bit quicker by using an Object instead of an Array, though I don't imagine the list ever growing so long that lookups become a problem.

Another alternative is to skip storing a list of events locally and search this.events directly:

isEventLogged (eventName) { return _.some(this.events, (event) => event.type === eventName); }

Another alternative is to skip storing a list of events locally and search this.events directly:

I don't think we can do that because those get flushed so it won't be logged once.
Otherwise an Object makes sense 👍

shane-tomlinson · 2016-09-16T09:25:33Z

app/scripts/lib/metrics.js

+     *
+     * @param {String} eventName
+     */
+    logEventOnce: function (eventName) {


One case to consider here is:

this.logEvent('eventName'); this.logEventOnce('eventName');

When logEventOnce is called, should it log an additional time? That seems a bit unexpected.

If you go with storing a list of events locally, I'd add the eventName to the list in logEvent and check whether it's already stored here.

shane-tomlinson · 2016-09-16T09:25:33Z

app/scripts/lib/metrics.js

+     * @param {String} eventName
+     */
+    logEventOnce: function (eventName) {
+      if (_.indexOf(this._eventMemory, eventName) === -1) {


See the previous comment about searching this.events directly instead of storing a local list.

shane-tomlinson · 2016-09-16T09:25:33Z

app/scripts/views/mixins/signin-mixin.js

@@ -24,13 +31,15 @@ define(function (require, exports, module) {
     * @return {object} promise
     */
    signIn: function (account, password) {
+      var self = this;
+      self.logEvent(`flow.${this.signInSubmitContext}.submit`);


When will client side created flow events be used instead of server side created events? Up to this point, I thought all flow events were server side created.

shane-tomlinson

Looks reasonable, though have a couple of questions/nits/suggestions.

shane-tomlinson · 2016-09-16T09:27:57Z

app/scripts/views/mixins/signup-mixin.js

@@ -31,6 +31,8 @@ define(function (require, exports, module) {
     */
    signUp: function (account, password) {
      var self = this;
+      self.logEvent('flow.signup.submit');


How will users that sign-in from the /signup page affect this stat?

i think we talked irc about that but then the flow would be:

begin -> signup.engage -> signin.submit -> account.login

How will users that sign-in from the /signup page affect this stat?

Yep, just to confirm @vladikoff's answer, that is by design as per the feature doc.

Fixes #4119

vladikoff · 2016-09-20T14:28:26Z

@philbooth final r?

philbooth · 2016-09-20T16:19:53Z

@vladikoff, LGTM, r+

vladikoff added this to the FxA-41:signin funnel metrics milestone Sep 14, 2016

vladikoff assigned philbooth Sep 14, 2016

vladikoff added the waffle:active label Sep 14, 2016

vladikoff reviewed Sep 14, 2016

View changes

rfk reviewed Sep 15, 2016

View changes

philbooth approved these changes Sep 15, 2016

View changes

vladikoff force-pushed the i4119 branch from 13bb469 to 2cd5b14 Sep 15, 2016

vladikoff added the shipit label Sep 15, 2016

shane-tomlinson reviewed Sep 16, 2016

View changes

vladikoff added the train-70 label Sep 16, 2016

vladikoff added 4 commits Sep 14, 2016

feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup

b29fb65

Fixes #4119

feat(metrics): finish up front-end flow updates and tests

308ed7a

fix(tests): adjust and add new tests for new flow events

c5ff3b0

fix(metrics): adjust event memory store

abd6dfa

vladikoff force-pushed the i4119 branch from 2cd5b14 to abd6dfa Sep 19, 2016

vladikoff deleted the i4119 branch Sep 20, 2016

vladikoff removed the waffle:active label Sep 20, 2016

mozilla / fxa-content-server Archived

feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup #4150

feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup #4150

vladikoff commented Sep 14, 2016 •

edited

vladikoff left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Sep 15, 2016

vladikoff commented Sep 15, 2016

rfk left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

philbooth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Sep 20, 2016

philbooth commented Sep 20, 2016

mozilla / fxa-content-server Archived

feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup #4150

feat(metrics): add flow.attempt_signin, flow.engage, flow.attempt_signup #4150

Conversation

vladikoff commented Sep 14, 2016 • edited

vladikoff left a comment

This comment has been minimized.

vladikoff Sep 14, 2016 Author Contributor

This comment has been minimized.

vladikoff Sep 14, 2016 Author Contributor

This comment has been minimized.

vladikoff Sep 14, 2016 Author Contributor

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

vladikoff Sep 15, 2016 Author Contributor

vladikoff commented Sep 15, 2016

vladikoff commented Sep 15, 2016

rfk left a comment

This comment has been minimized.

rfk Sep 15, 2016 Member

This comment has been minimized.

vladikoff Sep 15, 2016 Author Contributor

This comment has been minimized.

rfk Sep 16, 2016 Member

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

rfk Sep 15, 2016 Member

philbooth left a comment

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

vladikoff Sep 15, 2016 Author Contributor

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

philbooth Sep 15, 2016 Contributor

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

philbooth Sep 16, 2016 • edited Contributor

This comment has been minimized.

shane-tomlinson Sep 16, 2016 • edited Member

This comment has been minimized.

vladikoff Sep 16, 2016 Author Contributor

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

shane-tomlinson left a comment

This comment has been minimized.

shane-tomlinson Sep 16, 2016 Member

This comment has been minimized.

vladikoff Sep 16, 2016 Author Contributor

This comment has been minimized.

philbooth Sep 16, 2016 Contributor

vladikoff commented Sep 20, 2016

philbooth commented Sep 20, 2016

vladikoff commented Sep 14, 2016 •

edited

vladikoff Sep 14, 2016
Author Contributor

vladikoff Sep 14, 2016
Author Contributor

vladikoff Sep 14, 2016
Author Contributor

philbooth Sep 15, 2016
Contributor

vladikoff Sep 15, 2016
Author Contributor

rfk Sep 15, 2016
Member

vladikoff Sep 15, 2016
Author Contributor

rfk Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

rfk Sep 15, 2016
Member

philbooth Sep 15, 2016
Contributor

vladikoff Sep 15, 2016
Author Contributor

philbooth Sep 15, 2016
Contributor

philbooth Sep 15, 2016
Contributor

philbooth Sep 15, 2016
Contributor

philbooth Sep 15, 2016
Contributor

shane-tomlinson Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

philbooth Sep 16, 2016 •

edited

Contributor

shane-tomlinson Sep 16, 2016 •

edited

Member

vladikoff Sep 16, 2016
Author Contributor

shane-tomlinson Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

shane-tomlinson Sep 16, 2016
Member

vladikoff Sep 16, 2016
Author Contributor

philbooth Sep 16, 2016
Contributor