From 43f264ccfcdb923338406a4452e9741bacfa7f6d Mon Sep 17 00:00:00 2001 From: James Salter Date: Mon, 6 Dec 2021 21:43:42 +1100 Subject: [PATCH] Integrate analytics stubs (#7186) * Add matrix-analytics-events as a dependency * Make IEvent look like a stub definition * Update pageview tracking to track screens, using a hypothetical definition of a screen event * Remove distinction between pseudo and anon tracking, will need to rework it considering stubs --- package.json | 1 + src/DecryptionFailureTracker.ts | 13 ++-- src/PosthogAnalytics.ts | 74 ++++------------------ src/components/structures/MatrixChat.tsx | 47 ++++++++++++-- test/DecryptionFailureTracker-test.js | 31 +++++----- test/PosthogAnalytics-test.ts | 79 ++++++------------------ yarn.lock | 4 ++ 7 files changed, 101 insertions(+), 148 deletions(-) diff --git a/package.json b/package.json index 8b41dd61f9..ea04ab2f8b 100644 --- a/package.json +++ b/package.json @@ -84,6 +84,7 @@ "katex": "^0.12.0", "linkifyjs": "^2.1.9", "lodash": "^4.17.20", + "matrix-analytics-events": "https://github.com/matrix-org/matrix-analytics-events.git#1eab4356548c97722a183912fda1ceabbe8cc7c1", "maplibre-gl": "^1.15.2", "matrix-js-sdk": "github:matrix-org/matrix-js-sdk#develop", "matrix-widget-api": "^0.1.0-beta.18", diff --git a/src/DecryptionFailureTracker.ts b/src/DecryptionFailureTracker.ts index df306a54f5..dc8fdc5739 100644 --- a/src/DecryptionFailureTracker.ts +++ b/src/DecryptionFailureTracker.ts @@ -25,8 +25,11 @@ export class DecryptionFailure { } } -type TrackingFn = (count: number, trackedErrCode: string) => void; -type ErrCodeMapFn = (errcode: string) => string; +type ErrorCode = "OlmKeysNotSentError" | "OlmIndexError" | "UnknownError" | "OlmUnspecifiedError"; + +type TrackingFn = (count: number, trackedErrCode: ErrorCode) => void; + +export type ErrCodeMapFn = (errcode: string) => ErrorCode; export class DecryptionFailureTracker { // Array of items of type DecryptionFailure. Every `CHECK_INTERVAL_MS`, this list @@ -73,12 +76,12 @@ export class DecryptionFailureTracker { * @param {function?} errorCodeMapFn The function used to map error codes to the * trackedErrorCode. If not provided, the `.code` of errors will be used. */ - constructor(private readonly fn: TrackingFn, private readonly errorCodeMapFn?: ErrCodeMapFn) { + constructor(private readonly fn: TrackingFn, private readonly errorCodeMapFn: ErrCodeMapFn) { if (!fn || typeof fn !== 'function') { throw new Error('DecryptionFailureTracker requires tracking function'); } - if (errorCodeMapFn && typeof errorCodeMapFn !== 'function') { + if (typeof errorCodeMapFn !== 'function') { throw new Error('DecryptionFailureTracker second constructor argument should be a function'); } } @@ -195,7 +198,7 @@ export class DecryptionFailureTracker { public trackFailures(): void { for (const errorCode of Object.keys(this.failureCounts)) { if (this.failureCounts[errorCode] > 0) { - const trackedErrorCode = this.errorCodeMapFn ? this.errorCodeMapFn(errorCode) : errorCode; + const trackedErrorCode = this.errorCodeMapFn(errorCode); this.fn(this.failureCounts[errorCode], trackedErrorCode); this.failureCounts[errorCode] = 0; diff --git a/src/PosthogAnalytics.ts b/src/PosthogAnalytics.ts index 5619d3f0b2..45d2cd692f 100644 --- a/src/PosthogAnalytics.ts +++ b/src/PosthogAnalytics.ts @@ -40,12 +40,8 @@ import SettingsStore from "./settings/SettingsStore"; */ interface IEvent { - // The event name that will be used by PostHog. Event names should use snake_case. + // The event name that will be used by PostHog. Event names should use camelCase. eventName: string; - - // The properties of the event that will be stored in PostHog. This is just a placeholder, - // extending interfaces must override this with a concrete definition to do type validation. - properties: {}; } export enum Anonymity { @@ -54,39 +50,16 @@ export enum Anonymity { Pseudonymous } -// If an event extends IPseudonymousEvent, the event contains pseudonymous data -// that won't be sent unless the user has explicitly consented to pseudonymous tracking. -// For example, it might contain hashed user IDs or room IDs. -// Such events will be automatically dropped if PosthogAnalytics.anonymity isn't set to Pseudonymous. -export interface IPseudonymousEvent extends IEvent {} - -// If an event extends IAnonymousEvent, the event strictly contains *only* anonymous data; -// i.e. no identifiers that can be associated with the user. -export interface IAnonymousEvent extends IEvent {} - -export interface IRoomEvent extends IPseudonymousEvent { - hashedRoomId: string; -} - -interface IPageView extends IAnonymousEvent { - eventName: "$pageview"; - properties: { - durationMs?: number; - screen?: string; - }; -} - const whitelistedScreens = new Set([ "register", "login", "forgot_password", "soft_logout", "new", "settings", "welcome", "home", "start", "directory", "start_sso", "start_cas", "groups", "complete_security", "post_registration", "room", "user", "group", ]); -export async function getRedactedCurrentLocation( +export function getRedactedCurrentLocation( origin: string, hash: string, pathname: string, - anonymity: Anonymity, -): Promise { +): string { // Redact PII from the current location. // For known screens, assumes a URL structure of //might/be/pii if (origin.startsWith('file://')) { @@ -219,13 +192,12 @@ export class PosthogAnalytics { }; } - private async capture(eventName: string, properties: posthog.Properties) { + private capture(eventName: string, properties: posthog.Properties) { if (!this.enabled) { return; } const { origin, hash, pathname } = window.location; - properties['$redacted_current_url'] = await getRedactedCurrentLocation( - origin, hash, pathname, this.anonymity); + properties['$redacted_current_url'] = getRedactedCurrentLocation(origin, hash, pathname); this.posthog.capture(eventName, properties); } @@ -288,35 +260,13 @@ export class PosthogAnalytics { this.setAnonymity(Anonymity.Disabled); } - public async trackPseudonymousEvent( - eventName: E["eventName"], - properties: E["properties"] = {}, - ) { - if (this.anonymity == Anonymity.Anonymous || this.anonymity == Anonymity.Disabled) return; - await this.capture(eventName, properties); - } - - public async trackAnonymousEvent( - eventName: E["eventName"], - properties: E["properties"] = {}, - ): Promise { - if (this.anonymity == Anonymity.Disabled) return; - await this.capture(eventName, properties); - } - - public async trackPageView(durationMs: number): Promise { - const hash = window.location.hash; - - let screen = null; - const split = hash.split("/"); - if (split.length >= 2) { - screen = split[1]; - } - - await this.trackAnonymousEvent("$pageview", { - durationMs, - screen, - }); + public trackEvent( + event: E, + ): void { + if (this.anonymity == Anonymity.Disabled || this.anonymity == Anonymity.Anonymous) return; + const eventWithoutName = { ...event }; + delete eventWithoutName.eventName; + this.capture(event.eventName, eventWithoutName); } public async updatePlatformSuperProperties(): Promise { diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 241e5d580b..8912837f0c 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -18,6 +18,8 @@ import React, { ComponentType, createRef } from 'react'; import { createClient } from "matrix-js-sdk/src/matrix"; import { InvalidStoreError } from "matrix-js-sdk/src/errors"; import { MatrixEvent } from "matrix-js-sdk/src/models/event"; +import { Error as ErrorEvent } from "matrix-analytics-events/types/typescript/Error"; +import { Screen as ScreenEvent } from "matrix-analytics-events/types/typescript/Screen"; import { defer, IDeferred, QueryDict } from "matrix-js-sdk/src/utils"; // focus-visible is a Polyfill for the :focus-visible CSS pseudo-attribute used by _AccessibleButton.scss @@ -446,7 +448,7 @@ export default class MatrixChat extends React.PureComponent { const durationMs = this.stopPageChangeTimer(); Analytics.trackPageChange(durationMs); CountlyAnalytics.instance.trackPageChange(durationMs); - PosthogAnalytics.instance.trackPageView(durationMs); + this.trackScreenChange(durationMs); } if (this.focusComposer) { dis.fire(Action.FocusSendMessageComposer); @@ -465,6 +467,36 @@ export default class MatrixChat extends React.PureComponent { if (this.accountPasswordTimer !== null) clearTimeout(this.accountPasswordTimer); } + public trackScreenChange(durationMs: number): void { + const notLoggedInMap = {}; + notLoggedInMap[Views.LOADING] = "WebLoading"; + notLoggedInMap[Views.WELCOME] = "WebWelcome"; + notLoggedInMap[Views.LOGIN] = "WebLogin"; + notLoggedInMap[Views.REGISTER] = "WebRegister"; + notLoggedInMap[Views.FORGOT_PASSWORD] = "WebForgotPassword"; + notLoggedInMap[Views.COMPLETE_SECURITY] = "WebCompleteSecurity"; + notLoggedInMap[Views.E2E_SETUP] = "WebE2ESetup"; + notLoggedInMap[Views.SOFT_LOGOUT] = "WebSoftLogout"; + + const loggedInPageTypeMap = {}; + loggedInPageTypeMap[PageType.HomePage] = "Home"; + loggedInPageTypeMap[PageType.RoomView] = "Room"; + loggedInPageTypeMap[PageType.RoomDirectory] = "RoomDirectory"; + loggedInPageTypeMap[PageType.UserView] = "User"; + loggedInPageTypeMap[PageType.GroupView] = "Group"; + loggedInPageTypeMap[PageType.MyGroups] = "MyGroups"; + + const screenName = this.state.view === Views.LOGGED_IN ? + loggedInPageTypeMap[this.state.page_type] : + notLoggedInMap[this.state.view]; + + return PosthogAnalytics.instance.trackEvent({ + eventName: "Screen", + screenName, + durationMs, + }); + } + getFallbackHsUrl() { if (this.props.serverConfig && this.props.serverConfig.isDefault) { return this.props.config.fallback_hs_url; @@ -1595,17 +1627,22 @@ export default class MatrixChat extends React.PureComponent { const dft = new DecryptionFailureTracker((total, errorCode) => { Analytics.trackEvent('E2E', 'Decryption failure', errorCode, String(total)); CountlyAnalytics.instance.track("decryption_failure", { errorCode }, null, { sum: total }); + PosthogAnalytics.instance.trackEvent({ + eventName: "Error", + domain: "E2EE", + name: errorCode, + }); }, (errorCode) => { // Map JS-SDK error codes to tracker codes for aggregation switch (errorCode) { case 'MEGOLM_UNKNOWN_INBOUND_SESSION_ID': - return 'olm_keys_not_sent_error'; + return 'OlmKeysNotSentError'; case 'OLM_UNKNOWN_MESSAGE_INDEX': - return 'olm_index_error'; + return 'OlmIndexError'; case undefined: - return 'unexpected_error'; + return 'OlmUnspecifiedError'; default: - return 'unspecified_error'; + return 'UnknownError'; } }); diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js index bc751ba44e..4f0e4d2f09 100644 --- a/test/DecryptionFailureTracker-test.js +++ b/test/DecryptionFailureTracker-test.js @@ -39,7 +39,7 @@ describe('DecryptionFailureTracker', function() { const failedDecryptionEvent = createFailedDecryptionEvent(); let count = 0; - const tracker = new DecryptionFailureTracker((total) => count += total); + const tracker = new DecryptionFailureTracker((total) => count += total, () => "UnknownError"); const err = new MockDecryptionError(); tracker.eventDecrypted(failedDecryptionEvent, err); @@ -59,7 +59,7 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent = createFailedDecryptionEvent(); const tracker = new DecryptionFailureTracker((total) => { expect(true).toBe(false, 'should not track an event that has since been decrypted correctly'); - }); + }, () => "UnknownError"); const err = new MockDecryptionError(); tracker.eventDecrypted(decryptedEvent, err); @@ -81,7 +81,7 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent2 = createFailedDecryptionEvent(); let count = 0; - const tracker = new DecryptionFailureTracker((total) => count += total); + const tracker = new DecryptionFailureTracker((total) => count += total, () => "UnknownError"); // Arbitrary number of failed decryptions for both events const err = new MockDecryptionError(); @@ -112,7 +112,7 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent = createFailedDecryptionEvent(); let count = 0; - const tracker = new DecryptionFailureTracker((total) => count += total); + const tracker = new DecryptionFailureTracker((total) => count += total, () => "UnknownError"); // Indicate decryption const err = new MockDecryptionError(); @@ -140,7 +140,7 @@ describe('DecryptionFailureTracker', function() { const decryptedEvent = createFailedDecryptionEvent(); let count = 0; - const tracker = new DecryptionFailureTracker((total) => count += total); + const tracker = new DecryptionFailureTracker((total) => count += total, () => "UnknownError"); // Indicate decryption const err = new MockDecryptionError(); @@ -153,7 +153,7 @@ describe('DecryptionFailureTracker', function() { tracker.trackFailures(); // Simulate the browser refreshing by destroying tracker and creating a new tracker - const secondTracker = new DecryptionFailureTracker((total) => count += total); + const secondTracker = new DecryptionFailureTracker((total) => count += total, () => "UnknownError"); //secondTracker.loadTrackedEventHashMap(); @@ -170,28 +170,29 @@ describe('DecryptionFailureTracker', function() { const counts = {}; const tracker = new DecryptionFailureTracker( (total, errorCode) => counts[errorCode] = (counts[errorCode] || 0) + total, + (error) => error === "UnknownError" ? "UnknownError" : "OlmKeysNotSentError", ); // One failure of ERROR_CODE_1, and effectively two for ERROR_CODE_2 - tracker.addDecryptionFailure(new DecryptionFailure('$event_id1', 'ERROR_CODE_1')); - tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'ERROR_CODE_2')); - tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'ERROR_CODE_2')); - tracker.addDecryptionFailure(new DecryptionFailure('$event_id3', 'ERROR_CODE_2')); + tracker.addDecryptionFailure(new DecryptionFailure('$event_id1', 'UnknownError')); + tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'OlmKeysNotSentError')); + tracker.addDecryptionFailure(new DecryptionFailure('$event_id2', 'OlmKeysNotSentError')); + tracker.addDecryptionFailure(new DecryptionFailure('$event_id3', 'OlmKeysNotSentError')); // Pretend "now" is Infinity tracker.checkFailures(Infinity); tracker.trackFailures(); - expect(counts['ERROR_CODE_1']).toBe(1, 'should track one ERROR_CODE_1'); - expect(counts['ERROR_CODE_2']).toBe(2, 'should track two ERROR_CODE_2'); + expect(counts['UnknownError']).toBe(1, 'should track one UnknownError'); + expect(counts['OlmKeysNotSentError']).toBe(2, 'should track two OlmKeysNotSentError'); }); it('should map error codes correctly', () => { const counts = {}; const tracker = new DecryptionFailureTracker( (total, errorCode) => counts[errorCode] = (counts[errorCode] || 0) + total, - (errorCode) => 'MY_NEW_ERROR_CODE', + (errorCode) => 'OlmUnspecifiedError', ); // One failure of ERROR_CODE_1, and effectively two for ERROR_CODE_2 @@ -204,7 +205,7 @@ describe('DecryptionFailureTracker', function() { tracker.trackFailures(); - expect(counts['MY_NEW_ERROR_CODE']) - .toBe(3, 'should track three MY_NEW_ERROR_CODE, got ' + counts['MY_NEW_ERROR_CODE']); + expect(counts['OlmUnspecifiedError']) + .toBe(3, 'should track three OlmUnspecifiedError, got ' + counts['OlmUnspecifiedError']); }); }); diff --git a/test/PosthogAnalytics-test.ts b/test/PosthogAnalytics-test.ts index 65af8b51f3..336d9c5401 100644 --- a/test/PosthogAnalytics-test.ts +++ b/test/PosthogAnalytics-test.ts @@ -17,9 +17,7 @@ limitations under the License. import { Anonymity, getRedactedCurrentLocation, - IAnonymousEvent, - IPseudonymousEvent, - IRoomEvent, + IEvent, PosthogAnalytics, } from '../src/PosthogAnalytics'; @@ -41,25 +39,9 @@ class FakePosthog { } } -export interface ITestEvent extends IAnonymousEvent { - key: "jest_test_event"; - properties: { - foo: string; - }; -} - -export interface ITestPseudonymousEvent extends IPseudonymousEvent { - key: "jest_test_pseudo_event"; - properties: { - foo: string; - }; -} - -export interface ITestRoomEvent extends IRoomEvent { - key: "jest_test_room_event"; - properties: { - foo: string; - }; +export interface ITestEvent extends IEvent { + eventName: "JestTestEvents"; + foo: string; } describe("PosthogAnalytics", () => { @@ -127,27 +109,20 @@ describe("PosthogAnalytics", () => { analytics = new PosthogAnalytics(fakePosthog); }); - it("Should pass trackAnonymousEvent() to posthog", async () => { + it("Should pass event to posthog", () => { analytics.setAnonymity(Anonymity.Pseudonymous); - await analytics.trackAnonymousEvent("jest_test_event", { + analytics.trackEvent({ + eventName: "JestTestEvents", foo: "bar", }); - expect(fakePosthog.capture.mock.calls[0][0]).toBe("jest_test_event"); + expect(fakePosthog.capture.mock.calls[0][0]).toBe("JestTestEvents"); expect(fakePosthog.capture.mock.calls[0][1]["foo"]).toEqual("bar"); }); - it("Should pass trackPseudonymousEvent() to posthog", async () => { - analytics.setAnonymity(Anonymity.Pseudonymous); - await analytics.trackPseudonymousEvent("jest_test_pseudo_event", { - foo: "bar", - }); - expect(fakePosthog.capture.mock.calls[0][0]).toBe("jest_test_pseudo_event"); - expect(fakePosthog.capture.mock.calls[0][1]["foo"]).toEqual("bar"); - }); - - it("Should not track pseudonymous messages if anonymous", async () => { + it("Should not track events if anonymous", async () => { analytics.setAnonymity(Anonymity.Anonymous); - await analytics.trackPseudonymousEvent("jest_test_event", { + await analytics.trackEvent({ + eventName: "JestTestEvents", foo: "bar", }); expect(fakePosthog.capture.mock.calls.length).toBe(0); @@ -155,43 +130,25 @@ describe("PosthogAnalytics", () => { it("Should not track any events if disabled", async () => { analytics.setAnonymity(Anonymity.Disabled); - await analytics.trackPseudonymousEvent("jest_test_event", { + analytics.trackEvent({ + eventName: "JestTestEvents", foo: "bar", }); - await analytics.trackAnonymousEvent("jest_test_event", { - foo: "bar", - }); - await analytics.trackPageView(200); expect(fakePosthog.capture.mock.calls.length).toBe(0); }); - it("Should pseudonymise a location of a known screen", async () => { - const location = await getRedactedCurrentLocation( - "https://foo.bar", "#/register/some/pii", "/", Anonymity.Pseudonymous); + it("Should anonymise location of a known screen", async () => { + const location = getRedactedCurrentLocation("https://foo.bar", "#/register/some/pii", "/"); expect(location).toBe("https://foo.bar/#/register/"); }); - it("Should anonymise a location of a known screen", async () => { - const location = await getRedactedCurrentLocation( - "https://foo.bar", "#/register/some/pii", "/", Anonymity.Anonymous); - expect(location).toBe("https://foo.bar/#/register/"); - }); - - it("Should pseudonymise a location of an unknown screen", async () => { - const location = await getRedactedCurrentLocation( - "https://foo.bar", "#/not_a_screen_name/some/pii", "/", Anonymity.Pseudonymous); - expect(location).toBe("https://foo.bar/#//"); - }); - - it("Should anonymise a location of an unknown screen", async () => { - const location = await getRedactedCurrentLocation( - "https://foo.bar", "#/not_a_screen_name/some/pii", "/", Anonymity.Anonymous); + it("Should anonymise location of an unknown screen", async () => { + const location = getRedactedCurrentLocation("https://foo.bar", "#/not_a_screen_name/some/pii", "/"); expect(location).toBe("https://foo.bar/#//"); }); it("Should handle an empty hash", async () => { - const location = await getRedactedCurrentLocation( - "https://foo.bar", "", "/", Anonymity.Anonymous); + const location = getRedactedCurrentLocation("https://foo.bar", "", "/"); expect(location).toBe("https://foo.bar/"); }); diff --git a/yarn.lock b/yarn.lock index 0f94f38e55..35f4f717eb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5988,6 +5988,10 @@ mathml-tag-names@^2.1.3: resolved "https://registry.yarnpkg.com/mathml-tag-names/-/mathml-tag-names-2.1.3.tgz#4ddadd67308e780cf16a47685878ee27b736a0a3" integrity sha512-APMBEanjybaPzUrfqU0IMU5I0AswKMH7k8OTLs0vvV4KZpExkTkY87nR/zpbuTPj+gARop7aGUbl11pnDfW6xg== +"matrix-analytics-events@https://github.com/matrix-org/matrix-analytics-events.git#1eab4356548c97722a183912fda1ceabbe8cc7c1": + version "0.0.1" + resolved "https://github.com/matrix-org/matrix-analytics-events.git#1eab4356548c97722a183912fda1ceabbe8cc7c1" + "matrix-js-sdk@github:matrix-org/matrix-js-sdk#develop": version "15.1.1" resolved "https://codeload.github.com/matrix-org/matrix-js-sdk/tar.gz/55b489b17a519bc0d557c47ac9fe7ce54690b602"