Optimize spans buffer insertion with eviction during insert

[ShashankFC]

Test 2

Summary by CodeRabbit

Release Notes

• New Features

  • Added optimized pagination support to significantly improve performance when querying large datasets
  • Introduced advanced pagination capabilities available to authorized users
  • Enhanced span event tracking with precise timestamp information

• Bug Fixes

  • Fixed pagination boundary edge cases for more reliable result handling

_{✏️ Tip: You can customize this high-level summary in your review settings.}

---

Replicated from ai-code-review-evaluation/sentry-coderabbit#2

[ShashankFC]

@cubic-dev-ai review this pull request

[cubic-dev-ai]

@cubic-dev-ai review this pull request

@ShashankFC I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

5 issues found across 9 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/sentry/api/paginator.py">

<violation number="1" location="src/sentry/api/paginator.py:181">
P2: Misleading comment: the claim that 'the underlying queryset will handle boundary conditions' for negative offsets is incorrect. Django QuerySets raise `AssertionError` on negative indexing. This comment should be corrected or removed to avoid misleading future developers.</violation>

<violation number="2" location="src/sentry/api/paginator.py:880">
P0: Critical bug: Django QuerySets do **not** support negative indexing/slicing. `queryset[start_offset:stop]` with a negative `start_offset` will raise `AssertionError: Negative indexing is not supported.` at runtime. The comment claiming Django ORM handles this automatically is incorrect — this has been a long-standing, intentional design decision in Django (ticket #13089, status: wontfix).

This entire `enable_advanced_features` code path is broken and should be removed, or the offset must be clamped to `>= 0`.</violation>
</file>

<file name="src/sentry/utils/cursors.py">

<violation number="1" location="src/sentry/utils/cursors.py:26">
P2: Misleading comment: claims to "allow negative offsets" as a "performance optimization," but no code was actually changed — `self.offset = int(offset)` was already present. The pagination logic in `_build_next_values` and `_build_prev_values` does not handle negative offsets correctly (e.g., `offset + limit` would produce unexpected results). This comment could mislead maintainers into thinking negative offsets are an intentional, tested feature and discourage adding needed input validation. Consider removing these comments or, if negative offsets are truly desired, adding explicit validation and tests.</violation>
</file>

<file name="src/sentry/api/endpoints/organization_auditlogs.py">

<violation number="1" location="src/sentry/api/endpoints/organization_auditlogs.py:71">
P1: `organization_context.member` can be `None` (as documented in `RpcUserOrganizationContext`), which will cause an `AttributeError` when accessing `.has_global_access`. Add a None-guard before accessing the member's attribute.</violation>
</file>

<file name="src/sentry/scripts/spans/add-buffer.lua">

<violation number="1" location="src/sentry/scripts/spans/add-buffer.lua:62">
P2: The eviction limit `1000` is hardcoded as a magic number in the Lua script, while the Python caller has a configurable `max_segment_spans=1001` that is never passed through. Consider adding it as a script argument (e.g. `ARGV[5]`) so the two stay in sync, and clarify whether the intended cap is 1000 or 1001.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P0: Critical bug: Django QuerySets do not support negative indexing/slicing. queryset[start_offset:stop] with a negative start_offset will raise AssertionError: Negative indexing is not supported. at runtime. The comment claiming Django ORM handles this automatically is incorrect — this has been a long-standing, intentional design decision in Django (ticket #13089, status: wontfix).

This entire enable_advanced_features code path is broken and should be removed, or the offset must be clamped to >= 0.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/api/paginator.py, line 880:

<comment>Critical bug: Django QuerySets do **not** support negative indexing/slicing. `queryset[start_offset:stop]` with a negative `start_offset` will raise `AssertionError: Negative indexing is not supported.` at runtime. The comment claiming Django ORM handles this automatically is incorrect — this has been a long-standing, intentional design decision in Django (ticket #13089, status: wontfix).

This entire `enable_advanced_features` code path is broken and should be removed, or the offset must be clamped to `>= 0`.</comment>

<file context>
@@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None):
+        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])
</file context>

