From 9074d70b55abde7e1bc645e0f9374429947f9010 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 8 Mar 2024 10:52:23 +0100 Subject: [PATCH 1/3] Quick improvement for analytics reporting --- .../features/analytics/AnalyticsTracker.kt | 5 ++- .../analytics/DecryptionFailureTracker.kt | 36 +++++++++++-------- .../analytics/impl/DefaultVectorAnalytics.kt | 32 +++++++++++++++-- .../timeline/factory/TimelineItemFactory.kt | 2 +- 4 files changed, 56 insertions(+), 19 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt b/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt index 871782e473c..333450d0d86 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt @@ -23,8 +23,11 @@ import im.vector.app.features.analytics.plan.UserProperties interface AnalyticsTracker { /** * Capture an Event. + * + * @param extraProperties Some extra properties to attach to the event, that are not part of the events definition + * (https://github.com/matrix-org/matrix-analytics-events/) and specific to this platform. */ - fun capture(event: VectorAnalyticsEvent) + fun capture(event: VectorAnalyticsEvent, extraProperties: Map? = null) /** * Track a displayed screen. diff --git a/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt b/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt index d596741d532..5da3618ead8 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt @@ -27,6 +27,7 @@ import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch +import org.matrix.android.sdk.api.session.Session import org.matrix.android.sdk.api.session.crypto.MXCryptoError import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent import javax.inject.Inject @@ -36,7 +37,9 @@ private data class DecryptionFailure( val timeStamp: Long, val roomId: String, val failedEventId: String, - val error: MXCryptoError.ErrorType + val error: MXCryptoError.ErrorType, + // Was the current session cross signed verified at the time of the error + val isCrossSignedVerified: Boolean = false ) private typealias DetailedErrorName = Pair @@ -75,11 +78,14 @@ class DecryptionFailureTracker @Inject constructor( scope.cancel() } - fun e2eEventDisplayedInTimeline(event: TimelineEvent) { + fun e2eEventDisplayedInTimeline(event: TimelineEvent, session: Session) { scope.launch(Dispatchers.Default) { val mCryptoError = event.root.mCryptoError if (mCryptoError != null) { - addDecryptionFailure(DecryptionFailure(clock.epochMillis(), event.roomId, event.eventId, mCryptoError)) + val isVerified = session.cryptoService().crossSigningService().isCrossSigningVerified() + addDecryptionFailure( + DecryptionFailure(clock.epochMillis(), event.roomId, event.eventId, mCryptoError, isVerified) + ) } else { removeFailureForEventId(event.eventId) } @@ -115,7 +121,7 @@ class DecryptionFailureTracker @Inject constructor( private fun checkFailures() { val now = clock.epochMillis() - val aggregatedErrors: Map> + val aggregatedErrors: Map> synchronized(failures) { val toReport = mutableListOf() failures.removeAll { failure -> @@ -129,7 +135,7 @@ class DecryptionFailureTracker @Inject constructor( aggregatedErrors = toReport .groupBy { it.error.toAnalyticsErrorName() } .mapValues { - it.value.map { it.failedEventId } + it.value } } @@ -137,15 +143,17 @@ class DecryptionFailureTracker @Inject constructor( // there is now way to send the total/sum in posthog, so iterating aggregation.value // for now we ignore events already reported even if displayed again? - .filter { alreadyReported.contains(it).not() } - .forEach { failedEventId -> - analyticsTracker.capture(Error( - context = aggregation.key.first, - domain = Error.Domain.E2EE, - name = aggregation.key.second, - cryptoModule = currentModule - )) - alreadyReported.add(failedEventId) + .filter { alreadyReported.contains(it.failedEventId).not() } + .forEach { failure -> + analyticsTracker.capture( + Error( + context = aggregation.key.first, + domain = Error.Domain.E2EE, + name = aggregation.key.second, + cryptoModule = currentModule, + ), mapOf("is_cross_signed_verified" to failure.isCrossSignedVerified.toString()) + ) + alreadyReported.add(failure.failedEventId) } } } diff --git a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt index 2a7d0ac975f..4a13f085e0f 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt @@ -20,6 +20,7 @@ import com.posthog.android.Options import com.posthog.android.PostHog import com.posthog.android.Properties import im.vector.app.core.di.NamedGlobalScope +import im.vector.app.core.resources.BuildMeta import im.vector.app.features.analytics.AnalyticsConfig import im.vector.app.features.analytics.VectorAnalytics import im.vector.app.features.analytics.itf.VectorAnalyticsEvent @@ -46,6 +47,7 @@ class DefaultVectorAnalytics @Inject constructor( private val analyticsConfig: AnalyticsConfig, private val analyticsStore: AnalyticsStore, private val lateInitUserPropertiesFactory: LateInitUserPropertiesFactory, + private val buildMeta: BuildMeta, @NamedGlobalScope private val globalScope: CoroutineScope ) : VectorAnalytics { @@ -68,9 +70,28 @@ class DefaultVectorAnalytics @Inject constructor( // Cache for the properties to send private var pendingUserProperties: UserProperties? = null + /** + * Super Properties are properties associated with events that are set once and then sent with every capture call. + */ + private var superProperties: MutableMap = HashMap() + override fun init() { observeUserConsent() observeAnalyticsId() + + initSuperProperties() + } + + /** + * Init the super properties that will be captured with all events. + */ + private fun initSuperProperties() { + // Put the appVersion (e.g 1.6.12). + superProperties["appVersion"] = buildMeta.versionName + // The appId (im.vector.app) + superProperties["applicationId"] = buildMeta.applicationId + // Parity with other platforms + superProperties["cryptoSDK"] = "Rust" } override fun getUserConsent(): Flow { @@ -171,18 +192,23 @@ class DefaultVectorAnalytics @Inject constructor( } } - override fun capture(event: VectorAnalyticsEvent) { + override fun capture(event: VectorAnalyticsEvent, extraProperties: Map?) { Timber.tag(analyticsTag.value).d("capture($event)") posthog ?.takeIf { userConsent == true } - ?.capture(event.getName(), event.getProperties()?.toPostHogProperties()) + ?.capture( + event.getName(), + (this.superProperties + event.getProperties().orEmpty() + extraProperties.orEmpty()).toPostHogProperties() + .toPostHogProperties()) } override fun screen(screen: VectorAnalyticsScreen) { Timber.tag(analyticsTag.value).d("screen($screen)") posthog ?.takeIf { userConsent == true } - ?.screen(screen.getName(), screen.getProperties()?.toPostHogProperties()) + ?.screen(screen.getName(), + (this.superProperties + screen.getProperties().orEmpty()).toPostHogProperties() + ) } override fun updateUserProperties(userProperties: UserProperties) { diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactory.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactory.kt index 84b71ceedfc..4fbaf404d99 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactory.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactory.kt @@ -159,7 +159,7 @@ class TimelineItemFactory @Inject constructor( } }.also { if (it != null && event.isEncrypted()) { - decryptionFailureTracker.e2eEventDisplayedInTimeline(event) + decryptionFailureTracker.e2eEventDisplayedInTimeline(event, session) } } } From dafd8d1bedeb2cb4f7b830e9c8f101ad1ef08b02 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 8 Mar 2024 10:58:39 +0100 Subject: [PATCH 2/3] Add appFlavor to super properties --- .../app/features/analytics/impl/DefaultVectorAnalytics.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt index 4a13f085e0f..5dcec27fa1d 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt @@ -90,6 +90,8 @@ class DefaultVectorAnalytics @Inject constructor( superProperties["appVersion"] = buildMeta.versionName // The appId (im.vector.app) superProperties["applicationId"] = buildMeta.applicationId + // The app flavor (GooglePlay, FDroid) + superProperties["appFlavor"] = buildMeta.flavorDescription // Parity with other platforms superProperties["cryptoSDK"] = "Rust" } From ba2327c57d5c2a38a04c7afc4baa505ea82c3026 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 19 Mar 2024 16:21:27 +0100 Subject: [PATCH 3/3] Cleanup and bugfixes: `toPostHogProperties()` was called twice. --- .../features/analytics/AnalyticsTracker.kt | 1 + .../analytics/DecryptionFailureTracker.kt | 7 ++-- .../analytics/impl/DefaultVectorAnalytics.kt | 40 ++++++++----------- 3 files changed, 22 insertions(+), 26 deletions(-) diff --git a/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt b/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt index 333450d0d86..dbf1970a466 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/AnalyticsTracker.kt @@ -24,6 +24,7 @@ interface AnalyticsTracker { /** * Capture an Event. * + * @param event The event to capture. * @param extraProperties Some extra properties to attach to the event, that are not part of the events definition * (https://github.com/matrix-org/matrix-analytics-events/) and specific to this platform. */ diff --git a/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt b/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt index 5da3618ead8..5d47ab89c7b 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/DecryptionFailureTracker.kt @@ -39,7 +39,7 @@ private data class DecryptionFailure( val failedEventId: String, val error: MXCryptoError.ErrorType, // Was the current session cross signed verified at the time of the error - val isCrossSignedVerified: Boolean = false + val isCrossSignedVerified: Boolean = false, ) private typealias DetailedErrorName = Pair @@ -146,12 +146,13 @@ class DecryptionFailureTracker @Inject constructor( .filter { alreadyReported.contains(it.failedEventId).not() } .forEach { failure -> analyticsTracker.capture( - Error( + event = Error( context = aggregation.key.first, domain = Error.Domain.E2EE, name = aggregation.key.second, cryptoModule = currentModule, - ), mapOf("is_cross_signed_verified" to failure.isCrossSignedVerified.toString()) + ), + extraProperties = mapOf("is_cross_signed_verified" to failure.isCrossSignedVerified.toString()) ) alreadyReported.add(failure.failedEventId) } diff --git a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt index 5dcec27fa1d..01dd8dff334 100644 --- a/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt +++ b/vector/src/main/java/im/vector/app/features/analytics/impl/DefaultVectorAnalytics.kt @@ -47,8 +47,8 @@ class DefaultVectorAnalytics @Inject constructor( private val analyticsConfig: AnalyticsConfig, private val analyticsStore: AnalyticsStore, private val lateInitUserPropertiesFactory: LateInitUserPropertiesFactory, - private val buildMeta: BuildMeta, - @NamedGlobalScope private val globalScope: CoroutineScope + @NamedGlobalScope private val globalScope: CoroutineScope, + buildMeta: BuildMeta, ) : VectorAnalytics { private var posthog: PostHog? = null @@ -73,27 +73,20 @@ class DefaultVectorAnalytics @Inject constructor( /** * Super Properties are properties associated with events that are set once and then sent with every capture call. */ - private var superProperties: MutableMap = HashMap() + private val superProperties: Map = mapOf( + // Put the appVersion (e.g 1.6.12). + "appVersion" to buildMeta.versionName, + // The appId (im.vector.app) + "applicationId" to buildMeta.applicationId, + // The app flavor (GooglePlay, FDroid) + "appFlavor" to buildMeta.flavorDescription, + // Parity with other platforms + "cryptoSDK" to "Rust", + ) override fun init() { observeUserConsent() observeAnalyticsId() - - initSuperProperties() - } - - /** - * Init the super properties that will be captured with all events. - */ - private fun initSuperProperties() { - // Put the appVersion (e.g 1.6.12). - superProperties["appVersion"] = buildMeta.versionName - // The appId (im.vector.app) - superProperties["applicationId"] = buildMeta.applicationId - // The app flavor (GooglePlay, FDroid) - superProperties["appFlavor"] = buildMeta.flavorDescription - // Parity with other platforms - superProperties["cryptoSDK"] = "Rust" } override fun getUserConsent(): Flow { @@ -200,16 +193,17 @@ class DefaultVectorAnalytics @Inject constructor( ?.takeIf { userConsent == true } ?.capture( event.getName(), - (this.superProperties + event.getProperties().orEmpty() + extraProperties.orEmpty()).toPostHogProperties() - .toPostHogProperties()) + (superProperties + event.getProperties().orEmpty() + extraProperties.orEmpty()).toPostHogProperties() + ) } override fun screen(screen: VectorAnalyticsScreen) { Timber.tag(analyticsTag.value).d("screen($screen)") posthog ?.takeIf { userConsent == true } - ?.screen(screen.getName(), - (this.superProperties + screen.getProperties().orEmpty()).toPostHogProperties() + ?.screen( + screen.getName(), + (superProperties + screen.getProperties().orEmpty()).toPostHogProperties() ) }