feat(metrics): emit flow events for newsletter subscription #5160
Conversation
Yeah, we (at least I) are OK with that. |
There was a problem hiding this comment.
One question, one nit. Tests well, the expected events show up in the logs. With the nit addressed, r+. Thanks @philbooth! Do you need to update the metrics-events doc with this new event?
| @@ -31,6 +32,7 @@ define(function (require, exports, module) { | |||
| var notifier; | |||
| var preferencesUrl = 'https://marketing.preferences.com/user/user-token'; | |||
| var relier; | |||
| let sentryMetrics; | |||
There was a problem hiding this comment.
I don't see sentryMetrics actually used anywhere. Is it needed?
There was a problem hiding this comment.
Yes.
Unless by "it" you mean the local reference to it, in which case no, I could pass in new SentryMetrics() directly to the Metrics constructor directly. I was only trying to follow the convention used everywhere else but I can change it if it makes you happy.
There was a problem hiding this comment.
Oh, I see now. sentryMetrics is needed by the Flow model, which uses it when it logs errors.
| @@ -93,12 +94,23 @@ define(function (require, exports, module) { | |||
| }, | |||
|
|
|||
| setOptInStatus (newsletterId, isOptedIn) { | |||
| var method = isOptedIn ? 'optIn' : 'optOut'; | |||
| let method, eventPrefix; | |||
There was a problem hiding this comment.
Please use a separate line for each variable declaration.
| this.logViewEvent(method); | ||
|
|
||
| return this.getMarketingEmailPrefs()[method](newsletterId) | ||
| .then(() => { | ||
| this.logViewEvent(method + '.success'); | ||
| // Emit an additional flow event for consistency with | ||
| // the call to optIn in the account model | ||
| this.logFlowEvent(`newsletter.${eventPrefix}subscribed`); |
There was a problem hiding this comment.
Let's think about how we can consolidate metrics, ubiquitous language should apply to metric event names too! We have multiple places where both View events and Flow events are logged:
settings.communication-preferences.optIn.success and flow.newsletter.subscribed mean the same thing yet have two distinct names, this is a recipe for confusion.
There was a problem hiding this comment.
The reason we have two events is because flow.newsletter.subscribed is also emitted during sign-up if the user checked the box. Re-using the other event name there seemed like a bad option because it specifically mentions settings.communication.
There was a problem hiding this comment.
That makes sense, but it hints that our metrics aren't factored correctly. It seems like a lone event should be triggered, and if the event is done as part of a view, the viewName should be a tag of the event.
Originally, I think the viewName was made part of the event name because DataDog struggled with too many tags but didn't care about the number of distinct event names. I am not sure if this reasoning still holds.
philbooth
force-pushed
the
phil/newsletter-flow-events
branch
from
c5b7d29
to
d6cd53c
Compare
Fixes mozilla/fxa-activity-metrics#79, adding new flow events,
flow.newsletter.subscribedandflow.newsletter.unsubscribed, for tracking newsletter subscriptions.I had to stretch the definition of a flow event somewhat, in order to make this work. Until now we've been quite strict about saying that flow events only occur during sign-in and sign-up. However, users can change their newsletter subscription preferences at any time via the settings screen. So in order to produce meaningful metrics, I've introduced the possibility of emitting these flow events outside of the normal flows. Are we okay with that?
In addition to the events emitted from settings, a subscribed event is also emitted after verification if the user checked the newsletter checkbox during sign-up.
Here are some sample log lines showing the new events:
@mozilla/fxa-devs r?