PostHog / posthog-ios

PostHog iOS SDK
https://posthog.com/docs/libraries/ios
MIT License
28 stars 37 forks source link

Add session tracking #100

Closed mfclarke-cnx closed 5 months ago

mfclarke-cnx commented 5 months ago

:bulb: Motivation and Context

We would like to switch to PostHog, but the lack of session tracking on iOS was a dealbreaker. Although @marandaneto indicated it would be implemented this week in #97 , I got curious after looking at the old PR.

This is still a draft PR. I would like feedback from @marandaneto if they would like me to continue to finish it (refine logic, add tests). Otherwise I'm happy for this to be closed and wait for the official thing.

Closes https://github.com/PostHog/posthog-ios/issues/97

:green_heart: How did you test it?

Simply ran our app integrated with my local posthog-ios and saw that session ID appeared in the events, and I was able to track session related metrics https://us.posthog.com/shared/HeXYA1N8aHnvLpGfGrwpBzllJiswgw

I would like to add to the test suite if this PR is deemed workable.

:pencil: Checklist

marandaneto commented 5 months ago

One main difference is that on Android I used a timer to end the session after 30 minutes of inactivity:

https://github.com/PostHog/posthog-android/blob/5138d79a25801574a964b98020776680f6b35282/posthog-android/src/main/java/com/posthog/android/internal/PostHogLifecycleObserverIntegration.kt#L101

It is also ok to do that once the app is back in the foreground, but if background events are captured while the app is in the background for more than 30 minutes, technically those events will be part of that session still but it's not correct, maybe we do the same approach as on Android so the behavior is the same?

marandaneto commented 5 months ago

@mfclarke-cnx Thanks for the PR, amazing. I left a few comments and suggestions, it's going in the right direction, are you willing to address them?

mfclarke-cnx commented 5 months ago

Absolutely happy to address these things!

Re using a timer: on iOS they restrict how you can execute code in the background. There's no way to set a timer and guarantee that it will fire when you want it too. For some more info, see: https://forums.developer.apple.com/forums/thread/114859

So what I will do is set the sessionLastTime when the app is backgrounded, and clear it when the app is foregrounded. If a sessionLastTime exists and events are being logged, it will check sessionLastTime against the threshold and start a new session if needed.

What I will do is set sessionLastTimestamp each time an event is fired, except if the app is in the background. When the app is foregrounded, I will check sessionLastTimestamp against the threshold and start a new session if needed.

This does beg the question: if events fire in the background 30+ mins after backgrounding, is it a new session? Or is there now no session?

And another question: I believe Flurry makes this configurable. So you can opt to not include background events as part of sessions (keeping session as a purely "user using the app" based construct).

mfclarke-cnx commented 5 months ago

@marandaneto please take a look at my comment above and the supporting changes. Thank you!

marandaneto commented 5 months ago

@mfclarke-cnx Good point with the background tasks.

if events are fired during the background within the 30-minute threshold, it's part of the session, after that, not anymore, so the session_id should not be set in this case.

I think caching the time that the app went to the background and checking this timestamp only when its back to the foreground is less performance overhead and simpler than doing this during every event.

marandaneto commented 5 months ago

I made it a draft and removed draft com the title, since its a GH feature anyway.

mfclarke-cnx commented 5 months ago

@mfclarke-cnx Good point with the background tasks.

if events are fired during the background within the 30-minute threshold, it's part of the session, after that, not anymore, so the session_id should not be set in this case.

I think caching the time that the app went to the background and checking this timestamp only when its back to the foreground is less performance overhead and simpler than doing this during every event.

Right. And in addition, we need to check it if events fire and the app is in the background so we can nil the sessionId from that point on.

mfclarke-cnx commented 5 months ago

I made it a draft and removed draft com the title, since its a GH feature anyway.

Nice. I've spent too long with self hosted Bitbucket ;p

mfclarke-cnx commented 5 months ago

I think this is in a good place now @marandaneto

marandaneto commented 5 months ago

I think this is in a good place now @marandaneto

awesome, just a linting issue: PostHogSDK.swift:381:1: warning: (andOperator) Prefer comma over && in if, guard or while conditions.

and add a new entry to the changelog -> 3.1.0 and ready to roll.

mfclarke-cnx commented 5 months ago

awesome, just a linting issue: PostHogSDK.swift:381:1: warning: (andOperator) Prefer comma over && in if, guard or while conditions.

Ah, I didn't execute swiftformat in linting mode too. Fixed!