feat(metrics): measure page reloads by user #2455
Conversation
|
|
||
| if (refreshMetrics && refreshMetrics.url === currentPath) { | ||
| if (this._metrics) { | ||
| this._metrics.logEvent('refresh(' + currentPath + ')'); |
vladikoff
May 22, 2015
Contributor
- I don't think we have / support
( or ) in our metrics infrastructure.
- each view has a
logScreenEvent (and a screen name) that you might find useful instead of this._window.location.pathname
- I don't think we have / support
(or)in our metrics infrastructure. - each view has a
logScreenEvent(and a screen name) that you might find useful instead ofthis._window.location.pathname
zaach
May 22, 2015
Contributor
Yes, although you'd need access to the view in order to use those. It seems like this check might fit better in the router, in which case you would have access to the view.
Yes, although you'd need access to the view in order to use those. It seems like this check might fit better in the router, in which case you would have access to the view.
|
We'll also need unit tests for reload check. |
|
I moved the logic to the router and also used the logScreenEvent method in View. Also added a test. As always feedback is appreciated :) |
| @@ -207,6 +209,27 @@ function ( | |||
| return new View(viewOptions); | |||
| }, | |||
|
|
|||
| _checkForRefresh: function () { | |||
|
|
|||
vladikoff
May 22, 2015
Contributor
extra space above
extra space above
vladikoff
May 22, 2015
Contributor
aka line breaks 👾
aka line breaks
|
|
||
| if (refreshMetrics && refreshMetrics.view === screenName) { | ||
| if (this.metrics) { | ||
| currentView.logScreenEvent('refresh'); |
vladikoff
May 22, 2015
Contributor
extra if statements above
extra if statements above
|
|
||
| refreshMetrics = {}; | ||
| refreshMetrics.view = screenName; | ||
| refreshMetrics.timestamp = Date.now(); |
vladikoff
May 22, 2015
Contributor
nit: define this in a different way
refreshMetrics = {
view: screenName,
/// etc
}
nit: define this in a different way
refreshMetrics = {
view: screenName,
/// etc
}
zaach
May 22, 2015
Contributor
👍 for declarative style :D
| @@ -340,6 +340,19 @@ function (chai, sinon, _, Backbone, Router, SignInView, SignUpView, ReadyView, | |||
| 'screen.signup-complete')); | |||
| }); | |||
| }); | |||
|
|
|||
| it('logs view refreshs', function () { | |||
vladikoff
May 22, 2015
Contributor
typo: refreshes
typo: refreshes
|
|
||
| var refreshMetrics = storage.get('last_page_loaded'); | ||
|
|
||
| var currentView = this.currentView; |
vladikoff
May 22, 2015
Contributor
nit: not sure we need 2 extra spaces above
nit: not sure we need 2 extra spaces above
|
It would be great to have a functional test for this, but currently we don't have a way to check metrics in functional tests. This could be worth investigating. Ref #1861. |
|
Thank you for your feedback guys, I will try to investigate how to include metrics in functional tests in the meantime. |
|
@eoger awesome! |
| var screenName = currentView.getScreenName(); | ||
|
|
||
| if (refreshMetrics && refreshMetrics.view === screenName && this.metrics) { | ||
| currentView.logScreenEvent('refresh'); |
|
Aside from my one nit, this looks great! I see the the new event during manual testing. All new lines are tested. Since @vladikoff and @zaach have been working with you through this, I defer to them for the final |
|
|
feat(metrics): measure page reloads by user
Implementation draft for #2299