search

mozilla / fxa-activity-metrics

A server for managing the Firefox Accounts metrics database and pipeline
1 stars 3 forks source link

Password Reset Complete > Login Complete #110

Closed davismtl closed 5 years ago

davismtl commented 5 years ago

We're tracking a pretty low login success rate after a password reset: https://analytics.amplitude.com/mozilla-corp/chart/mw2zpyf

However, if we look for activities like a cert sign which would require the successful login, we have a much higher success rate. https://analytics.amplitude.com/mozilla-corp/chart/3pp9mw5

Why the huge discrepency between the two?

davismtl commented 5 years ago

CC @irrationalagent

philbooth commented 5 years ago

However, if we look for activities like a cert sign which would require the successful login

IIUC, it's worth noting that the successful login is only required on secondary devices. The device that you reset the password on already has a fresh session token from the reset flow, so doesn't need to log in again. So, if I'm thinking straight, device activity should be expected to be higher than successful login ratio, right?

davismtl commented 5 years ago

Given the password reset is occurring during a login flow, with the purpose of successfully logging in, I expected that we track a login_complete event.

If we think of it purely from tracking logins, it means we wouldn't be tracking all logins since we don't include occurred through password reset. This doesn't seem right to me.

@irrationalagent do you have a different perspective or do you think there would be value in including successful password resets in our login volume?

philbooth commented 5 years ago

Given the password reset is occurring during a login flow, with the purpose of successfully logging in, I expected that we track a login_complete event.

It should be a one-liner to fix this fwiw, I just need to always emit a login_complete every time we emit a reset_complete. Happy to do that for train 126 if we're agreed?

irrationalagent commented 5 years ago

This works for me, agreed with Alex that since reset is done in the course of a login, and a successful reset leads to the resetting device being logged in, a successful reset should emit both reset_complete and login_complete.

philbooth commented 5 years ago

One thing I just want to double check before making the change though: This means we won't be able to do a direct comparison between the overall numbers for login submit and login complete any more.

I figure that's fine because of how funnels/filtering work in Amplitude, we never really need to compare those numbers from a high level. Right?

irrationalagent commented 5 years ago

When we do a funnel in amplitude starting with (or including) a login_submit and subsequently add a login_complete step we are essentially saying that both events must be completed in that order to count toward the end-of-funnel conversion rate. When a user diverts to the reset flow, then we would track that in a different funnel and there login_complete would only count subsequent to the password reset events.

The one thing this will change is the estimated volume of total login_complete events (it will increase). However, at least I was working under the implied assumption that a password reset would count to these already, so this change would just fix things to align to my previously wrong world view.

davismtl commented 5 years ago

This means we won't be able to do a direct comparison between the overall numbers for login submit and login complete any more.

When doing a funnel, it will ignore automatically the logins generated by password resets.

Before we rush into this change, @irrationalagent , do you think we should have a property for the login event to tell the two apart? (thinking out loud here)

irrationalagent commented 5 years ago

I suppose if we ever wanted to know the total number of logins that are NOT due to/following a reset then we would want a property, e.g. password_reset = (none) or password_reset = true for logins due to reset**. The use case here in terms of analysis is unclear to me at the moment though.

More likely, we will want to know the number of logins that ARE due to reset, in which case we can just make a funnel. No need for an event property there.

@philbooth this makes me think however, we will need to be certain that the login_complete event has a timestamp that is after the reset_complete event otherwise amplitude will construct the funnels incorrectly.

**Ideally (none) would be false but doing so would entail adding the property to all other login events as well. If we just add true to the post-reset logins then all other login events will just get none and we can infer it to mean false.