Implement analytics plan #4734
Implement analytics plan #4734
Conversation
bmarty
force-pushed
the
feature/bma/analytics_next
branch
from
1c2653f to
880b97c
Compare
|
is the plan to have future generated events pulled in automatically? #4956 |
| @@ -324,7 +334,7 @@ abstract class VectorBaseActivity<VB : ViewBinding> : AppCompatActivity(), Maver | |||
| override fun onResume() { | |||
| super.onResume() | |||
| Timber.i("onResume Activity ${javaClass.simpleName}") | |||
|
|
|||
| screenEvent = analyticsScreenName?.let { ScreenEvent(it) } | |||
There was a problem hiding this comment.
out of interest, how come we track on exit rather than entry?
I'm not sure if this has already been defined but it might be helpful to write up what counts as a screen view event across the different platforms
There was a problem hiding this comment.
We track on exit because we want to report the duration, and this is managed by ScreenEvent. It's a bit weird, I know, generally the duration could be managed by the analytics tools (since a screen replaces another, duration can be deduced). But as we do not track every screens it probably better like that
There was a problem hiding this comment.
makes sense, thanks for explaining!
Matrix SDKIntegration Tests Results:
|
| @@ -56,7 +56,7 @@ interface SingletonEntryPoint { | |||
|
|
|||
| fun pinLocker(): PinLocker | |||
|
|
|||
| fun analytics(): VectorAnalytics | |||
| fun analyticsTracker(): AnalyticsTracker | |||
| @@ -363,6 +373,7 @@ abstract class VectorBaseActivity<VB : ViewBinding> : AppCompatActivity(), Maver | |||
|
|
|||
| override fun onPause() { | |||
| super.onPause() | |||
| screenEvent?.send(analyticsTracker, analyticsScreenName) | |||
There was a problem hiding this comment.
do we need to be able to override the screen name in the send, can screens dynamically change at runtime?
if not we could use a val instead of var and drop the extra screenName parameter
// or non null and abstract to force all screens to supply a name
protected open val name: ScreenName? = null
screenEvent = screenName?.let { ScreenEvent(it) }
...
screenEvent?.send(analyticsTracker)There was a problem hiding this comment.
Yes, the screen name can change during the screen life. I made this for this particular case - LoginActivity
This is not ideal, I admit and a bit not clear...
| /** | ||
| * Track a screen display. Unique usage. | ||
| */ | ||
| class ScreenEvent(val screenName: Screen.ScreenName) { |
There was a problem hiding this comment.
small nit - mainly thinking out loud, tying the start time to the object creation is a little bit hidden and means where the instance is created is implicitly tied to the duration
another way to model the behaviour could be with a separate ScreenTracker (which could have its own injected dependencies for clocks, trackers etc)
data class TimedScreen(val startTime: Long, val screenName: ScreenName)
val timedScreen: TimedScreen = screenTracker.trackEntry(screenName)
screenTracker.trackExit(trackedScreen)There was a problem hiding this comment.
Yes, it's probably a little better. Happy to iterate on a dedicated PR later.
AnalyticsTrackerinterface which is now injected to be able to send event (only if user has opt-in of course)=> Could be nice to merge this PR, which has just been rebased, and iterate later for the remaining Events.