mozilla / fxa-activity-metrics

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

Anomolies in Screenshot Metrics #114

Closed davismtl closed 5 years ago

davismtl commented 5 years ago

See:

https://analytics.amplitude.com/mozilla-corp/chart/xb48sco/edit/kqjovwg

There are X amount of registrations since screenshots launched accounts. There are Y number of logins. But there are [2 * (Y+X) ] active users for screenshots.

Something doesn't add up.

davismtl commented 5 years ago

CC @irrationalagent

irrationalagent commented 5 years ago

I guess the question I have here is, what causes certificates to be signed for oauth reliers? Is just viewing the form enough to generate a certificate?

We thought the answer here was "no" since if we filter out null uids in amplitude the certificate numbers don't change. But I'm wondering if in the oauth case flow_id ends up counting as a "temporary" user ID, so the number of certificates here do count form views?

irrationalagent commented 5 years ago

from mtg:

we trigger a cert signed event when a user is already logged in to sync and arrives at the "profile picture" screen when trying to add screenshots to their account. so the additional cert signed events in the chart above are attributable to that.

shane-tomlinson commented 5 years ago

so the additional cert signed events in the chart above are attributable to that.

I'd imagine quite a few are attributed to fetching the profile info since these certs will be created if a user is signed into Sync and opens FxA even if they never complete the signin.

Here is a screen cap for a full login showing both cert signs:

screenshot 2018-12-11 at 16 09 06

It looks like the event that causes fxa_activity - cert_signed to be emit does not propagate the service, which would result in both cert_signed events being emit using the client_id for that flow. @philbooth do I have that right?

philbooth commented 5 years ago

...do I have that right?

I don't think so, they're read automagically from the request and the stashed metrics context without needing to be set in the data argument, see:

https://github.com/mozilla/fxa-auth-server/blob/master/lib/metrics/amplitude.js#L200-L214

philbooth commented 5 years ago

Btw, we have logic for ignoring the first of those two events for the flow metrics, precisely because of this issue of double-counting account activity:

https://github.com/mozilla/fxa-auth-server/blob/master/lib/metrics/events.js#L93-L96

I never made the equivalent change for Amplitude, despite considering it on more than one occasion, because I wasn't sure whether we were making the same association there. In hindsight, now, that seems really dumb of me.

philbooth commented 5 years ago

I never made the equivalent change for Amplitude...

@irrationalagent, this might be a nice fix for you to pick up, just to rework the logic in that link so that the exclusion works for Amplitude too (if we want to stop double-counting there). And drop the FLOW_ part of the const's name.

vladikoff commented 5 years ago

@johngruen Hey John, is the FxA integration going away for Screenshots? (AKA do we close this?)

johngruen commented 5 years ago

Stumbled on this issue in a deep dive of my mentions. We are disallowing sign in on screenshots, so i am going to close this