-
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+834
to
+836
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| 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
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| # 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]) | |
| # Disallow negative offsets in pagination to avoid exposing data outside intended bounds. | |
| # Negative indices in Python/Django slices access elements from the end of the sequence and | |
| # can bypass normal pagination limits, so we clamp forward offsets to zero instead. | |
| if cursor.offset < 0 and not cursor.is_prev: | |
| logging.getLogger(__name__).warning( | |
| "Negative cursor.offset (%s) encountered in paginator; clamping to 0", | |
| cursor.offset, | |
| ) | |
| start_offset = max(0, offset) if not cursor.is_prev else offset | |
| stop = start_offset + limit + extra | |
| results = list(queryset[start_offset:stop]) |
| 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 | ||||||||||||||
|
||||||||||||||
| for i = 0, 1000 do | |
| -- Follow redirect chains to find the final set_span_id. | |
| -- The upper bound of 10,000 iterations is intentionally high to support | |
| -- very deep (but valid) segment trees while still guarding against | |
| -- corrupted data or cycles that could otherwise cause an infinite loop. | |
| for i = 0, 10000 do |
| 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) | ||
| if cursor == 0: | ||
| del cursors[key] | ||
| else: | ||
|
Comment on lines
+449
to
452
|
||
|
|
||
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.
The variable
enable_advancedcould beNoneiforganization_context.memberisNone, leading to a boolean evaluation issue. Additionally, relying solely onis_superuserorhas_global_accessfor advanced pagination features may not be sufficient given the security concerns with negative offsets. Consider adding explicit feature flags or more granular permission checks.