-
Notifications
You must be signed in to change notification settings - Fork 348
New timing logger, for use with the timing viz tool #870
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| import logging | ||
| import time | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| import kiln_ai.utils.timing_logger as timing_logger | ||
| from kiln_ai.utils.timing_logger import TimingLogger, time_operation | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def reset_show_timing(): | ||
| old_show_timing = timing_logger._show_timing | ||
| yield | ||
| timing_logger._show_timing = old_show_timing | ||
|
|
||
|
|
||
| class TestTimingLogger: | ||
| """Test cases for TimingLogger class.""" | ||
|
|
||
| def test_timing_logger_basic_usage(self, caplog): | ||
| """Test basic timing logger functionality.""" | ||
| timing_logger._show_timing = True | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "test_operation"): | ||
| time.sleep(0.1) | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert "timing_logger [test][test_operation]" in caplog.records[0].message | ||
| assert caplog.records[0].levelno == logging.WARNING | ||
|
|
||
| def test_timing_logger_logs_at_warning_level(self, caplog): | ||
| """Test that timing logger uses WARNING level when enabled.""" | ||
| timing_logger._show_timing = True | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "level_test"): | ||
| time.sleep(0.05) | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert caplog.records[0].levelname == "WARNING" | ||
| assert caplog.records[0].levelno == logging.WARNING | ||
|
|
||
| def test_timing_logger_not_logged_when_disabled(self, caplog): | ||
| """Test that timing messages don't appear when timing is disabled.""" | ||
| timing_logger._show_timing = False | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "filtered_test"): | ||
| time.sleep(0.05) | ||
|
|
||
| assert len(caplog.records) == 0 | ||
|
|
||
| @patch("kiln_ai.utils.timing_logger.time.time") | ||
| def test_timing_calculation(self, mock_time, caplog): | ||
| """Test that timing calculation is accurate.""" | ||
| timing_logger._show_timing = True | ||
| # Mock time.time() to return predictable values | ||
| # Need 3 values: __enter__, __exit__, and logging internal call | ||
| mock_time.side_effect = [1000.0, 1002.5, 1003.0] # 2.5 second difference | ||
|
|
||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "precise_test"): | ||
| pass | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert "timing_logger [test][precise_test][2.50s]" in caplog.records[0].message | ||
|
|
||
| def test_timestamp_format(self): | ||
| """Test that timestamp format is ISO format.""" | ||
| timing_logger = TimingLogger("test", "timestamp_test") | ||
| timestamp = timing_logger.timestamp() | ||
|
|
||
| # Should be in ISO format (contains T and colons) | ||
| assert "T" in timestamp | ||
| assert ":" in timestamp | ||
|
|
||
|
|
||
| class TestTimeOperationContextManager: | ||
| """Test cases for time_operation context manager function.""" | ||
|
|
||
| def test_time_operation_basic(self, caplog): | ||
| """Test basic time_operation context manager.""" | ||
| timing_logger._show_timing = True | ||
| with caplog.at_level(logging.WARNING): | ||
| with time_operation("test", "context_test"): | ||
| time.sleep(0.1) | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert "timing_logger [test][context_test]" in caplog.records[0].message | ||
|
|
||
| def test_time_operation_exception_handling(self, caplog): | ||
| """Test that timing is still logged even if an exception occurs.""" | ||
| timing_logger._show_timing = True | ||
| with caplog.at_level(logging.WARNING): | ||
| with pytest.raises(ValueError): | ||
| with time_operation("test", "exception_test"): | ||
| raise ValueError("Test exception") | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert "timing_logger [test][exception_test]" in caplog.records[0].message | ||
|
|
||
|
|
||
| class TestShowTimingControl: | ||
| """Test cases for _show_timing flag control of timing logging.""" | ||
|
|
||
| def test_timing_disabled_by_default(self, caplog): | ||
| """Test that timing is disabled when _show_timing is False.""" | ||
| timing_logger._show_timing = False | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "default_test"): | ||
| time.sleep(0.05) | ||
|
|
||
| assert len(caplog.records) == 0 | ||
|
|
||
| def test_timing_disabled_when_false(self, caplog): | ||
| """Test that timing is disabled when _show_timing is False.""" | ||
| timing_logger._show_timing = False | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "false_test"): | ||
| time.sleep(0.05) | ||
|
|
||
| assert len(caplog.records) == 0 | ||
|
|
||
|
Comment on lines
+105
to
+122
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove duplicate test and fix misleading test name. The tests Apply this diff to remove the duplicate and clarify the remaining test: - def test_timing_disabled_by_default(self, caplog):
- """Test that timing is disabled when _show_timing is False."""
- timing_logger._show_timing = False
- with caplog.at_level(logging.WARNING):
- with TimingLogger("test", "default_test"):
- time.sleep(0.05)
-
- assert len(caplog.records) == 0
-
def test_timing_disabled_when_false(self, caplog):
"""Test that timing is disabled when _show_timing is False."""
timing_logger._show_timing = False🤖 Prompt for AI Agents |
||
| def test_timing_enabled_when_true(self, caplog): | ||
| """Test that timing is enabled when _show_timing is True.""" | ||
| timing_logger._show_timing = True | ||
| with caplog.at_level(logging.WARNING): | ||
| with TimingLogger("test", "true_test"): | ||
| time.sleep(0.05) | ||
|
|
||
| assert len(caplog.records) == 1 | ||
| assert "timing_logger [test][true_test]" in caplog.records[0].message | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,70 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||||||||||||||
| from contextlib import contextmanager | ||||||||||||||||||||||||||||||||||||||||||||||
| from datetime import datetime | ||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Generator, Optional | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| _show_timing = os.getenv("KILN_SHOW_TIMING", "false") == "true" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| class TimingLogger: | ||||||||||||||||||||||||||||||||||||||||||||||
| """A utility for timing code execution with context manager support.""" | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, operation: str, name: str): | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| Initialize the timing logger. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||
| name: Name/description of the operation being timed | ||||||||||||||||||||||||||||||||||||||||||||||
| print_start: Whether to print when the operation starts | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| self.operation = operation | ||||||||||||||||||||||||||||||||||||||||||||||
| self.name = name | ||||||||||||||||||||||||||||||||||||||||||||||
| self.start_time: Optional[float] = None | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+16
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove obsolete parameter from docstring. The docstring references a Apply this diff to fix the docstring: def __init__(self, operation: str, name: str):
"""
Initialize the timing logger.
Args:
- name: Name/description of the operation being timed
- print_start: Whether to print when the operation starts
+ operation: Type/category of the operation being timed
+ name: Name/description of the operation being timed
"""📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def timestamp(self) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||
| """Generate a timestamp string in ISO format.""" | ||||||||||||||||||||||||||||||||||||||||||||||
| return datetime.now().isoformat() | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def __enter__(self): | ||||||||||||||||||||||||||||||||||||||||||||||
| """Enter the context manager and start timing.""" | ||||||||||||||||||||||||||||||||||||||||||||||
| self.start_time = time.time() | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| return self | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| def __exit__(self, exc_type, exc_val, exc_tb): | ||||||||||||||||||||||||||||||||||||||||||||||
| """Exit the context manager and print timing results.""" | ||||||||||||||||||||||||||||||||||||||||||||||
| if self.start_time is not None: | ||||||||||||||||||||||||||||||||||||||||||||||
| if not _show_timing: | ||||||||||||||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| duration = time.time() - self.start_time | ||||||||||||||||||||||||||||||||||||||||||||||
| printable_operation = self.operation.replace("[", "_").replace("]", "_") | ||||||||||||||||||||||||||||||||||||||||||||||
| printable_name = self.name.replace("[", "_").replace("]", "_") | ||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||||||||||||||
| f"{self.timestamp()} timing_logger [{printable_operation}][{printable_name}][{duration:.2f}s]" | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| @contextmanager | ||||||||||||||||||||||||||||||||||||||||||||||
| def time_operation( | ||||||||||||||||||||||||||||||||||||||||||||||
| operation: str, name: str = "unknown" | ||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Generator[None, None, None]: | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| Context manager for timing operations. | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||
| type: Type/category of the operation being timed | ||||||||||||||||||||||||||||||||||||||||||||||
| name: Name/description of the operation being timed | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| Example: | ||||||||||||||||||||||||||||||||||||||||||||||
| with time_operation("api", "my_task"): | ||||||||||||||||||||||||||||||||||||||||||||||
| # Your code here | ||||||||||||||||||||||||||||||||||||||||||||||
| time.sleep(1) | ||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||
| timing_logger = TimingLogger(operation, name) | ||||||||||||||||||||||||||||||||||||||||||||||
| with timing_logger: | ||||||||||||||||||||||||||||||||||||||||||||||
| yield | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+52
to
+70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix parameter name in docstring. The docstring references a parameter named Apply this diff to fix the docstring: """
Context manager for timing operations.
Args:
- type: Type/category of the operation being timed
+ operation: Type/category of the operation being timed
name: Name/description of the operation being timed🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Environment variable set after module import time initialization
The
KILN_SHOW_TIMINGenvironment variable is set on line 18, but thetiming_loggermodule is already imported transitively throughmake_appon line 8. Since_show_timingintiming_logger.pyis evaluated at module load time usingos.getenv(), it will always beFalsewhen running the dev server. The environment variable needs to be set before the import chain that loadstiming_logger.Additional Locations (1)
libs/core/kiln_ai/utils/timing_logger.py#L9-L10