From 01dd3879702ed19b60d882b183338b62c797d967 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 28 Jun 2018 15:03:47 +0100 Subject: [PATCH] Track UISIs in bulk Piwik supports sending an event value, which we can use to indicate cardinality of UISIs to be tracked instead of tracking them individually. This means we can track them at a lower frequency of (fairly arbitrary) 60s. --- src/Analytics.js | 4 +- src/DecryptionFailureTracker.js | 8 ++-- src/components/structures/MatrixChat.js | 4 +- test/DecryptionFailureTracker-test.js | 52 +++++++------------------ 4 files changed, 21 insertions(+), 47 deletions(-) diff --git a/src/Analytics.js b/src/Analytics.js index fbc8fb79c8..d85d635b28 100644 --- a/src/Analytics.js +++ b/src/Analytics.js @@ -199,9 +199,9 @@ class Analytics { this._paq.push(['trackPageView']); } - trackEvent(category, action, name) { + trackEvent(category, action, name, value) { if (this.disabled) return; - this._paq.push(['trackEvent', category, action, name]); + this._paq.push(['trackEvent', category, action, name, value]); } logout() { diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js index b1c6a71289..447562af26 100644 --- a/src/DecryptionFailureTracker.js +++ b/src/DecryptionFailureTracker.js @@ -40,9 +40,8 @@ export default class DecryptionFailureTracker { checkInterval = null; trackInterval = null; - // Spread the load on `Analytics` by sending at most 1 event per - // `TRACK_INTERVAL_MS`. - static TRACK_INTERVAL_MS = 1000; + // Spread the load on `Analytics` by tracking at a low frequency, `TRACK_INTERVAL_MS`. + static TRACK_INTERVAL_MS = 60000; // Call `checkFailures` every `CHECK_INTERVAL_MS`. static CHECK_INTERVAL_MS = 5000; @@ -163,7 +162,8 @@ export default class DecryptionFailureTracker { */ trackFailure() { if (this.failuresToTrack.length > 0) { - this.trackDecryptionFailure(this.failuresToTrack.shift()); + // Remove all failures, and expose the number of failures + this.trackDecryptionFailure(this.failuresToTrack.splice(0).length); } } } diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 4b8b75ad74..293b0130d0 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -1304,9 +1304,9 @@ export default React.createClass({ } }); - const dft = new DecryptionFailureTracker((failure) => { + const dft = new DecryptionFailureTracker((total) => { // TODO: Pass reason for failure as third argument to trackEvent - Analytics.trackEvent('E2E', 'Decryption failure'); + Analytics.trackEvent('E2E', 'Decryption failure', null, total); }); // Shelved for later date when we have time to think about persisting history of diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js index c4f3116cba..4979fb9bb4 100644 --- a/test/DecryptionFailureTracker-test.js +++ b/test/DecryptionFailureTracker-test.js @@ -33,10 +33,9 @@ function createFailedDecryptionEvent() { describe('DecryptionFailureTracker', function() { it('tracks a failed decryption', function(done) { const failedDecryptionEvent = createFailedDecryptionEvent(); - let trackedFailure = null; - const tracker = new DecryptionFailureTracker((failure) => { - trackedFailure = failure; - }); + + let count = 0; + const tracker = new DecryptionFailureTracker((total) => count += total); tracker.eventDecrypted(failedDecryptionEvent); @@ -46,14 +45,14 @@ describe('DecryptionFailureTracker', function() { // Immediately track the newest failure, if there is one tracker.trackFailure(); - expect(trackedFailure).toNotBe(null, 'should track a failure for an event that failed decryption'); + expect(count).toNotBe(0, 'should track a failure for an event that failed decryption'); done(); }); it('does not track a failed decryption where the event is subsequently successfully decrypted', (done) => { const decryptedEvent = createFailedDecryptionEvent(); - const tracker = new DecryptionFailureTracker((failure) => { + const tracker = new DecryptionFailureTracker((total) => { expect(true).toBe(false, 'should not track an event that has since been decrypted correctly'); }); @@ -76,7 +75,7 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent2 = createFailedDecryptionEvent(); let count = 0; - const tracker = new DecryptionFailureTracker((failure) => count++); + const tracker = new DecryptionFailureTracker((total) => count += total); // Arbitrary number of failed decryptions for both events tracker.eventDecrypted(decryptedEvent); @@ -102,36 +101,11 @@ describe('DecryptionFailureTracker', function() { done(); }); - it('track failures in the order they occured', (done) => { - const decryptedEvent = createFailedDecryptionEvent(); - const decryptedEvent2 = createFailedDecryptionEvent(); - - const failures = []; - const tracker = new DecryptionFailureTracker((failure) => failures.push(failure)); - - // Indicate decryption - tracker.eventDecrypted(decryptedEvent); - tracker.eventDecrypted(decryptedEvent2); - - // Pretend "now" is Infinity - tracker.checkFailures(Infinity); - - // Simulated polling of `trackFailure`, an arbitrary number ( > 2 ) times - tracker.trackFailure(); - tracker.trackFailure(); - - expect(failures.length).toBe(2, 'expected 2 failures to be tracked, got ' + failures.length); - expect(failures[0].failedEventId).toBe(decryptedEvent.getId(), 'the first failure should be tracked first'); - expect(failures[1].failedEventId).toBe(decryptedEvent2.getId(), 'the second failure should be tracked second'); - - done(); - }); - it('should not track a failure for an event that was tracked previously', (done) => { const decryptedEvent = createFailedDecryptionEvent(); - const failures = []; - const tracker = new DecryptionFailureTracker((failure) => failures.push(failure)); + let count = 0; + const tracker = new DecryptionFailureTracker((total) => count += total); // Indicate decryption tracker.eventDecrypted(decryptedEvent); @@ -146,7 +120,7 @@ describe('DecryptionFailureTracker', function() { tracker.trackFailure(); - expect(failures.length).toBe(1, 'should only track a single failure per event'); + expect(count).toBe(1, 'should only track a single failure per event'); done(); }); @@ -157,8 +131,8 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent = createFailedDecryptionEvent(); - const failures = []; - const tracker = new DecryptionFailureTracker((failure) => failures.push(failure)); + let count = 0; + const tracker = new DecryptionFailureTracker((total) => count += total); // Indicate decryption tracker.eventDecrypted(decryptedEvent); @@ -170,7 +144,7 @@ describe('DecryptionFailureTracker', function() { tracker.trackFailure(); // Simulate the browser refreshing by destroying tracker and creating a new tracker - const secondTracker = new DecryptionFailureTracker((failure) => failures.push(failure)); + const secondTracker = new DecryptionFailureTracker((total) => count += total); //secondTracker.loadTrackedEventHashMap(); @@ -178,7 +152,7 @@ describe('DecryptionFailureTracker', function() { secondTracker.checkFailures(Infinity); secondTracker.trackFailure(); - expect(failures.length).toBe(1, 'should track a single failure per event per session, got ' + failures.length); + expect(count).toBe(1, count + ' failures tracked, should only track a single failure per event'); done(); });