LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.Date; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
|
|
@@ -29,6 +30,7 @@ | |
| import org.openmrs.Concept; | ||
| import org.openmrs.ConceptNumeric; | ||
| import org.openmrs.Encounter; | ||
| import org.openmrs.Location; | ||
| import org.openmrs.Obs; | ||
| import org.openmrs.Patient; | ||
| import org.openmrs.Person; | ||
|
|
@@ -39,6 +41,7 @@ | |
| import org.openmrs.api.ConceptService; | ||
| import org.openmrs.api.context.Context; | ||
| import org.openmrs.module.legacyui.GeneralUtils; | ||
| import org.openmrs.util.OpenmrsConstants.PERSON_TYPE; | ||
| import org.openmrs.util.PrivilegeConstants; | ||
| import org.openmrs.web.WebConstants; | ||
| import org.springframework.util.StringUtils; | ||
|
|
@@ -49,6 +52,8 @@ public class PortletController implements Controller { | |
|
|
||
| protected Log log = LogFactory.getLog(this.getClass()); | ||
|
|
||
| private static final int DEFAULT_PAGE_SIZE = 50; | ||
|
|
||
| /** | ||
| * This method produces a model containing the following mappings: | ||
| * | ||
|
|
@@ -106,14 +111,15 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| Object uri = request.getAttribute("javax.servlet.include.servlet_path"); | ||
| String portletPath = ""; | ||
| Map<String, Object> model = null; | ||
|
|
||
| { | ||
| HttpSession session = request.getSession(); | ||
| String uniqueRequestId = (String) request.getAttribute(WebConstants.INIT_REQ_UNIQUE_ID); | ||
| String lastRequestId = (String) session.getAttribute(WebConstants.OPENMRS_PORTLET_LAST_REQ_ID); | ||
| if (uniqueRequestId.equals(lastRequestId)) { | ||
| model = (Map<String, Object>) session.getAttribute(WebConstants.OPENMRS_PORTLET_CACHED_MODEL); | ||
|
|
||
| // remove cached parameters | ||
| List<String> parameterKeys = (List<String>) model.get("parameterKeys"); | ||
| if (parameterKeys != null) { | ||
| for (String key : parameterKeys) { | ||
|
|
@@ -161,7 +167,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| } | ||
| model.put("parameterKeys", parameterKeys); // so we can clean these up in the next request | ||
|
|
||
| // if there's an authenticated user, put them, and their patient set, in the | ||
| // model | ||
| if (Context.getAuthenticatedUser() != null) { | ||
| model.put("authenticatedUser", Context.getAuthenticatedUser()); | ||
| } | ||
|
|
@@ -195,8 +202,38 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| } | ||
|
|
||
| if (Context.hasPrivilege(PrivilegeConstants.GET_OBS)) { | ||
| // Get pagination parameters | ||
| Integer page = getPageParameter(request); | ||
| Integer pageSize = getPageSizeParameter(request, as); | ||
|
|
||
| Person person = (Person) p; | ||
| List<Person> persons = Collections.singletonList(person); | ||
|
|
||
| // Setup parameters for database-level pagination | ||
| List<Encounter> encounters = null; | ||
| List<Concept> questions = null; | ||
| List<Concept> answers = null; | ||
| List<PERSON_TYPE> personTypes = null; | ||
| List<Location> locations = null; | ||
| List<String> sort = Collections.singletonList("obsDatetime desc"); | ||
| Integer obsGroupId = null; | ||
| Date fromDate = null; | ||
| Date toDate = null; | ||
| boolean includeVoided = false; | ||
|
|
||
| // Get observations for the current page | ||
| List<Obs> paginatedObs = Context.getObsService().getObservations(persons, encounters, questions, | ||
| answers, personTypes, locations, sort, null, obsGroupId, fromDate, toDate, includeVoided); | ||
|
|
||
| // Get total count for pagination | ||
| Integer totalCount = Context.getObsService().getObservationCount(persons, encounters, questions, | ||
| answers, personTypes, locations, obsGroupId, fromDate, toDate, includeVoided); | ||
|
|
||
| model.put("currentPage", page); | ||
| model.put("pageSize", pageSize); | ||
| model.put("totalObsCount", totalCount); | ||
| model.put("patientObs", paginatedObs); | ||
|
|
||
| Obs latestWeight = null; | ||
| Obs latestHeight = null; | ||
| String bmiAsString = "?"; | ||
|
|
@@ -211,25 +248,35 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| if (StringUtils.hasLength(heightString)) { | ||
| heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId()); | ||
| } | ||
|
There was a problem hiding this comment. Did you intentionally remove this for loop? Is it related to the pagination? There was a problem hiding this comment. Rather than loading all observations and finding the latest in memory, we would leverage the database to do this work more efficiently. Not directly related to pagination but aligns with the optimization goals since we no longer need to iterate over all patientObs to extract weight and height. There was a problem hiding this comment. With performance optimisations, unless you have tested and confirmed, you can be very shocked to find a different outcome. The database itself being an expensive resource, and you have anyway already fetched these observations in the above call, you can find that actually iterating through the collection in memory may be faster than making another expensive database call. So i would remove that loop only after i have done some confirmations. There was a problem hiding this comment. The queries for weight and height are correctly implemented, unless there are other dependencies on the for loop logic that exist that are not unaccounted for I think I will put this back to be on a safer side. There was a problem hiding this comment. @dkayiwa The paginated list only contains a limited number of observations (default 50), therefore BMI calculation couldn't find the necessary values, resulting in "?" if at all we add back the for loop while paginating. Now for this to work I would have to do something like this // Paginated observations for display
List<Obs> paginatedObs = Context.getObsService().getObservations(..., limit, startIndex, ...);
// Additional query to get ALL observations for BMI calculation
List<Obs> allObs = Context.getObsService().getObservations(persons, null, null, null, null,
null, null, null, null, null, null, false);
// Finding weight/height in complete set of observations
for (Obs obs : allObs) {
// ... looking for weight/height
}In essence, we'd now be using:
Does this sound an approach to use? There was a problem hiding this comment. What do you see as the best approach? There was a problem hiding this comment. I think I will go with this option I have suggested, in the long run we have separation of concerns. Change made |
||
|
|
||
| if (weightConcept != null) { | ||
| List<Concept> weightQuestions = Collections.singletonList(weightConcept); | ||
| List<String> weightSort = Collections.singletonList("obsDatetime desc"); | ||
|
|
||
| List<Obs> weightObs = Context.getObsService().getObservations(persons, null, | ||
| weightQuestions, null, null, null, weightSort, 1, null, null, null, false); | ||
|
|
||
| if (!weightObs.isEmpty()) { | ||
| latestWeight = weightObs.get(0); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. According to the original code, latestHeight or latestWeight was put in the model as long as it was not null. Did you intentionally change it? |
||
|
|
||
| if (heightConcept != null) { | ||
| List<Concept> heightQuestions = Collections.singletonList(heightConcept); | ||
| List<String> heightSort = Collections.singletonList("obsDatetime desc"); | ||
|
|
||
| List<Obs> heightObs = Context.getObsService().getObservations(persons, null, | ||
| heightQuestions, null, null, null, heightSort, 1, null, null, null, false); | ||
|
|
||
| if (!heightObs.isEmpty()) { | ||
| latestHeight = heightObs.get(0); | ||
| } | ||
| } | ||
|
|
||
| if (latestWeight != null && latestHeight != null) { | ||
| model.put("patientWeight", latestWeight); | ||
|
There was a problem hiding this comment. What happens to the model when latestHeight is null but latestWeight is not null? There was a problem hiding this comment. No BMI in the model, since both latestWeight and latestHeight are required for BMI calculation, the model will not have patientBmi or patientBmiAsString if either is null. he model will only include the observation that is not null (patientWeight in this case). But since I will revert this, I think any changes like setting a default value or log warning wouldn't be necessary |
||
| model.put("patientHeight", latestHeight); | ||
|
|
||
| double weightInKg; | ||
| double heightInM; | ||
| if (weightConcept.getUnits().equalsIgnoreCase("kg")) { | ||
|
|
@@ -354,7 +401,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| } | ||
| } | ||
|
|
||
| // if an encounter id is available, put "encounter" and "encounterObs" in the | ||
| // model | ||
| o = request.getAttribute("org.openmrs.portlet.encounterId"); | ||
| if (o != null && !model.containsKey("encounterId")) { | ||
| if (!model.containsKey("encounter") && Context.hasPrivilege(PrivilegeConstants.GET_ENCOUNTERS)) { | ||
|
|
@@ -425,4 +473,31 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons | |
| protected void populateModel(HttpServletRequest request, Map<String, Object> model) { | ||
| } | ||
|
|
||
| private Integer getPageParameter(HttpServletRequest request) { | ||
| try { | ||
| return Integer.parseInt(request.getParameter("page")); | ||
|
There was a problem hiding this comment. Can you take a look at our naming convention for the pagination parameters? https://openmrs.atlassian.net/wiki/spaces/docs/pages/25469830/REST+Web+Services+API+For+Clients#Limiting-the-number-of-results-and-paging There was a problem hiding this comment. Basing on the user interface screen that this controller serves, could you remind me on how this parameter is changed to a different value? There was a problem hiding this comment. So the method retrieves the page parameter from the HTTP request, and if it'ss missing or is invalid like non numeric it will default to 0. The page value calculates which subset of data (encounters or observations) to fetch There was a problem hiding this comment. What i mean is, how is this parameter changed in the HTTP request, by the user interface? There was a problem hiding this comment. With the dropdown selector There was a problem hiding this comment. Can you share a screenshot of that entire page? There was a problem hiding this comment. Did you check the link about naming conventions? There was a problem hiding this comment. Yes, I have changed that, please review |
||
| } | ||
| catch (NumberFormatException e) { | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| private Integer getPageSizeParameter(HttpServletRequest request, AdministrationService as) { | ||
| try { | ||
| String pageSizeParam = request.getParameter("pageSize"); | ||
| if (pageSizeParam != null) { | ||
| return Integer.parseInt(pageSizeParam); | ||
| } | ||
|
|
||
| String globalPageSize = as.getGlobalProperty("dashboard.defaultPageSize"); | ||
| if (globalPageSize != null) { | ||
| return Integer.parseInt(globalPageSize); | ||
| } | ||
| } | ||
| catch (NumberFormatException e) { | ||
| log.debug("Unable to parse page size parameter, using default", e); | ||
| } | ||
| return DEFAULT_PAGE_SIZE; | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
or using date filtering
Your thoughts @dkayiwa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limiting the number of loaded obs with
mostRecentNmakes 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay!
@eudson Any thoughts on this, is it acceptable to only show the most recent observations?