Skip to content
This repository has been archived by the owner. It is now read-only.

fix(metrics): Add flowId and flowBeginTime to headers if avalible #241

Merged
merged 1 commit into from Dec 20, 2016

Conversation

@vbudhram
Copy link
Contributor

@vbudhram vbudhram commented Dec 16, 2016

This PR adds support for forwarding the flowId and flowBeginTime values in the email headers. These will be used to track email bounce events that correspond to flows. This has been added to all our emails except verificationReminderEmail.

Part of mozilla/fxa-auth-server#1425

@seanmonstar
Copy link
Member

@seanmonstar seanmonstar commented Dec 16, 2016

This looks great to me.

Did we really need the flowBeginTime? I'd expect that to have been part of the initial flow creation, and then we wouldn't need it again. All time lengths can be computed by subtracting the original stored begin time in the DB.

@@ -268,15 +268,22 @@ module.exports = function (log) {

var links = this._generateLinks(this.verificationUrl, message.email, query, templateName)

var headers = {

This comment has been minimized.

@seanmonstar

seanmonstar Dec 16, 2016
Member

Nit if you want, I guess we prefer let now over var wherever possible. If I didn't have the question about flowBeginTime, I would have merged regardless, so however you like.

This comment has been minimized.

@vbudhram

vbudhram Dec 19, 2016
Author Contributor

Updated this to use let but the l10 extraction complains that it is a reserved keyword. @vladikoff is this something we should update to exclude let or should we hold off on using it in repo for now?

@rfk
Copy link
Member

@rfk rfk commented Dec 19, 2016

Did we really need the flowBeginTime?

Yes, although one could argue that it's mostly for historical reasons at this point. You need to know the flowBeginTime to validate the signature in the flowId, and we use it to calculate and log the time between start of flow and each particular event.

You're right that this second usage could be done in post-processing based on data in the DB, but that wasn't obvious when we were designing these events initially. If it becomes a PITA to keep passing this timestamp around then we could try to refactor ourselves towards doing it in the DB.

@vbudhram
Copy link
Contributor Author

@vbudhram vbudhram commented Dec 19, 2016

@rfk Made mozilla/fxa-auth-server#1594 to help track instances we might need to refactorflowBeginTime and help build a case for doing it.

@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Dec 19, 2016

from mtg: don't use let for now.

@vbudhram
Copy link
Contributor Author

@vbudhram vbudhram commented Dec 19, 2016

@vladikoff @seanmonstar Mind an official r?

@vbudhram vbudhram requested a review from seanmonstar Dec 19, 2016
@vbudhram vbudhram self-assigned this Dec 19, 2016
@vbudhram vbudhram requested a review from vladikoff Dec 19, 2016
Copy link
Member

@seanmonstar seanmonstar left a comment

Looks good!

@vbudhram vbudhram merged commit 4525f9c into master Dec 20, 2016
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 98.185%
Details
@vbudhram vbudhram deleted the add-flow-metrics branch Dec 20, 2016
@rfk rfk added this to the FxA-41:signin funnel metrics milestone Jan 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants