From 69d9080dd5207fffe59e306540b24ece6e90f0dd Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 27 Mar 2018 11:17:49 +0100 Subject: [PATCH 1/5] Track duration of page changes 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 | 21 +++++++++++++++++++++ src/components/structures/MatrixChat.js | 19 +++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/Analytics.js b/src/Analytics.js index 5f4a0d0c77..ab47d30c1d 100644 --- a/src/Analytics.js +++ b/src/Analytics.js @@ -74,6 +74,7 @@ class Analytics { this._paq = null; this.disabled = true; this.firstPage = true; + this.generationTimeMs = null; } /** @@ -147,6 +148,23 @@ class Analytics { return true; } + startPageChangeTimer() { + performance.clearMarks('riot_page_change_start'); + 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; + } + trackPageChange() { if (this.disabled) return; if (this.firstPage) { @@ -156,6 +174,9 @@ class Analytics { return; } this._paq.push(['setCustomUrl', getRedactedUrl()]); + if (typeof this.generationTimeMs === 'number') { + this._paq.push(['setGenerationTimeMs', this.generationTimeMs]); + } this._paq.push(['trackPageView']); } diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 1312abda09..a898808141 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -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!"); @@ -1341,7 +1357,6 @@ export default React.createClass({ if (this.props.onNewScreen) { this.props.onNewScreen(screen); } - Analytics.trackPageChange(); }, onAliasClick: function(event, alias) { From c8312dd5ae0e94629bb6e69f420f3654ff42e94a Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 28 Mar 2018 10:25:28 +0100 Subject: [PATCH 2/5] Use a less fragile API to track page change performance --- src/Analytics.js | 23 +++------------- src/components/structures/MatrixChat.js | 35 ++++++++++++++++++++++--- 2 files changed, 35 insertions(+), 23 deletions(-) diff --git a/src/Analytics.js b/src/Analytics.js index ab47d30c1d..bb9f900145 100644 --- a/src/Analytics.js +++ b/src/Analytics.js @@ -148,24 +148,7 @@ class Analytics { return true; } - startPageChangeTimer() { - performance.clearMarks('riot_page_change_start'); - 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; - } - - trackPageChange() { + trackPageChange(generationTimeMs) { if (this.disabled) return; if (this.firstPage) { // De-duplicate first page @@ -174,8 +157,8 @@ class Analytics { return; } this._paq.push(['setCustomUrl', getRedactedUrl()]); - if (typeof this.generationTimeMs === 'number') { - this._paq.push(['setGenerationTimeMs', this.generationTimeMs]); + if (typeof generationTimeMs === 'number') { + this._paq.push(['setGenerationTimeMs', generationTimeMs]); } this._paq.push(['trackPageView']); } diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index a898808141..2c38e3d26e 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -370,14 +370,14 @@ export default React.createClass({ componentWillUpdate: function(props, state) { if (this.shouldTrackPageChange(this.state, state)) { - Analytics.startPageChangeTimer(); + this.startPageChangeTimer(); } }, componentDidUpdate: function(prevProps, prevState) { if (this.shouldTrackPageChange(prevState, this.state)) { - Analytics.stopPageChangeTimer(); - Analytics.trackPageChange(); + const durationMs = this.stopPageChangeTimer(); + Analytics.trackPageChange(durationMs); } if (this.focusComposer) { dis.dispatch({action: 'focus_composer'}); @@ -385,6 +385,35 @@ export default React.createClass({ } }, + startPageChangeTimer() { + // This shouldn't happen because componentWillUpdate and componentDidUpdate + // are used. + if (this._pageChanging) { + console.warn('MatrixChat.startPageChangeTimer: timer already started'); + return; + } + this._pageChanging = true; + performance.mark('riot_MatrixChat_page_change_start'); + }, + + stopPageChangeTimer() { + if (!this._pageChanging) { + console.warn('MatrixChat.stopPageChangeTimer: timer not started'); + return; + } + this._pageChanging = false; + performance.mark('riot_MatrixChat_page_change_stop'); + performance.measure( + 'riot_MatrixChat_page_change_delta', + 'riot_MatrixChat_page_change_start', + 'riot_MatrixChat_page_change_stop', + ); + performance.clearMarks('riot_MatrixChat_page_change_start'); + performance.clearMarks('riot_MatrixChat_page_change_stop'); + const measurement = performance.getEntriesByName('riot_MatrixChat_page_change_delta').pop(); + return measurement.duration; + }, + shouldTrackPageChange(prevState, state) { return prevState.currentRoomId !== state.currentRoomId || prevState.view !== state.view || From 187e8ab8a8d07b6efc63801fc356133c154c6fa8 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 28 Mar 2018 10:27:24 +0100 Subject: [PATCH 3/5] Remove unused variable --- src/Analytics.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Analytics.js b/src/Analytics.js index bb9f900145..bb49c19cf5 100644 --- a/src/Analytics.js +++ b/src/Analytics.js @@ -74,7 +74,6 @@ class Analytics { this._paq = null; this.disabled = true; this.firstPage = true; - this.generationTimeMs = null; } /** From f29b58aba542350f2729bcd61932b6a5ebb41bc4 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 28 Mar 2018 10:31:03 +0100 Subject: [PATCH 4/5] Always expect generationTimeMs --- src/Analytics.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Analytics.js b/src/Analytics.js index bb49c19cf5..2ef058b11b 100644 --- a/src/Analytics.js +++ b/src/Analytics.js @@ -148,6 +148,9 @@ class Analytics { } trackPageChange(generationTimeMs) { + if (typeof generationTimeMs !== 'number') { + throw new Error('Analytics.trackPageChange: expected generationTimeMs to be a number'); + } if (this.disabled) return; if (this.firstPage) { // De-duplicate first page @@ -156,9 +159,7 @@ class Analytics { return; } this._paq.push(['setCustomUrl', getRedactedUrl()]); - if (typeof generationTimeMs === 'number') { - this._paq.push(['setGenerationTimeMs', generationTimeMs]); - } + this._paq.push(['setGenerationTimeMs', generationTimeMs]); this._paq.push(['trackPageView']); } From 00167fbc0622e2b2a08247622bd00229b6e67ed4 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 28 Mar 2018 11:25:28 +0100 Subject: [PATCH 5/5] Init page change state in willMount --- src/components/structures/MatrixChat.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 2c38e3d26e..92baecb787 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -291,6 +291,8 @@ export default React.createClass({ this.handleResize(); window.addEventListener('resize', this.handleResize); + this._pageChanging = false; + // check we have the right tint applied for this theme. // N.B. we don't call the whole of setTheme() here as we may be // racing with the theme CSS download finishing from index.js