Skip to content
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

Add session tracking #100

Merged
merged 13 commits into from
Feb 7, 2024
Merged

Conversation

mfclarke-cnx
Copy link
Contributor

@mfclarke-cnx mfclarke-cnx commented Feb 5, 2024

💡 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 #97

💚 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.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

PostHog/PostHogSDK.swift Outdated Show resolved Hide resolved
@marandaneto
Copy link
Member

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
Copy link
Member

@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?

Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
@mfclarke-cnx
Copy link
Contributor Author

mfclarke-cnx commented Feb 5, 2024

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
Copy link
Contributor Author

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

@marandaneto
Copy link
Member

@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 marandaneto marked this pull request as draft February 6, 2024 09:32
@marandaneto marandaneto changed the title [Draft] Add session tracking Add session tracking Feb 6, 2024
@marandaneto
Copy link
Member

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

@mfclarke-cnx
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

I think this is in a good place now @marandaneto

@marandaneto marandaneto marked this pull request as ready for review February 6, 2024 12:55
@marandaneto
Copy link
Member

marandaneto commented Feb 6, 2024

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
Copy link
Contributor Author

mfclarke-cnx commented Feb 6, 2024

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!

@marandaneto marandaneto merged commit c33f2ec into PostHog:main Feb 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$session_id support
2 participants