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

Introduce API Client #4082

Closed
wants to merge 6 commits into from
Closed

Introduce API Client #4082

wants to merge 6 commits into from

Conversation

boundsj
Copy link
Contributor

@boundsj boundsj commented Feb 23, 2016

This introduces a new utility class that wraps networking via NSURLSession. All related code that used to live inside the telemetry MGLMapboxEvents class has been pulled into the new MGLAPIClient. An API client instance is used as a service by telemetry and can be reused in the future when and if our networking needs grow or become more complex.

There are some nice side effects as a result of this refactor such as:

  • The turnstile event can now be guaranteed to have a send attempt without having to be special cased and searched for inside a queue of other events
  • We can react to failures (for now I've just added debug logging but in the future it would be nice to have a back off strategy when the network just doesn't work)
  • Future work to make the MGLMapboxEvents class more testable will be easier since the networking client can be mocked and injected under test so tests are more controllable and determinate
  • In flight requests can be cancelled

The main benefit of the api client is the API it presents to its clients. The public interface is:

- (void)postEvents:(nonnull NS_ARRAY_OF(MGLMapboxEventAttributes *) *)events completionHandler:(nullable void (^)(NSError * _Nullable error))completionHandler;
- (void)postEvent:(nonnull MGLMapboxEventAttributes *)event completionHandler:(nullable void (^)(NSError * _Nullable error))completionHandler;
- (void)cancelAll;

An example usage of this for sending the turnstile event specifically is:

[self.apiClient postEvent:eventAttributes completionHandler:^(NSError * _Nullable error) {
    if (error) { /* log error and return */ }
    [self writeEventToLocalDebugLog:turnstileEventAttributes];
}];

Addresses #3704

@boundsj
Reduce async complexity
88572f1 
Since MGLMapboxEvents is only accessed via the main thread the
complexity to ensure thread safety is not required. This removes
that extra code so that future work can benefit from being based
on a simpler foundation.

The main changes in this commit are:

- Untangle nested calls to GCD dispatch_async (i.e. `pushEvents` used to
dispatch onto a global (concurrent) background queue from a public
class method and then those blocks would be dispatched onto a custom
serial queue). Now, the only calls to dispatch_async that are left are
in `- postEvents:` and `- writeEventToLocalDebugLog:` and the
performance requirement to have even that dispatching done is still
unclear. I'm keeping it for now only because it allows for non-blocking
JSON serialization and pre-validation (network requests are already
non-blocking anyway)
- Since the async dispatches are pushed down to just wrap around
the JSON serialization / networking, this commit also removes the main
thread marshaling that was done in many places (i.e. guarding access
to UIKit methods that provide data for events that used to be
(synchronously) gathered via the main thread)
- Remove assertions that we are running on the main thread.
- Pushed some bools used for guards (i.e. `debugLoggingEnabled`) down
into the methods that really need to know about the bool value to
eliminate some repetitive boilerplate guarding all over the place
- Make progress towards a consistent code style
@boundsj
Revert notification observer back to ivar from property
360e218
@boundsj
Remove unused debugLoggingEnabled convenience method
f5cd312
@boundsj
Reduce repeated calls to locationManager property get with call to set
4b2e765
@boundsj
Increase readability of event dictionary creation
f6785a4
@boundsj
WIP: Introduce API Client
baff1d0
@boundsj boundsj added iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries refactor labels Feb 23, 2016
@boundsj boundsj added this to the ios-v3.2.0 milestone Feb 23, 2016
@boundsj boundsj self-assigned this Feb 23, 2016
@boundsj
Copy link
Contributor Author

boundsj commented Feb 23, 2016

@1ec5 @friedbunny oops please look over here instead (based off of the wrong branch before). Still looking to get 👀 on this so I can polish it up as required today.

boundsj
boundsj reviewed Feb 23, 2016
View reviewed changes
platform/ios/src/MGLAPIClient.m
[request setValue:self.userAgent forHTTPHeaderField:MGLAPIClientHeaderFieldUserAgentKey];
[request setValue:MGLAPIClientHeaderFieldContentTypeValue forHTTPHeaderField:MGLAPIClientHeaderFieldContentTypeKey];
[request setHTTPMethod:MGLAPIClientHTTPMethodPost];
NSData *jsonData = [NSJSONSerialization dataWithJSONObject:events options:NSJSONWritingPrettyPrinted error:nil];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. I forgot to remove the "pretty print" option. That does nothing for us since we are sending this data over they wire and not printing it for a human

@boundsj boundsj force-pushed the boundsj-events-on-main-thread-3717 branch from f6785a4 to 3340b62 Compare February 24, 2016 00:25
@boundsj boundsj closed this Feb 24, 2016
@boundsj boundsj removed this from the ios-v3.2.0 milestone Feb 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@boundsj boundsj

Labels
iOS Mapbox Maps SDK for iOS refactor telemetry Integration with Mapbox Telemetry libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@boundsj @friedbunny @1ec5 @jfirebaugh @lucaswoj