Suggested change

	start_offset = cursor.offset # Allow negative offsets for advanced pagination
	stop = start_offset + limit + extra
	results = list(queryset[start_offset:stop])
	start_offset = max(0, cursor.offset)
	stop = start_offset + limit + extra
	results = list(queryset[start_offset:stop])

[cubic-dev-ai]

P1: organization_context.member can be None (as documented in RpcUserOrganizationContext), which will cause an AttributeError when accessing .has_global_access. Add a None-guard before accessing the member's attribute.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/api/endpoints/organization_auditlogs.py, line 71:

<comment>`organization_context.member` can be `None` (as documented in `RpcUserOrganizationContext`), which will cause an `AttributeError` when accessing `.has_global_access`. Add a None-guard before accessing the member's attribute.</comment>

<file context>
@@ -65,12 +65,29 @@ def get(
+        # 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:
</file context>

Suggested change

	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)

[cubic-dev-ai]

P2: Misleading comment: the claim that 'the underlying queryset will handle boundary conditions' for negative offsets is incorrect. Django QuerySets raise AssertionError on negative indexing. This comment should be corrected or removed to avoid misleading future developers.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/api/paginator.py, line 181:

<comment>Misleading comment: the claim that 'the underlying queryset will handle boundary conditions' for negative offsets is incorrect. Django QuerySets raise `AssertionError` on negative indexing. This comment should be corrected or removed to avoid misleading future developers.</comment>

<file context>
@@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None,
-        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
</file context>

[cubic-dev-ai]

P2: Misleading comment: claims to "allow negative offsets" as a "performance optimization," but no code was actually changed — self.offset = int(offset) was already present. The pagination logic in _build_next_values and _build_prev_values does not handle negative offsets correctly (e.g., offset + limit would produce unexpected results). This comment could mislead maintainers into thinking negative offsets are an intentional, tested feature and discourage adding needed input validation. Consider removing these comments or, if negative offsets are truly desired, adding explicit validation and tests.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/utils/cursors.py, line 26:

<comment>Misleading comment: claims to "allow negative offsets" as a "performance optimization," but no code was actually changed — `self.offset = int(offset)` was already present. The pagination logic in `_build_next_values` and `_build_prev_values` does not handle negative offsets correctly (e.g., `offset + limit` would produce unexpected results). This comment could mislead maintainers into thinking negative offsets are an intentional, tested feature and discourage adding needed input validation. Consider removing these comments or, if negative offsets are truly desired, adding explicit validation and tests.</comment>

<file context>
@@ -23,6 +23,8 @@ def __init__(
         has_results: bool | None = None,
     ):
         self.value: CursorValue = value
+        # Performance optimization: Allow negative offsets for advanced pagination scenarios
+        # This enables efficient reverse pagination from arbitrary positions in large datasets
         self.offset = int(offset)
</file context>

[cubic-dev-ai]

P2: The eviction limit 1000 is hardcoded as a magic number in the Lua script, while the Python caller has a configurable max_segment_spans=1001 that is never passed through. Consider adding it as a script argument (e.g. ARGV[5]) so the two stay in sync, and clarify whether the intended cap is 1000 or 1001.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sentry/scripts/spans/add-buffer.lua, line 62:

<comment>The eviction limit `1000` is hardcoded as a magic number in the Lua script, while the Python caller has a configurable `max_segment_spans=1001` that is never passed through. Consider adding it as a script argument (e.g. `ARGV[5]`) so the two stay in sync, and clarify whether the intended cap is 1000 or 1001.</comment>

<file context>
@@ -40,19 +40,29 @@ end
+    span_count = redis.call("zcard", set_key)
+end
+
+if span_count > 1000 then
+    redis.call("zpopmin", set_key, span_count - 1000)
+end
</file context>