Report crypto sdk in posthog #11834
Report crypto sdk in posthog #11834
Conversation
BillCarsonFr
added
the
T-Task
src/PosthogAnalytics.ts
Outdated
| // Update super property for cryptoSDK in posthog. | ||
| // This property will be subsequently passed in every event. | ||
| const value = this.getCryptoSDKPropertyValue(); | ||
| this.registerSuperProperties({ cryptoSDK: value }); |
There was a problem hiding this comment.
Don't these clobber between sessions of the same account?
There was a problem hiding this comment.
No I tested (same users with a rust and a legacy "device")
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
src/PosthogAnalytics.ts
Outdated
| // 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; |
There was a problem hiding this comment.
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
src/PosthogAnalytics.ts
Outdated
| if (["feature_rust_crypto"].includes(settingsPayload.settingName)) { | ||
| this.updateCryptoSuperProperty(); | ||
| } |
There was a problem hiding this comment.
This code will never fire, no? Given the feature is not runtime mutable and must be set via config.
There was a problem hiding this comment.
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
Fixes element-hq/element-web#26449
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.