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 3 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
115 changes: 95 additions & 20 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 @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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:
*
Expand Down Expand Up @@ -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
// remove cached parameters
List<String> parameterKeys = (List<String>) model.get("parameterKeys");
if (parameterKeys != null) {
for (String key : parameterKeys) {
Expand Down Expand Up @@ -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 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 +202,38 @@ 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);

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,
Copy link
Copy Markdown
Member

@dkayiwa dkayiwa Dec 2, 2024

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?

Copy link
Copy Markdown
Member Author

@ODORA0 ODORA0 Dec 2, 2024

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

Copy link
Copy Markdown
Member

@dkayiwa dkayiwa Dec 2, 2024

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?

Copy link
Copy Markdown
Member Author

@ODORA0 ODORA0 Dec 2, 2024

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

Suggested change
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

Suggested change
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

Copy link
Copy Markdown
Member

@dkayiwa dkayiwa Dec 2, 2024

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 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.

Copy link
Copy Markdown
Member Author

@ODORA0 ODORA0 Dec 2, 2024

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?

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 = "?";
Expand All @@ -211,25 +248,35 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
if (StringUtils.hasLength(heightString)) {
heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId());
}
for (Obs obs : patientObs) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

if (obs.getConcept().equals(weightConcept)) {
if (latestWeight == null
|| obs.getObsDatetime().compareTo(latestWeight.getObsDatetime()) > 0) {
latestWeight = obs;
}
} else if (obs.getConcept().equals(heightConcept)
&& (latestHeight == null || obs.getObsDatetime().compareTo(
latestHeight.getObsDatetime()) > 0)) {
latestHeight = obs;

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);
}
}
if (latestWeight != null) {
Copy link
Copy Markdown
Member

@dkayiwa dkayiwa Dec 13, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@dkayiwa dkayiwa Dec 13, 2024

Choose a reason for hiding this comment

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

Did you see the above comment?

model.put("patientWeight", latestWeight);
}
if (latestHeight != null) {
model.put("patientHeight", latestHeight);

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);
Copy link
Copy Markdown
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.

What happens to the model when latestHeight is null but latestWeight is not null?

Copy link
Copy Markdown
Member Author

@ODORA0 ODORA0 Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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")) {
Expand Down Expand Up @@ -354,7 +401,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 +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"));
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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;
}

}