Adapt CaseSearchProfiler for more general use #35743
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: corehq/apps/case_search/utils.py
Did you find this useful? React with a 👍 or 👎 |
|
@CodeRabbit Please review |
🧩 Analysis chain🏁 Script executed: Length of output: 5977 @Charl1996 Thank you for the PR! I've reviewed the changes and here's my feedback: 👍 Positives:
🤔 Suggestions:
def _get_search_class(self):
if not self.search_class:
raise NotImplementedError(
"You must define a search_class attribute that inherits from HQESQuery"
)
return self.search_class
from typing import Type, Optional
# ...
def get_search_class(self, slug: Optional[str] = None) -> Type[HQESQuery]:❗ Required Changes:
Let me know if you'd like me to elaborate on any of these points! ✏️ Learnings added
✅ Actions performedReview triggered.
|
WalkthroughThe changes refactor the search profiling mechanism used across case search and report generation. The original profiler class has been renamed from Sequence Diagram(s)sequenceDiagram
participant Caller as QueryHelper/RegistryQueryHelper
participant ESProfiler as ESQueryProfiler
participant Dynamic as ProfiledSearchClass
Caller->>ESProfiler: get_search_class(slug)
ESProfiler->>ESProfiler: __post_init__ (initialize TimingContext)
ESProfiler->>Dynamic: Define dynamic inner class based on search_class
ESProfiler-->>Caller: Return ProfiledSearchClass
sequenceDiagram
participant Report as CaseListReport/CaseListMixin
participant Mixin as ESQueryProfilerMixin
participant ESProfiler as ESQueryProfiler
Report->>Mixin: Initialize report instance
Mixin->>ESProfiler: Setup profiler (if enabled)
ESProfiler-->>Mixin: Return profiler instance with search_class
Mixin-->>Report: Integrate profiler context for report generation
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
corehq/apps/reports/standard/__init__.py (2)
311-328: Improve attribute assignment and initialization.The mixin is well-designed but has a minor issue with the use of
setattr.Replace
setattrwith direct assignment for better readability:- setattr(self, 'search_class', self.profiler.get_search_class(slug=self.__class__.__name__)) + self.search_class = self.profiler.get_search_class(slug=self.__class__.__name__)🧰 Tools
🪛 Ruff (0.8.2)
327-327: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
333-336: Enhance error message for missing search_class.The error message could be more descriptive to help developers understand how to fix the issue.
- raise NotImplementedError("You must define a search_class attribute.") + raise NotImplementedError( + "You must define a search_class attribute in your class. " + "This should be a subclass of HQESQuery that defines the search functionality." + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
corehq/apps/case_search/utils.py(4 hunks)corehq/apps/reports/standard/__init__.py(2 hunks)corehq/apps/reports/standard/cases/basic.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: - Review the code following best practises and st...
**/*.py: - Review the code following best practises and standards
corehq/apps/reports/standard/__init__.pycorehq/apps/reports/standard/cases/basic.pycorehq/apps/case_search/utils.py
📓 Learnings (3)
📓 Common learnings
Learnt from: Charl1996
PR: dimagi/commcare-hq#0
File: :0-0
Timestamp: 2025-02-06T15:30:24.017Z
Learning: The ESQueryProfilerMixin is used to profile Elasticsearch queries in CommCare HQ. It requires a search_class attribute to be defined and can be enabled/disabled via the profiler_enabled flag.
corehq/apps/reports/standard/__init__.py (1)
Learnt from: Charl1996
PR: dimagi/commcare-hq#0
File: :0-0
Timestamp: 2025-02-06T15:30:24.017Z
Learning: The ESQueryProfilerMixin is used to profile Elasticsearch queries in CommCare HQ. It requires a search_class attribute to be defined and can be enabled/disabled via the profiler_enabled flag.
corehq/apps/case_search/utils.py (1)
Learnt from: Charl1996
PR: dimagi/commcare-hq#0
File: :0-0
Timestamp: 2025-02-06T15:30:24.017Z
Learning: The ESQueryProfilerMixin is used to profile Elasticsearch queries in CommCare HQ. It requires a search_class attribute to be defined and can be enabled/disabled via the profiler_enabled flag.
🪛 Ruff (0.8.2)
corehq/apps/reports/standard/__init__.py
327-327: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests (python-sharded-and-javascript)
🔇 Additional comments (4)
corehq/apps/case_search/utils.py (3)
57-67: LGTM! Well-structured base profiler class.The refactoring of
CaseSearchProfilertoESQueryProfileris clean and follows Python best practices:
- Good use of
@dataclassto reduce boilerplate- Proper initialization of
timing_contextin__post_init__- Sensible defaults for fields
68-92: LGTM! Well-designed profiling mechanism.The
get_search_classmethod effectively wraps the search class with profiling capabilities:
- Proper timing context management
- Debug mode support with query profiling
- Clean implementation of the inner
ProfiledSearchClass
95-100: LGTM! Well-designed specialized profiler.The new
CaseSearchProfilerclass:
- Properly inherits from
ESQueryProfiler- Correctly specializes for case search with
CaseSearchES- Adds relevant counters for case search profiling
corehq/apps/reports/standard/cases/basic.py (1)
32-44: LGTM! Clean integration of profiling capabilities.The addition of
ESQueryProfilerMixinis well-integrated:
- Proper order in inheritance chain
- Required
search_classattribute is already defined- Maintains existing functionality
This PR is laying the groundwork for profiling the Case List Explorer.
Technical Summary
Ticket
Tech spec
This PR pulls out some functionality from the existing CaseSearchProfiler such that it can be used more generally where ES queries are executed with classes inheriting from
HQESQuery.A new mixin,
ESQueryProfilerMixin, now makes use of this new profiler class. Any class making use of this mixin have access to a profiler which can be used to profile various class methods arbitrarily deep.How to use mixin
Using the mixin and profiler is really easy. Say I have the following class definition:
If I now want to see how long
some_complex_methodtakes to execute I can simply add the mixin, which gives me access to the profiler which I can useNote that
some_complex_methoddoes not have to necessarily execute an ES query; you can use the profiler to profile basically any method duration with thetiming_contextattribute.How do I access the results?
The results can be accessed via the
profiler.timing_contextattribute. See the tech spec for more information.Safety Assurance
Tested locally - more tests needed before opening for review.
Safety story
Automated test coverage
Tests to be added.
QA Plan
No QA planned for this PR - QA will be done as part of
Rollback instructions
Labels & Review