-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: Improve background batch uploading and session management #238
feat: Improve background batch uploading and session management #238
Conversation
…ver be true handleApplicationWillEnterForeground is ALWAYS called before handleApplicationDidBecomeActive, and in willEnterForeground we ALWAYS set the resignedActive flag back to false….right before checking to see if it’s true in didBecomeActive… So this logic is completely unnecessary.
… main thread usage
…ackground upload logic
…ents after testing finished
…meout (now correctly expires the session)
4b6e33b
to
0bf5ded
Compare
…ad interval with iOS. No need for such short timeouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Non-blocking questions left inline.
} | ||
|
||
self.uploadSource = [self createSourceTimer:self.uploadInterval eventHandler:^{ | ||
[self waitForKitsAndUploadWithCompletionHandler:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking - I see we're calling waitForKitsAndUpload (which will make batches) within beginUploadTimer
, which is called within applicationDidBecomeActive
- does an app become "inactive" only after the countdown timer goes to zero? If so, then this is fine.
or does the app not become inactive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samdozor I’m not sure I fully understand the question, but for clarification waitForKitsAndUpload
is being called in the upload timer’s even handler not inside beginUploadTimer, so it’s just there to trigger the upload when the timer fires. The upload timer is only used while the app is active and inactive, so every X seconds (600 by default, aka 10 minutes) this will fire, whether we’re active or inactive. We only disable it when the app is put to sleep by the OS so it doesn’t automatically fire if we wake up later.
[MParticle sharedInstance].persistenceController = [[MPPersistenceController alloc] init]; | ||
|
||
if (shouldBeginSession) { | ||
[self beginSessionWithIsManual:!MParticle.sharedInstance.automaticSessionTracking date:date]; | ||
} | ||
|
||
MPMessageBuilder *messageBuilder = [MPMessageBuilder newBuilderWithMessageType:MPMessageTypeFirstRun session:self.session messageInfo:nil]; | ||
|
||
[self processOpenSessionsEndingCurrent:NO completionHandler:^(void) {}]; | ||
[self waitForKitsAndUploadWithCompletionHandler:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? since we'll call it again within beginUploadTimer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samdozor I actually explicitly added this in based on notes from one of our conversations. :)
I believe the thinking was since the session is ending a batch is mandatory anyway, so it was better to just upload it immediately than to wait since it wouldn't matter for the "events per batch" metric anyway. But you're correct it's not strictly necessary as it will be uploaded on the next timeout (if the app doesn't crash or and isn't force killed by the user).
🎉 This PR is included in version 8.17.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [8.17.0](v8.16.0...v8.17.0) (2023-11-30) ### Bug Fixes * CFNetwork upload warning setting body twice ([#240](#240)) ([0a177f9](0a177f9)) ### Features * 5343 Refactor Kit Filter Hash Functions ([#228](#228)) ([5c26a9e](5c26a9e)) * Begin Implementation MPSideloadedKit Filtering Methods ([#231](#231)) ([ad543fb](ad543fb)) * Finish Implementation of MPSideLoadedKits ([#239](#239)) ([96e62f5](96e62f5)) * Improve background batch uploading and session management ([#238](#238)) ([dc6a9cb](dc6a9cb))
Summary
The core goal of this change is to reduce the unnecessary uploading of batches when in the background, especially during long running background usage such as music playback.
Over the course of implementing that, it was also necessary to refactor the background session handling as it directly ties into that logic.
Most of the changes are in the
MPBackendController
class.Some highlights:
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)