Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt CaseSearchProfiler for more general use #35743

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Charl1996
Copy link
Contributor

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:

class MyClass:
    search_class = ... # any ES class inheriting from HQESQuery 
    def some_complex_method(self):
        # some complex logic
        ...

If I now want to see how long some_complex_method takes to execute I can simply add the mixin, which gives me access to the profiler which I can use

class MyClass(ESQueryProfilerMixin):
    search_class = ... # any ES class inheriting from HQESQuery 
    def some_complex_method(self):
        # some complex logic
        with self.profiler.timing_context('Tracking some complex method'):
            ...

Note that some_complex_method does not have to necessarily execute an ES query; you can use the profiler to profile basically any method duration with the timing_context attribute.

How do I access the results?

The results can be accessed via the profiler.timing_context attribute. 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

  • This PR can be reverted after deploy with no further considerations

Labels & Review

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

@Charl1996 Charl1996 requested a review from Jtang-1 February 6, 2025 15:28
Copy link

sentry-io bot commented Feb 6, 2025

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: corehq/apps/case_search/utils.py

Function Unhandled Issue
run ESError: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='10.202.40.159', port=9200... ...
Event Count: 3
run ESError: ConnectionTimeout caused by - ReadTimeoutError(HTTPConnectionPool(host='10.202.40.159', port=9200... ...
Event Count: 2

Did you find this useful? React with a 👍 or 👎

@Charl1996
Copy link
Contributor Author

@CodeRabbit Please review

This comment was marked as resolved.

This comment was marked as resolved.

Copy link

@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

🧹 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 setattr with 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 setattr with a constant attribute value. It is not any safer than normal property access.

Replace setattr with 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9fe973a and a52fa68.

📒 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__.py
  • corehq/apps/reports/standard/cases/basic.py
  • corehq/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 CaseSearchProfiler to ESQueryProfiler is clean and follows Python best practices:

  • Good use of @dataclass to reduce boilerplate
  • Proper initialization of timing_context in __post_init__
  • Sensible defaults for fields

68-92: LGTM! Well-designed profiling mechanism.

The get_search_class method 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 CaseSearchProfiler class:

  • 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 ESQueryProfilerMixin is well-integrated:

  • Proper order in inheritance chain
  • Required search_class attribute is already defined
  • Maintains existing functionality

@Charl1996 Charl1996 added the product/invisible Change has no end-user visible impact label Feb 13, 2025
@Charl1996 Charl1996 requested a review from Jtang-1 as a code owner February 13, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant