braintree / braintree_ios

Braintree SDK for iOS
https://developer.paypal.com/braintree/docs/start/hello-client/ios/v5
MIT License
560 stars 296 forks source link

[QL] Batch Analytics Events #1295

Closed jaxdesmarais closed 4 months ago

jaxdesmarais commented 5 months ago

Summary of changes

Checklist

Authors

scannillo commented 5 months ago

I ran through a few flows (paypal, then card, then venmo, then paypal) with Xcode's network inspector. I am seeing more requests to v1/tracking than expected, and a few of them are returning a 400. I notice these have an empty event_params array:


Screenshot 2024-05-08 at 10 30 51 AM

Maybe part of the issue is re-setting a new Timer instance every time performEventRequest is called.

jaxdesmarais commented 4 months ago

In this commit: https://github.com/braintree/braintree_ios/pull/1295/commits/09da56cf9e101246df9b784e423bde902ad3b8c4 I moved to use a DispatchQueue. After doing some additional research on using Timer I found that it was deallocating and resetting unexpectedly. Also each time the method was called it would reset the timer in relation to the 30s interval. It only runs on the main thread and if the main thread is overladed it essentially just discards the timer as a whole. Using a DispatchQueue resulted in much more consistent firing of analytics.

KunJeongPark commented 4 months ago

This is really interesting PR Jax. I've run through all the scenarios I could think of and don't see any issues but I'm a bit nervous about mixing old and new. I've seen warnings on using checkedContinuation, for example, sometimes.

KunJeongPark commented 4 months ago

Another question I had was we have unit test failures, I see this on my PR as well.

jaxdesmarais commented 4 months ago

Another question I had was we have unit test failures, I see this on my PR as well.

@KunJeongPark seems like it was a flake since they are passing now. The UI test failures have been failing for most of this week. Running the failing UI test locally is passing.