Pass service with cert_signed amplitude event

[philbooth]

MOVED FROM mozilla/fxa-amplitude-send#66:

When looking at DAU using cert_signed (our primary indicator), we can't group or filter on service. Service rare seems defined.:

https://analytics.amplitude.com/mozilla-corp/chart/yswb4kd/edit/rksvzu1

Given that we are signing certificates for Lockbox and Notes, we should probably be able to tell the difference.

┆Issue is synchronized with this Jira Task ┆Issue Number: FXA-668

[irrationalagent]

Realizing that this is probably important to do. Maybe I can do it.

[philbooth]

@irrationalagent, looking at the code, I think all of the pieces are in place on our side of the equation.

• We accept a service query param on /certificate/sign here.

• We propagate that to Amplitude here.

• We map that from the hexadecimal relier id to a metrics-friendly service name here.

So, if Lockbox/Notes/whoever hits /certificate/sign?service=foo instead of plain /certificate/sign, replacing foo with their hexadecimal relier id, it should work automagically.

[rfk]

Lockbox/Notes/whoever should not be calling /certificate/sign on a regular basis, because they are OAuth clients. Instead we expect them to be hitting the oauth /token endpoint with their refresh_token.

Is the goal here to be able to measure DAU or similar on signed-in app instances?

[irrationalagent]

I was actually referring to sync in this issue. From my understanding we would need to make changes on the client to pass the service parameter. From what I can tell desktop sync does not append the service parameter on certificate requests for your everyday run-of-the-mill sync. It only appears to happen on reg and login, AFICT. https://analytics.amplitude.com/mozilla-corp/chart/new/87hzkhe

[philbooth]

Lockbox/Notes/whoever should not be calling /certificate/sign on a regular basis

🤦‍♂️😊

I was actually referring to sync in this issue. From my understanding we would need to make changes on the client to pass the service parameter.

Ok, I think we can infer it's Sync in the auth server for those without needing client changes, because the content server does set the service param on that endpoint:

https://github.com/mozilla/fxa-content-server/blob/261b9e690a922d5ac938f04304de75921e4a7a87/app/scripts/lib/fxa-client.js#L691-L698

We wouldn't want to make the change in the main getService function in lib/metrics/amplitude.js because then it would take effect on all Amplitude events. But if you add a service property to the second, data argument in the call to request.emitMetricsEvent('account.signed', ...) in lib/routes/sign.js and default the value to 'sync' if it's not set on the request, that should get to where you want to be.

Make sense @irrationalagent (and anyone else)?

[irrationalagent]

@philbooth yea I think I follow that. One question I have is that (I believe) Lockbox is a relier that also uses sync 1.5 (to sync credentials). Is that another case where we would be signing certificates without client changes, and thus be getting empty service params? No worries if you don't know the answer off the top of your head, I can ask the lockbox team.

[rfk]

Is that another case where we would be signing certificates without client changes, and thus be getting empty service params?

No, because Lockbox access sync via OAuth tokens rather than BrowserID assertions, so it will be hitting the oauth /token endpoint with its refresh_token rather than calling /certificate/sign.

[davismtl]

Per comments in bug #1356 , we should look into resolving this.

[davismtl]

@irrationalagent do we still need to do this or is this covered by something else now?

[irrationalagent]

So I seem to recall that we tried just assigning sync to all cert signed events (per phil's comment above) and it went wrong so we had to revert. Though I can't seem to find that pr/issue now. I'll keep looking.

It would be nice to have this though. I don't think its covered by another other work. Putting it as a p2 for now, feel free to change it.