Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix telemetry opt-out through attribution dialog #758

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Fix telemetry opt-out through attribution dialog #758

merged 2 commits into from
Oct 14, 2021

Conversation

vayesh
Copy link
Contributor

@vayesh vayesh commented Oct 13, 2021 •
edited
Loading

PRs must be submitted under the terms of our Contributor License Agreement CLA.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-ios changelog: <changelog></changelog>.
  • Update the migration guide, API Docs, Markdown files - Readme, Developing, etc

Summary of changes

  1. Revert commit Fix telemetry opt-out through attribution dialog #743 as it have migration from v6 issues
  1. Reimplement telemetry opt-in-out fix.

User impact (optional)

@vayesh vayesh added the bug 🪲 Something is broken! label Oct 13, 2021
@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 3 times, most recently from b2678aa to 5b5cb77 Compare October 13, 2021 18:59
macdrevx
macdrevx reviewed Oct 13, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 2 times, most recently from df734dc to c955538 Compare October 13, 2021 19:06
@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 2 times, most recently from 5e31878 to 4ecaa56 Compare October 13, 2021 19:47
@vayesh vayesh marked this pull request as ready for review October 13, 2021 19:49
@vayesh vayesh requested a review from bamx23 October 13, 2021 19:57
@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 3 times, most recently from 17324af to 872b82c Compare October 13, 2021 20:37
macdrevx
macdrevx reviewed Oct 13, 2021
View reviewed changes
Tests/MapboxMapsTests/Foundation/Events/EventsManagerTests.swift Outdated

final class EventsManagerTests: XCTestCase {

let eventsManager = EventsManager(accessToken: "empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to see this be created on setUp and destroyed on tearDown. XCTestCase instances don't necessarily get deallocated right away, so keeping this alive throughout the test run could lead to unintended effects for other tests.

macdrevx
macdrevx reviewed Oct 13, 2021
View reviewed changes
Tests/MapboxMapsTests/Foundation/Events/EventsManagerTests.swift Outdated
Comment on lines 8 to 14
override class func setUp() {
UserDefaults.mme_configuration().set(true, forKey: "MMECollectionEnabledInSimulator")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link

Choose a reason for hiding this comment

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

UserDefaults.mme_configuration().mme_isCollectionEnabled alsways return false on simulator.
I guess this one can be moved to testMGLMapboxMetricsEqualsMMECollectionEnabled.

macdrevx
macdrevx reviewed Oct 13, 2021
View reviewed changes
Tests/MapboxMapsTests/Foundation/Events/EventsManagerTests.swift Outdated
Comment on lines 12 to 30
func testUserDefaultsDynamicProperty() {
UserDefaults.standard.MGLMapboxMetricsEnabled = true
XCTAssertEqual(UserDefaults.standard.bool(forKey: "MGLMapboxMetricsEnabled"), true)

UserDefaults.standard.MGLMapboxMetricsEnabled = false
XCTAssertEqual(UserDefaults.standard.bool(forKey: "MGLMapboxMetricsEnabled"), false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test the getter too?

bamx23
bamx23 reviewed Oct 14, 2021
View reviewed changes
CHANGELOG.md Outdated
@@ -7,6 +7,7 @@ Mapbox welcomes participation and contributions from everyone.
* Make `PointAnnotation.Image`'s fields public. ([#753](https://github.com/mapbox/mapbox-maps-ios/pull/753))
* Passing an unsupported locale into `Style.localizeLabels(into:forLayerIds:)` throws an error instead of crashing. ([#752](https://github.com/mapbox/mapbox-maps-ios/pull/752))
* Set `MapboxMap` flags during gestures and animations. ([#754](https://github.com/mapbox/mapbox-maps-ios/pull/754))
* Fix telemetry opt-out through attribution dialog (better fix). ([#758](https://github.com/mapbox/mapbox-maps-ios/pull/758))
Copy link

Choose a reason for hiding this comment

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

I'm not sure we should compare fixes in the changelog

Tests/MapboxMapsTests/Foundation/Events/EventsManagerTests.swift Outdated
Comment on lines 8 to 14
override class func setUp() {
UserDefaults.mme_configuration().set(true, forKey: "MMECollectionEnabledInSimulator")
}
Copy link

Choose a reason for hiding this comment

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

UserDefaults.mme_configuration().mme_isCollectionEnabled alsways return false on simulator.
I guess this one can be moved to testMGLMapboxMetricsEqualsMMECollectionEnabled.

Sources/MapboxMaps/Foundation/Events/EventsManager.swift
internal class EventsManager: EventsListener {
private enum Constants {
static let MGLAPIClientUserAgentBase = "mapbox-maps-ios"
}

var telemetry: TelemetryProtocol!
private var metricsEnabledObservation: NSKeyValueObservation?
Copy link

Choose a reason for hiding this comment

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

Please add

deinit {
    metricsEnabledObservation?.invalidate()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was there initially, but Andrew mentioned in one of his comments that I should remove it :)
#758 (comment)

Copy link

Choose a reason for hiding this comment

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

Oh, I see, thanks!

Sources/MapboxMaps/Foundation/Events/EventsManager.swift Outdated
init(with telemetry: TelemetryProtocol?) {
self.telemetry = telemetry
metricsEnabledObservation = UserDefaults.standard.observe(\.MGLMapboxMetricsEnabled, options: [.new, .initial]) { _, change in
guard let newValue = change.newValue else { return }
Copy link

Choose a reason for hiding this comment

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

Should we care about thread safety here? I can't find if this code is called from the same queue the changes are made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macdrevx what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

UserDefaults is thread safe, including mme_configuration().mme_isCollectionEnabled, but I don't think pauseOrResumeMetricsCollectionIfRequired() is. Seems like a good idea to async to the main queue in this block.

Tests/MapboxMapsTests/Foundation/Events/EventsManagerTests.swift
}

func testMGLMapboxMetricsEqualsMMECollectionEnabled() {
UserDefaults.standard.MGLMapboxMetricsEnabled = true
Copy link

Choose a reason for hiding this comment

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

Could you add UserDefaults.mme_configuration().mme_isCollectionEnabled = false before this call to verify the switching works in both cases?

@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 6 times, most recently from a3efeda to 5cfb86d Compare October 14, 2021 11:19
vayesh
vayesh commented Oct 14, 2021
View reviewed changes
Sources/MapboxMaps/Foundation/Events/EventsManager.swift

init(accessToken: String) {
let sdkVersion = "10.0.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macdrevx Not sure, why was it hardcoded before. Is it fine to read it from the bundle?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading from the bundle seems good to me.

@vayesh vayesh changed the title Vesh93/telemetry fix Fix telemetry opt-out through attribution dialog Oct 14, 2021
@vayesh vayesh force-pushed the vesh93/telemetry_fix branch 3 times, most recently from be5976e to f374a71 Compare October 14, 2021 13:54
bamx23
bamx23 approved these changes Oct 14, 2021
View reviewed changes
Sources/MapboxMaps/Foundation/Events/EventsManager.swift
internal class EventsManager: EventsListener {
private enum Constants {
static let MGLAPIClientUserAgentBase = "mapbox-maps-ios"
}

var telemetry: TelemetryProtocol!
private var metricsEnabledObservation: NSKeyValueObservation?
Copy link

Choose a reason for hiding this comment

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

Oh, I see, thanks!

Sources/MapboxMaps/Foundation/Events/EventsManager.swift
self.telemetry = telemetry
metricsEnabledObservation = UserDefaults.standard.observe(\.MGLMapboxMetricsEnabled, options: [.initial, .new]) { _, change in
DispatchQueue.main.async {
guard let newValue = change.newValue else { return }
Copy link

Choose a reason for hiding this comment

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

NIT: We can move this outside of the async call

@vayesh vayesh merged commit d0e344f into main Oct 14, 2021
@vayesh vayesh deleted the vesh93/telemetry_fix branch October 14, 2021 15:35
macdrevx added a commit that referenced this pull request Oct 14, 2021
@macdrevx
Fix telemetry opt-out through attribution dialog (#758)
3d8a6d6 
* Revert "Fix telemetry opt-out through attribution dialog (#743)"
This reverts commit 9a0c82b.

* Fix telemetry opt-out through attribution dialog (better fix)
OdNairy pushed a commit that referenced this pull request Aug 22, 2023
@macdrevx
Merge pull request #758 from mapbox/ah/prerequisites
66d9862 
Add prerequisites section to debug-env/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bamx23 bamx23 bamx23 approved these changes

@macdrevx macdrevx macdrevx approved these changes

Assignees
No one assigned
Labels
bug 🪲 Something is broken!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vayesh @macdrevx @bamx23