Measure the performance in launchable record tests command
#966
Conversation
WalkthroughThe changes focus on performance tracking enhancements in the Launchable testing infrastructure. A new performance tracking mechanism has been introduced by adding high-resolution time measurement capabilities to the record tests command. This involves importing Changes
Poem
✨ 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
CodeRabbit Configuration File (
|
This comment has been minimized.
This comment has been minimized.
![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 (3)
launchable/utils/tracking.py (1)
16-16: LGTM! Consider adding docstring.The addition of the
PERFORMANCEevent type follows the existing pattern. Consider adding a docstring to document its purpose and expected metadata structure.class Event(Enum): SHALLOW_CLONE = 'SHALLOW_CLONE' # this event is an example - PERFORMANCE = 'PERFORMANCE' + PERFORMANCE = 'PERFORMANCE' # tracks execution time of operationslaunchable/commands/record/tests.py (2)
539-548: Consider adding error handling for performance tracking.The performance tracking implementation looks good, but consider wrapping it in a try-except block to ensure any timing issues don't affect the main functionality.
+ try: start = time_ns() tc = testcases(self.reports) end = time_ns() tracking_client.send_event( event_name=Tracking.Event.PERFORMANCE, metadata={ "elapsedTime": end - start, "measurementTarget": "testcases method(parsing report file)" } ) + except Exception as e: + logger.warning(f"Failed to track performance: {e}")
Line range hint
556-576: Consider consolidating performance tracking logic.The performance tracking code is duplicated. Consider extracting it into a helper method for reusability and consistency.
+ def track_performance(tracking_client, operation_name): + def decorator(func): + def wrapper(*args, **kwargs): + try: + start = time_ns() + result = func(*args, **kwargs) + end = time_ns() + tracking_client.send_event( + event_name=Tracking.Event.PERFORMANCE, + metadata={ + "elapsedTime": end - start, + "measurementTarget": operation_name + } + ) + return result + except Exception as e: + logger.warning(f"Failed to track performance: {e}") + return func(*args, **kwargs) + return wrapper + return decoratorThen use it like this:
+ @track_performance(tracking_client, "testcases method(parsing report file)") def testcases(reports: List[str]) -> Generator[CaseEventType, None, None]: # ... existing code ... + @track_performance(tracking_client, "events API") def send(payload: Dict[str, Union[str, List]]) -> None: # ... existing code ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
launchable/commands/record/tests.py(3 hunks)launchable/utils/tracking.py(1 hunks)
🔇 Additional comments (1)
launchable/commands/record/tests.py (1)
7-7: LGTM! Good choice of timing function.Using
time_ns()is appropriate for high-resolution timing measurements.
ono-max
force-pushed
the
measure-performance
branch
from
9e62b23 to
a1a2cd1
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
launchable/commands/helper.py (1)
141-144: Consider using decimal.Decimal for more precise nanosecond conversion.The current implementation might lose precision due to floating-point arithmetic when multiplying by 1e9.
Consider this alternative implementation for better precision:
+from decimal import Decimal + -def time_ns(): +def time_ns() -> int: # time.time_ns() method is new in Python version 3.7 # As a workaround, we convert time.time() to nanoseconds. - return int(time() * 1e9) + return int(Decimal.from_float(time()) * Decimal('1e9'))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
launchable/commands/helper.py(2 hunks)launchable/commands/record/tests.py(3 hunks)launchable/utils/tracking.py(1 hunks)tests/commands/test_api_error.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/commands/test_api_error.py
- launchable/utils/tracking.py
- launchable/commands/record/tests.py
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (windows-latest, 3.12)
- GitHub Check: build (windows-latest, 3.11)
- GitHub Check: build (windows-latest, 3.10)
- GitHub Check: build (windows-latest, 3.9)
- GitHub Check: build (windows-latest, 3.8)
- GitHub Check: build (windows-latest, 3.7)
- GitHub Check: build (windows-latest, 3.6)
- GitHub Check: build (ubuntu-20.04, 3.12)
- GitHub Check: build (ubuntu-20.04, 3.11)
- GitHub Check: build (ubuntu-20.04, 3.10)
- GitHub Check: build (ubuntu-20.04, 3.9)
- GitHub Check: build (ubuntu-20.04, 3.8)
- GitHub Check: build (ubuntu-20.04, 3.7)
- GitHub Check: build (ubuntu-20.04, 3.6)
🔇 Additional comments (1)
launchable/commands/helper.py (1)
141-144: Verify Python version compatibility requirements.Since this is a workaround for Python < 3.7, we should verify if this compatibility is still needed.
✅ Verification successful
The time.time_ns() workaround is still necessary
The codebase actively maintains Python 3.6 compatibility as evidenced by version-specific dependencies in setup.cfg. Since time.time_ns() was introduced in Python 3.7, this workaround should be kept to ensure compatibility with Python 3.6.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Python version requirements in setup.py or similar files echo "Checking Python version requirements..." fd -e py -e txt -e ini -e toml -e cfg -H "setup|requirements|pyproject" | xargs rg -i "python.*version|requires.*python" # Check if time.time_ns is used elsewhere in the codebase echo "Checking time.time_ns usage..." rg "time\.time_ns\(\)"Length of output: 601
| def time_ns(): | ||
| # time.time_ns() method is new in Python version 3.7 | ||
| # As a workaround, we convert time.time() to nanoseconds. | ||
| return int(time() * 1e9) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and documentation for the time_ns() function.
The function should follow the codebase's conventions for type hints and documentation.
Apply this diff to improve the function:
-def time_ns():
+def time_ns() -> int:
+ """Return the current time in nanoseconds.
+
+ This is a compatibility function for Python versions < 3.7 that don't have time.time_ns().
+ Note: The precision might be lower than actual time.time_ns() due to floating-point arithmetic.
+
+ Returns:
+ int: Current time in nanoseconds
+ """
# time.time_ns() method is new in Python version 3.7
# As a workaround, we convert time.time() to nanoseconds.
return int(time() * 1e9)📝 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.
| def time_ns(): | |
| # time.time_ns() method is new in Python version 3.7 | |
| # As a workaround, we convert time.time() to nanoseconds. | |
| return int(time() * 1e9) | |
| def time_ns() -> int: | |
| """Return the current time in nanoseconds. | |
| This is a compatibility function for Python versions < 3.7 that don't have time.time_ns(). | |
| Note: The precision might be lower than actual time.time_ns() due to floating-point arithmetic. | |
| Returns: | |
| int: Current time in nanoseconds | |
| """ | |
| # time.time_ns() method is new in Python version 3.7 | |
| # As a workaround, we convert time.time() to nanoseconds. | |
| return int(time() * 1e9) |
Here is the example of metadata after executing
launchable record tests command.Summary by CodeRabbit
New Features
PERFORMANCEevent type for tracking performance-related events.Tests
Chores