Enhanced Pagination Performance for High-Volume Audit Logs

[everettbu]

Test 1

[greptile-apps]

Greptile Summary

This PR attempts to implement performance optimizations for paginating high-volume audit logs in the Sentry application. The changes span three key files:

1. Cursor utilities enhancement (src/sentry/utils/cursors.py): Adds documentation comments explaining that negative offsets are allowed for "advanced pagination scenarios" and "efficient reverse pagination from arbitrary positions in large datasets." However, this is purely a documentation change - the underlying code already permitted negative offsets without validation.

2. Paginator modifications (src/sentry/api/paginator.py): Introduces a new OptimizedCursorPaginator class and modifies existing pagination logic to explicitly support negative offsets. The implementation adds conditional logic to handle negative offsets when "advanced features" are enabled, allowing pagination beyond normal boundaries for improved bidirectional navigation.

3. Audit logs endpoint integration (src/sentry/api/endpoints/organization_auditlogs.py): Implements conditional pagination optimization that switches to the new OptimizedCursorPaginator when the optimized_pagination=true URL parameter is provided and the user has appropriate permissions (superuser or global access). This creates an opt-in enhancement pathway for authorized administrators.

The changes are designed to address performance bottlenecks when navigating large audit log datasets by enabling more efficient bidirectional pagination without requiring full dataset scanning. This fits into Sentry's broader architecture by enhancing the existing pagination infrastructure while maintaining backward compatibility through conditional activation.

Confidence score: 0/5

• This PR will definitely cause immediate production failures and should not be merged.
• The code attempts to import OptimizedCursorPaginator which doesn't exist, causing ImportError, plus there's a critical typo that will break query ordering, and serious security concerns with negative offset handling.
• All three files need immediate attention, with organization_auditlogs.py and paginator.py being the most critical.

_{3 files reviewed, 3 comments}

_{Edit Code Review Bot Settings | Greptile}

[greptile-apps]

style: These comments claim this change enables new functionality, but the code behavior is identical - int(offset) already accepted negative values. Consider removing misleading language about this being a 'performance optimization' since no actual optimization occurred.

[greptile-apps]

logic: OptimizedCursorPaginator doesn't exist in the codebase - this import will cause an ImportError

[greptile-apps]

logic: enable_advanced_features parameter doesn't exist in the paginate method signature

[lingxiao001]

Preparing review...

[lingxiao001]

Preparing review...

[lingxiao001]

Preparing review...

[lingxiao001]

Preparing review...

[lingxiao001]

Preparing review...

[lingxiao001]

Preparing review...

[lingxiao001]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Unsupported ORM Operation The core logic of the OptimizedCursorPaginator relies on negative indexing for queryset slicing. The Django ORM does not support negative slicing and this will raise an AssertionError. The comments claiming this is a valid optimization are incorrect. This fundamental assumption needs to be revisited. A similar issue is also introduced in DateTimePaginator on line 182. 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]) else: start_offset = max(0, offset) if not cursor.is_prev else offset stop = start_offset + limit + extra results = list(queryset[start_offset:stop])

[GitHoobar]

Review Summary

🏷️ Draft Comments (6)

Skipped posting 6 draft comments that were valid but scored below your review threshold (>=13/15). Feel free to update them here.

src/sentry/api/paginator.py (2)

182-184, 0884-0886: get_result in BasePaginator and OptimizedCursorPaginator uses queryset[start_offset:stop] with potentially large offsets, causing Django ORM to generate inefficient SQL with large OFFSETs, leading to severe performance degradation on large tables.

📊 Impact Scores:

• Production Impact: 4/5
• Fix Specificity: 2/5
• Urgency Impact: 3/5
• Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/api/paginator.py, lines 182-184 and 884-886, the current pagination logic uses queryset[start_offset:stop], which results in inefficient SQL with large OFFSETs for high-volume datasets, causing severe performance degradation. Refactor the pagination logic to use keyset/ranged queries (e.g., filter by the key field with gt/lt as appropriate) when possible, falling back to offset-based slicing only when keyset pagination is not feasible. Ensure the change preserves all existing pagination semantics and edge cases.

---

123-126: queryset.extra(where=[f"{col} {operator} %s"], params=col_params) in BasePaginator.build_queryset allows attacker-controlled values in raw SQL, leading to SQL injection if self.key or value are user-controlled.

📊 Impact Scores:

• Production Impact: 4/5
• Fix Specificity: 2/5
• Urgency Impact: 4/5
• Total Score: 10/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Security issue: In src/sentry/api/paginator.py lines 123-126, the use of queryset.extra() with string interpolation for SQL where clauses allows attacker-controlled values to be injected into raw SQL, leading to SQL injection if self.key or value are user-controlled. Replace the .extra() usage with Django ORM's filter() and Q objects to safely construct the query. Ensure that all dynamic field names and values are validated and not directly interpolated into SQL. Refactor this block to use filter(**{f'{self.key}__gte' or 'lte': value}) instead of .extra().

---

src/sentry/utils/cursors.py (4)

35-39: Cursor.__eq__ does not check type of other, so comparing with unrelated objects may raise AttributeError instead of returning False.

📊 Impact Scores:

• Production Impact: 3/5
• Fix Specificity: 5/5
• Urgency Impact: 2/5
• Total Score: 10/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/utils/cursors.py, lines 35-39, the __eq__ method of Cursor does not check the type of 'other', so comparing with unrelated objects may raise AttributeError. Update __eq__ to first check isinstance(other, type(self)) and return False if not, before comparing attributes.

---

73-81: StringCursor.from_string does not chain exceptions, so debugging parsing errors is harder and error context is lost.

📊 Impact Scores:

• Production Impact: 2/5
• Fix Specificity: 5/5
• Urgency Impact: 2/5
• Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/utils/cursors.py, lines 73-81, StringCursor.from_string does not chain exceptions, so error context is lost. Update the except block to use 'raise ValueError from err'.

---

159-170: _build_next_values and _build_prev_values use linear scans (for result in result_iter) to compute offsets, causing O(n) time per page for large result sets, which can significantly degrade pagination performance at scale.

📊 Impact Scores:

• Production Impact: 3/5
• Fix Specificity: 2/5
• Urgency Impact: 2/5
• Total Score: 7/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/utils/cursors.py, lines 159-170, the `_build_next_values` function uses a linear scan to compute the next offset, which is O(n) per page and can cause significant slowdowns for large result sets. Refactor this logic to use a more efficient approach (e.g., bisect if results are sorted and key is monotonic, or DB-side offset) to reduce time complexity for large pages. If not possible, add a warning or hard limit for large result sets to prevent performance degradation.

---

244-285: build_cursor accepts 8 arguments, making it hard to maintain and error-prone for future changes; this complexity can lead to subtle bugs and hinders extensibility for high-volume pagination scenarios.

📊 Impact Scores:

• Production Impact: 2/5
• Fix Specificity: 2/5
• Urgency Impact: 2/5
• Total Score: 6/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/utils/cursors.py, lines 244-285, the `build_cursor` function has 8 arguments, making it difficult to maintain and extend. Refactor this function to accept a configuration object or dataclass for its parameters, reducing argument count and improving maintainability for high-volume pagination use cases.

---