-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/189/session replay min duration #199
Draft
karntrehan
wants to merge
13
commits into
PostHog:main
Choose a base branch
from
karntrehan:feature/189/session-replay-min-duration
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+142
−24
Draft
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ca15b03
[#189] Adding minSessionDuration to config and update usage
karntrehan 478cd56
[#189] WIP: Delete recording if less than min
karntrehan 69a3056
Move to capture events only when min duration passed.
karntrehan e9d0a21
Prevent frequent calculations if minthreshold is crossed once.
karntrehan d846f98
Read values from server config for min duration
karntrehan 787a06f
Fix bug of server min duration priority
karntrehan 66faf33
Rollback all formatting changes
karntrehan a1b0479
PR Feedback: Saving server config in config object, minSessionDuratio…
karntrehan f4d0d2f
Formatting fixes
karntrehan 92a4d5b
Fix initialization of sessionStartTime
karntrehan ac53a8a
Sync events if threshold is crossed but no new events have come in.
karntrehan f7f6ff4
PR Feedback: minSessionDurationMs default to be null
karntrehan b6fbed1
Move to multiple session ID handling
karntrehan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -853,7 +853,8 @@ public class PostHog private constructor( | |
// this is used in cases where we know the session is already active | ||
// so we spare another locker | ||
private fun isSessionReplayFlagActive(): Boolean { | ||
return config?.sessionReplay == true && featureFlags?.isSessionReplayFlagActive() == true | ||
//FIXME -> Remove this true hardcoding | ||
return true || config?.sessionReplay == true && featureFlags?.isSessionReplayFlagActive() == true | ||
Comment on lines
+856
to
+857
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rollback |
||
} | ||
|
||
override fun isSessionReplayActive(): Boolean { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Looking at PostHogSessionManager right now. Thanks!