-
Notifications
You must be signed in to change notification settings - Fork 56
LCORE-776: Allow started_at and completed_at timestamps to be store in database #618
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
Conversation
WalkthroughAdds started_at and completed_at timestamps to conversation data: propagated from endpoints through utils to cache models and storage (Postgres/SQLite), exposed in conversations_v2 transformed messages, and covered by updated unit tests. SQL schemas and statements were extended; function signatures updated where necessary. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as API Endpoint
participant LLM as LLM Provider
participant Cache as Cache Store
User->>API: Send query
API->>API: started_at = now()
API->>LLM: Request completion
LLM-->>API: Response
API->>API: completed_at = now()
API->>Cache: store(query, response, started_at, completed_at, metadata)
Cache-->>API: ack
API-->>User: Return response + ids
note over API,Cache: Timestamps persisted with conversation
sequenceDiagram
autonumber
actor User
participant API as Streaming Endpoint
participant LLM as Streaming Provider
participant Cache as Cache Store
User->>API: Start streaming query
API->>API: started_at = now()
API->>LLM: Initiate stream
LLM-->>API: Stream chunks
API-->>User: Relay chunks (loop)
API->>API: completed_at = now() (on stream end)
API->>Cache: store(..., started_at, completed_at)
Cache-->>API: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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.
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 (2)
src/models/cache_entry.py (1)
6-21: Update the docstring to document the new timestamp fields.The
Attributessection in the docstring doesn't includestarted_atandcompleted_at, which are now part of the model.Apply this diff to complete the documentation:
class CacheEntry(BaseModel): """Model representing a cache entry. Attributes: query: The query string response: The response string provider: Provider identification model: Model identification + started_at: ISO 8601 timestamp when processing started (UTC with Z suffix) + completed_at: ISO 8601 timestamp when processing completed (UTC with Z suffix) """ query: str response: str provider: str model: str started_at: str completed_at: strBased on coding guidelines.
src/utils/endpoints.py (1)
184-217: Update the docstring to document new parameters.The function signature was extended to include
started_atandcompleted_at, but the docstring lacks anArgssection describing these new parameters.As per coding guidelines, apply this diff to add complete parameter documentation:
def store_conversation_into_cache( config: AppConfig, user_id: str, conversation_id: str, provider_id: str, model_id: str, query: str, response: str, started_at: str, completed_at: str, _skip_userid_check: bool, topic_summary: str | None, ) -> None: - """Store one part of conversation into conversation history cache.""" + """Store one part of conversation into conversation history cache. + + Args: + config: Application configuration containing cache settings. + user_id: User identification. + conversation_id: Conversation ID unique for the given user. + provider_id: Model provider identifier. + model_id: Model identifier. + query: User query text. + response: Model response text. + started_at: ISO 8601 timestamp when the query processing started. + completed_at: ISO 8601 timestamp when the query processing completed. + _skip_userid_check: Skip user_id validation check. + topic_summary: Optional topic summary for the conversation. + """
🧹 Nitpick comments (2)
src/app/endpoints/conversations_v2.py (1)
241-252: Update docstring to document the new fields in the return value.The function now returns
started_atandcompleted_atfields, but the docstring doesn't document the complete structure of the returned dictionary.Apply this diff to enhance the docstring:
def transform_chat_message(entry: CacheEntry) -> dict[str, Any]: - """Transform the message read from cache into format used by response payload.""" + """Transform the message read from cache into format used by response payload. + + Args: + entry: The cache entry containing query, response, and metadata. + + Returns: + A dictionary containing provider, model, messages list, started_at, and completed_at. + """ return {Based on coding guidelines.
src/cache/sqlite_cache.py (1)
22-40: Optional: Fix stale documentation.The class docstring mentions
"cache_key_key" UNIQUE CONSTRAINT, btree (key)which does not exist in the actual schema (there is nokeycolumn). This is a pre-existing documentation issue unrelated to the current changes.Consider removing these lines from the docstring:
Indexes: "cache_pkey" PRIMARY KEY, btree (user_id, conversation_id, created_at) - "cache_key_key" UNIQUE CONSTRAINT, btree (key) "timestamps" btree (updated_at) - Access method: heap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/endpoints/conversations_v2.py(1 hunks)src/app/endpoints/query.py(3 hunks)src/app/endpoints/streaming_query.py(4 hunks)src/cache/postgres_cache.py(5 hunks)src/cache/sqlite_cache.py(5 hunks)src/models/cache_entry.py(1 hunks)src/utils/endpoints.py(2 hunks)tests/unit/app/endpoints/test_conversations_v2.py(2 hunks)tests/unit/cache/test_noop_cache.py(1 hunks)tests/unit/cache/test_postgres_cache.py(1 hunks)tests/unit/cache/test_sqlite_cache.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/cache/test_postgres_cache.pysrc/app/endpoints/conversations_v2.pysrc/cache/sqlite_cache.pysrc/app/endpoints/query.pysrc/models/cache_entry.pysrc/app/endpoints/streaming_query.pytests/unit/app/endpoints/test_conversations_v2.pytests/unit/cache/test_sqlite_cache.pytests/unit/cache/test_noop_cache.pysrc/utils/endpoints.pysrc/cache/postgres_cache.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/cache/test_postgres_cache.pytests/unit/app/endpoints/test_conversations_v2.pytests/unit/cache/test_sqlite_cache.pytests/unit/cache/test_noop_cache.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/cache/test_postgres_cache.pytests/unit/app/endpoints/test_conversations_v2.pytests/unit/cache/test_sqlite_cache.pytests/unit/cache/test_noop_cache.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/app/endpoints/conversations_v2.pysrc/cache/sqlite_cache.pysrc/app/endpoints/query.pysrc/models/cache_entry.pysrc/app/endpoints/streaming_query.pysrc/utils/endpoints.pysrc/cache/postgres_cache.py
src/app/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use standard FastAPI imports (from fastapi import APIRouter, HTTPException, Request, status, Depends) in FastAPI app code
Files:
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query.py
src/{app/**/*.py,client.py}
📄 CodeRabbit inference engine (CLAUDE.md)
Use async def for I/O-bound operations and external API calls
Files:
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query.py
src/app/endpoints/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
In API endpoints, raise FastAPI HTTPException with appropriate status codes for error handling
Files:
src/app/endpoints/conversations_v2.pysrc/app/endpoints/query.pysrc/app/endpoints/streaming_query.py
src/{models/**/*.py,configuration.py}
📄 CodeRabbit inference engine (CLAUDE.md)
src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)
Files:
src/models/cache_entry.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Pydantic models: use BaseModel for data models and extend ConfigurationBase for configuration
Use @model_validator and @field_validator for Pydantic model validation
Files:
src/models/cache_entry.py
🧬 Code graph analysis (3)
tests/unit/cache/test_postgres_cache.py (1)
src/models/cache_entry.py (1)
CacheEntry(6-21)
tests/unit/cache/test_sqlite_cache.py (1)
src/models/cache_entry.py (1)
CacheEntry(6-21)
tests/unit/cache/test_noop_cache.py (1)
src/models/cache_entry.py (1)
CacheEntry(6-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (14)
tests/unit/cache/test_postgres_cache.py (1)
18-33: LGTM!The test fixtures correctly include the new
started_atandcompleted_atfields, aligning with the extendedCacheEntrymodel. The ISO 8601 timestamp format with UTC 'Z' suffix is appropriate for storage.src/app/endpoints/query.py (1)
244-244: LGTM!The timestamps are captured at appropriate points in the request lifecycle:
started_atimmediately after authenticationcompleted_atafter LLM processing completesThe ISO 8601 format with second precision is suitable for this use case.
Also applies to: 334-344
tests/unit/app/endpoints/test_conversations_v2.py (1)
12-34: LGTM!The test correctly validates that the
transform_chat_messagefunction includes the new timestamp fields in its output. The assertions verify both presence and values ofstarted_atandcompleted_at.src/app/endpoints/streaming_query.py (1)
600-600: LGTM!The timestamps are captured at appropriate points in the streaming lifecycle:
started_atcaptured at the beginning of the handlercompleted_atcaptured after streaming completes but before cache storageThe timestamp format is consistent with the non-streaming endpoint.
Also applies to: 724-734
tests/unit/cache/test_sqlite_cache.py (1)
20-35: LGTM!The test fixtures correctly include the new timestamp fields, maintaining consistency with the updated
CacheEntrymodel and other test files.tests/unit/cache/test_noop_cache.py (1)
12-27: LGTM!The test fixtures are correctly updated with the new timestamp fields, ensuring consistency across all cache implementation tests.
src/cache/sqlite_cache.py (4)
42-55: LGTM! Schema extension is correct.The CREATE_CACHE_TABLE statement correctly adds
started_atandcompleted_atas nullable text columns, consistent with the existing schema design.
72-83: LGTM! SQL statements correctly handle new fields.Both SELECT_CONVERSATION_HISTORY_STATEMENT and INSERT_CONVERSATION_HISTORY_STATEMENT are properly updated to include
started_atandcompleted_atin the correct positions.
210-220: LGTM! Data mapping is correct.The
get()method correctly maps the query results to CacheEntry fields, withstarted_atat index 4 andcompleted_atat index 5, matching the SELECT statement column order.
246-260: LGTM! Insert operation correctly passes timestamp fields.The
insert_or_append()method correctly extractsstarted_atandcompleted_atfrom the cache_entry and passes them to the INSERT statement in the correct parameter positions.src/cache/postgres_cache.py (4)
38-51: LGTM! Schema extension is correct.The CREATE_CACHE_TABLE statement correctly adds
started_atandcompleted_atas nullable text columns, consistent with the SQLite implementation and existing schema design.
68-79: LGTM! SQL statements correctly handle new fields.Both SELECT_CONVERSATION_HISTORY_STATEMENT and INSERT_CONVERSATION_HISTORY_STATEMENT properly include
started_atandcompleted_at, with correct parameterization for PostgreSQL.
212-223: LGTM! Data mapping is correct.The
get()method correctly maps query results to CacheEntry fields, withstarted_atat index 4 andcompleted_atat index 5, matching the SELECT statement column order.
250-262: LGTM! Insert operation correctly passes timestamp fields.The
insert_or_append()method correctly extractsstarted_atandcompleted_atfrom the cache_entry and passes them to the INSERT statement in the correct parameter positions.
Description
LCORE-776: Allow started_at and completed_at timestamps to be store in database
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Tests