Skip to content

add caching and logging for kpis#936

Open
calellowitz wants to merge 1 commit intomainfrom
ce/kpi-cache-and-log
Open

add caching and logging for kpis#936
calellowitz wants to merge 1 commit intomainfrom
ce/kpi-cache-and-log

Conversation

@calellowitz
Copy link
Copy Markdown
Collaborator

@calellowitz calellowitz commented Jan 19, 2026

Technical Summary

Adds simple caching and logging to the kpi report page. The logging is designed to inform future optimizations, and specifically to understand how often other filters are used.

Safety Assurance

Safety story

No change to the calculations just caching, and internal use only.

Labels & Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 19, 2026

Walkthrough

The changes introduce caching and logging infrastructure to the helpers.py module. A module-level logger is created using Python's logging module. The get_table_data_for_year_month function is decorated with a @quickcache decorator that caches results based on specified vary_on keys with an extended timeout. Additionally, a logging statement is added within the function to record KPI filter parameters. Total additions: 12 lines across one file.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: adding caching and logging for KPIs, which matches the core modifications to get_table_data_for_year_month.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose of the caching and logging additions and their intended use for optimization insights.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ce/kpi-cache-and-log

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

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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)
commcare_connect/reports/helpers.py (1)

75-93: Cache key does not reflect dynamically computed defaults, causing stale data.

The vary_on keys use the raw parameter values (from_date=None, to_date=None), but the function body applies now().date() as the actual value (lines 92-93). This means:

  1. A call with from_date=None on January 1st caches with key (..., None, ...).
  2. A call with from_date=None on January 2nd hits the same cache entry and returns stale January 1st data.

Normalize defaults before caching, or pass the cache key computation through a skip condition.

Suggested fix: normalize defaults before caching

Extract the default normalization into a wrapper or compute the actual values first:

+def get_table_data_for_year_month(
+    from_date=None,
+    to_date=None,
+    delivery_type=None,
+    program=None,
+    network_manager=None,
+    opportunity=None,
+    country_currency=None,
+):
+    today = now().date()
+    from_date = from_date or today
+    to_date = to_date if to_date and to_date <= today else today
+    return _get_table_data_for_year_month_cached(
+        from_date, to_date, delivery_type, program, network_manager, opportunity, country_currency
+    )
+
+
 `@quickcache`(
     vary_on=["from_date", "to_date", "delivery_type", "program", "network_manager", "opportunity", "country_currency"],
     timeout=60 * 60 * 24,
 )
-def get_table_data_for_year_month(
+def _get_table_data_for_year_month_cached(
     from_date=None,
     to_date=None,
     ...
 ):
-    from_date = from_date or now().date()
-    to_date = to_date if to_date and to_date <= now().date() else now().date()
+    # from_date and to_date are already normalized by the wrapper
🧹 Nitpick comments (1)
commcare_connect/reports/helpers.py (1)

88-91: Logging inside cached function only captures cache misses.

The PR states the intent is to "understand how often other filters are used." Since logger.info is inside the cached function, it only logs when the cache is bypassed. Cache hits (potentially the majority of requests) are not logged.

If you need full usage analytics, move logging to the calling view or add a non-cached wrapper. If logging cache misses only is intentional (e.g., to track cache effectiveness), this is fine—consider adding a brief comment to clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants