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.

Sign up for GitHub

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

recording: do not rotate the session id for hybrid SDKs #253

Merged
merged 4 commits into from
Nov 18, 2024
Merged

recording: do not rotate the session id for hybrid SDKs #253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c5d5934
recording: do not rotate the session id for hybrid SDKs
marandaneto Nov 15, 2024
b51a5f2
pr id
marandaneto Nov 15, 2024
1b2c021
fix
marandaneto Nov 15, 2024
4ee8de5
fix
marandaneto Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## Next

- recording: do not rotate the session id for hybrid SDKs ([#253](https://github.com/PostHog/posthog-ios/pull/253))

## 3.15.2 - 2024-11-13

- fix: allow changing person properties after identify ([#249](https://github.com/PostHog/posthog-ios/pull/249))
Expand Down
20 changes: 20 additions & 0 deletions PostHog/PostHogSessionManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,17 @@ import Foundation
timeNow - sessionLastTimestamp > sessionChangeThreshold
}

private func isiOSNativeSdk() -> Bool {
// postHogSdkName will be set to eg posthog-react-native if not
postHogSdkName == postHogiOSSdkName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to compute this once? Depends on when postHogSdkName is assigned, but after that it probably won't be changing value

// postHogSdkName will be set to eg posthog-react-native if not
private let isiOSNativeSdk: Bool = postHogSdkName == postHogiOSSdkName

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The session manager is a singleton that is created before the SDK is initiated so in this case the only "safe" way is every time.
Before moving the check from PostHogSDK to here, I computed only once because I knew the new value was already assigned.
So sadly not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a lazy var that's calculated the first time it's accessed. If that won't work either then it means that some sessions may be cleared or rotated as well unintentionally

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and that's what I want to avoid since the bug is exactly about that, so it's ok to pay the performance cost which is minimal, and if the bug is solved, I can look into making this better.

}

func resetSessionIfExpired(_ completion: () -> Void) {
// for hybrid SDKs, the session is handled by the hybrid SDK
guard isiOSNativeSdk() else {
return
}

sessionLock.withLock {
let timeNow = now().timeIntervalSince1970
if sessionId != nil,
Expand Down Expand Up @@ -79,6 +89,11 @@ import Foundation
}

func rotateSessionIdIfRequired(_ completion: @escaping (() -> Void)) {
// for hybrid SDKs, the session is handled by the hybrid SDK
guard isiOSNativeSdk() else {
return
}

sessionLock.withLock {
let timeNow = now().timeIntervalSince1970

Expand All @@ -94,6 +109,11 @@ import Foundation
}

func updateSessionLastTime() {
// for hybrid SDKs, the session is handled by the hybrid SDK
guard isiOSNativeSdk() else {
return
}

sessionLock.withLock {
sessionLastTimestamp = now().timeIntervalSince1970
}
Expand Down
3 changes: 2 additions & 1 deletion PostHog/PostHogVersion.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ import Foundation
// This property is internal only
public var postHogVersion = "3.15.2"

public let postHogiOSSdkName = "posthog-ios"
// This property is internal only
public var postHogSdkName = "posthog-ios"
public var postHogSdkName = postHogiOSSdkName
Loading