chore(weave): sane limits on delete handling #3895
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=9c285734ad7112b2ea89fb0361092dc882d9f4b3 |
WalkthroughThis pull request revises the Changes
Sequence Diagram(s)sequenceDiagram
participant Req as Client Request
participant Srv as ClickHouseTraceServer
participant DB as Database
Req->>Srv: Invoke calls_delete()
Srv->>Srv: Retrieve parent calls and extract parent_trace_ids
Srv->>DB: Execute query using parent_trace_ids with LIMIT 10000
DB-->>Srv: Return matching calls
Srv->>Srv: Process deletion of calls
Srv-->>Req: Return deletion result
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 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
Documentation and Community
|
![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 (2)
weave/trace_server/clickhouse_trace_server_batched.py (2)
490-498: Add the query limit as a constant rather than using a magic number.Adding a limit to prevent unbounded queries is excellent, but the
10_000limit should be defined as a constant at the module level (like other constants such asMAX_DELETE_CALLS_COUNT).- limit=10_000, + limit=MAX_CALLS_DELETE_BATCH_SIZE,Add this constant to the constants section around line 137:
MAX_DELETE_CALLS_COUNT = 1000 MAX_CALLS_DELETE_BATCH_SIZE = 10000 INITIAL_CALLS_STREAM_BATCH_SIZE = 50
490-498: Consider adding logging or handling for truncated results.The current implementation limits results to 10,000 calls but doesn't provide any notification or special handling if the actual number of calls exceeds this limit. Consider adding a warning log when the limit is reached to help diagnose potential issues with incomplete deletions.
all_calls = list( self.calls_query_stream( tsi.CallsQueryReq( project_id=req.project_id, filter=tsi.CallsFilter(trace_ids=parent_trace_ids), columns=["id", "parent_id"], limit=10_000, ) ) ) + if len(all_calls) == 10_000: + logger.warning( + f"Potentially truncated delete operation. Limit of 10,000 calls reached for project_id={req.project_id}" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
weave/trace_server/clickhouse_trace_server_batched.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Focus on pythonic code patterns. Check for proper exception handling. Verify type hints usage where applicable. Look for potential performance improvements. Don't commen...
**/*.py: Focus on pythonic code patterns.
Check for proper exception handling.
Verify type hints usage where applicable.
Look for potential performance improvements.
Don't comment on formatting if black/isort is configured.
Check for proper dependency injection patterns.
Verify proper async handling if applicable.
weave/trace_server/clickhouse_trace_server_batched.py
🧬 Code Definitions (1)
weave/trace_server/clickhouse_trace_server_batched.py (3)
weave/trace_server/sqlite_trace_server.py (1) (1)
calls_query_stream(534:535)weave/trace_server/trace_server_interface.py (3) (3)
calls_query_stream(914:914)CallsQueryReq(319:346)CallsFilter(298:307)tests/conftest.py (1) (1)
calls_query_stream(117:118)
⏰ Context from checks skipped due to timeout of 90000ms (40)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
- GitHub Check: Trace nox tests (3, 13, trace)
- GitHub Check: Trace nox tests (3, 12, trace)
- GitHub Check: Trace nox tests (3, 11, trace)
- GitHub Check: Trace nox tests (3, 10, trace)
🔇 Additional comments (1)
weave/trace_server/clickhouse_trace_server_batched.py (1)
476-488: Good refactor: Extracting parent trace IDs improves code readability.This refactoring improves the code by extracting
parent_trace_idsas a named variable, making the code more readable and the intention clearer.
Description
In the pathological case the delete path has an unbounded query (all traces in a project are children of the same parent), put a sane limit on the query.
This is a temporary solution, the robust solution will require a trace to know about its children. Right now, traces only have pointers to parents, so we can't walk down the tree (only up).
Testing
👀
Summary by CodeRabbit