LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209
Conversation
|
Did you get a chance to take a look at this? https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477199/Pull+Request+Tips |
ODORA0
changed the title
|
@dkayiwa please review again |
|
Can we also include the ticket id in the commit message? |
| Integer page = getPageParameter(request); | ||
| Integer pageSize = getPageSizeParameter(request, as); | ||
|
|
||
| // Get all observations |
There was a problem hiding this comment.
For each time that one moves through the pages, you are still fetching and loading all the patient's observations in memory. My impression was that you would load in memory only those observations for the current page.
| boolean includeVoided = false; | ||
|
|
||
| // Get observations for the current page | ||
| List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions, |
There was a problem hiding this comment.
Though the comment says get observations for the current page, i do not seen anything in the getObservations method call that does so. Was it just a typo?
There was a problem hiding this comment.
Ooh yeah, my bad the getObservations() method doesn't have parameters for offset/limit pagination, let me remove that comment
There was a problem hiding this comment.
So then, shall we then have any dashboard loading optimisation if the method loads all of them in memory?
There was a problem hiding this comment.
Okay, I critically looked at the ObsService interface, didn't notice that it lacks proper pagination parameters (offset/limit). Maybe this should be done as a DAO level implementation so we can have a DB level pagination support, but I would think of doing it this way as well i.e. we can use an existing API features like mostRecentN to limit initial load like so
| List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions, | |
| List<Obs> paginatedObs = Context.getObsService().getObservations( | |
| persons, | |
| null, | |
| null, | |
| null, | |
| null, | |
| null, | |
| Collections.singletonList("obsDatetime desc"), | |
| pageSize, // mostRecentN - only fetch what we need | |
| null, | |
| null, | |
| null, | |
| false | |
| ); |
or using date filtering
| List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions, | |
| Calendar cal = Calendar.getInstance(); | |
| cal.add(Calendar.DAY_OF_MONTH, -30); | |
| Date thirtyDaysAgo = cal.getTime(); | |
| List<Obs> recentObs = Context.getObsService().getObservations( | |
| persons, | |
| null, | |
| null, | |
| null, | |
| null, | |
| null, | |
| sort, | |
| null, | |
| null, | |
| thirtyDaysAgo, | |
| new Date(), | |
| false | |
| ); |
Your thoughts @dkayiwa
There was a problem hiding this comment.
Limiting the number of loaded obs with mostRecentN makes lots of sense to me. For as long as your use case allows it. That is, as long as you are aware that it behaves differently from page size in the sense that the rest of the obs will never be fetched.
There was a problem hiding this comment.
Okay!
@eudson Any thoughts on this, is it acceptable to only show the most recent observations?
Add pagination for patient observations in dashboard
Description:
Previously, the dashboard would load all patient observations at once, causing
performance issues for patients with many observations. This change:
Testing:
Ticket: https://openmrs.atlassian.net/browse/LUI-198