Merge pull request #2027 from matrix-org/luke/track-uisis-piwik-improved

Improve tracking of UISIs

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() {

src/DecryptionFailureTracker.js

@@ -40,16 +40,15 @@ export default class DecryptionFailureTracker {
     checkInterval = null;
     trackInterval = null;
 
-    // Spread the load on `Analytics` by sending at most 1 event per
+    // Spread the load on `Analytics` by tracking at a low frequency, `TRACK_INTERVAL_MS`.
-    // `TRACK_INTERVAL_MS`.
+    static TRACK_INTERVAL_MS = 60000;
-    static TRACK_INTERVAL_MS = 1000;
 
     // Call `checkFailures` every `CHECK_INTERVAL_MS`.
     static CHECK_INTERVAL_MS = 5000;
 
     // Give events a chance to be decrypted by waiting `GRACE_PERIOD_MS` before moving
     // the failure to `failuresToTrack`.
-    static GRACE_PERIOD_MS = 5000;
+    static GRACE_PERIOD_MS = 60000;
 
     constructor(fn) {
         if (!fn || typeof fn !== 'function') {
@@ -158,12 +157,16 @@ export default class DecryptionFailureTracker {
     }
 
     /**
-     * If there is a failure that should be tracked, call the given trackDecryptionFailure
+     * If there are failures that should be tracked, call the given trackDecryptionFailure
-     * function with the first failure in the FIFO of failures that should be tracked.
+     * function with the number of failures that should be tracked.
      */
     trackFailure() {
         if (this.failuresToTrack.length > 0) {
-            this.trackDecryptionFailure(this.failuresToTrack.shift());
+            // Remove all failures, and expose the number of failures for now.
+            //
+            // TODO: Track a histogram of error types to cardinailty to allow for
+            // aggregation by error type.
+            this.trackDecryptionFailure(this.failuresToTrack.splice(0).length);
         }
     }
 }

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

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) => {
+        let count = 0;
-            trackedFailure = failure;
+        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 = [];
+        let count = 0;
-        const tracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+        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 = [];
+        let count = 0;
-        const tracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+        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();
     });