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 [QL] Measure API Request Latency for PayPal Flows #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

• 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.

[jaxdesmarais]

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.

[scannillo]

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

[jaxdesmarais]

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)

[scannillo]

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.

[jaxdesmarais]

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.

[scannillo]

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.

[KunJeongPark]

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

[jaxdesmarais]

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!

[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:
#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:
#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.