perf: Add server-side filtering for ep logs to improve performance#408
perf: Add server-side filtering for ep logs to improve performance#408
Conversation
When opening the logs UI with a filterConfig URL parameter (e.g., filtering by invocation_id), the backend now filters data server-side instead of sending all logs to the frontend for client-side filtering. Changes: - Add invocation_ids and limit parameters to SqliteEvaluationRowStore.read_rows() - Add server-side filtering using SQLite JSON extraction - Default limit of 1000 rows for initial WebSocket load - WebSocket endpoint parses invocation_ids from URL query params - Frontend extracts invocation_ids from URL filterConfig and passes to WebSocket - Real-time broadcast messages filtered per-connection based on subscribed invocation_ids - Order results by most recent first (DESC by rowid) This significantly speeds up ep logs when viewing filtered results by avoiding loading all historical data.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84949cd665
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @abstractmethod | ||
| def read(self, row_id: Optional[str] = None) -> List["EvaluationRow"]: | ||
| def read( | ||
| self, | ||
| rollout_id: Optional[str] = None, | ||
| invocation_ids: Optional[List[str]] = None, | ||
| limit: Optional[int] = None, |
There was a problem hiding this comment.
The abstract DatasetLogger.read signature now includes rollout_id, invocation_ids, and limit, but LocalFSDatasetLoggerAdapter still only defines read(self, row_id=...). Any caller that treats a LocalFSDatasetLoggerAdapter as a DatasetLogger and passes the new keywords (for example, read(invocation_ids=[...]) or read(rollout_id=...)) will now hit a TypeError for unexpected keyword arguments. To preserve compatibility with existing adapters, they should be updated to accept the new parameters (even if they ignore them).
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| try: | ||
| logger.debug(f"[WEBSOCKET_SEND] Preparing to send to connection {i}") | ||
| tasks.append(connection.send_text(text)) | ||
| task_connections.append(connection) |
There was a problem hiding this comment.
Failed connection preparation skips cleanup
Low Severity
In _send_text_to_filtered_connections, when an exception occurs during coroutine preparation (lines 231-232), the connection is no longer added to failed_connections for cleanup. The original code added failing connections to failed_connections in the except block, but this was removed during refactoring. Now failed_connections is initialized after the loop (line 235), making it impossible to track preparation failures. Connections that fail during preparation will remain in active_connections and _connection_filters until they fail during an actual send attempt.
Additional Locations (1)
| eligible_connections.append(conn) | ||
| elif row_invocation_id is not None and row_invocation_id in conn_filter: | ||
| # Row's invocation_id matches connection's filter | ||
| eligible_connections.append(conn) |
There was a problem hiding this comment.
Empty invocation_ids filter causes inconsistent behavior
Low Severity
Empty invocation_ids list is handled inconsistently between initial load and real-time updates. In read_rows(), the condition len(invocation_ids) > 0 treats an empty list as "no filter" (returns all rows). But in _send_text_to_filtered_connections(), the check conn_filter is None is False for [], and row_invocation_id in [] is always False, so the connection receives no updates. This causes a scenario where a WebSocket connection with ?invocation_ids=, receives all initial logs but never receives real-time broadcasts.
Additional Locations (1)
1 participant
Summary
When opening the logs UI with a filterConfig URL parameter (e.g., filtering by invocation_id), the backend now filters data server-side instead of sending all logs to the frontend for client-side filtering.
Problem
ep logswas very slow when opening the UI with rollouts because:This was especially painful with large databases.
Solution
Backend Changes
sqlite_evaluation_row_store.py): Addedinvocation_idsandlimitparameters toread_rows()using SQLite JSON extractionsqlite_dataset_logger_adapter.py,dataset_logger.py,__init__.py): Updatedread()method signaturelogs_server.py):invocation_idsandlimitfrom WebSocket URL query paramsFrontend Changes
extractInvocationIdsFromUrl()to parse invocation IDs from URL filterConfigHow It Works
http://localhost:8000?filterConfig=[{"logic":"AND","filters":[{"field":"$.execution_metadata.invocation_id","operator":"equals","value":"inv-123"}]}]inv-123from filter configws://localhost:8000/ws?invocation_ids=inv-123Testing
Note
Improves log loading performance by filtering and limiting data at the source.
DatasetLogger.read()to acceptrollout_id,invocation_ids, andlimit; implement insqlite_dataset_logger_adapter.pyandsqlite_evaluation_row_store.py(JSON extract on$.execution_metadata.invocation_id, ordered most-recent-first, optionallimit)logs_server.py: WebSocket acceptsinvocation_idsandlimitquery params, sends only matching initial rows (defaultlimit=1000), and filters real-time broadcasts per-connectionconfig.tsaddsextractInvocationIdsFromUrl()and augmentsgetWebSocketUrl()to includeinvocation_ids;App.tsxpasses extracted IDs when connectingWritten by Cursor Bugbot for commit 84949cd. This will update automatically on new commits. Configure here.