-
Notifications
You must be signed in to change notification settings - Fork 0
Automated Test: performance-enhancement-complete #322
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]) | ||||||||||||||||||||||||
|
Comment on lines
+179
to
+184
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. Misleading comment about negative offset support. The comment claims to "allow negative offsets" but the code uses Consider clarifying the comment or adding bounds checking for the prev cursor case: Suggested clarification- # 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
+ # For non-prev cursors, clamp offset to 0 to handle edge cases.
+ # For prev cursors, offset is expected to be non-negative from cursor construction.
+ start_offset = max(0, offset)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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]) | ||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||
| start_offset = max(0, offset) if not cursor.is_prev else offset | ||||||||||||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||||||||||||
|
Comment on lines
+874
to
+886
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. 🧩 Analysis chain🌐 Web query:
💡 Result: Django At the implementation level, Sources 🏁 Script executed: # Verify the file and line numbers exist
if [ -f "src/sentry/api/paginator.py" ]; then
echo "File exists"
# Check line count
wc -l src/sentry/api/paginator.py
# Get the relevant section around lines 874-886
sed -n '874,886p' src/sentry/api/paginator.py
else
echo "File not found, searching for paginator files"
find . -name "*paginator*" -type f | grep -v __pycache__
fiRepository: admin-coderabbit/coderabbit-sentry-mirror Length of output: 1027 Critical: Django QuerySets do not support negative indexing. The comment on line 876 states "The underlying Django ORM properly handles negative slicing automatically"—this is incorrect. Django QuerySets explicitly raise a When Additionally, the comment on lines 878–879 about enabling "access to data beyond normal pagination bounds" raises security concerns about potential pagination bypass. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||||||||||||
| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: | ||||||||||||||||||||||||
| results = results[1:] | ||||||||||||||||||||||||
| elif len(results) == offset + limit + extra: | ||||||||||||||||||||||||
| results = results[:-1] | ||||||||||||||||||||||||
|
Comment on lines
+888
to
+892
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. Inconsistent use of Line 891 compares against Suggested fix if cursor.is_prev and cursor.value:
if results and self.get_item_key(results[0], for_prev=True) == cursor.value:
results = results[1:]
- elif len(results) == offset + limit + extra:
+ elif len(results) == start_offset + limit + extra:
results = results[:-1]📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if cursor.is_prev: | ||||||||||||||||||||||||
| results.reverse() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| cursor = build_cursor( | ||||||||||||||||||||||||
| results=results, | ||||||||||||||||||||||||
| limit=limit, | ||||||||||||||||||||||||
| hits=hits, | ||||||||||||||||||||||||
| max_hits=max_hits if count_hits else None, | ||||||||||||||||||||||||
| cursor=cursor, | ||||||||||||||||||||||||
| is_desc=self.desc, | ||||||||||||||||||||||||
| key=self.get_item_key, | ||||||||||||||||||||||||
| on_results=self.on_results, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if self.post_query_filter: | ||||||||||||||||||||||||
| cursor.results = self.post_query_filter(cursor.results) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return cursor | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,7 @@ local main_redirect_key = string.format("span-buf:sr:{%s}", project_and_trace) | |
| local set_span_id = parent_span_id | ||
| local redirect_depth = 0 | ||
|
|
||
| for i = 0, 10000 do -- theoretically this limit means that segment trees of depth 10k may not be joined together correctly. | ||
| for i = 0, 1000 do | ||
| local new_set_span = redis.call("hget", main_redirect_key, set_span_id) | ||
| redirect_depth = i | ||
| if not new_set_span or new_set_span == set_span_id then | ||
|
|
@@ -40,19 +40,29 @@ end | |
| redis.call("hset", main_redirect_key, span_id, set_span_id) | ||
| redis.call("expire", main_redirect_key, set_timeout) | ||
|
|
||
| local span_count = 0 | ||
|
|
||
| local set_key = string.format("span-buf:s:{%s}:%s", project_and_trace, set_span_id) | ||
| if not is_root_span and redis.call("scard", span_key) > 0 then | ||
| redis.call("sunionstore", set_key, set_key, span_key) | ||
| if not is_root_span and redis.call("zcard", span_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, span_key) | ||
| redis.call("unlink", span_key) | ||
| end | ||
|
|
||
| local parent_key = string.format("span-buf:s:{%s}:%s", project_and_trace, parent_span_id) | ||
| if set_span_id ~= parent_span_id and redis.call("scard", parent_key) > 0 then | ||
| redis.call("sunionstore", set_key, set_key, parent_key) | ||
| if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key) | ||
| redis.call("unlink", parent_key) | ||
| end | ||
| redis.call("expire", set_key, set_timeout) | ||
|
|
||
| if span_count == 0 then | ||
| span_count = redis.call("zcard", set_key) | ||
| end | ||
|
|
||
| if span_count > 1000 then | ||
| redis.call("zpopmin", set_key, span_count - 1000) | ||
| end | ||
|
Comment on lines
+62
to
+64
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. Hardcoded limit The Lua script hardcodes the limit to 1000, while 🤖 Prompt for AI Agents |
||
|
|
||
| local has_root_span_key = string.format("span-buf:hrs:%s", set_key) | ||
| local has_root_span = redis.call("get", has_root_span_key) == "1" or is_root_span | ||
| if has_root_span then | ||
|
|
||
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.
Potential
AttributeErrorwhenorganization_context.memberisNone.Per the
RpcUserOrganizationContextmodel,membercan beNonewhen the user doesn't have membership. Accessing.has_global_accessonNonewill raise anAttributeError.🐛 Proposed fix
use_optimized = request.GET.get("optimized_pagination") == "true" - enable_advanced = request.user.is_superuser or organization_context.member.has_global_access + enable_advanced = request.user.is_superuser or ( + organization_context.member is not None + and organization_context.member.has_global_access + )📝 Committable suggestion
🤖 Prompt for AI Agents