medic / care-teams

For Product Management
0 stars 0 forks source link

contact_list apdex: research and development cycle 2 #98

Closed michaelkohn closed 7 months ago

michaelkohn commented 7 months ago

Iteration 1 results here 👇

Benmuiruri commented 7 months ago

Main research outcome

  1. I reviewed all the functions in contactsComponent particularly formatContacts (code) and query (code) which do the heavy work in the component.
  1. I also reviewed the searchService

    • I did not see any glaring inefficiencies / code redundancies that could improve performance.
    • The search service is already using debounce (code) which prevents identical subsequent searches
    • The getLastVisitedDates is already using searchResultsCachewhich uses cached last visited dates to avoid unnecessary data fetching when applicable.
    • One of the contacts related functionality in the searchService is getLastVisitedDates but the service efficiently and conditionally fetches the last visited date only when requested.
  2. I reviewed the forum and CHT-Core issues:

    • I found this issue that suggets possible pagination for RHS contacts. However given a place with 5000 contacts is an outlier, I did not consider the issue for now, I think we can address the issue as a code health ticket.
    • I found this issue with suggestions of improving the ContactsService but from my analysis that would not improve loading the ContactsComponent.

cc @garethbowen do you have any suggestions I could consider or comment on anything I may have missed that could improve performance ? Perhaps insights from previous discussions / code health issues.

garethbowen commented 7 months ago

However given a place with 5000 contacts is an outlier, I did not consider the issue for now, I think we can address the issue as a code health ticket.

As this came up in a production instance I wouldn't be so quick to write it off as an outlier. We should put a limit on what's rendered on the RHS because showing 5k children is useless anyway.

The main improvement in the search service is to do with freetext queries which don't behave as expected and slow down the entire application by generating indexes that are far too large. This isn't a quick win because we need to verify how people use the search service before we go changing how it behaves, but I think it will have a massive impact on the overall performance of the application. I recommend taking a scientific approach to these changes and measuring what improvements could be gained by a massive reduction in index size, for example, removing the field name prefix in the index (which I suspect is never used) and/or indexing only the name field.

There is some more info on the forum: https://forum.communityhealthtoolkit.org/t/unexpected-search-results/3288

Benmuiruri commented 7 months ago

Gareth makes good points above

  1. It is important to limit number of contacts in the RHS but that won't help the scope of contact_list apdex because our CHW Areas have 100 contacts and households have 6-8 contacts.
  2. The improvement in the search service will have possible massive improvements but as Gareth points out, it is not a quick win. It needs involves much more work put in and more considerations.

Possible Performance Improvement

Having not found code inefficiencies calling out to be improved (kudos to the devs ), I experimented with reducing the number of docs fetched.

Performance when loading 50 records ``` "contact_list:load": { "sum": 8258, "min": 650, "max": 5877, "count": 4, "sumsqr": 36500994 }, "contact_list:load:apdex:satisfied": { "sum": 2381, "min": 650, "max": 1009, "count": 3, "sumsqr": 1961865 }, "contact_list:load:apdex:tolerable": { "sum": 5877, "min": 5877, "max": 5877, "count": 1, "sumsqr": 34539129 }, "contact_list:query": { "sum": 9956, "min": 285, "max": 5687, "count": 7, "sumsqr": 36196828 }, "contact_list:query:apdex:satisfied": { "sum": 4269, "min": 285, "max": 1270, "count": 6, "sumsqr": 3854859 }, "contact_list:query:apdex:tolerable": { "sum": 5687, "min": 5687, "max": 5687, "count": 1, "sumsqr": 32341969 } ```
Performance when loading 25 records ``` "contact_list:load": { "sum": 1205, "min": 370, "max": 465, "count": 3, "sumsqr": 490025 }, "contact_list:load:apdex:satisfied": { "sum": 1205, "min": 370, "max": 465, "count": 3, "sumsqr": 490025 }, "contact_list:query": { "sum": 2433, "min": 279, "max": 793, "count": 6, "sumsqr": 1172589 }, "contact_list:query:apdex:satisfied": { "sum": 2433, "min": 279, "max": 793, "count": 6, "sumsqr": 1172589 } ```

cc @michaelkohn and @n-orlowski is changing the number of docs fetched something we can consider ?

michaelkohn commented 7 months ago

The reality is that on many of the mobile devices being used by CHWs, there are only 8-10 rows shown at a time anyway so they'd have to scroll through 2-5 pages of households before the second "load" happens (if we set it at 25). Desktop view will have more rows in the list but they'll also have better memory/processors. Also, I note that you mention testing with "Good 3G" but since these are offline users, I would not expect any network activity when loading the contact list anyway, if there is... that seems wrong.

Here's an example of a user on a low spec phone loading the contacts list. It takes about 8-9 seconds to load and there are about 6 rows shown at a time. This is also an older version of the CHT (bottom action bar instead of FAB) so they'd see maybe 1 more row if they had the FAB.

Here's what a user sees on a Safaricom phone ![Screenshot 2024-04-19 at 11 25 46 AM](https://github.com/medic/care-teams/assets/10096985/ee6f42af-711b-4367-bf91-8f9a6526c594)

From a UX perspective, I'd be open to changing to 25 if the performance was significantly better in our target setup (low spec phone) and it was a quick change... as long as @n-orlowski was OK with the UX.

n-orlowski commented 7 months ago

+1 changing to 25 with that loading data

Benmuiruri commented 7 months ago

Conclusion of cycle 2

  1. There was no particular changes that could lead to significant performance improvements.
  2. Gareth had 2 suggestions but they do not directly improve contact_list:load. Updating the index could potentially improve contact_list:query but:
    • It requires more development time as Gareth explains
    • To get the performance improvements of changing the indexes, partners would have to re-index their databases (A time expensive affair)
  3. Since initial tests on my dev setup showed slightly better performance when loading 25 records, I will work with @ralfudx to run comparative tests (25 vs 50) to determine whether it is worth making the change.
michaelkohn commented 7 months ago

Since initial tests on my dev setup showed slightly better performance when loading 25 records, I will work with @ralfudx to run comparative tests (25 vs 50) to determine whether it is worth making the change.

Cool, thanks @Benmuiruri . So the goal for Monday will be to get a baseline loading on low spec device, make the code change, test again, determine whether it is worth the change. I'm going to close this ticket and create another one for the next sprint.

I'd also like to understand / explore the index update route in more detail as well. Let's follow up and think of a plan for that next week.