Skip to content

Conversation

@xurui-c
Copy link
Member

@xurui-c xurui-c commented Jan 29, 2026

reverts #7614 manually because using the label ran into a merge conflict

@xurui-c xurui-c marked this pull request as ready for review January 29, 2026 23:39
@xurui-c xurui-c requested a review from a team as a code owner January 29, 2026 23:39
Copilot AI review requested due to automatic review settings January 29, 2026 23:39
@xurui-c xurui-c requested a review from a team as a code owner January 29, 2026 23:39
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR manually reverts the pagination speedup optimization from PR #7614 for the GetTrace endpoint. The optimization attempted to improve query performance by aligning the ORDER BY clause with ClickHouse's table sort key and adjusting the pagination logic accordingly.

Changes:

  • Simplified the EndpointGetTracePageToken from tracking 6 fields (project_id, item_type, timestamp, trace_id, item_id) to 2 fields (timestamp_precise, item_id)
  • Reverted the ORDER BY clause from 6 columns (organization_id, project_id, item_type, timestamp, trace_id, item_id) to 3 columns (timestamp, item_timestamp, item_id)
  • Simplified the pagination filter logic to work with the reverted sorting approach
  • Removed the set_skip_transform_order_by() call that prevented ORDER BY transformation
  • Removed the test test_no_transformation_on_order_by that validated the optimization behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
snuba/web/rpc/v1/endpoint_get_trace.py Reverted pagination implementation including page token structure, ORDER BY clause, pagination filter logic, and query settings
tests/web/rpc/v1/test_endpoint_get_trace.py Removed test case validating the optimization behavior and cleaned up unused imports

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +107 to 108
if len(filters) != 3:
raise ValueError("Invalid page token")
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page token format has changed from expecting 6 filters to 3 filters. This will cause existing in-flight page tokens from before this revert to fail with "Invalid page token" error. Users will need to restart their pagination from the beginning. Consider whether this breaking change needs to be communicated to API consumers or if there should be a transition period that handles both token formats.

Copilot uses AI. Check for mistakes.
@xurui-c xurui-c merged commit 1785874 into master Jan 30, 2026
52 of 56 checks passed
@xurui-c xurui-c deleted the rachel/revert branch January 30, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants