-
Notifications
You must be signed in to change notification settings - Fork 0
Optimize spans buffer insertion with eviction during insert #19
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 | ||
|
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. OptimizedCursorPaginator crashes on datetime fieldsHigh Severity
Additional Locations (1) |
||
|
|
||
| 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]) | ||
|
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. Negative queryset offset crashes Django ORMHigh Severity The Additional Locations (1) |
||
| else: | ||
| 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 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,7 @@ class Span(NamedTuple): | |
| parent_span_id: str | None | ||
| project_id: int | ||
| payload: bytes | ||
| end_timestamp_precise: float | ||
| is_segment_span: bool = False | ||
|
|
||
| def effective_parent_id(self): | ||
|
|
@@ -193,7 +194,9 @@ def process_spans(self, spans: Sequence[Span], now: int): | |
| with self.client.pipeline(transaction=False) as p: | ||
| for (project_and_trace, parent_span_id), subsegment in trees.items(): | ||
| set_key = f"span-buf:s:{{{project_and_trace}}}:{parent_span_id}" | ||
| p.sadd(set_key, *[span.payload for span in subsegment]) | ||
| p.zadd( | ||
| set_key, {span.payload: span.end_timestamp_precise for span in subsegment} | ||
| ) | ||
|
|
||
| p.execute() | ||
|
|
||
|
|
@@ -428,13 +431,13 @@ def _load_segment_data(self, segment_keys: list[SegmentKey]) -> dict[SegmentKey, | |
| with self.client.pipeline(transaction=False) as p: | ||
| current_keys = [] | ||
| for key, cursor in cursors.items(): | ||
| p.sscan(key, cursor=cursor, count=self.segment_page_size) | ||
| p.zscan(key, cursor=cursor, count=self.segment_page_size) | ||
| current_keys.append(key) | ||
|
|
||
| results = p.execute() | ||
|
|
||
| for key, (cursor, spans) in zip(current_keys, results): | ||
| sizes[key] += sum(len(span) for span in spans) | ||
| for key, (cursor, zscan_values) in zip(current_keys, results): | ||
| sizes[key] += sum(len(span) for span, _ in zscan_values) | ||
| if sizes[key] > self.max_segment_bytes: | ||
| metrics.incr("spans.buffer.flush_segments.segment_size_exceeded") | ||
| logger.error("Skipping too large segment, byte size %s", sizes[key]) | ||
|
|
@@ -443,15 +446,7 @@ def _load_segment_data(self, segment_keys: list[SegmentKey]) -> dict[SegmentKey, | |
| del cursors[key] | ||
| continue | ||
|
|
||
| payloads[key].extend(spans) | ||
| if len(payloads[key]) > self.max_segment_spans: | ||
| metrics.incr("spans.buffer.flush_segments.segment_span_count_exceeded") | ||
| logger.error("Skipping too large segment, span count %s", len(payloads[key])) | ||
|
|
||
| del payloads[key] | ||
| del cursors[key] | ||
| continue | ||
|
|
||
| payloads[key].extend(span for span, _ in zscan_values) | ||
|
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. Unused
|
||
| if cursor == 0: | ||
| del cursors[key] | ||
| else: | ||
|
|
||


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.
Negative offset allowed in base paginator for prev cursors
Medium Severity
The new comment in
Cursorexplicitly encourages negative offsets for "advanced pagination scenarios," and theBasePaginatorchange usesoffsetdirectly (without clamping) whencursor.is_previsTrue. If a client sends a cursor with a negative offset andis_prev=1,queryset[negative:stop]will crash Django with anAssertionError. The misleading comment normalizes constructing negative-offset cursors, increasing the risk of triggering this crash.Additional Locations (1)
src/sentry/utils/cursors.py#L25-L28