From 01dd3879702ed19b60d882b183338b62c797d967 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Thu, 28 Jun 2018 15:03:47 +0100
Subject: [PATCH 1/3] 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();
});
From ab990d8cec6caba17b6716a12ad3d12bc67ddafd Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Thu, 28 Jun 2018 15:07:58 +0100
Subject: [PATCH 2/3] Increase grace period to allow time for key sharing
---
src/DecryptionFailureTracker.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 447562af26..58ba1ae8dc 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -48,7 +48,7 @@ export default class DecryptionFailureTracker {
// 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') {
From 3846aef8a1ce05fe1722122a7e63ce181ec48c54 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Thu, 28 Jun 2018 16:06:12 +0100
Subject: [PATCH 3/3] Alter docs, add comments
---
src/DecryptionFailureTracker.js | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 58ba1ae8dc..e7809d2f6c 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -157,12 +157,15 @@ export default class DecryptionFailureTracker {
}
/**
- * If there is a failure that should be tracked, call the given trackDecryptionFailure
- * function with the first failure in the FIFO of failures that should be tracked.
+ * If there are failures that should be tracked, call the given trackDecryptionFailure
+ * function with the number of failures that should be tracked.
*/
trackFailure() {
if (this.failuresToTrack.length > 0) {
- // Remove all failures, and expose the number of failures
+ // 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);
}
}