Analytics - only fetch config before flushing events cache

[scannillo]

Background

• We have seen an unexpectedly high number of "core:api-request-latency" analytics for the /v1/configuration endpoint in FPTI, compared to the number of occurrences for each attempted payment method flow (see data below)
  • This analytic event was added in https://github.com/braintree/braintree_ios/pull/1292
• We haven't been able to replicate the exact issue we see in the FPTI dataset, but we do know that it is related to a quick succession of calls to /v1/configuration, while another is in-flight, resulting in duplicate/overlapping requests.

This PR reduces the number of calls to BTAPIClient.fetchConfiguration() in our Analytics layer. I don't know if this will solve 100% of the issue, but it should help.

Changes

• Move BTAPIClient.fetchConfiguration() call in BTAnalyticsService
  • Before it was being called in every sendAnalyticsEvent() method call
  • Now it is only called right before we flush our batch of analytics (every 20 seconds)
  • Shoutout to @sshropshire for this idea in stand-up!

Checklist

• [X] Added a changelog entry

Authors

@scannillo

[scannillo]

We could add a unit test like below, but I don't think it brings that much value. I think we should make a ticket for a holistic integration test for analytics, the batch POST, & the config GET. Open to folks opinions though.

func testSendAnalyticsEvent_doesNotMakeGETRequestForConfig() {
    let stubAPIClient: MockAPIClient = stubbedAPIClientWithAnalyticsURL("test://do-not-send.url")
    let analyticsService = BTAnalyticsService(apiClient: stubAPIClient)

    analyticsService.sendAnalyticsEvent("any.analytics.event1")

    XCTAssertFalse(stubAPIClient.fetchOrReturnRemoteConfigCalled)
}

[KunJeongPark]

We could add a unit test like below, but I don't think it brings that much value. I think we should make a ticket for a holistic integration test for analytics, the batch POST, & the config GET. Open to folks opinions though.

func testSendAnalyticsEvent_doesNotMakeGETRequestForConfig() {
    let stubAPIClient: MockAPIClient = stubbedAPIClientWithAnalyticsURL("test://do-not-send.url")
    let analyticsService = BTAnalyticsService(apiClient: stubAPIClient)

    analyticsService.sendAnalyticsEvent("any.analytics.event1")

    XCTAssertFalse(stubAPIClient.fetchOrReturnRemoteConfigCalled)
}

I think in some functions, we call sendAnalytics event before the fetch function. So if tokenize is called immediately after BTAPIClient instantiation, we would have prefetch call from BTAPIClient instantiation and possibly fetch call from analytics. Correct me if I am wrong. We definitely want to move the first sendAnalyticsEvent call in tokenize functions in the fetch call completion handler. I know that batched call would introduce a delay, but it's hard to predict timing of these events especially with cache expiration. And definitely really tough to write tests for different scenarios.

[scannillo]

I think in some functions, we call sendAnalytics event before the fetch function. So if tokenize is called immediately after BTAPIClient instantiation, we would have prefetch call from BTAPIClient instantiation and possibly fetch call from analytics. Correct me if I am wrong.

I'm not following this. This PR is really just a small change/fix. We never actually needed to fetch the config as early as we were before this PR (on each call to sendAnalyticsEvent). We only need the config value when we construct the batch of events to send to FPTI.

This PR now only retrieves the config (ideally it's cached by now) before we're ready to batch upload our queue of analytic events. This batch happens on a 20 second timer.

[KunJeongPark]

I think in some functions, we call sendAnalytics event before the fetch function. So if tokenize is called immediately after BTAPIClient instantiation, we would have prefetch call from BTAPIClient instantiation and possibly fetch call from analytics. Correct me if I am wrong.

I'm not following this. This PR is really just a small change/fix. We never actually needed to fetch the config as early as we were before this PR (on each call to sendAnalyticsEvent). We only need the config value when we construct the batch of events to send to FPTI.

This PR now only retrieves the config (ideally it's cached by now) before we're ready to batch upload our queue of analytic events. This batch happens on a 20 second timer.

Yes, I understand that. I was just trying to understand the timeline of whether we are trying to make these calls less frequent or injecting config value into BTAnalyticsService as we discussed as a possibility for scenario in this PR: https://github.com/braintree/braintree_ios/pull/1308

[scannillo]

Yes, I understand that. I was just trying to understand the timeline of whether we are trying to make these calls less frequent or injecting config value into BTAnalyticsService as we discussed as a possibility for scenario in this PR: https://github.com/braintree/braintree_ios/pull/1308

Like Rich called out, there is a problem with injecting it, since it could be expired.

This PR is a simple fix that could help things. It doesn't change any public APIs within the SDK.