Add perf monitoring trace for case search time #3308
Conversation
📝 WalkthroughWalkthroughAdds Firebase Performance tracing around case search and loading. EntityStringFilterer.filter() starts a CCPerfMonitoring trace (TRACE_CASE_SEARCH_TIME), stops it unless cancelled, and attaches attributes for results count and search query length via a map. CCPerfMonitoring gains new constants (TRACE_CASE_SEARCH_TIME, ATTR_RESULTS_COUNT, ATTR_SEARCH_QUERY_LENGTH) and updates stopTracing to accept optional attributes and apply them before stopping. EntityLoaderTask switches from direct trace.putAttribute calls to building an attribute map (including number of cases loaded) and passing it to CCPerfMonitoring.stopTracing. No public API classes added; only CCPerfMonitoring.stopTracing signature expanded. Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant ESF as EntityStringFilterer
participant Perf as CCPerfMonitoring
participant FB as Firebase Trace
UI->>ESF: filter(searchTerms)
ESF->>Perf: startTracing(TRACE_CASE_SEARCH_TIME)
Perf-->>ESF: Trace instance
ESF->>ESF: perform filtering (may cancel)
alt not cancelled
ESF->>Perf: stopTracing(trace, {results_count, query_length})
Perf->>FB: putAttribute(k,v) for each
Perf->>FB: stop()
else cancelled
ESF-->>UI: early return (no stopTracing)
end
ESF-->>UI: matchList
sequenceDiagram
autonumber
participant Task as EntityLoaderTask
participant Perf as CCPerfMonitoring
participant FB as Firebase Trace
Task->>Task: doInBackground()
note over Task: Load entities
Task->>Perf: stopTracing(trace, {num_cases_loaded})
Perf->>FB: putAttribute(k,v)
Perf->>FB: stop()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 1
🧹 Nitpick comments (4)
app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (2)
34-43: Fix log message; prefer Map over MutableMap; add overload/back-compat
- Log says “starting” in stopTracing; change to “stopping”.
- Accept Map<String,String> to avoid unnecessary mutability.
- Provide an overload (or @jvmoverloads default) to preserve any existing one-arg call sites.
Apply:
-fun stopTracing(trace: Trace?, attrs: MutableMap<String, String>?) { +fun stopTracing(trace: Trace?, attrs: Map<String, String>?) { try { - attrs?.forEach { - (key, value) -> trace?.putAttribute(key, value) - } + attrs?.forEach { (key, value) -> trace?.putAttribute(key, value) } trace?.stop(); } catch (exception: Exception) { - Logger.exception("Error starting perf trace: ${trace?.name}", exception) + Logger.exception("Error stopping perf trace: ${trace?.name}", exception) } }Additionally (outside this hunk), consider adding:
@JvmOverloads fun stopTracing(trace: Trace?) = stopTracing(trace, null)
19-31: Minor: remove extra semicolons (Kotlin style)Optional cleanup: drop trailing semicolons on start()/stop(). No behavior change.
app/src/org/commcare/tasks/EntityLoaderTask.java (2)
67-71: Inline singleton map to avoid temporary allocation (optional) and redundant check
- Since only one attribute is sent, you can avoid a HashMap allocation.
- The second isAsync check is redundant if trace != null guarantees non-async.
Apply:
- Map<String, String> attrs = new HashMap<>(); - attrs.put(CCPerfMonitoring.ATTR_NUM_CASES_LOADED, - String.valueOf((entities == null || entities.first == null ? 0 : entities.first.size()))); - CCPerfMonitoring.INSTANCE.stopTracing(trace, attrs); + CCPerfMonitoring.INSTANCE.stopTracing( + trace, + java.util.Collections.singletonMap( + CCPerfMonitoring.ATTR_NUM_CASES_LOADED, + String.valueOf(entities == null || entities.first == null ? 0 : entities.first.size()) + ) + );
55-79: Consider ensuring the trace is always stoppedIf an exception other than XPathException occurs before stopping, the trace will be dropped. A finally block would ensure proper closure (with 0 cases if unknown).
Would you like me to draft the revised try/catch/finally structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/src/org/commcare/adapters/EntityStringFilterer.java(4 hunks)app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt(2 hunks)app/src/org/commcare/tasks/EntityLoaderTask.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/org/commcare/tasks/EntityLoaderTask.java (1)
app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (1)
stopTracing(34-43)
app/src/org/commcare/adapters/EntityStringFilterer.java (1)
app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (2)
startTracing(19-32)stopTracing(34-43)
🔇 Additional comments (4)
app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (2)
12-17: LGTM: New trace and attribute keysNames are concise and within Firebase Perf limits.
19-31: Confirm PII policy for attributesstartTracing adds username/domain/app info to every trace. If policy changed for Firebase Perf, ensure this remains approved.
app/src/org/commcare/adapters/EntityStringFilterer.java (2)
56-58: Start trace only if meaningful? (confirm requirement)If counting “empty search” as a data point isn’t desired, gate startTracing on !isFilterEmpty. Otherwise ignore.
67-76: Cancellation behavior: confirm Firebase handling of un-stopped tracesComment assumes cancelled traces are discarded. Please confirm this matches Firebase Perf behavior across app lifecycle; otherwise consider stopping with an “cancelled=true” attribute.
| attrs.put(CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH, | ||
| String.valueOf((searchTerms == null ? 0 : | ||
| (Arrays.toString(searchTerms).length()-((searchTerms.length-1)*2)-2)))); | ||
| CCPerfMonitoring.INSTANCE.stopTracing(trace, attrs); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix query length computation for empty-array case and simplify
Current math returns 2 when searchTerms.length == 0. Sum term lengths instead; handles nulls and is clearer.
Apply:
- attrs.put(CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH,
- String.valueOf((searchTerms == null ? 0 :
- (Arrays.toString(searchTerms).length()-((searchTerms.length-1)*2)-2))));
+ int queryLen = 0;
+ if (searchTerms != null && searchTerms.length > 0) {
+ for (String term : searchTerms) {
+ if (term != null) {
+ queryLen += term.length();
+ }
+ }
+ }
+ attrs.put(CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH, String.valueOf(queryLen));📝 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.
| attrs.put(CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH, | |
| String.valueOf((searchTerms == null ? 0 : | |
| (Arrays.toString(searchTerms).length()-((searchTerms.length-1)*2)-2)))); | |
| CCPerfMonitoring.INSTANCE.stopTracing(trace, attrs); | |
| // Compute the total length of all non-null search terms (0 if none) | |
| int queryLen = 0; | |
| if (searchTerms != null && searchTerms.length > 0) { | |
| for (String term : searchTerms) { | |
| if (term != null) { | |
| queryLen += term.length(); | |
| } | |
| } | |
| } | |
| attrs.put( | |
| CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH, | |
| String.valueOf(queryLen) | |
| ); | |
| CCPerfMonitoring.INSTANCE.stopTracing(trace, attrs); |
🤖 Prompt for AI Agents
In app/src/org/commcare/adapters/EntityStringFilterer.java around lines 72-75,
the current computation for query length uses a fragile string-math expression
that yields 2 for empty arrays; replace it by computing the total length by
summing the lengths of each searchTerms element (handle searchTerms==null by
treating length as 0 and skip any null elements), then put
String.valueOf(totalLength) into the attrs map and call
CCPerfMonitoring.INSTANCE.stopTracing(trace, attrs).
| } | ||
|
|
||
| fun stopTracing(trace: Trace?) { | ||
| fun stopTracing(trace: Trace?, attrs: MutableMap<String, String>?) { |
There was a problem hiding this comment.
think trace should be non null here
| @Override | ||
| protected void filter() { | ||
| long startTime = System.currentTimeMillis(); | ||
| // Capture case_search_time trace for performance monitoring |
There was a problem hiding this comment.
nit: think the comments should not convey what but only why. If why is obvious, comment is generally useless.
| String.valueOf((matchList == null ? 0 : matchList.size()))); | ||
| attrs.put(CCPerfMonitoring.ATTR_SEARCH_QUERY_LENGTH, | ||
| String.valueOf((searchTerms == null ? 0 : | ||
| (Arrays.toString(searchTerms).length()-((searchTerms.length-1)*2)-2)))); |
There was a problem hiding this comment.
what is searchTerms.length-1)*2)-2 omitting ?
There was a problem hiding this comment.
To calculate the total length of the items inside the array considering the [a, b, c, ...] format, but I have refactored this here
avazirna
force-pushed
the
add-case-search-time-perf-monitoring-trace
branch
from
469c0dc to
761c1f9
Compare
avazirna
force-pushed
the
add-case-search-time-perf-monitoring-trace
branch
from
761c1f9 to
66b64a8
Compare
avazirna
force-pushed
the
add-case-search-time-perf-monitoring-trace
branch
from
b230331 to
cd114c5
Compare
Product Description
This PR implements a performance custom trace to measure case search times. This is part of an effort to collect performance data related key events in CommCare.
Ticket: https://dimagi.atlassian.net/browse/SAAS-18090
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