Skip to content

feat(spans): Optimize spans buffer insertion with eviction during insert#3

Open
zaibkhan wants to merge 3 commits into
performance-optimization-baselinefrom
performance-enhancement-complete
Open

feat(spans): Optimize spans buffer insertion with eviction during insert#3
zaibkhan wants to merge 3 commits into
performance-optimization-baselinefrom
performance-enhancement-complete

Conversation

@zaibkhan

@zaibkhan zaibkhan commented Sep 3, 2025

Copy link
Copy Markdown

This change introduces span buffer eviction during insert operations to reduce memory growth under high-volume tracing workloads.

Key improvements:

  • Adds eviction logic to span buffer when capacity is reached during insertion
  • Optimizes memory usage patterns for long-running processes
  • Ensures eviction maintains ordering guarantees for spans
  • Improves throughput in high-traffic environments

Benchmark reference:
Replicated from ai-code-review-evaluation/sentry-greptile PR #2

jan-auer and others added 2 commits June 2, 2025 12:06
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>
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 3, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 3, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 3, 2025
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 3, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Fix datetime key handling, prevent negative slicing, add guard
What’s good: Solid move to zset-based span buffering with eviction; the Lua and Python changes align and reduce memory growth under load.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters File
High Correctness — Optimized paginator crashes on datetime fields get_item_key applies floor/ceil to the ordering value, which will raise a TypeError when ordering by a DateTimeField (e.g., the audit log endpoint passes order_by="-datetime"). Convert datetimes to epoch seconds before applying numeric logic to avoid crashes. …/api/paginator.py
High Correctness — Negative queryset slicing causes runtime errors Django QuerySets do not support negative slicing; using a negative start index will raise an AssertionError at runtime. Normalize to a non-negative start offset to avoid crashes while maintaining behavior. …/api/paginator.py
Key Feedback (click to expand)
  • Needs improvement: The new optimized paginator needs to align with existing DateTime pagination semantics and Django queryset slicing capabilities.
  • Testing: Add tests that exercise OptimizedCursorPaginator with order_by on a DateTimeField (e.g., AuditLogEntry.datetime) and verify pagination doesn't crash. Also add a test to ensure negative offsets are rejected or normalized (no negative queryset slices).
  • Documentation: Document that OptimizedCursorPaginator supports datetime fields (or is numeric-only), and clarify any feature flags/eligibility checks necessary to enable advanced pagination.
  • Compatibility: Django QuerySet slicing does not support negative indices; relying on negative offsets will cause runtime errors across ORM backends.
  • Open questions: Should the optimized paginator be specialized for numeric fields only, or should it explicitly support datetime ordering (as used by audit logs)?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

queryset = self.build_queryset(cursor_value, cursor.is_prev)

if max_hits is None:
max_hits = MAX_HITS_LIMIT

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: get_item_key applies floor/ceil to the ordering value, which will raise a TypeError when ordering by a DateTimeField (e.g., the audit log endpoint passes order_by="-datetime"). Convert datetimes to epoch seconds before applying numeric logic to avoid crashes.

Suggested change
max_hits = MAX_HITS_LIMIT
return int(value.replace(tzinfo=timezone.utc).timestamp()) if isinstance(value, datetime) else int(math.floor(value) if self._is_asc(for_prev) else math.ceil(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]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Django QuerySets do not support negative slicing; using a negative start index will raise an AssertionError at runtime. Normalize to a non-negative start offset to avoid crashes while maintaining behavior.

Suggested change
results = results[:-1]
start_offset = max(0, cursor.offset)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants