Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Track duration of page changes #1814

Merged
merged 6 commits into from Mar 28, 2018
Merged

Track duration of page changes #1814

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69d9080
Track duration of page changes
lukebarnard1 Mar 27, 2018
c8312dd
Use a less fragile API to track page change performance
lukebarnard1 Mar 28, 2018
187e8ab
Remove unused variable
lukebarnard1 Mar 28, 2018
f29b58a
Always expect generationTimeMs
lukebarnard1 Mar 28, 2018
8d994f2
Merge branch 'develop' into luke/track-page-change-duration
lukebarnard1 Mar 28, 2018
00167fb
Init page change state in willMount
lukebarnard1 Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/Analytics.js
View file
Open in desktop
Expand Up @@ -74,6 +74,7 @@ class Analytics {
this._paq = null;
this.disabled = true;
this.firstPage = true;
this.generationTimeMs = null;
}

/**
Expand Down Expand Up @@ -147,6 +148,23 @@ class Analytics {
return true;
}

startPageChangeTimer() {
performance.clearMarks('riot_page_change_start');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll have to use these APIs conditionally: they aren't available in all browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://caniuse.com/#feat=user-timing says otherwise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yes - MDN lied to me! Fair enough then.

performance.mark('riot_page_change_start');
}

stopPageChangeTimer() {
performance.mark('riot_page_change_stop');
performance.measure(
'riot_page_change_delta',
'riot_page_change_start',
'riot_page_change_stop',
);

const measurement = performance.getEntriesByName('riot_page_change_delta').pop();
this.generationTimeMs = measurement.duration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you're calling this (ie. with willupdate / didupdate) this looks fine, but if these can ever overlap then this will go very wonky, so might be worth calling this out. Also, any reason, not to clear the marks as soon as we have our number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth calling this out

As in, with docs on the functions?

Also, any reason, not to clear the marks as soon as we have our number?

Nope, I guess not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, or in conjunction with comment below, change the API so start returns some unique handle that you then pass into trackPageChange? (and remove stopPageChangeTimer). It may be more effort than it's worth though.

}

trackPageChange() {
if (this.disabled) return;
if (this.firstPage) {
Expand All @@ -156,6 +174,9 @@ class Analytics {
return;
}
this._paq.push(['setCustomUrl', getRedactedUrl()]);
if (typeof this.generationTimeMs === 'number') {
this._paq.push(['setGenerationTimeMs', this.generationTimeMs]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saving this as a member variable seems like quite an error prone API: is trackPageChange() called anywhere else (or will it be in the future?) If so, if the caller doesn't also use the page change timer, it will end up submitting whatever the last page change timer value was.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that we'd leak incorrect generation times when we don't use both in conjunction.

We could set it to null after it's been sent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, but nulling it out is probably acceptable.

}
this._paq.push(['trackPageView']);
}

Expand Down
19 changes: 17 additions & 2 deletions src/components/structures/MatrixChat.js
View file
Open in desktop
Expand Up @@ -368,13 +368,29 @@ export default React.createClass({
window.removeEventListener('resize', this.handleResize);
},

componentDidUpdate: function() {
componentWillUpdate: function(props, state) {
if (this.shouldTrackPageChange(this.state, state)) {
Analytics.startPageChangeTimer();
}
},

componentDidUpdate: function(prevProps, prevState) {
if (this.shouldTrackPageChange(prevState, this.state)) {
Analytics.stopPageChangeTimer();
Analytics.trackPageChange();
}
if (this.focusComposer) {
dis.dispatch({action: 'focus_composer'});
this.focusComposer = false;
}
},

shouldTrackPageChange(prevState, state) {
return prevState.currentRoomId !== state.currentRoomId ||
prevState.view !== state.view ||
prevState.page_type !== state.page_type;
},

setStateForNewView: function(state) {
if (state.view === undefined) {
throw new Error("setStateForNewView with no view!");
Expand Down Expand Up @@ -1341,7 +1357,6 @@ export default React.createClass({
if (this.props.onNewScreen) {
this.props.onNewScreen(screen);
}
Analytics.trackPageChange();
},

onAliasClick: function(event, alias) {
Expand Down