Optimize spans buffer insertion with eviction during insert#19
Optimize spans buffer insertion with eviction during insert#19ShashankFC wants to merge 2 commits into
Conversation
A proof of concept that limits the number of spans per segment during insertion. Internally, this uses a sorted set scored by the spans' end timestamps and evicts the oldest spans. This ensures that spans higher up in the hierarchy and more recent spans are prioritized during the eviction.
…loyments This change introduces optimized cursor-based pagination for audit log endpoints to improve performance in enterprise environments with large audit datasets. Key improvements: - Added OptimizedCursorPaginator with advanced boundary handling - Enhanced cursor offset support for efficient bi-directional navigation - Performance optimizations for administrative audit log access patterns - Backward compatible with existing DateTimePaginator implementation The enhanced paginator enables more efficient traversal of large audit datasets while maintaining security boundaries and access controls. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| # 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.
Negative queryset offset crashes Django ORM
High Severity
The OptimizedCursorPaginator explicitly allows negative offsets when enable_advanced_features is True and cursor.offset < 0, resulting in queryset[negative:stop]. Django querysets do not support negative indexing and will raise an AssertionError("Negative indexing is not supported."). The comment claiming "The underlying Django ORM properly handles negative slicing automatically" is incorrect. This means the "advanced pagination" feature will always crash when triggered via the audit logs endpoint with a negative cursor offset.
Additional Locations (1)
| # 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]) |
There was a problem hiding this comment.
Negative offset allowed in base paginator for prev cursors
Medium Severity
The new comment in Cursor explicitly encourages negative offsets for "advanced pagination scenarios," and the BasePaginator change uses offset directly (without clamping) when cursor.is_prev is True. If a client sends a cursor with a negative offset and is_prev=1, queryset[negative:stop] will crash Django with an AssertionError. The misleading comment normalizes constructing negative-offset cursors, increasing the risk of triggering this crash.
Additional Locations (1)
| del cursors[key] | ||
| continue | ||
|
|
||
| payloads[key].extend(span for span, _ in zscan_values) |
There was a problem hiding this comment.
Unused max_segment_spans parameter is now dead code
Low Severity
The max_segment_spans parameter is still accepted in SpansBuffer.__init__ and stored as self.max_segment_spans, but the only code that read it (the span count check in _load_segment_data) was removed. This makes the parameter dead code that misleadingly suggests a configurable span count limit still exists at flush time.
Additional Locations (1)
| 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.
OptimizedCursorPaginator crashes on datetime fields
High Severity
OptimizedCursorPaginator.get_item_key calls math.floor directly on the raw attribute value, but when used with order_by="-datetime" in the audit logs endpoint, getattr(item, "datetime") returns a datetime object. math.floor(datetime) raises a TypeError. The DateTimePaginator it replaces first converts via value.strftime("%s.%f") * multiplier. Similarly, value_from_cursor returns the raw numeric cursor value instead of converting it back to a datetime, so page-2+ queries would also fail with a database type mismatch. Any audit log request using this paginator that returns results will crash.
Additional Locations (1)
2 participants
Test 2
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/sentry-coderabbit#2
Note
Medium Risk
Touches core pagination and Redis-backed span buffering logic; incorrect slicing/negative-offset behavior or eviction ordering could change results or drop spans under load.
Overview
Improves spans buffer performance/memory by switching Redis storage from sets to sorted sets scored by
end_timestamp_precise, merging viaZUNIONSTORE, and evicting oldest spans during insertion to cap each segment at 1000 entries.Adds an opt-in
optimized_pagination=truepath for organization audit logs that uses a newOptimizedCursorPaginator, and updates cursor/queryset slicing to better handle reverse/offset edge cases (including optional negative-offset behavior for advanced usage). Span ingestion/consumer and tests are updated to require and propagateend_timestamp_precise.Written by Cursor Bugbot for commit 8ab8814. Configure here.