Automattic-Tracks-Android icon indicating copy to clipboard operation
Automattic-Tracks-Android copied to clipboard

Move decorating events with mutable device info to a background thread

Open wzieba opened this issue 1 year ago • 3 comments

Description

This PR is related to improving performance of TracksClient#track method. This is crucial for performance of projects using this library - this method is called in numerous places, very often on the main thread. Internally we discussed and measure impact: paqN3M-18i-p2

Fix

I moved execution of the longest method, DeviceInformation#getMutableDeviceInfo, to a thread that is responsible for moving events to local database. As a result, getMutableDeviceInfo is executed a few milliseconds later than previously, but in reality it shouldn't affect business value that we gain from these properties.

Test

Performance testing

I've introduced benchmark module to perform these tests. I don't add CI check of any kind, as it's not trivial when it comes to performance benchmarking and requires rather a solid setup which holds results from many previous executions. (https://medium.com/androiddevelopers/fighting-regressions-with-benchmarks-in-ci-6ea9a14b5c71).

  1. Run basicTrackMethod() on a real device
  2. Write down the median value (see test result right panel > "Benchmark" tab)
  3. Revert the fix: git revert 82f903cf53b3cfcc8f8af5d81e7e1842029943af
  4. Run basicTrackMethod() on a real device again
  5. Compare results from both executions: first result should be significantly lower.

Validation testing

In this test, we want only to validate if "mutable device info" are still updated.

  1. I used WooCommerce for this. If https://github.com/woocommerce/woocommerce-android/pull/11416 is not yet merged, please use it as a base as it addresses breaking changes introduced with Tracks 5.0.0 release.
  2. Replace a version of Tracks with the one from this PR (recent: 214-5c57d4048541a07f1e7b8aa011366e6844e71aee)
  3. Use some network intercepting software like Charles or Proxyman
  4. Perform some navigation (e.g. open order details): see that after some time there's a call to https://public-api.wordpress.com/rest/v1.1/tracks/record with commonProps array and device_info_device_orientation in it. Verify the value was correct.
  5. Change phone orientation
  6. Perform again some operations and verify if after a few seconds there's another call to tracks, this time with opposite value for device_info_device_orientation.

Screenshots

Those are from my performance tests. They show median execution time over multiple executions.

Before

image

After

image

wzieba avatar Apr 29 '24 11:04 wzieba

Because we discussed around this topic recently, and it's relatively easy to test this change on WooCommerce - @hichamboushaba do you mind reviewing this PR?

wzieba avatar Apr 30 '24 07:04 wzieba

Because we discussed around this topic recently, and it's relatively easy to test this change on WooCommerce - @hichamboushaba do you mind reviewing this PR?

Sorry for the late reply @wzieba, would it be OK to review this tomorrow or next Monday? I have to focus on finishing up a project before the code freeze.

hichamboushaba avatar May 02 '24 09:05 hichamboushaba

No worries @hichamboushaba , tomorrow or next Monday is totally fine! Thanks

wzieba avatar May 02 '24 09:05 wzieba