Add perf monitoring trace for form loading time

[avazirna]

Product Description

This PR implements a performance custom trace to measure form loading times. This is part of an effort to collect performance data related key events in CommCare.

Ticket: https://dimagi.atlassian.net/browse/SAAS-18091

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

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[coderabbitai]

📝 Walkthrough

Walkthrough

• Introduces two new public constants in CCPerfMonitoring: FORM_LOADING_TIME and ATTR_FORM_NAME.
• Instruments FormLoaderTask.doTaskBackground with Firebase Performance tracing for form load.
• Starts a trace named FORM_LOADING_TIME at task start, sets attribute ATTR_FORM_NAME with the form’s name after controller creation, and stops the trace.
• No changes to existing method signatures or control flow beyond instrumentation.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant FLT as FormLoaderTask
  participant CC as CCPerfMonitoring
  participant FP as FirebasePerf Trace

  User->>FLT: Trigger form load
  note over FLT: doTaskBackground()
  FLT->>CC: startTracing(FORM_LOADING_TIME)
  CC-->>FLT: Trace (FP) or null

  alt Trace available
    FLT->>FP: putAttribute(ATTR_FORM_NAME, formName)
    FLT->>CC: stopTracing(FP)
  else No trace
    note over FLT: Continue without perf trace
  end

  FLT-->>User: Form loaded (controller ready)

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• Add perf monitoring trace for case list loading time #3297 — Adds similar Firebase perf traces for case list loading using CCPerfMonitoring; parallel instrumentation pattern.
• Enable Firebase Performance Monitoring #2845 — Introduces Firebase Performance plugin/dependency needed for trace APIs used here.

Suggested reviewers

• Jignesh-dimagi
• shubham1g5

Pre-merge checks (1 passed, 2 warnings)

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The description currently only contains the template headings without any of the required content such as a user-facing product description, a link to the related ticket, the rationale and design decisions in the technical summary, feature flag information, details of the safety story, automated test coverage, QA plan, or appropriate labels, leaving the template entirely unpopulated.	Please populate each section of the template by adding a concise product description, linking to the relevant ticket, explaining the technical rationale and design decisions, specifying any feature flag usage, detailing the safety story and automated test coverage, outlining the QA plan, and applying the appropriate labels for risk, QA, and release notes.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The current title “Add perf monitoring trace for form loading time” succinctly and accurately describes the main change introduced in the pull request by highlighting the addition of performance monitoring instrumentation around form loading. It is focused, clear, and omits unnecessary details, making it easy for teammates to understand the primary purpose at a glance.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check	⚠️ Warning	The PR description includes a short Product Description, a Safety story, and a ticket reference but does not follow the repository template: the Technical Summary, Feature Flag, Automated test coverage, and QA Plan sections are empty or missing. These sections provide rationale, testing details, and risk assessment needed for reviewers. Because multiple required template sections are absent or incomplete, the description is not sufficient for review.	Please complete the template by adding a Technical Summary that links the ticket and explains the rationale and design decisions, state whether a feature flag applies, and provide automated test coverage details or justification for none. Add a QA plan with verification steps and risk mitigation, and update Labels and Review fields to indicate reviewers and risk level. These additions will allow reviewers to assess safety and testing requirements for the change.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Add perf monitoring trace for form loading time" is a concise, single-sentence summary that directly describes the primary change: adding performance tracing for form load. It accurately reflects the code modifications in the changeset and avoids extraneous detail. A reviewer scanning history will understand the PR intent from the title alone.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch add-form-loading-time-perf-monitoring-trace

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

app/src/org/commcare/tasks/FormLoaderTask.java (1)

100-154: Always stop the trace via finally; null-safe form name; keep naming consistent.

Guarantees trace closure on all paths (including exceptions), avoids dangling traces, and handles potential null form names. Also updates constant name per naming convention.

