-
Notifications
You must be signed in to change notification settings - Fork 368
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
[Improvement] limit refresh User and GET IAMs to foreground #2036
Conversation
7074f70
to
94fe2a8
Compare
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.
Tested with Android emulator Pixel 3a API 34 that when the app is swiped up (in background,) the app does not call to refresh user.
I'm running into wonky behavior testing this PR when session time is sent after the app is swiped away. Haven't dug into why.
D [Thread-15] SessionService.backgroundRun()
D [Thread-15] SessionService: Session ended. activeDuration: 0
D [Thread-15] OperationRepo.enqueue(operation: {"name":"track-session-end","appId":"<id>","onesignalId":"<id>","sessionTime":0}, flush: false)
D [DefaultDispatcher-worker-7] HttpClient: POST outcomes/measure - {"app_id":"<id>","onesignal_id":"<id>","subscription":{"id":"<id>","type":"AndroidPush"},"id":"os__session_duration"}
D [DefaultDispatcher-worker-7] HttpClient: POST outcomes/measure - FAILED STATUS: 400
W [DefaultDispatcher-worker-7] HttpClient: POST RECEIVED JSON: {"errors":["There was a problem in the JSON you submitted","\"session_time\" must be positive when submitting os__session_duration"],"success":false}
W [Thread-16] OutcomeEventsController.sendAndCreateOutcomeEvent: Sending outcome with name: os__session_duration failed with status code: 400 and response: {"errors":["There was a problem in the JSON you submitted","\"session_time\" must be positive when submitting os__session_duration"],"success":false}
Outcome event was cached and will be reattempted on app cold start
D [OpRepo] UpdateUserOperationExecutor(operation: [{"name":"track-session-end","appId":"<id>","onesignalId":"<id>","sessionTime":0,"id":"<id>"}])
D [DefaultDispatcher-worker-7] HttpClient: PATCH apps/<id>/users/by/onesignal_id/<id> - {"refresh_device_metadata":false,"deltas":{"session_time":0}}
D [DefaultDispatcher-worker-7] HttpClient: PATCH apps/<id>users/by/onesignal_id/<id> - STATUS: 202 JSON: {"properties":{}} If I test this same flow without the changes in this PR, the session time seems to be calculated correctly and sent. D [Thread-15] SessionService.backgroundRun()
D [Thread-15] SessionService: Session ended. activeDuration: 96363
D [DefaultDispatcher-worker-2] HttpClient: POST outcomes/measure - {"app_id":"<id>","onesignal_id":"<id>","subscription":{"id":"<id>","type":"AndroidPush"},"id":"os__session_duration","session_time":96}
D [DefaultDispatcher-worker-3] HttpClient: PATCH apps/<id>-c72e141824ef/users/by/onesignal_id/<id> - {"refresh_device_metadata":false,"deltas":{"session_time":96}} |
af70434
to
d798815
Compare
We want to prevent wasted resources polling for IAMs durning times where it is unlikely they will be seen. Two changes are being made; 1. ensure we only poll when the isInForeground is true. 2. poll the first time the app is foregrounded, incase it is not when the SDK is initialized.
Ensure cache for the user is refreshed once per cold start when app is in the foreground. This saves resources as there are a number of events (such as push received or non-OneSignal events) that start the app in the background but will never read/write any user properties.
94fe2a8
to
3b85296
Compare
@nan-li Hmm, I am not able to reproduce this bug from some reason. I tried it a few times on an Android 14 emulator, both as a clean install and and existing install. It looks like you are using the default app_id we have in our example app for the 2nd test (ending in |
I had tested two fresh installs on Android 13 emulator that reproduced the session time of 0 when I posted the comment above. Then another fresh install without the changes in this PR. All with same default app_id. But now, trying it again, I am still running new installs and only reproduced it in 1 out of 5 new app installs. Might be random and not related to the changes in this PR. |
From looking at the session code I don't see how this PR could be related to the issue, so going to merge this PR in. Good find on this bug though, I think it might be related to the time not always getting written to disk before the app is killed. I'll put these issues on our bug backlog. |
Description
One Line Summary
Limit refresh User and GET IAMs to foreground to save resources.
Details
Motivation
There are a number of background events that can start an app in the background (example push received), however were wasting resources (both on the device and OneSignal's backend) if the app is never foregrounded.
Scope
Only affect when we fetch IAMs and when refresh the User cache.
Testing
Manual testing
Tested on an Android 14 emulator, ensure we don't fetch IAMs and call GET User until the app is foregrounded.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is