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

Analytics - only fetch config before flushing events cache #1337

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Analytics - only fetch config before flushing events cache #1337

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
590df57
Only retrieve config when flushing cache, not per sendEvent analytics…
scannillo Jun 14, 2024
d6e608c
Add CHANGELOG entry
scannillo Jun 17, 2024
5f53eeb
Update Sources/BraintreeCore/Analytics/BTAnalyticsService.swift
scannillo Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Braintree iOS SDK Release Notes

## unreleased
* BraintreeCore
* For analytics, only call `fetchOrReturnRemoteConfig()` when batch uploading, not on each analytic event enqueue

## 6.21.0 (2024-06-12)
* BraintreePayPal
* Add PayPal App Switch vault flow (BETA)
Expand Down
74 changes: 37 additions & 37 deletions Sources/BraintreeCore/Analytics/BTAnalyticsService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,52 +103,52 @@ class BTAnalyticsService: Equatable {

await BTAnalyticsService.events.append(event)

do {
let configuration = try await apiClient.fetchConfiguration()

// TODO: - Refactor to make HTTP non-optional property and instantiate in init()
if self.http == nil {
self.http = BTHTTP(authorization: self.apiClient.authorization, customBaseURL: BTAnalyticsService.url)
}

// A special value passed in by unit tests to prevent BTHTTP from actually posting
if let http = self.http, http.customBaseURL?.absoluteString == "test://do-not-send.url" {
return
}
// TODO: - Refactor to make HTTP non-optional property and instantiate in init()
if self.http == nil {
self.http = BTHTTP(authorization: self.apiClient.authorization, customBaseURL: BTAnalyticsService.url)
}

// A special value passed in by unit tests to prevent BTHTTP from actually posting
if let http = self.http, http.customBaseURL?.absoluteString == "test://do-not-send.url" {
return
}

if shouldBypassTimerQueue {
await self.sendQueuedAnalyticsEvents()
return
}

if BTAnalyticsService.timer == nil {
BTAnalyticsService.timer = DispatchSource.makeTimerSource(queue: self.http?.dispatchQueue)
BTAnalyticsService.timer?.schedule(
deadline: .now() + .seconds(self.timerInterval),
repeating: .seconds(self.timerInterval),
leeway: .seconds(1)
)

if shouldBypassTimerQueue {
await self.sendQueuedAnalyticsEvents(configuration: configuration)
return
}

if BTAnalyticsService.timer == nil {
BTAnalyticsService.timer = DispatchSource.makeTimerSource(queue: self.http?.dispatchQueue)
BTAnalyticsService.timer?.schedule(
deadline: .now() + .seconds(self.timerInterval),
repeating: .seconds(self.timerInterval),
leeway: .seconds(1)
)

BTAnalyticsService.timer?.setEventHandler {
Task {
await self.sendQueuedAnalyticsEvents(configuration: configuration)
}
BTAnalyticsService.timer?.setEventHandler {
Task {
await self.sendQueuedAnalyticsEvents()
}

BTAnalyticsService.timer?.resume()
}
} catch {
return
BTAnalyticsService.timer?.resume()
}
}

// MARK: - Helpers

func sendQueuedAnalyticsEvents(configuration: BTConfiguration) async {
func sendQueuedAnalyticsEvents() async {
if await !BTAnalyticsService.events.isEmpty {
let postParameters = await createAnalyticsEvent(config: configuration, sessionID: apiClient.metadata.sessionID, events: BTAnalyticsService.events.allValues)
http?.post("v1/tracking/batch/events", parameters: postParameters) { _, _, _ in }
await BTAnalyticsService.events.removeAll()
do {
let configuration = try await apiClient.fetchConfiguration()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we also want to consider setting a property for this in the class and using an if let to continue to reuse the config if not nil? That could let us reuse the config for the entire flow vs fetching it every 20 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern there is if the config had expired, we would be using an expired config

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's stored as a singleton in the ConfigurationCache couldn't we check that? Psudocode but something like:

if let configuration, configuration = try? ConfigurationCache.shared.getFromCache(authorization: self.authorization.bearer)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But honestly, does that even matter? Let's say a merchant uses our SDK with multiple merchantIDs (ie refreshing their clientToken would possible use a new merchantID). They would have to re-create a BTAPIClient with that refreshed clientToken which would create a new instance of BTAnalyticsService anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in cases where merchants don't, which I believe is more common, it would avoid an additional hop to fetchConfiguration which while minor could reduce some latency. It is a take it or leave it comment so if you don't agree we don't need to implement it, was just asking a question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh weird! My comment "But honestly, does that even matter?" was in response to questioning my previous comment here 😆 . I must not have refreshed to see your comment, my apologies!

Since it's stored as a singleton in the ConfigurationCache couldn't we check that? Psudocode but something like:

We could but I don't think we want to leak logic of checking the config cache into the analytics layer. I think @richherrera is working on moving to the ConfigurationLoader pattern, like Android, which we can call in here once that's ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rich and I are pairing on working on the ConfigurationLoader pattern like you had suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could but I don't think we want to leak logic of checking the config cache into the analytics layer. I think @richherrera is working on moving to the ConfigurationLoader pattern, like Android, which we can call in here once that's ready.

That makes sense, we can punt on it until the ConfigurationLoader pattern is established!


scannillo marked this conversation as resolved.
Show resolved Hide resolved
let postParameters = await createAnalyticsEvent(config: configuration, sessionID: apiClient.metadata.sessionID, events: BTAnalyticsService.events.allValues)
http?.post("v1/tracking/batch/events", parameters: postParameters) { _, _, _ in }
await BTAnalyticsService.events.removeAll()
} catch {
return
}
}
}

Expand Down
Loading