-
Notifications
You must be signed in to change notification settings - Fork 120
fix(metrics): separate the begin and view flow events #4440
Conversation
@@ -469,6 +469,9 @@ define(function (require, exports, module) { | |||
} | |||
|
|||
this.focusAutofocusElement(); | |||
|
|||
this.logFlowEvent('view', this.viewName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each child view of the settings page is logged at once when the settings page is opened:
To avoid this problem with the "normal" events, we log the screen being opened in app.js in two places, here for top level views, here for child views.
I like the concept here though, and think there's a lot of merit in only logging a view iff it's actually displayed, after all of the possible startup XHR stuff. Maybe that's what the normal events should do too, view.logView()
could possibly move to just before line 116. If that happened, it seems like logView
could log the view
flow event itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting to summarize some comments that we discussed IRL, none of which I think need to block this PR:
- It seems like there's a lot of duplication between the normal event stream and flow events. It might be nice to unify the two as much as possible so that we can look for flow events in datadog and normal events in re:dash?
- Similarly, can flow events by generated from normal events, perhaps via server side processing?
The only possible blocker on this PR is https://github.com/mozilla/fxa-content-server/pull/4440/files#r89324776
51f7d3c
to
07218ab
Compare
Thanks for the feedback @shane-tomlinson. I've tried to implement two of your suggestions in 07218ab:
Can I get another r? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests all go green, ! Thanks @philbooth!
// logging itself, each child view would be logged at the same time. | ||
// We only want to log the screen being displayed, child views will | ||
// be logged when they are opened. | ||
viewToShow.logView(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 - I think this moving logView
here is a good change.
FWIW, one other potential benefit of this, is that we could one day log the |
Although that would mean we also emit |
Related to mozilla/fxa-activity-metrics#19.
This PR makes two changes:
flow.${viewName}.begin
is renamed toflow.begin
(yes, again).A new
flow.${viewName}.view
event is added.The rationale is that we wanted to simplify the model for people writing queries in redash. And removing the view name from the
flow.begin
event makes it pleasingly symmetrical with theflow.complete
, which is emitted by the auth server.Adding the
flow.${viewName}.view
event enables us to more explicitly track which views any given flow travels through. And because it is emitted at the end ofafterVisible
itsflow_time
can be used as an approximation of the aggregated request time + initial client-side execution time, which may or may not be useful.Here is an excerpt from the logs, showing what a flow looks like when a user lands on sign-in, then clicks on
Create account
to show the sign-up view, then clicks onHave an account?
to go back to the sign-in view, then clicks onForgot password
to show the reset-password view:@shane-tomlinson r?