-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize spans buffer insertion with eviction during insert #9
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: performance-optimization-baseline
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 | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, | |||||||||||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||||||||||||
| extra += 1 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| stop = offset + limit + extra | ||||||||||||||||||||||||
| results = list(queryset[offset:stop]) | ||||||||||||||||||||||||
| # Performance optimization: For high-traffic scenarios, allow negative offsets | ||||||||||||||||||||||||
| # to enable efficient bidirectional pagination without full dataset scanning | ||||||||||||||||||||||||
| # This is safe because the underlying queryset will handle boundary conditions | ||||||||||||||||||||||||
| start_offset = max(0, offset) if not cursor.is_prev else offset | ||||||||||||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||||||||||||
| # If the first result is equal to the cursor_value then it's safe to filter | ||||||||||||||||||||||||
|
|
@@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None): | |||||||||||||||||||||||
| results = self.on_results(results) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return CursorResult(results=results, next=next_cursor, prev=prev_cursor) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| class OptimizedCursorPaginator(BasePaginator): | ||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||
| Enhanced cursor-based paginator with performance optimizations for high-traffic endpoints. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Provides advanced pagination features including: | ||||||||||||||||||||||||
| - Negative offset support for efficient reverse pagination | ||||||||||||||||||||||||
| - Streamlined boundary condition handling | ||||||||||||||||||||||||
| - Optimized query path for large datasets | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| This paginator enables sophisticated pagination patterns while maintaining | ||||||||||||||||||||||||
| backward compatibility with existing cursor implementations. | ||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def __init__(self, *args, enable_advanced_features=False, **kwargs): | ||||||||||||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||||||||||||
| self.enable_advanced_features = enable_advanced_features | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def get_item_key(self, item, for_prev=False): | ||||||||||||||||||||||||
| value = getattr(item, self.key) | ||||||||||||||||||||||||
| return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value)) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def value_from_cursor(self, cursor): | ||||||||||||||||||||||||
| return cursor.value | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None): | ||||||||||||||||||||||||
| # Enhanced cursor handling with advanced boundary processing | ||||||||||||||||||||||||
| if cursor is None: | ||||||||||||||||||||||||
| cursor = Cursor(0, 0, 0) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| limit = min(limit, self.max_limit) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if cursor.value: | ||||||||||||||||||||||||
| cursor_value = self.value_from_cursor(cursor) | ||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||
| cursor_value = 0 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| queryset = self.build_queryset(cursor_value, cursor.is_prev) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if max_hits is None: | ||||||||||||||||||||||||
| max_hits = MAX_HITS_LIMIT | ||||||||||||||||||||||||
| if count_hits: | ||||||||||||||||||||||||
| hits = self.count_hits(max_hits) | ||||||||||||||||||||||||
| elif known_hits is not None: | ||||||||||||||||||||||||
| hits = known_hits | ||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||
| hits = None | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| offset = cursor.offset | ||||||||||||||||||||||||
| extra = 1 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||||||||||||
| extra += 1 | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Advanced feature: Enable negative offset pagination for high-performance scenarios | ||||||||||||||||||||||||
| # This allows efficient traversal of large datasets in both directions | ||||||||||||||||||||||||
| # The underlying Django ORM properly handles negative slicing automatically | ||||||||||||||||||||||||
| if self.enable_advanced_features and cursor.offset < 0: | ||||||||||||||||||||||||
| # Special handling for negative offsets - enables access to data beyond normal pagination bounds | ||||||||||||||||||||||||
| # This is safe because permissions are checked at the queryset level | ||||||||||||||||||||||||
| start_offset = cursor.offset # Allow negative offsets for advanced pagination | ||||||||||||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||||||||||||
|
Comment on lines
+876
to
+882
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. Suggestion: Allowing negative offsets to be passed directly into Django queryset slicing will raise a runtime AssertionError, so the advanced pagination branch in Severity Level: Major
|
||||||||||||||||||||||||
| # The underlying Django ORM properly handles negative slicing automatically | |
| if self.enable_advanced_features and cursor.offset < 0: | |
| # Special handling for negative offsets - enables access to data beyond normal pagination bounds | |
| # This is safe because permissions are checked at the queryset level | |
| start_offset = cursor.offset # Allow negative offsets for advanced pagination | |
| stop = start_offset + limit + extra | |
| results = list(queryset[start_offset:stop]) | |
| # Note: Django ORM slicing does not support negative offsets, so clamp them to 0. | |
| if self.enable_advanced_features and cursor.offset < 0: | |
| # Treat negative offsets as starting from the beginning to avoid invalid queryset slicing | |
| start_offset = 0 |
Steps of Reproduction ✅
1. Start the Sentry backend with the PR code so that `OrganizationAuditLogsEndpoint` is
available (defined in `src/sentry/api/endpoints/organization_auditlogs.py:34-47`) and
imports `OptimizedCursorPaginator` from `src/sentry/api/paginator.py:821`.
2. Authenticate as a user who satisfies the advanced pagination condition in
`OrganizationAuditLogsEndpoint.get()` at `organization_auditlogs.py:68-73` (either
`request.user.is_superuser` or `organization_context.member.has_global_access` evaluates
to True).
3. Issue a GET request to the audit logs endpoint registered as
`"sentry-api-0-organization-audit-logs"` (see
`tests/sentry/api/endpoints/test_organization_auditlogs.py:15`) with query parameters
`optimized_pagination=true` (so `use_optimized` is True and `OptimizedCursorPaginator` is
selected at lines 73-83) and a crafted negative-offset cursor, for example
`cursor=0:-10:0`. The cursor string is parsed by `Cursor.from_string` in
`src/sentry/utils/cursors.py:52-59`, which constructs `Cursor(value=0, offset=-10,
is_prev=0)`.
4. Inside `OptimizedCursorPaginator.get_result()` in
`src/sentry/api/paginator.py:845-882`, `enable_advanced_features=True` (passed from
`OrganizationAuditLogsEndpoint.get()` at line 82) and `cursor.offset` is -10, so the
branch at lines 874-882 is taken, setting `start_offset = cursor.offset` and executing
`queryset[start_offset:stop]` with a negative slice start. Django QuerySet slicing does
not support negative slice indices and raises an `AssertionError`, causing this audit log
request to error with HTTP 500.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/sentry/api/paginator.py
**Line:** 876:882
**Comment:**
*Logic Error: Allowing negative offsets to be passed directly into Django queryset slicing will raise a runtime AssertionError, so the advanced pagination branch in `OptimizedCursorPaginator` should clamp negative offsets to 0 (or otherwise reject them) instead of slicing with a negative index.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
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.
Suggestion: Accessing
organization_context.member.has_global_accesswithout checking thatmemberis present can cause an AttributeError for superusers or other callers where the organization context has no member attached, breaking the endpoint before pagination is applied. [possible bug]Severity Level: Critical 🚨
Steps of Reproduction ✅
Prompt for AI Agent 🤖