feat: auto-rotate session after 24 hours #247
Conversation
pauldambra
requested review from
a team,
sortafreel and
veryayskiy
and removed request for
a team
| @@ -29,10 +29,10 @@ internal class FakePostHogDateProvider : PostHogDateProvider { | |||
| } | |||
|
|
|||
| override fun currentTimeMillis(): Long { | |||
| return System.currentTimeMillis() | |||
| return currentDate?.time ?: System.currentTimeMillis() | |||
| } | ||
|
|
||
| override fun nanoTime(): Long { | ||
| return System.nanoTime() | ||
| return currentDate?.time?.times(1000000) ?: System.nanoTime() |
| synchronized(sessionLock) { | ||
| if (sessionId == sessionIdNone) { | ||
| sessionId = TimeBasedEpochGenerator.generate() | ||
| sessionId = TimeBasedEpochGenerator.generate(sessionStartTime ?: System.currentTimeMillis()) |
There was a problem hiding this comment.
I passed the time in here...
since we have a date provider really we should pass it around everywhere
but I guess it's only for tests so I suppose this is OK
There was a problem hiding this comment.
thats fine, another approach would be to make a TimeBasedEpochGenerator an interface, and the current TimeBasedEpochGenerator is the production implementation and we could have a test implementation and swap during SDK initialization just for testing, the prod code would be cleaner but we would pay the price of an extra interface
| * we can convert a session id into the session start timestamp | ||
| * and then use that to enforce the maximum session length | ||
| */ | ||
| private fun enforceMaxSessionLength(buildProperties: Map<String, Any>): Map<String, Any> { |
There was a problem hiding this comment.
react native has its own session id rotation so we should not do this for RN I think
https://github.com/PostHog/posthog-js-lite/blob/b8372284df5489457d95b16b0e4b12a91dd66c26/posthog-core/src/index.ts#L1309-L1315
otherwise they will be out of sync (RN uses a different session id than Android)
There was a problem hiding this comment.
so, needs to be configurable? and then RN would turn off the Android one?
or sync up a release and RN stops doing its own?
There was a problem hiding this comment.
yes, we have something similar for flutter already
today the way how it works is react native manages the session id and update the session id on android when its rotated.
now we want to do the other way around, from android to call the react native but you cant do this because the bridge only works in one direction, so its better to let the react native manage the whole session id thing otherwise they will be out of sync
| PostHogSessionManager.endSession() | ||
| PostHogSessionManager.startSession(currentTimeMillis) |
There was a problem hiding this comment.
this changes the session id but the session replay would need to be notified to eg send a new full snapshot with metadata etc otherwise it'll assume its already sent and the replay will be broken (missing meta event and full snapshot for the new session id)
There was a problem hiding this comment.
🤔 in the JS SDK we have an event emitter that we pass around that can be used for this kind of "onBlah" behavior
I guess the sessionManager can have a onSessionStarted that accepts a callback?
Is there a more android-y way of doing it?
There was a problem hiding this comment.
yeah a callback would be one way, since PostHogSessionManager lives in the java-only lib and most of the replay code is Android-dependent, we will need to pass callbacks around, or implement a pub/sub approach for "app is in background, app is in foreground, etc" so the java bits only can do things
| appendSharedProps = !snapshotEvent, | ||
| // only append groups if not a group identify event and not a snapshot | ||
| appendGroups = !groupIdentify, | ||
| enforceMaxSessionLength( |
There was a problem hiding this comment.
This will only be executed if an event is captured instead of when 24 hours have passed. Is that intentional? should we also do this eg when the app is put on foreground/backgorund?
There was a problem hiding this comment.
i guess the foreground change is righ there
was assuming that we didn't want to run a timer just for this - although ultimately that's where JS SDK ended up
There was a problem hiding this comment.
we already have a timer when the app is put in the background, see here
There was a problem hiding this comment.
maybe that timer should also call enforceMaxSessionLength when its back in the foreground so we don't rely on the next event but rather if the app is back visible to the user
There was a problem hiding this comment.
me: "let's open a PR and see how much I don't know"
me: *regrets life choices
🤣
| @@ -4,6 +4,8 @@ import com.posthog.PostHogInternal | |||
| import com.posthog.vendor.uuid.TimeBasedEpochGenerator | |||
| import java.util.UUID | |||
|
|
|||
| public const val MAX_SESSION_AGE_MILLIS: Int = 24 * 60 * 60 * 60 | |||
There was a problem hiding this comment.
can it be private instead?
if needed you can put it inside a companion object
see PostHog/posthog#30889
android doesn't rotate session ID after 24 hours
i think this PR adds that 🙈