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

Commit 983ea95

Browse files
dbkrdbkr
authored
Merge pull request #2045 from matrix-org/luke/track-decryption-failures-specific
Implement aggregation by error type for tracked decryption failures
2 parents 7f3b593 + 5af8ddc commit 983ea95

File tree

3 files changed

+156
-60
lines changed

3 files changed

+156
-60
lines changed

‎src/DecryptionFailureTracker.js‎

Lines changed: 56 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -14,22 +14,25 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
class DecryptionFailure {
18-
constructor(failedEventId) {
17+
export class DecryptionFailure {
18+
constructor(failedEventId, errorCode) {
1919
this.failedEventId = failedEventId;
20+
this.errorCode = errorCode;
2021
this.ts = Date.now();
2122
}
2223
}
2324

24-
export default class DecryptionFailureTracker {
25+
export class DecryptionFailureTracker {
2526
// Array of items of type DecryptionFailure. Every `CHECK_INTERVAL_MS`, this list
2627
// is checked for failures that happened > `GRACE_PERIOD_MS` ago. Those that did
27-
// are added to `failuresToTrack`.
28+
// are accumulated in `failureCounts`.
2829
failures = [];
2930

30-
// Every TRACK_INTERVAL_MS (so as to spread the number of hits done on Analytics),
31-
// one DecryptionFailure of this FIFO is removed and tracked.
32-
failuresToTrack = [];
31+
// A histogram of the number of failures that will be tracked at the next tracking
32+
// interval, split by failure error code.
33+
failureCounts = {
34+
// [errorCode]: 42
35+
};
3336

3437
// Event IDs of failures that were tracked previously
3538
trackedEventHashMap = {
@@ -46,16 +49,35 @@ export default class DecryptionFailureTracker {
4649
// Call `checkFailures` every `CHECK_INTERVAL_MS`.
4750
static CHECK_INTERVAL_MS = 5000;
4851

49-
// Give events a chance to be decrypted by waiting `GRACE_PERIOD_MS` before moving
50-
// the failure to `failuresToTrack`.
52+
// Give events a chance to be decrypted by waiting `GRACE_PERIOD_MS` before counting
53+
// the failure in `failureCounts`.
5154
static GRACE_PERIOD_MS = 60000;
5255

53-
constructor(fn) {
56+
/**
57+
* Create a new DecryptionFailureTracker.
58+
*
59+
* Call `eventDecrypted(event, err)` on this instance when an event is decrypted.
60+
*
61+
* Call `start()` to start the tracker, and `stop()` to stop tracking.
62+
*
63+
* @param {function} fn The tracking function, which will be called when failures
64+
* are tracked. The function should have a signature `(count, trackedErrorCode) => {...}`,
65+
* where `count` is the number of failures and `errorCode` matches the `.code` of
66+
* provided DecryptionError errors (by default, unless `errorCodeMapFn` is specified.
67+
* @param {function?} errorCodeMapFn The function used to map error codes to the
68+
* trackedErrorCode. If not provided, the `.code` of errors will be used.
69+
*/
70+
constructor(fn, errorCodeMapFn) {
5471
if (!fn || typeof fn !== 'function') {
5572
throw new Error('DecryptionFailureTracker requires tracking function');
5673
}
5774

58-
this.trackDecryptionFailure = fn;
75+
if (errorCodeMapFn && typeof errorCodeMapFn !== 'function') {
76+
throw new Error('DecryptionFailureTracker second constructor argument should be a function');
77+
}
78+
79+
this._trackDecryptionFailure = fn;
80+
this._mapErrorCode = errorCodeMapFn;
5981
}
6082

6183
// loadTrackedEventHashMap() {
@@ -66,17 +88,17 @@ export default class DecryptionFailureTracker {
6688
// localStorage.setItem('mx-decryption-failure-event-id-hashes', JSON.stringify(this.trackedEventHashMap));
6789
// }
6890

69-
eventDecrypted(e) {
70-
if (e.isDecryptionFailure()) {
71-
this.addDecryptionFailureForEvent(e);
91+
eventDecrypted(e, err) {
92+
if (err) {
93+
this.addDecryptionFailure(new DecryptionFailure(e.getId(), err.code));
7294
} else {
7395
// Could be an event in the failures, remove it
7496
this.removeDecryptionFailuresForEvent(e);
7597
}
7698
}
7799

78-
addDecryptionFailureForEvent(e) {
79-
this.failures.push(new DecryptionFailure(e.getId()));
100+
addDecryptionFailure(failure) {
101+
this.failures.push(failure);
80102
}
81103

82104
removeDecryptionFailuresForEvent(e) {
@@ -93,7 +115,7 @@ export default class DecryptionFailureTracker {
93115
);
94116

95117
this.trackInterval = setInterval(
96-
() => this.trackFailure(),
118+
() => this.trackFailures(),
97119
DecryptionFailureTracker.TRACK_INTERVAL_MS,
98120
);
99121
}
@@ -106,7 +128,7 @@ export default class DecryptionFailureTracker {
106128
clearInterval(this.trackInterval);
107129

108130
this.failures = [];
109-
this.failuresToTrack = [];
131+
this.failureCounts = {};
110132
}
111133

112134
/**
@@ -153,20 +175,28 @@ export default class DecryptionFailureTracker {
153175

154176
const dedupedFailures = dedupedFailuresMap.values();
155177

156-
this.failuresToTrack = [...this.failuresToTrack, ...dedupedFailures];
178+
this._aggregateFailures(dedupedFailures);
179+
}
180+
181+
_aggregateFailures(failures) {
182+
for (const failure of failures) {
183+
const errorCode = failure.errorCode;
184+
this.failureCounts[errorCode] = (this.failureCounts[errorCode] || 0) + 1;
185+
}
157186
}
158187

159188
/**
160189
* If there are failures that should be tracked, call the given trackDecryptionFailure
161190
* function with the number of failures that should be tracked.
162191
*/
163-
trackFailure() {
164-
if (this.failuresToTrack.length > 0) {
165-
// Remove all failures, and expose the number of failures for now.
166-
//
167-
// TODO: Track a histogram of error types to cardinailty to allow for
168-
// aggregation by error type.
169-
this.trackDecryptionFailure(this.failuresToTrack.splice(0).length);
192+
trackFailures() {
193+
for (const errorCode of Object.keys(this.failureCounts)) {
194+
if (this.failureCounts[errorCode] > 0) {
195+
const trackedErrorCode = this._mapErrorCode ? this._mapErrorCode(errorCode) : errorCode;
196+
197+
this._trackDecryptionFailure(this.failureCounts[errorCode], trackedErrorCode);
198+
this.failureCounts[errorCode] = 0;
199+
}
170200
}
171201
}
172202
}

‎src/components/structures/MatrixChat.js‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import PropTypes from 'prop-types';
2323
import Matrix from "matrix-js-sdk";
2424

2525
import Analytics from "../../Analytics";
26-
import DecryptionFailureTracker from "../../DecryptionFailureTracker";
26+
import { DecryptionFailureTracker } from "../../DecryptionFailureTracker";
2727
import MatrixClientPeg from "../../MatrixClientPeg";
2828
import PlatformPeg from "../../PlatformPeg";
2929
import SdkConfig from "../../SdkConfig";
@@ -1304,9 +1304,20 @@ export default React.createClass({
13041304
}
13051305
});
13061306

1307-
const dft = new DecryptionFailureTracker((total) => {
1308-
// TODO: Pass reason for failure as third argument to trackEvent
1309-
Analytics.trackEvent('E2E', 'Decryption failure', 'unspecified_error', total);
1307+
const dft = new DecryptionFailureTracker((total, errorCode) => {
1308+
Analytics.trackEvent('E2E', 'Decryption failure', errorCode, total);
1309+
}, (errorCode) => {
1310+
// Map JS-SDK error codes to tracker codes for aggregation
1311+
switch (errorCode) {
1312+
case 'MEGOLM_UNKNOWN_INBOUND_SESSION_ID':
1313+
return 'olm_keys_not_sent_error';
1314+
case 'OLM_UNKNOWN_MESSAGE_INDEX':
1315+
return 'olm_index_error';
1316+
case undefined:
1317+
return 'unexpected_error';
1318+
default:
1319+
return 'unspecified_error';
1320+
}
13101321
});
13111322

13121323
// Shelved for later date when we have time to think about persisting history of
@@ -1317,7 +1328,7 @@ export default React.createClass({
13171328

13181329
// When logging out, stop tracking failures and destroy state
13191330
cli.on("Session.logged_out", () => dft.stop());
1320-
cli.on("Event.decrypted", (e) => dft.eventDecrypted(e));
1331+
cli.on("Event.decrypted", (e, err) => dft.eventDecrypted(e, err));
13211332

13221333
const krh = new KeyRequestHandler(cli);
13231334
cli.on("crypto.roomKeyRequest", (req) => {

‎test/DecryptionFailureTracker-test.js‎

Lines changed: 84 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,18 @@ limitations under the License.
1616

1717
import expect from 'expect';
1818

19-
import DecryptionFailureTracker from '../src/DecryptionFailureTracker';
19+
import { DecryptionFailure, DecryptionFailureTracker } from '../src/DecryptionFailureTracker';
2020

2121
import { MatrixEvent } from 'matrix-js-sdk';
2222

23+
class MockDecryptionError extends Error {
24+
constructor(code) {
25+
super();
26+
27+
this.code = code || 'MOCK_DECRYPTION_ERROR';
28+
}
29+
}
30+
2331
function createFailedDecryptionEvent() {
2432
const event = new MatrixEvent({
2533
event_id: "event-id-" + Math.random().toString(16).slice(2),
@@ -37,13 +45,14 @@ describe('DecryptionFailureTracker', function() {
3745
let count = 0;
3846
const tracker = new DecryptionFailureTracker((total) => count += total);
3947

40-
tracker.eventDecrypted(failedDecryptionEvent);
48+
const err = new MockDecryptionError();
49+
tracker.eventDecrypted(failedDecryptionEvent, err);
4150

4251
// Pretend "now" is Infinity
4352
tracker.checkFailures(Infinity);
4453

45-
// Immediately track the newest failure, if there is one
46-
tracker.trackFailure();
54+
// Immediately track the newest failures
55+
tracker.trackFailures();
4756

4857
expect(count).toNotBe(0, 'should track a failure for an event that failed decryption');
4958

@@ -56,17 +65,18 @@ describe('DecryptionFailureTracker', function() {
5665
expect(true).toBe(false, 'should not track an event that has since been decrypted correctly');
5766
});
5867

59-
tracker.eventDecrypted(decryptedEvent);
68+
const err = new MockDecryptionError();
69+
tracker.eventDecrypted(decryptedEvent, err);
6070

6171
// Indicate successful decryption: clear data can be anything where the msgtype is not m.bad.encrypted
6272
decryptedEvent._setClearData({});
63-
tracker.eventDecrypted(decryptedEvent);
73+
tracker.eventDecrypted(decryptedEvent, null);
6474

6575
// Pretend "now" is Infinity
6676
tracker.checkFailures(Infinity);
6777

68-
// Immediately track the newest failure, if there is one
69-
tracker.trackFailure();
78+
// Immediately track the newest failures
79+
tracker.trackFailures();
7080
done();
7181
});
7282

@@ -78,23 +88,24 @@ describe('DecryptionFailureTracker', function() {
7888
const tracker = new DecryptionFailureTracker((total) => count += total);
7989

8090
// Arbitrary number of failed decryptions for both events
81-
tracker.eventDecrypted(decryptedEvent);
82-
tracker.eventDecrypted(decryptedEvent);
83-
tracker.eventDecrypted(decryptedEvent);
84-
tracker.eventDecrypted(decryptedEvent);
85-
tracker.eventDecrypted(decryptedEvent);
86-
tracker.eventDecrypted(decryptedEvent2);
87-
tracker.eventDecrypted(decryptedEvent2);
88-
tracker.eventDecrypted(decryptedEvent2);
91+
const err = new MockDecryptionError();
92+
tracker.eventDecrypted(decryptedEvent, err);
93+
tracker.eventDecrypted(decryptedEvent, err);
94+
tracker.eventDecrypted(decryptedEvent, err);
95+
tracker.eventDecrypted(decryptedEvent, err);
96+
tracker.eventDecrypted(decryptedEvent, err);
97+
tracker.eventDecrypted(decryptedEvent2, err);
98+
tracker.eventDecrypted(decryptedEvent2, err);
99+
tracker.eventDecrypted(decryptedEvent2, err);
89100

90101
// Pretend "now" is Infinity
91102
tracker.checkFailures(Infinity);
92103

93-
// Simulated polling of `trackFailure`, an arbitrary number ( > 2 ) times
94-
tracker.trackFailure();
95-
tracker.trackFailure();
96-
tracker.trackFailure();
97-
tracker.trackFailure();
104+
// Simulated polling of `trackFailures`, an arbitrary number ( > 2 ) times
105+
tracker.trackFailures();
106+
tracker.trackFailures();
107+
tracker.trackFailures();
108+
tracker.trackFailures();
98109

99110
expect(count).toBe(2, count + ' failures tracked, should only track a single failure per event');
100111

@@ -108,17 +119,18 @@ describe('DecryptionFailureTracker', function() {
108119
const tracker = new DecryptionFailureTracker((total) => count += total);
109120

110121
// Indicate decryption
111-
tracker.eventDecrypted(decryptedEvent);
122+
const err = new MockDecryptionError();
123+
tracker.eventDecrypted(decryptedEvent, err);
112124

113125
// Pretend "now" is Infinity
114126
tracker.checkFailures(Infinity);
115127

116-
tracker.trackFailure();
128+
tracker.trackFailures();
117129

118130
// Indicate a second decryption, after having tracked the failure
119-
tracker.eventDecrypted(decryptedEvent);
131+
tracker.eventDecrypted(decryptedEvent, err);
120132

121-
tracker.trackFailure();
133+
tracker.trackFailures();
122134

123135
expect(count).toBe(1, 'should only track a single failure per event');
124136

@@ -135,25 +147,68 @@ describe('DecryptionFailureTracker', function() {
135147
const tracker = new DecryptionFailureTracker((total) => count += total);
136148

137149
// Indicate decryption
138-
tracker.eventDecrypted(decryptedEvent);
150+
const err = new MockDecryptionError();
151+
tracker.eventDecrypted(decryptedEvent, err);
139152

140153
// Pretend "now" is Infinity
141154
// NB: This saves to localStorage specific to DFT
142155
tracker.checkFailures(Infinity);
143156

144-
tracker.trackFailure();
157+
tracker.trackFailures();
145158

146159
// Simulate the browser refreshing by destroying tracker and creating a new tracker
147160
const secondTracker = new DecryptionFailureTracker((total) => count += total);
148161

149162
//secondTracker.loadTrackedEventHashMap();
150163

151-
secondTracker.eventDecrypted(decryptedEvent);
164+
secondTracker.eventDecrypted(decryptedEvent, err);
152165
secondTracker.checkFailures(Infinity);
153-
secondTracker.trackFailure();
166+
secondTracker.trackFailures();
154167

155168
expect(count).toBe(1, count + ' failures tracked, should only track a single failure per event');
156169

157170
done();
158171
});
172+
173+
it('should count different error codes separately for multiple failures with different error codes', () => {
174+
const counts = {};
175+
const tracker = new DecryptionFailureTracker(
176+
(total, errorCode) => counts[errorCode] = (counts[errorCode] || 0) + total,
177+
);
178+
179+
// One failure of ERROR_CODE_1, and effectively two for ERROR_CODE_2
180+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id1', 'ERROR_CODE_1'));
181+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'ERROR_CODE_2'));
182+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'ERROR_CODE_2'));
183+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id3', 'ERROR_CODE_2'));
184+
185+
// Pretend "now" is Infinity
186+
tracker.checkFailures(Infinity);
187+
188+
tracker.trackFailures();
189+
190+
expect(counts['ERROR_CODE_1']).toBe(1, 'should track one ERROR_CODE_1');
191+
expect(counts['ERROR_CODE_2']).toBe(2, 'should track two ERROR_CODE_2');
192+
});
193+
194+
it('should map error codes correctly', () => {
195+
const counts = {};
196+
const tracker = new DecryptionFailureTracker(
197+
(total, errorCode) => counts[errorCode] = (counts[errorCode] || 0) + total,
198+
(errorCode) => 'MY_NEW_ERROR_CODE',
199+
);
200+
201+
// One failure of ERROR_CODE_1, and effectively two for ERROR_CODE_2
202+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id1', 'ERROR_CODE_1'));
203+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'ERROR_CODE_2'));
204+
tracker.addDecryptionFailure(new DecryptionFailure('$event_id3', 'ERROR_CODE_3'));
205+
206+
// Pretend "now" is Infinity
207+
tracker.checkFailures(Infinity);
208+
209+
tracker.trackFailures();
210+
211+
expect(counts['MY_NEW_ERROR_CODE'])
212+
.toBe(3, 'should track three MY_NEW_ERROR_CODE, got ' + counts['MY_NEW_ERROR_CODE']);
213+
});
159214
});

0 commit comments

Comments
 (0)