Skip to content
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

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

Open
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

Show file tree
Hide file tree
Changes from 12 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
111 changes: 90 additions & 21 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 Down Expand Up @@ -49,6 +50,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 +109,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 +165,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 All @@ -183,7 +188,11 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons

// add encounters if this user can view them
if (Context.hasPrivilege(PrivilegeConstants.GET_ENCOUNTERS)) {
model.put("patientEncounters", Context.getEncounterService().getEncountersByPatient(p));
Integer limit = getLimitParameter(request, as, "dashboard.encounters.maximumNumberToShow");
Integer startIndex = getStartIndexParameter(request);

model.put("patientEncounters",
Context.getEncounterService().getEncounters(null, p.getPatientId(), startIndex, limit, false));
}

// add visits if this user can view them
Expand All @@ -195,43 +204,68 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
}

if (Context.hasPrivilege(PrivilegeConstants.GET_OBS)) {
List<Obs> patientObs = Context.getObsService().getObservationsByPerson(p);
model.put("patientObs", patientObs);
Integer limit = getLimitParameter(request, as, "dashboard.observations.maximumNumberToShow");
Integer startIndex = getStartIndexParameter(request);

Person person = (Person) p;
List<Person> persons = Collections.singletonList(person);

// Get most recent observations using limit parameter
List<Obs> paginatedObs = Context.getObsService().getObservations(persons, null, null, null, null,
Copy link
Member

Choose a reason for hiding this comment

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

According to the user interface you shared, what do you do with the startIndex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Select the page size

Copy link
Member

Choose a reason for hiding this comment

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

Is the page size the same as startIndex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, based on the link you shared I renamed pageSize to that

Copy link
Member

Choose a reason for hiding this comment

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

Where are you passing the two parameters in the getObservations api call?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are only passing the limit and not startIndex directly in that method call

Copy link
Member

Choose a reason for hiding this comment

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

Which means that if you are on the last page, you still load all obs from the database to memory.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dkayiwa Yes that's right, added that

null, Collections.singletonList("obsDatetime desc"), limit, startIndex, null, null, false);

model.put("limit", limit);
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me of why we are storing limit in the model?

Copy link
Member Author

Choose a reason for hiding this comment

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

We storing limit in the model to make it accessible to/for the JSP and also keeping the pagination logic synchronised between the backend and JSP frontend

Copy link
Member

Choose a reason for hiding this comment

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

Can you point me to the JSP that is currently using this limit that you have put in the model?

Copy link
Member Author

Choose a reason for hiding this comment

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

The patientEncounters jsp

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind pointing me to the actual line that uses limit?

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

Looking at the dashboard JSP file, no references to a limit or pagination for observations, safe deleting it.

model.put("patientObs", paginatedObs);

// Handle BMI calculation
Obs latestWeight = null;
Obs latestHeight = null;
String bmiAsString = "?";

try {
String weightString = as.getGlobalProperty("concept.weight");
ConceptNumeric weightConcept = null;
if (StringUtils.hasLength(weightString)) {
weightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(weightString).getConceptId());
}

String heightString = as.getGlobalProperty("concept.height");
ConceptNumeric heightConcept = null;
if (StringUtils.hasLength(heightString)) {
heightConcept = cs.getConceptNumeric(GeneralUtils.getConcept(heightString).getConceptId());
}
for (Obs obs : patientObs) {
Copy link
Member

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

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

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

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

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

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
Member

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
Member

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
Member

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
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")) {
weightInKg = latestWeight.getValueNumeric();
} else if (weightConcept.getUnits().equalsIgnoreCase("lb")) {
Expand All @@ -240,6 +274,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
throw new IllegalArgumentException("Can't handle units of weight concept: "
+ weightConcept.getUnits());
}

if (heightConcept.getUnits().equalsIgnoreCase("cm")) {
heightInM = latestHeight.getValueNumeric() / 100;
} else if (heightConcept.getUnits().equalsIgnoreCase("m")) {
Expand All @@ -250,6 +285,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
throw new IllegalArgumentException("Can't handle units of height concept: "
+ heightConcept.getUnits());
}

double bmi = weightInKg / (heightInM * heightInM);
model.put("patientBmi", bmi);
String temp = "" + bmi;
Expand All @@ -261,6 +297,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
log.error("Failed to calculate BMI even though a weight and height were found", ex);
}
}

model.put("patientBmiAsString", bmiAsString);
} else {
model.put("patientObs", new HashSet<Obs>());
Expand Down Expand Up @@ -354,7 +391,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 +463,35 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons
protected void populateModel(HttpServletRequest request, Map<String, Object> model) {
}

private Integer getStartIndexParameter(HttpServletRequest request) {
try {
String startIndexParam = request.getParameter("startIndex");
if (StringUtils.hasText(startIndexParam)) {
return Integer.parseInt(startIndexParam);
}
}
catch (NumberFormatException e) {
log.debug("Unable to parse startIndex parameter, using default", e);
}
return 0; // Return first page if not specified or invalid
}

private Integer getLimitParameter(HttpServletRequest request, AdministrationService as, String globalPropertyKey) {
try {
String limitParam = request.getParameter("limit");
if (StringUtils.hasText(limitParam)) {
return Integer.parseInt(limitParam);
}

String globalLimit = as.getGlobalProperty(globalPropertyKey);
if (StringUtils.hasText(globalLimit)) {
return Integer.parseInt(globalLimit);
}
}
catch (NumberFormatException e) {
log.debug("Unable to parse limit parameter, using default", e);
}
return DEFAULT_PAGE_SIZE;
}

}
9 changes: 8 additions & 1 deletion omod/src/main/resources/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,13 @@
Allows one to limit the number of encounters shown on the form entry tab of the patient dashboard specifically
</description>
</globalProperty>
<globalProperty>
<property>dashboard.observations.maximumNumberToShow</property>
<defaultValue></defaultValue>
<description>
Allows one to limit the number of observations shown on the patient dashboard
specifically.
</description>
</globalProperty>

</module>

Loading