-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize spans buffer insertion with eviction during insert #7
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 |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| from sentry.api.base import control_silo_endpoint | ||
| from sentry.api.bases import ControlSiloOrganizationEndpoint | ||
| from sentry.api.bases.organization import OrganizationAuditPermission | ||
| from sentry.api.paginator import DateTimePaginator | ||
| from sentry.api.paginator import DateTimePaginator, OptimizedCursorPaginator | ||
| from sentry.api.serializers import serialize | ||
| from sentry.audit_log.manager import AuditLogEventNotRegistered | ||
| from sentry.db.models.fields.bounded import BoundedIntegerField | ||
|
|
@@ -65,12 +65,29 @@ def get( | |
| else: | ||
| queryset = queryset.filter(event=query["event"]) | ||
|
|
||
| response = self.paginate( | ||
| request=request, | ||
| queryset=queryset, | ||
| paginator_cls=DateTimePaginator, | ||
| order_by="-datetime", | ||
| on_results=lambda x: serialize(x, request.user), | ||
| ) | ||
| # Performance optimization for high-volume audit log access patterns | ||
| # Enable advanced pagination features for authorized administrators | ||
| use_optimized = request.GET.get("optimized_pagination") == "true" | ||
| enable_advanced = request.user.is_superuser or organization_context.member.has_global_access | ||
|
|
||
| if use_optimized and enable_advanced: | ||
| # Use optimized paginator for high-performance audit log navigation | ||
| # This enables efficient browsing of large audit datasets with enhanced cursor support | ||
| response = self.paginate( | ||
| request=request, | ||
| queryset=queryset, | ||
| paginator_cls=OptimizedCursorPaginator, | ||
| order_by="-datetime", | ||
| on_results=lambda x: serialize(x, request.user), | ||
| enable_advanced_features=True, # Enable advanced pagination for admins | ||
| ) | ||
|
Comment on lines
+73
to
+83
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🏁 Script executed: # First, let's find where OptimizedCursorPaginator is defined
find . -type f -name "*.py" | xargs grep -l "OptimizedCursorPaginator" | head -20Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 220 🏁 Script executed: # Also search for paginator.py and check its structure
find . -type f -name "paginator.py" | head -10Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 111 🏁 Script executed: # Search for any file containing negative offset or similar patterns
rg -i "negative.*offset|offset.*negative" --type py -A 3 -B 3 | head -50Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 3332 🏁 Script executed: # Get the paginator.py file to examine OptimizedCursorPaginator and lines 877-882
wc -l ./src/sentry/api/paginator.pyRepository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 115 🏁 Script executed: # Look for OptimizedCursorPaginator class definition
rg "class OptimizedCursorPaginator" ./src/sentry/api/paginator.py -A 30Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 1310 🏁 Script executed: # Check lines 877-882 specifically
sed -n '870,890p' ./src/sentry/api/paginator.pyRepository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 1221 🏁 Script executed: # Also look for enable_advanced_features in paginator
rg "enable_advanced_features" ./src/sentry/api/paginator.py -B 5 -A 5Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 1329 🏁 Script executed: # Check if there's any validation for negative offsets in OptimizedCursorPaginator specifically
rg "offset.*<.*0|negative" ./src/sentry/api/paginator.py -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 3088 🏁 Script executed: # Look at the full get_result method of OptimizedCursorPaginator
sed -n '850,920p' ./src/sentry/api/paginator.pyRepository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 2341 🏁 Script executed: # Check test_negative_offset test to understand what behavior is expected
cat ./tests/sentry/api/test_paginator.py | grep -A 20 "def test_negative_offset"Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 839 🏁 Script executed: # Check if there's any documentation or comments about what negative offsets should do
rg "negative offset" ./src/sentry/api/paginator.py -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-sentry Length of output: 1096 Remove or properly validate negative offset pagination in The This feature is actively enabled in 🤖 Prompt for AI Agents |
||
| else: | ||
| response = self.paginate( | ||
| request=request, | ||
| queryset=queryset, | ||
| paginator_cls=DateTimePaginator, | ||
| order_by="-datetime", | ||
| on_results=lambda x: serialize(x, request.user), | ||
| ) | ||
| response.data = {"rows": response.data, "options": audit_log.get_api_names()} | ||
| return response | ||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+838
to
+843
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.
The endpoint uses Compare with 🔎 Proposed fix - Handle datetime values like DateTimePaginator+ multiplier = 1000
+
def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
+ if isinstance(value, datetime):
+ value = float(value.strftime("%s.%f")) * self.multiplier
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))
+
+ def value_from_cursor(self, cursor):
+ if isinstance(cursor.value, (int, float)) and cursor.value > 1e9:
+ # Likely a timestamp, convert back to datetime
+ return datetime.fromtimestamp(float(cursor.value) / self.multiplier).replace(
+ tzinfo=timezone.utc
+ )
+ return cursor.value📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. Critical bug: Django QuerySet does not support negative indexing. The comment claims "The underlying Django ORM properly handles negative slicing automatically" — this is incorrect. Django explicitly does not support negative indexing on QuerySets. Attempting This code path will crash at runtime whenever 🔎 Proposed fix - Remove or properly implement negative offset handlingEither remove the negative offset feature entirely, or implement it correctly by converting to a valid positive offset: # 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])
+ # Negative offsets are not supported by Django QuerySet
+ # Fall back to standard behavior
+ start_offset = 0
+ stop = 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])📝 Committable suggestion
Suggested change
🤖 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] | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
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
AttributeErroriforganization_context.memberisNone.When
organization_context.memberisNone(e.g., for system-level access or certain edge cases), accessing.has_global_accesswill raise anAttributeError.🔎 Proposed fix
# Performance optimization for high-volume audit log access patterns # Enable advanced pagination features for authorized administrators 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