Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Track duration of page changes #1814

Merged
merged 6 commits into from
Mar 28, 2018
Merged

Conversation

lukebarnard1
Copy link
Contributor

@lukebarnard1 lukebarnard1 commented Mar 27, 2018

The duration measured is between

  • componentWillUpdate of MatrixChat and
  • componentDidUpdate of MatrixChat.

This does not account for all changes to the view that occur
when a room switch happens, for example. But it does at least
capture the difference between switching to a "big" room and
switching to a small test room.

(as per https://matomo.org/blog/2017/02/how-to-track-single-page-websites-using-piwik-analytics/ - thanks @michaelkaye for finding that!)

Fixes element-hq/element-web#6325

The duration measured is between
 - componentWillUpdate of MatrixChat and
 - componentDidUpdate of MatrixChat.

This does not account for *all* changes to the view that occur
when a room switch happens, for example. But it does at least
capture the difference between switching to a "big" room and
switching to a small test room.
src/Analytics.js Outdated
@@ -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.

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.

src/Analytics.js Outdated
);

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.

src/Analytics.js Outdated
@@ -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.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Mar 27, 2018
@lukebarnard1 lukebarnard1 assigned dbkr and unassigned lukebarnard1 Mar 27, 2018
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Could do with this. _pageChanging being initialised in componentWillMount, otherwise lgtm

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Mar 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants