[NT-685] Abstract TrackingClient to be configurable #983
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justinswart
commented
Dec 5, 2019
Library/TestHelpers/TestCase.swift
Outdated
| internal let dateType = MockDate.self | ||
| internal let mainBundle = MockBundle() | ||
| internal let reachability = MutableProperty(Reachability.wifi) | ||
| internal let scheduler = TestScheduler(startDate: MockDate().date) | ||
| internal let trackingClient = MockTrackingClient() | ||
| internal let trackingClient = MockTrackingClient() // TODO: Rename |
There was a problem hiding this comment.
Renaming this resulted in ~700 changes so I will do that separately.
| @@ -54,7 +55,11 @@ internal class TestCase: FBSnapshotTestCase { | |||
| debounceInterval: .seconds(0), | |||
| device: MockDevice(), | |||
| isVoiceOverRunning: { false }, | |||
| koala: Koala(client: self.trackingClient, loggedInUser: nil), | |||
| koala: Koala( | |||
| dataLakeClient: self.dataLakeTrackingClient, | |||
There was a problem hiding this comment.
There is a mock client set for this by default in TestCase but I think we can add tests once we whitelist events.
Library/Tracking/Koala.swift
Outdated
| @@ -355,15 +356,17 @@ public final class Koala { | |||
|
|
|||
| public init( | |||
| bundle: NSBundleType = Bundle.main, | |||
| client: TrackingClientType, | |||
| dataLakeClient: TrackingClientType = TrackingClient(.dataLake), // TODO: Remove default value | |||
There was a problem hiding this comment.
Also to reduce the amount of line changes in this PR I'm passing in a default value here. We can tackle this separately.
| @@ -2028,7 +2031,12 @@ public final class Koala { | |||
|
|
|||
| self.logEventCallback?(event, props) | |||
|
|
|||
| self.client.track( | |||
There was a problem hiding this comment.
Currently we just send all events to both clients. We will whitelist before this point.
|
|
||
| if dataString.count >= 10_000 { | ||
| // swiftlint:disable:next line_length | ||
| print("\(config.identifier.emoji) [\(config.identifier) Error]: Base64 payload is longer than 10,000 characters.") |
There was a problem hiding this comment.
This was just something we printed at this point previously, not sure how important it is to keep.
ifbarrera
approved these changes
Dec 6, 2019
Generated by 🚫 Danger |
justinswart
force-pushed
the
new-tracking-client
branch
from
6148b32 to
8d87cd1
Compare
ifbarrera
approved these changes
Dec 9, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📲 What
Renames our existing
KoalaTrackingClienttoTrackingClientand addsTrackingClientConfigurationwhich is a struct with which we can initializeTrackingClientso that we can use it for more than one tracking endpoint.Note: We could even create a
TrackingClientto forward our events to something likeCrashlyticsfor our breadcrumbs. Currently we do this with a callback at the time of tracking, but I think we should move that to be its ownTrackingClient.Also note:
Koalaitself will be renamed separately too. Currently it really just behaves as an interface for sending the events to the clients.🤔 Why
We're going to be sending tracking events to two endpoints until we're able to fully deprecate
Koala, this allows us to do so in a somewhat controlled manner and still keeps things testable.🛠 How
TrackingClienttoTrackingClientConfigurationeither via simple properties or closures.Koala's initializer so that it can be initialized with two tracking clients.Koalawill send all tracking events to both clients, we will be doing some filtering of whitelisted events one we know what they are. I would suggest that we do this filtering inKoalaso that we can test it through mocked clients.TrackingClientConfiguration, this actually gives us a bit more test coverage than before because these parts ofKoalaTrackingClientweren't tested.✅ Acceptance criteria
With the simulator set to staging, return
trueforisKoalaTrackingEnabled(to be renamed) inEnvironmentVariables.swift. Use Charles Proxy to inspect traffic.DataLakeandKoalaendpoint.200and are in the correct structure for each endpoint (e.g.POSTforKoala,PUTforDataLake, etc.).⏰ TODO