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

Report crypto sdk in posthog #11834

Merged
merged 7 commits into from
Nov 13, 2023
Merged

Report crypto sdk in posthog #11834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ae8f787
Report crypto sdk in posthog
BillCarsonFr Nov 8, 2023
3ccc67e
fix import
BillCarsonFr Nov 9, 2023
990dad7
Merge branch 'develop' into valere/element-r/posthog
BillCarsonFr Nov 9, 2023
219c6c4
do not use the setting as not live
BillCarsonFr Nov 9, 2023
2684f97
Merge branch 'develop' into valere/element-r/posthog
BillCarsonFr Nov 9, 2023
43a5225
Merge branch 'develop' into valere/element-r/posthog
BillCarsonFr Nov 10, 2023
ba14536
Merge branch 'develop' into valere/element-r/posthog
BillCarsonFr Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion src/PosthogAnalytics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ export class PosthogAnalytics {
private authenticationType: Signup["authenticationType"] = "Other";
private watchSettingRef?: string;

// Temporary flag until we can switch to the Rust SDK without restarting the app.
// Currently, you have to set up the flag then logout and login again to switch.
// On login the matrixClient is passed to the analytics object, and we can check the crypto backend version.
private hasLoggedInWithRustCrypto = false;
Copy link
Member Author

@BillCarsonFr BillCarsonFr Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewer: Ideally we should just listen to the feature_rust_crypto settings, and this is what we will do when we will have migration in place. But until then you need to enable that feature then logout and login, that's why there is this flag.

It will be set when the current MatrixClient is passed to the analytics object (on login / page load / ...), the trick is to look at the crypto version.

I am not sure if it's right to listen to the setting now as it's not yet working. Maybe it should for now just check the crypto version from matrix client for now, and then later when we have migration we use the setting?
Let me know and I will update

Update: Simplified, will now just check the actual crypto version


public static get instance(): PosthogAnalytics {
if (!this._instance) {
this._instance = new PosthogAnalytics(posthog);
Expand Down Expand Up @@ -169,7 +174,9 @@ export class PosthogAnalytics {
dis.register(this.onAction);
SettingsStore.monitorSetting("layout", null);
SettingsStore.monitorSetting("useCompactLayout", null);
SettingsStore.monitorSetting("feature_rust_crypto", null);
this.onLayoutUpdated();
this.updateCryptoSuperProperty();
}

private onLayoutUpdated = (): void => {
Expand Down Expand Up @@ -198,6 +205,9 @@ export class PosthogAnalytics {
if (["layout", "useCompactLayout"].includes(settingsPayload.settingName)) {
this.onLayoutUpdated();
}
if (["feature_rust_crypto"].includes(settingsPayload.settingName)) {
this.updateCryptoSuperProperty();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code will never fire, no? Given the feature is not runtime mutable and must be set via config.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I simplified all, it will probably not be fully live ever. Just using the matrixClient passed on load/login/account data and read the crypto version

};

// we persist the last `$screen_name` and send it for all events until it is replaced
Expand Down Expand Up @@ -251,6 +261,14 @@ export class PosthogAnalytics {
};
}

private getCryptoSDKPropertyValue(): "Rust" | "Legacy" {
// Until we have migration code to switch crypto sdk, we need to track which sdk is being used.
if (SettingsStore.getValue("feature_rust_crypto") && this.hasLoggedInWithRustCrypto) {
return "Rust";
}
return "Legacy";
}

// eslint-disable-nextline no-unused-vars
private capture(eventName: string, properties: Properties, options?: CaptureOptions): void {
if (!this.enabled) {
Expand Down Expand Up @@ -278,6 +296,8 @@ export class PosthogAnalytics {
this.registerSuperProperties(this.platformSuperProperties);
}
this.anonymity = anonymity;
// update anyhow, no-op if not enabled or Disabled.
this.updateCryptoSuperProperty();
}

private static getRandomAnalyticsId(): string {
Expand Down Expand Up @@ -367,7 +387,25 @@ export class PosthogAnalytics {
this.registerSuperProperties(this.platformSuperProperties);
}

private updateCryptoSuperProperty(): void {
if (!this.enabled || this.anonymity === Anonymity.Disabled) return;
// Update super property for cryptoSDK in posthog.
// This property will be subsequently passed in every event.
const value = this.getCryptoSDKPropertyValue();
this.registerSuperProperties({ cryptoSDK: value });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't these clobber between sessions of the same account?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I tested (same users with a rust and a legacy "device")
image

As per doc

However, please note that this does not store properties against the User, only against their events.

It's technically stored in a cookie:

Setting Super Properties creates a cookie on the client with the respective properties and their values.

There is a thing maybe annoying: if you switch to rust stack and we do PageView per unique user, the same user will report in both Legacy and Rust for some time. However for web insights we can use per Unique Session that could be better for us

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting Super Properties creates a cookie on the client with the respective properties and their values.

Then we can't use it without updating the cookie policy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting Super Properties creates a cookie on the client with the respective properties and their values.

Then we can't use it without updating the cookie policy.

We are already using superProperties for appVersion and appPlatform. So the cookie policy is already outdated?

}

public async updateAnonymityFromSettings(client: MatrixClient, pseudonymousOptIn: boolean): Promise<void> {
// Temporary until we have migration code to switch crypto sdk.
if (client.getCrypto()) {
const cryptoVersion = client.getCrypto()!.getVersion();
// version for rust is something like "Rust SDK 0.6.0 (9c6b550), Vodozemac 0.5.0"
// for legacy it will be 'Olm x.x.x"
if (cryptoVersion.indexOf("Rust SDK") != -1) {
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
this.hasLoggedInWithRustCrypto = true;
}
}

// Update this.anonymity based on the user's analytics opt-in settings
const anonymity = pseudonymousOptIn ? Anonymity.Pseudonymous : Anonymity.Disabled;
this.setAnonymity(anonymity);
Expand All @@ -379,7 +417,8 @@ export class PosthogAnalytics {
}

if (anonymity !== Anonymity.Disabled) {
await PosthogAnalytics.instance.updatePlatformSuperProperties();
await this.updatePlatformSuperProperties();
this.updateCryptoSuperProperty();
}
}

Expand Down
101 changes: 101 additions & 0 deletions test/PosthogAnalytics-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import defaultDispatcher from "../src/dispatcher/dispatcher";
import { Action } from "../src/dispatcher/actions";
import { SettingLevel } from "../src/settings/SettingLevel";
import { CryptoApi, MatrixClient } from "../../matrix-js-sdk";

Check failure on line 28 in test/PosthogAnalytics-test.ts

View workflow job for this annotation

GitHub Actions / Typescript Syntax Check 

Cannot find module '../../matrix-js-sdk' or its corresponding type declarations.

const getFakePosthog = (): PostHog =>
({
Expand All @@ -37,6 +38,7 @@
persistence: {
get_user_state: jest.fn(),
},
identifyUser: jest.fn(),
} as unknown as PostHog);

interface ITestEvent extends IPosthogEvent {
Expand Down Expand Up @@ -274,4 +276,103 @@
});
});
});

describe("CryptoSdk", () => {
let analytics: PosthogAnalytics;
const getFakeClient = (): MatrixClient =>
({
getCrypto: jest.fn(),
setAccountData: jest.fn(),
// just fake return an `im.vector.analytics` content
getAccountDataFromServer: jest.fn().mockReturnValue({
id: "0000000",
pseudonymousAnalyticsOptIn: true,
}),
} as unknown as MatrixClient);

beforeEach(async () => {
SdkConfig.put({
brand: "Testing",
posthog: {
project_api_key: "foo",
api_host: "bar",
},
});

analytics = new PosthogAnalytics(fakePosthog);
});

async function simulateLoginWorkaroundUntilMigration(pseudonymous = true) {
// To simulate a switch we call updateAnonymityFromSettings.
// As per documentation this function is called On login.
const mockClient = getFakeClient();
mocked(mockClient.getCrypto).mockReturnValue({
getVersion: () => "Rust SDK 0.6.0 (9c6b550), Vodozemac 0.5.0",
} as unknown as CryptoApi);
await analytics.updateAnonymityFromSettings(mockClient, pseudonymous);
}

it("should send rust cryptoSDK superProperty correctly", async () => {
analytics.setAnonymity(Anonymity.Pseudonymous);

await SettingsStore.setValue("feature_rust_crypto", null, SettingLevel.DEVICE, true);
defaultDispatcher.dispatch(
{
action: Action.SettingUpdated,
settingName: "feature_rust_crypto",
},
true,
);

// Has for now you need to logout and login again to make the switch, until then it sbould stay as Legacy
expect(mocked(fakePosthog).register.mock.lastCall![0]["cryptoSDK"]).toStrictEqual("Legacy");
// Simulate a logout / login with a rust crypto backend
await simulateLoginWorkaroundUntilMigration();

// Super Properties are properties associated with events that are set once and then sent with every capture call.
// They are set using posthog.register
expect(mocked(fakePosthog).register.mock.lastCall![0]["cryptoSDK"]).toStrictEqual("Rust");
});

it("should send Legacy cryptoSDK superProperty correctly", async () => {
analytics.setAnonymity(Anonymity.Pseudonymous);

await SettingsStore.setValue("feature_rust_crypto", null, SettingLevel.DEVICE, false);
defaultDispatcher.dispatch(
{
action: Action.SettingUpdated,
settingName: "feature_rust_crypto",
},
true,
);
// Super Properties are properties associated with events that are set once and then sent with every capture call.
// They are set using posthog.register
expect(mocked(fakePosthog).register.mock.lastCall![0]["cryptoSDK"]).toStrictEqual("Legacy");
});

it("should send cryptoSDK superProperty when enabling analytics", async () => {
analytics.setAnonymity(Anonymity.Disabled);

await SettingsStore.setValue("feature_rust_crypto", null, SettingLevel.DEVICE, true);

defaultDispatcher.dispatch(
{
action: Action.SettingUpdated,
settingName: "feature_rust_crypto",
},
true,
);

await simulateLoginWorkaroundUntilMigration(false);

// This initial call is due to the call to register platformSuperProperties
expect(mocked(fakePosthog).register.mock.lastCall![0]).toStrictEqual({});

// switching to pseudonymous should ensure that the cryptoSDK superProperty is set correctly
analytics.setAnonymity(Anonymity.Pseudonymous);
// Super Properties are properties associated with events that are set once and then sent with every capture call.
// They are set using posthog.register
expect(mocked(fakePosthog).register.mock.lastCall![0]["cryptoSDK"]).toStrictEqual("Rust");
});
});
});
Loading