Feature/189/session replay min duration #199
Conversation
|
@marandaneto could you please review the implementation? Thanks! Would then go ahead with the test cases. |
|
Hie @marandaneto just following up on this PR. |
Sorry i am on pto this week so a bit slow to review, maybe @ioannisj or @pauldambra can take a look for now? Otherwise i will have a look within the next couple days |
|
No problem. Take your time @marandaneto. Also, please feel free to mention me in other issues that you think I can contribute to. I have kept some time aside to make the hog hoggier! |
posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt
Show resolved
Hide resolved
posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt
Outdated
Show resolved
Hide resolved
posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt
Outdated
Show resolved
Hide resolved
| @@ -118,6 +119,15 @@ public class PostHogReplayIntegration( | |||
| private val isSessionReplayEnabled: Boolean | |||
| get() = PostHog.isSessionReplayActive() | |||
|
|
|||
| private var sessionStartTime = 0L | |||
| private val events = mutableListOf<RREvent>() | |||
| private var minSessionThresholdCrossed = false | |||
There was a problem hiding this comment.
This flag has to be cached per session id, so ideally this is part of ViewTreeSnapshotStatus I think, or it has to be reset when the session_id changes as well.
There was a problem hiding this comment.
This is not very clear to me. What we are doing here is setting the threshold once for 1 launch session of the user.
The behavior we have currently is: User launches the app, we start caching events & wait for the minimum threshold, if the user closes the app before the threshold the events are not captured / dropped. Once the threshold is crossed, all cached events are captured immediately and subsequent events are captured right away. Is this the expected behavior?
I am not sure of ViewTreeSnapshotStatus and where the session_id is set / unset. Could you help me with more info on this?
There was a problem hiding this comment.
So all this information should be related to a specific sessionId.
Right now the sessionStartTime is gonna be used even if the session rotates, etc.
So we need a way of Map<SessionId, Object> where Object contains the properties (sessionStartTime, events, mouseInteractions, etc...)
There was a problem hiding this comment.
An example is that if the app is in the background for more than 30 minutes, the sessionId rotates, and the new session will start once the app is in the background, but we are still using the sessionStartTime from the last session.
There was a problem hiding this comment.
Got it. Looking at PostHogSessionManager right now. Thanks!
posthog-android/src/main/java/com/posthog/android/replay/PostHogSessionReplayConfig.kt
Outdated
Show resolved
Hide resolved
| //FIXME -> Remove this true hardcoding | ||
| return true || config?.sessionReplay == true && featureFlags?.isSessionReplayFlagActive() == true |
posthog/src/main/java/com/posthog/internal/PostHogPreferences.kt
Outdated
Show resolved
Hide resolved
@karntrehan did another pass, let me know if something is unclear, I understand some parts might be tricky |
|
@marandaneto I have made some changes basis your feedback. Please check. It has helped me understand the code flow better. Thanks! |
Thanks @karntrehan , I am currently on an offsite with my team but will review asap. |
|
|
posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt
Outdated
Show resolved
Hide resolved
posthog-android/src/main/java/com/posthog/android/replay/PostHogSessionReplayConfig.kt
Outdated
Show resolved
Hide resolved
|
@karntrehan left a few new comments, a few things to address but it's in the right direction. |
💡 Motivation and Context
#189
💚 How did you test it?
WIP
📝 Checklist