-        Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.FORM_LOADING_TIME);
-        FormDef fd = null;
+        Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.TRACE_FORM_LOADING_TIME);
+        FormDef fd = null;
+        try {
         FormDefRecord formDefRecord = FormDefRecord.getFormDef(
                 CommCareApplication.instance().getAppStorage(FormDefRecord.class),
                 formDefId[0]);
 
         File formXml = new File(formDefRecord.getFilePath());
         String formHash = FileUtil.getMd5Hash(formXml);
         File formBin = getCachedForm(formHash);
 
         if (formBin.exists()) {
             // if we have binary, deserialize binary
             Log.i(TAG, "Attempting to load " + formXml.getName() +
                     " from cached file: " + formBin.getAbsolutePath());
             fd = deserializeFormDef((Context)activity, formBin);
             if (fd == null) {
                 Logger.log(LogTypes.TYPE_RESOURCES,
                         "Deserialization of " + formXml.getName() + " form failed.");
                 // Remove the file, and make a new .formdef from xml
                 formBin.delete();
             }
         }
 
         // If we couldn't find a cached version, load the form from the XML
         if (fd == null) {
             fd = loadFormFromFile(formXml);
         }
 
         // Try to write the form definition to a cached location
         try {
             serializeFormDef(fd, formDefRecord.getFilePath());
         } catch (Exception e) {
             // The cache is a bonus, so if we can't write it, don't crash, but log
             // it so we can clean up whatever is preventing the cached version from
             // working
             Logger.log(LogTypes.TYPE_RESOURCES, "XForm could not be serialized. Error trace:\n" + ForceCloseLogger.getStackTrace(e));
         }
 
         FormEntryController fec = initFormDef(fd);
 
         // Remove previous forms
         ReferenceManager.instance().clearSession();
 
         setupFormMedia(formDefRecord.getMediaPath());
 
         AndroidFormController formController = new AndroidFormController(fec, mReadOnly, savedFormSession);
 
         data = new FECWrapper(formController);
-
-        if (trace != null) {
-            trace.putAttribute(CCPerfMonitoring.ATTR_FORM_NAME, fd.getName());
-            CCPerfMonitoring.INSTANCE.stopTracing(trace);
-        }
-        return data;
+        if (trace != null) {
+            String formName = (fd != null && fd.getName() != null) ? fd.getName() : formXml.getName();
+            trace.putAttribute(CCPerfMonitoring.ATTR_FORM_NAME, formName);
+        }
+        return data;
+        } finally {
+            CCPerfMonitoring.INSTANCE.stopTracing(trace);
+        }

🧹 Nitpick comments (1)

app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (1)

12-12: Keep trace constant naming consistent with existing TRACE_ prefix.*

Rename to align with TRACE_SYNC_ENTITY_LIST_LOADING.

-    const val FORM_LOADING_TIME = "form_loading_time"
+    const val TRACE_FORM_LOADING_TIME = "form_loading_time"

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5295a7f and c86b4c2.

📒 Files selected for processing (2)

• app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (1 hunks)
• app/src/org/commcare/tasks/FormLoaderTask.java (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/src/org/commcare/tasks/FormLoaderTask.java (2)

app/src/org/commcare/CommCareApplication.java (1)

• CommCareApplication (157-1279)

app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (2)

• startTracing (18-31)
• stopTracing (33-39)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Lint Code Base

🔇 Additional comments (4)

app/src/org/commcare/google/services/analytics/CCPerfMonitoring.kt (1)

16-16: Ensure no more than 5 custom attributes per Firebase Trace
You’re now at the 5-attribute cap (startTracing() sets four, plus ATTR_FORM_NAME). Verify no other code adds attributes to this trace.

app/src/org/commcare/tasks/FormLoaderTask.java (3)

6-7: LGTM: Needed import for Trace.

---

15-15: LGTM: CCPerfMonitoring import.

---

150-153: Edge-of-limit on attributes.

This adds the 5th attribute to the trace. Please ensure no other code adds more attributes to this same trace name.

[shubham1g5]

• can we put this in a try-catch all exception block, as this should not affect functionality in case of bugs like NPEs

[avazirna]

@shubham1g5 Can FormDef be null at this point? There is a try-catch block inside stopTracing()

[shubham1g5]

I don't think it can be but just suggested it from more of a best practice perspective, happy for the PR to be merged without that as well

[avazirna]

My question was more in the sense that we probably want to "cancel" the tracing if the FormDef is null, because otherwise it will mean uploading incomplete data. I've opted for Null checking FormDef

[shubham1g5]

@avazirna I probably didn't clearify my point completely but the best practice I was considering here is to wrap the non-essential code in a catch all exception block (like we do inReportingUtils ) instead of null checks or a particular edge case speifically. Think the null checks are worse here as they give an impression that they can be null which they can't be.

[avazirna]

@shubham1g5 yeah, I get you, and it makes sense. I'm a bit conservative when it comes to try-catches like that, but let me add that.

[shubham1g5]

think we would still want to log the exception atleast

The base branch was changed.