Skip to content

LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ODORA0 wants to merge 16 commits into
base: master
Choose a base branch
from
Open

LUI-198: Optimize patient dashboard loading by implementing pagination for observations #209

Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b03e0de
(Enhancement) Optimize patient dashboard loading by implementing pagi…
ODORA0 Nov 13, 2024
a06309a
Refactor to focus on pagination
ODORA0 Nov 14, 2024
ba7a0fc
DB-level pagination to only load in memory obs for current page
ODORA0 Nov 29, 2024
fa01899
Limit loaded obs to MostRecentN
ODORA0 Dec 4, 2024
e1c2f60
Remove hard coding and keep values dynamic based on GP set with defau…
ODORA0 Dec 4, 2024
9456377
Change naming conventions
ODORA0 Dec 5, 2024
98301a5
Create seperate GP for observations
ODORA0 Dec 6, 2024
639a616
Remove extra formatting
ODORA0 Dec 6, 2024
3426cb9
Remove extra formatting
ODORA0 Dec 6, 2024
1bf5bea
Remove false as default value
ODORA0 Dec 6, 2024
6bcaeda
Add StartIndex to getObservation api
ODORA0 Dec 6, 2024
d7e5866
Add proper null checks using StringUtils.hasText(), update getEncount…
ODORA0 Dec 8, 2024
a6b840c
Keep obs set for BMI calculations, remove unused limit param from model
ODORA0 Dec 9, 2024
b763c92
Enhance obs to load latest weight and height
ODORA0 Dec 13, 2024
568c1f9
Remove commented out code
ODORA0 Dec 13, 2024
9bf378f
Add model for latestHeight and weight
ODORA0 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 61 additions & 11 deletions omod/src/main/java/org/openmrs/web/controller/PortletController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,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:
*
Expand Down Expand Up @@ -101,7 +103,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons

AdministrationService as = Context.getAdministrationService();
ConceptService cs = Context.getConceptService();

// find the portlet that was identified in the openmrs:portlet taglib
Object uri = request.getAttribute("javax.servlet.include.servlet_path");
String portletPath = "";
Expand All @@ -112,8 +114,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
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

// remove cached parameters
List<String> parameterKeys = (List<String>) model.get("parameterKeys");
if (parameterKeys != null) {
for (String key : parameterKeys) {
Expand All @@ -128,11 +130,11 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
session.setAttribute(WebConstants.OPENMRS_PORTLET_CACHED_MODEL, model);
}
}

if (uri != null) {
long timeAtStart = System.currentTimeMillis();
portletPath = uri.toString();

// Allowable extensions are '' (no extension) and '.portlet'
if (portletPath.endsWith("portlet")) {
portletPath = portletPath.replace(".portlet", "");
Expand Down Expand Up @@ -160,8 +162,9 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
parameterKeys.addAll(moreParams.keySet());
}
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 there's an authenticated user, put them, and their patient set, in the
// model
if (Context.getAuthenticatedUser() != null) {
model.put("authenticatedUser", Context.getAuthenticatedUser());
}
Expand Down Expand Up @@ -195,8 +198,24 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}

if (Context.hasPrivilege(PrivilegeConstants.GET_OBS)) {
List<Obs> patientObs = Context.getObsService().getObservationsByPerson(p);
model.put("patientObs", patientObs);
// Get pagination parameters
Integer page = getPageParameter(request);
Integer pageSize = getPageSizeParameter(request, as);

// Get all observations
Copy link
Member

@dkayiwa dkayiwa Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

List<Obs> allObs = Context.getObsService().getObservationsByPerson(p);

// Manual pagination
int startIndex = page * pageSize;
int endIndex = Math.min(startIndex + pageSize, allObs.size());
List<Obs> paginatedObs = allObs.subList(startIndex, endIndex);

// Add pagination metadata to the model
model.put("currentPage", page);
model.put("pageSize", pageSize);
model.put("totalObsCount", allObs.size());
model.put("patientObs", paginatedObs);

Obs latestWeight = null;
Obs latestHeight = null;
String bmiAsString = "?";
Expand All @@ -211,7 +230,10 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
if (StringUtils.hasLength(heightString)) {
heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId());
}
for (Obs obs : patientObs) {
Copy link
Member

@dkayiwa dkayiwa Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intentionally remove this for loop? Is it related to the pagination?

Copy link
Member Author

@ODORA0 ODORA0 Dec 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dkayiwa dkayiwa Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  • Paginated query for display purposes (performance optimization)
  • Complete query for BMI calculation (functionality requirement)

Does this sound an approach to use?

Copy link
Member

@dkayiwa dkayiwa Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you see as the best approach?

Copy link
Member Author

@ODORA0 ODORA0 Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will go with this option I have suggested, in the long run we have separation of concerns. Change made


// Use allObs for weight/height calculation since we need all records to find
// the latest
for (Obs obs : allObs) {
if (obs.getConcept().equals(weightConcept)) {
if (latestWeight == null
|| obs.getObsDatetime().compareTo(latestWeight.getObsDatetime()) > 0) {
Expand Down Expand Up @@ -354,7 +376,8 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}
}

// if an encounter id is available, put "encounter" and "encounterObs" in the model
// 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)) {
Expand Down Expand Up @@ -425,4 +448,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"));
Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basing on the user interface screen that this controller serves, could you remind me on how this parameter is changed to a different value?

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 patientDashboard?page=2

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What i mean is, how is this parameter changed in the HTTP request, by the user interface?

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the dropdown selector

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share a screenshot of that entire page?

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!! 👍

Copy link
Member

@dkayiwa dkayiwa Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check the link about naming conventions?

Copy link
Member Author

@ODORA0 ODORA0 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

}