Add perf monitoring trace for case list loading time #3297
Conversation
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant Task as EntityLoaderTask
participant Helper as EntityLoaderHelper
participant Perf as FirebasePerformance Trace
participant Utils as ReportingUtils
Task->>Helper: isAsyncNodeEntityFactory()
alt Non-async factory
Task->>Perf: startTrace(CCPerfMonitoring.SYNC_ENTITY_LIST_LOADING)
Task->>Utils: addAttribute(CCAnalyticsParam.CCHQ_DOMAIN, domain)
Task->>Utils: addAttribute(CCAnalyticsParam.USERNAME, username)
end
Task->>Helper: loadEntities()
Helper-->>Task: entities
alt Non-async factory
Task->>Perf: putAttribute(CCPerfMonitoring.NUM_CASES_LOADED, entities.count)
Task->>Perf: stopTrace()
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~9 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/org/commcare/tasks/EntityLoaderTask.java (1)
59-65: Optimize trace initialization and consider error handling.The trace is always initialized but only started conditionally, which is inefficient. Consider moving the trace initialization inside the condition to avoid unnecessary object creation.
- Trace trace = FirebasePerformance.getInstance().newTrace(CCPerfMonitoring.SYNC_ENTITY_LIST_LOADING); - if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { - // Capture sync_case_list_loading trace for performance monitoring - trace.putAttribute(CCAnalyticsParam.CCHQ_DOMAIN, ReportingUtils.getDomain()); - trace.putAttribute(CCAnalyticsParam.USERNAME, ReportingUtils.getUser()); - trace.start(); - } + Trace trace = null; + if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { + // Capture sync_case_list_loading trace for performance monitoring + trace = FirebasePerformance.getInstance().newTrace(CCPerfMonitoring.SYNC_ENTITY_LIST_LOADING); + trace.putAttribute(CCAnalyticsParam.CCHQ_DOMAIN, ReportingUtils.getDomain()); + trace.putAttribute(CCAnalyticsParam.USERNAME, ReportingUtils.getUser()); + trace.start(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java(2 hunks)app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt(1 hunks)app/src/org/commcare/tasks/EntityLoaderHelper.kt(2 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/org/commcare/tasks/EntityLoaderTask.java (2)
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
CCAnalyticsParam(7-41)app/src/org/commcare/tasks/EntityLoaderHelper.kt (2)
isAsyncNodeEntityFactory(83-85)loadEntities(58-81)
🔇 Additional comments (7)
app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (1)
3-9: LGTM! Clean performance monitoring constants definition.The Kotlin object provides a centralized location for Firebase Performance monitoring constants with clear naming and proper categorization. The constants follow standard naming conventions and align with the Firebase Performance API expectations.
app/src/org/commcare/tasks/EntityLoaderHelper.kt (2)
83-85: LGTM! Good refactoring to eliminate code duplication.The extraction of the type check into a dedicated method improves code readability and maintainability by centralizing the logic for determining async factory behavior.
62-62: LGTM! Consistent usage of the extracted helper method.The replacement of inline type checks with the
isAsyncNodeEntityFactory()method call maintains the same behavior while improving code consistency and maintainability.Also applies to: 75-75
app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java (1)
13-13: LGTM! Appropriate visibility change for cross-module analytics.Making
CCHQ_DOMAINandUSERNAMEconstants public enables their use in the performance monitoring instrumentation while maintaining the existing functionality and string values.Also applies to: 26-26
app/src/org/commcare/tasks/EntityLoaderTask.java (3)
5-6: LGTM! Appropriate imports for Firebase Performance instrumentation.The imports are necessary for the performance monitoring functionality and follow standard Java import conventions.
Also applies to: 9-9, 12-13
66-66: LGTM! Good refactoring to capture load results.Storing the load results in a local variable improves code readability and enables the performance monitoring logic to access the entity count for metrics recording.
Also applies to: 73-73
74-79: LGTM! Improved error handling with proper exception propagation.The enhanced XPathException handling properly logs the error and sets
mExceptionto ensure the error is propagated toonPostExecutefor proper error delivery to the listener.
| try { | ||
| return entityLoaderHelper.loadEntities(nodeset[0], this); | ||
| Trace trace = FirebasePerformance.getInstance().newTrace(CCPerfMonitoring.SYNC_ENTITY_LIST_LOADING); | ||
| if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that entityLoaderHelper null check is redundant.
The null check for entityLoaderHelper appears redundant since it's initialized in the constructor and should never be null during execution.
Also applies to: 68-68
🏁 Script executed:
#!/bin/bash
# Description: Verify that entityLoaderHelper is always non-null in EntityLoaderTask
# Expected: entityLoaderHelper should be initialized in constructor and never set to null
# Search for entityLoaderHelper field assignments
ast-grep --pattern 'class EntityLoaderTask {
$$$
entityLoaderHelper = $_;
$$$
}'
# Search for any null assignments to entityLoaderHelper
rg -A 5 "entityLoaderHelper\s*=\s*null"
# Search for constructor to confirm initialization
ast-grep --pattern 'EntityLoaderTask($$$) {
$$$
entityLoaderHelper = $_;
$$$
}'Length of output: 724
🏁 Script executed:
#!/bin/bash
# Locate the EntityLoaderTask source file and inspect entityLoaderHelper usage
# 1. Find the file containing the class declaration
FILE=$(rg -l "class EntityLoaderTask" --max-count 1)
echo "Found file: $FILE"
echo
# 2. List all references to entityLoaderHelper in that file
echo "=== entityLoaderHelper references ==="
rg -n "entityLoaderHelper" "$FILE" || echo "No occurrences found"
echo
# 3. Show the constructor signatures for EntityLoaderTask
echo "=== Constructors in EntityLoaderTask ==="
rg -n "EntityLoaderTask\(" -C2 "$FILE" || echo "No constructors found"Length of output: 1664
Remove redundant null checks for entityLoaderHelper
Both null checks around entityLoaderHelper are unnecessary because it’s a final field initialized in every constructor. You can simplify the two if statements as follows:
- app/src/org/commcare/tasks/EntityLoaderTask.java:60
- app/src/org/commcare/tasks/EntityLoaderTask.java:68
Suggested diff:
- if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) {
+ if (!entityLoaderHelper.isAsyncNodeEntityFactory()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { | |
| if (!entityLoaderHelper.isAsyncNodeEntityFactory()) { |
🤖 Prompt for AI Agents
In app/src/org/commcare/tasks/EntityLoaderTask.java around lines 60 and 68,
remove the redundant null checks for the final field entityLoaderHelper (it is
initialized in every constructor) by simplifying the two if statements to only
check isAsyncNodeEntityFactory() (i.e., drop "entityLoaderHelper != null &&"),
leaving just the necessary boolean condition so the code is clearer and avoids
unnecessary null checks.
| if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { | ||
| trace.putAttribute(CCPerfMonitoring.NUM_CASES_LOADED, | ||
| String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size()))); | ||
| trace.stop(); | ||
| } |
There was a problem hiding this comment.
Add null safety check for trace object.
The trace stopping logic assumes the trace was initialized, but it could be null if the condition wasn't met during initialization.
- if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) {
+ if (trace != null && entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) {
trace.putAttribute(CCPerfMonitoring.NUM_CASES_LOADED,
String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size())));
trace.stop();
}Note: This change should be applied together with the trace initialization optimization suggested above.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { | |
| trace.putAttribute(CCPerfMonitoring.NUM_CASES_LOADED, | |
| String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size()))); | |
| trace.stop(); | |
| } | |
| if (trace != null && entityLoaderHelper != null && !entityLoaderHelper.isAsyncNodeEntityFactory()) { | |
| trace.putAttribute(CCPerfMonitoring.NUM_CASES_LOADED, | |
| String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size()))); | |
| trace.stop(); | |
| } |
🤖 Prompt for AI Agents
In app/src/org/commcare/tasks/EntityLoaderTask.java around lines 68-72, the code
calls trace.putAttribute(...) and trace.stop() without ensuring trace is
non-null; add a null check (e.g., if (trace != null) { ... }) around both
operations so you only putAttribute and stop when trace was actually
initialized, and ensure this change is coordinated with the trace initialization
optimization mentioned in the other review.
avazirna
force-pushed
the
add-case-list-loading-perf-monitoring-trace
branch
from
509eeef to
58d9765
Compare
avazirna
force-pushed
the
add-case-list-loading-perf-monitoring-trace
branch
from
58d9765 to
a6dc921
Compare
|
@damagatchi retest this please |
| trace.putAttribute(CCAnalyticsParam.CCHQ_DOMAIN, ReportingUtils.getDomain()); | ||
| trace.putAttribute(CCAnalyticsParam.USERNAME, ReportingUtils.getUser()); | ||
| trace.start(); |
There was a problem hiding this comment.
can we put all of this in CCPerfMonitoring. createTrace instead ? (maybe directly call it 1startTracing`). (the idea being one should not repeat this code for every trace we add)
There was a problem hiding this comment.
good point, let me do that
| object CCPerfMonitoring { | ||
| // Traces | ||
| // Measures the duration of synchronous case list loading | ||
| const val SYNC_ENTITY_LIST_LOADING = "sync_case_list_loading" |
There was a problem hiding this comment.
opt: would be nice to adapt a naming conventions here that distinguishes between the trace and attributes like - TRACE_ SYNC_ENTITY_LIST_LOADING and ATTR_ NUM_CASES_LOADED
| trace.stop(); | ||
| CCPerfMonitoring.INSTANCE.stopTracing(trace); |
There was a problem hiding this comment.
seems like we are calling stop twice now
avazirna
force-pushed
the
add-case-list-loading-perf-monitoring-trace
branch
from
e31df7a to
304ece5
Compare
| try { | ||
| val trace = FirebasePerformance.getInstance().newTrace(traceName) | ||
| trace.putAttribute(CCAnalyticsParam.CCHQ_DOMAIN, ReportingUtils.getDomain()) | ||
| trace.putAttribute(CCAnalyticsParam.USERNAME, ReportingUtils.getUser()) |
There was a problem hiding this comment.
Actually i think we should be using the same list of attributes as we send to analytics user properties here, think app id is definitely a big one that we should be including here.
avazirna
force-pushed
the
add-case-list-loading-perf-monitoring-trace
branch
from
304ece5 to
1cf88be
Compare
shubham1g5
left a comment
shubham1g5
left a comment
There was a problem hiding this comment.
Left a suggestion as nice to have but nothing blocking
| trace.putAttribute(CCPerfMonitoring.ATTR_NUM_CASES_LOADED, | ||
| String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size()))); | ||
| CCPerfMonitoring.INSTANCE.stopTracing(trace); |
There was a problem hiding this comment.
think all trace handling should be as fail safe as possible, as such think we should pass the attributes as a dictionary to the stopTracing method as well and have it call the trace.putAttribute on that dictionary in the catch all block itself.
There was a problem hiding this comment.
Yes, I made this change on a different branch (for another trace), I will push PR this separately
Product Description
This PR implements a performance custom trace for synchronous case list loading. This is part of an effort to collect performance data related key events in CommCare.
Ticket: https://dimagi.atlassian.net/browse/SAAS-18089
Safety Assurance
Safety story
This is supposed to be a lightweight, null-safe operation that has no impact on the user experience.
Labels and Review