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

Updated Session / App Lifecycle logic #21

Merged
merged 14 commits into from
May 1, 2024
Merged

Conversation

bsneed
Copy link
Contributor

@bsneed bsneed commented May 1, 2024

  • More accurately replicated Amplitude's Session logic.
  • Added support for amplitude lifecycle events.
  • Amplitude specific events are blocked from all destinations other than Amplitude.
  • Added privacy manifest.

}

func write<T: Codable>(key: String, value: T?) {
if let value, isBasicType(value: value) {

Choose a reason for hiding this comment

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

Is Codable a BasicType ?

Copy link
Contributor Author

@bsneed bsneed May 1, 2024

Choose a reason for hiding this comment

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

Codable says the value can be encoded/decoded somehow, in this case to UserDefaults. BasicType is something like a primitive. whereas if it's not, and it's a dictionary, we need to store it differently.

@bsneed bsneed merged commit ef0335f into main May 1, 2024
2 checks passed
@bsneed bsneed deleted the bsneed/session_lifecycle branch May 1, 2024 19:55
Copy link
Contributor

@alanjcharles alanjcharles left a comment

Choose a reason for hiding this comment

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

excited for this one

@skydivedan
Copy link

Hi, @bsneed , would you be able to elaborate about the lifecycle event support? I just got a note from someone at my group in the data team, and they were curious why there were seeing these "[Amplitude] Application Opened" events. I can tell them "because that's what the new update does", but was hoping for a better explanation.

@bsneed
Copy link
Contributor Author

bsneed commented Jun 10, 2024

Hi @skydivedan, yeah sure! Those application opened events are what the amplitude SDK would do to properly track mobile sessions. I would've preferred that the Action Destination did this on the segment.com side, but that team wasn't going to have any time opening up in the near future to fix it. I'm not entirely sure how amplitude uses those events internally, but we received reports from customers that their stats on amplitude weren't correct because of these missing events.

// if it's amp specific stuff, disable all the integrations except for amp.
if eventName.contains(Constants.ampPrefix) || eventName == Constants.ampSessionStartEvent || eventName == Constants.ampSessionEndEvent {
var integrations = disableAllIntegrations(integrations: trackEvent.integrations)
integrations?.setValue(["session_id": sessionID], forKeyPath: KeyPath(key))
Copy link

@bharath-kamath bharath-kamath Aug 19, 2024

Choose a reason for hiding this comment

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

Should the session_id be set outside this block for all events being fired? Currently I see that on Amplitude dashboard all my events are not being grouped together because the session iD is being passed as -1. In earlier version, the timestamp was being set to the session Id and it was correctly grouping all related session events.

New sdk
Screenshot 2024-08-18 at 8 54 58 PM

Previous sdk
Screenshot 2024-08-18 at 9 03 46 PM

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.

5 participants