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

Send turnstile event when telemetry is paused #3495

Merged
merged 1 commit into from
Jan 14, 2016

Conversation

friedbunny
Copy link
Contributor

The turnstile event is used solely to count monthly active users and should always be sent, regardless of whether the user has opted themselves out of telemetry.

The turnstile wasn't being turned if telemetry was paused, because _session is invalided and nil'd in pauseMetricsCollection. This change instantiates a temporary NSURLSession for the turnstile event.

@1ec5 I could use your advice on my use of strongSelf.session here — before, we were straight-up calling _session, but inside of a background async block.

@friedbunny friedbunny added bug iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries labels Jan 9, 2016
@friedbunny friedbunny self-assigned this Jan 9, 2016
@friedbunny friedbunny added this to the ios-v3.1.0 milestone Jan 9, 2016
friedbunny added a commit that referenced this pull request Jan 9, 2016
delegate:strongSelf
delegateQueue:nil];
[[strongSelf.session dataTaskWithRequest:request] resume];
[strongSelf.session finishTasksAndInvalidate];
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than checking for the existence of MGLMapboxEvents.session, I think it’d be better to check for paused state, or better yet, for an indication that this is a turnstile event. If we’re getting in here without a session for anything other than a turnstile event, that’s an error on our part, so the original code attempts to fail gracefully.

Additionally, could we keep this synchronous, turnstile-specific session a local variable, to avoid confusion about what kind of session is being stored in the ivar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this more specific to the turnstile sounds good, forget why I didn't go with isPaused here but will look at casing this to the turnstile only.

👍 on using a standalone NSURLSession here.

@friedbunny friedbunny force-pushed the no-hopping-the-turnstile branch from 0dcd052 to dfc30f6 Compare January 14, 2016 22:28
@friedbunny
Copy link
Contributor Author

Squashed and rebased, will merge when the tests go green.

@friedbunny friedbunny force-pushed the no-hopping-the-turnstile branch from dfc30f6 to b57e237 Compare January 14, 2016 23:31
@friedbunny friedbunny merged commit b57e237 into master Jan 14, 2016
@friedbunny friedbunny deleted the no-hopping-the-turnstile branch January 14, 2016 23:52
friedbunny added a commit that referenced this pull request Jan 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 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.

4 participants