Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios] Optimize event uploading #6737

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

boundsj
Copy link
Contributor

@boundsj boundsj commented Oct 18, 2016

Upload any events collected when the app was in use for apps that only have "when in use" location permissions. Also, for all apps, avoid posting event data if there is only a single event.

@boundsj boundsj added iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries labels Oct 18, 2016
@boundsj boundsj added this to the ios-v3.4.0 milestone Oct 18, 2016
@boundsj boundsj self-assigned this Oct 18, 2016
@mention-bot
Copy link

@boundsj, thanks for your PR! By analyzing the history of the files in this pull request, we identified @friedbunny, @1ec5 and @bleege to be potential reviewers.

@@ -489,6 +504,9 @@ - (void)postEvents:(NS_ARRAY_OF(MGLMapboxEventAttributes *) *)events {
}
[strongSelf pushDebugEvent:MGLEventTypeLocalDebug withAttributes:@{MGLEventKeyLocalDebugDescription: @"post",
@"debug.eventsCount": @(events.count)}];

[[UIApplication sharedApplication] endBackgroundTask:_backgroundTaskIdentifier];
_backgroundTaskIdentifier = UIBackgroundTaskInvalid;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the background task won’t be invalidated if there was a network error (see L503), which would mean the app continues on in the background until it’s killed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed 7688bcc

@boundsj boundsj changed the base branch from master to release-ios-v3.4.0 October 18, 2016 20:35
@friedbunny
Copy link
Contributor

friedbunny commented Oct 18, 2016

This was originally based on the current master, but the release-ios-v3.4.0 branch is one Android commit behind that — could you rebase on the release branch?

@@ -485,10 +500,15 @@ - (void)postEvents:(NS_ARRAY_OF(MGLMapboxEventAttributes *) *)events {
if (error) {
[strongSelf pushDebugEvent:MGLEventTypeLocalDebug withAttributes:@{MGLEventKeyLocalDebugDescription: @"Network error",
@"error": error}];
[[UIApplication sharedApplication] endBackgroundTask:_backgroundTaskIdentifier];
_backgroundTaskIdentifier = UIBackgroundTaskInvalid;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we removed this return and then enclosed the subsequent pushDebugEvent in an else statement, I think we could avoid duplicating the background task invalidation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again. d648594 refactors as you described and squashes this all into a single commit on top of the release branch.

@boundsj boundsj force-pushed the boundsj-upload-events-on-background branch from 7688bcc to d403c77 Compare October 18, 2016 20:53
Upload any events collected when the app was in use for apps that only
have "when in use" location permissions. Also, for all apps, avoid
posting event data if there is only a single event.
@boundsj boundsj force-pushed the boundsj-upload-events-on-background branch from d403c77 to d648594 Compare October 18, 2016 20:57
@boundsj boundsj merged commit 0b37408 into release-ios-v3.4.0 Oct 18, 2016
@boundsj boundsj deleted the boundsj-upload-events-on-background branch October 18, 2016 21:32
friedbunny pushed a commit that referenced this pull request Oct 26, 2016
Upload any events collected when the app was in use for apps that only
have "when in use" location permissions. Also, for all apps, avoid
posting event data if there is only a single event.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants