Optimize spans buffer insertion with eviction during insert - #1
Optimize spans buffer insertion with eviction during insert#1linxia0415 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>
📝 WalkthroughWalkthroughThis PR adds timestamp-based ordering to span buffering via Redis sorted sets and introduces an optimized cursor paginator with advanced features. Span ingestion now captures and stores ChangesSpan Buffering with Timestamp Ordering
Optimized Cursor Pagination
🎯 3 (Moderate) | ⏱️ ~22 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/sentry/utils/cursors.py (1)
26-28: 💤 Low valueComment overstates capability; no logic actually enables negative-offset traversal here.
These lines only document intent —
int(offset)happily accepts negatives, but nothing inCursorvalidates or leverages them. The behavior is entirely driven by the consumers inpaginator.py. Consider trimming the comment to avoid implying a feature exists at this layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/utils/cursors.py` around lines 26 - 28, The comment on Cursor's offset overstates functionality — update the comment around Cursor (self.offset = int(offset)) to remove the claim that this class “allows negative offsets for advanced pagination” and instead state plainly that the attribute accepts negative integers but that negative-offset traversal logic is implemented by callers (see paginator.py), or simply drop the explanatory sentence; leave only the factual assignment and minimal note that validation/behavior is handled by consumers.src/sentry/api/paginator.py (1)
179-184: 💤 Low valueComment contradicts the code: this clamps negative offsets, it does not allow them.
For non-
is_prevcursorsstart_offset = max(0, offset)discards any negative offset, which is the opposite of what the comment claims. Behavior for normal (non-negative) offsets is unchanged, so this is just a misleading comment — but it should be corrected to avoid confusing future readers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/api/paginator.py` around lines 179 - 184, The comment above the start_offset calculation is misleading: the code clamps negative offsets for non-previous cursors (start_offset = max(0, offset)) rather than allowing them. Update the comment to accurately describe the behavior (e.g., "Clamp negative offsets to 0 for forward pagination; use raw offset for cursor.is_prev to allow backward pagination") or remove the incorrect claim; refer to start_offset, offset, cursor.is_prev and the queryset[start_offset:stop] slicing so the reader can locate and verify the logic.src/sentry/api/endpoints/organization_auditlogs.py (1)
68-91: 💤 Low valueMaintainability: query-param-gated dual pagination path duplicates the
paginate(...)call.The two branches differ only in
paginator_clsand the extraenable_advanced_featureskwarg. Once the underlying paginator issues are resolved, consider collapsing to a singlepaginate(...)call by computingpaginator_clsand the kwargs up front, to keep theon_results/order_byarguments from drifting between branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sentry/api/endpoints/organization_auditlogs.py` around lines 68 - 91, The paginate call is duplicated; compute the paginator class and optional kwargs first then call self.paginate once to avoid drift: determine use_optimized and enable_advanced as in the snippet, set paginator_cls = OptimizedCursorPaginator if use_optimized and enable_advanced else DateTimePaginator, build an kwargs dict that includes order_by="-datetime" and on_results=lambda x: serialize(x, request.user) and conditionally add enable_advanced_features=True when using OptimizedCursorPaginator, then invoke self.paginate(request=request, queryset=queryset, paginator_cls=paginator_cls, **kwargs); this keeps the pagination call centralized (references: self.paginate, OptimizedCursorPaginator, DateTimePaginator, enable_advanced_features).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sentry/api/endpoints/organization_auditlogs.py`:
- Around line 70-83: The current enable_advanced calculation can raise
AttributeError because organization_context.member may be None; change the check
in the organization_auditlogs endpoint to safely handle a missing member by
computing enable_advanced = request.user.is_superuser or
(organization_context.member is not None and
organization_context.member.has_global_access), then only call
self.paginate(..., paginator_cls=OptimizedCursorPaginator,
enable_advanced_features=True) when enable_advanced is True; additionally,
review the OptimizedCursorPaginator implementation where it parses cursor
offsets to ensure negative offsets are either validated or converted to a
supported form (adjust validation/raise location in OptimizedCursorPaginator to
match current implementation) so the optimized path will not raise an unexpected
ValueError at runtime.
In `@src/sentry/api/paginator.py`:
- Around line 845-911: OptimizedCursorPaginator.get_result largely duplicates
BasePaginator.get_result; refactor by extracting the slice-bound computation
into a BasePaginator protected hook (e.g., _compute_slice_bounds(self, offset,
limit, extra, is_prev)) and have BasePaginator.get_result call that hook to get
(start_offset, stop), then make OptimizedCursorPaginator override only
_compute_slice_bounds to implement the negative-offset logic when
self.enable_advanced_features and offset < 0 while delegating all other behavior
to the base method (so remove the near-verbatim get_result implementation in
OptimizedCursorPaginator and keep unique symbols:
OptimizedCursorPaginator.get_result -> use BasePaginator.get_result, add
BasePaginator._compute_slice_bounds, and reference self.enable_advanced_features
for the negative-offset branch).
- Around line 874-882: The negative-offset pagination branch (check of
enable_advanced_features and cursor.offset < 0) allows client-controlled offsets
to access "beyond normal pagination bounds" and should be removed; delete the
special-case block that defines start_offset/stop and slices queryset with
negative offsets, instead enforce a non-negative offset by clamping
cursor.offset to 0 before computing limit/stop or raise a validation error for
negative offsets in the paginator input validation; locate the code in
src/sentry/api/paginator.py referencing enable_advanced_features, cursor.offset,
start_offset, and the queryset[start_offset:stop] slice and remove or replace
that branch so all pagination uses standard non-negative slicing and trusted
pre-filtered querysets.
- Around line 877-886: The branch using enable_advanced_features and
cursor.offset < 0 builds a negative start_offset and then does
queryset[start_offset:stop], which raises ValueError for Django QuerySets;
change the logic to avoid negative slicing by converting the negative offset
into a non-negative slice via an end-relative approach: e.g., detect
cursor.offset < 0, reverse the queryset (use queryset.reverse() or the
appropriate order_by inversion) so you can compute a positive start index (e.g.,
start_pos = max(0, computed_count + cursor.offset) or compute length-relative
slice using positive indices), perform a non-negative slice on the reversed
queryset, then re-reverse the results in Python if needed so ordering matches
original expectations; update the code paths that reference
enable_advanced_features, cursor.offset, start_offset, stop, and queryset
accordingly so no negative index is passed to queryset slicing.
In `@src/sentry/scripts/spans/add-buffer.lua`:
- Around line 46-53: The zunionstore calls in add-buffer.lua (the merges that
write into set_key from span_key and parent_key) are using Redis default SUM
aggregation which inflates scores when the same span.payload member exists in
both sets; change both redis.call("zunionstore", set_key, 2, set_key, span_key)
and redis.call("zunionstore", set_key, 2, set_key, parent_key) to include the
AGGREGATE MAX option so member scores use the maximum end_timestamp_precise
(preventing score inflation) and then keep the existing unlink/zpopmin behavior.
---
Nitpick comments:
In `@src/sentry/api/endpoints/organization_auditlogs.py`:
- Around line 68-91: The paginate call is duplicated; compute the paginator
class and optional kwargs first then call self.paginate once to avoid drift:
determine use_optimized and enable_advanced as in the snippet, set paginator_cls
= OptimizedCursorPaginator if use_optimized and enable_advanced else
DateTimePaginator, build an kwargs dict that includes order_by="-datetime" and
on_results=lambda x: serialize(x, request.user) and conditionally add
enable_advanced_features=True when using OptimizedCursorPaginator, then invoke
self.paginate(request=request, queryset=queryset, paginator_cls=paginator_cls,
**kwargs); this keeps the pagination call centralized (references:
self.paginate, OptimizedCursorPaginator, DateTimePaginator,
enable_advanced_features).
In `@src/sentry/api/paginator.py`:
- Around line 179-184: The comment above the start_offset calculation is
misleading: the code clamps negative offsets for non-previous cursors
(start_offset = max(0, offset)) rather than allowing them. Update the comment to
accurately describe the behavior (e.g., "Clamp negative offsets to 0 for forward
pagination; use raw offset for cursor.is_prev to allow backward pagination") or
remove the incorrect claim; refer to start_offset, offset, cursor.is_prev and
the queryset[start_offset:stop] slicing so the reader can locate and verify the
logic.
In `@src/sentry/utils/cursors.py`:
- Around line 26-28: The comment on Cursor's offset overstates functionality —
update the comment around Cursor (self.offset = int(offset)) to remove the claim
that this class “allows negative offsets for advanced pagination” and instead
state plainly that the attribute accepts negative integers but that
negative-offset traversal logic is implemented by callers (see paginator.py), or
simply drop the explanatory sentence; leave only the factual assignment and
minimal note that validation/behavior is handled by consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8beb5ef7-b321-405a-bc7b-c4aa197edf1c
📒 Files selected for processing (9)
src/sentry/api/endpoints/organization_auditlogs.pysrc/sentry/api/paginator.pysrc/sentry/scripts/spans/add-buffer.luasrc/sentry/spans/buffer.pysrc/sentry/spans/consumers/process/factory.pysrc/sentry/utils/cursors.pytests/sentry/spans/consumers/process/test_consumer.pytests/sentry/spans/consumers/process/test_flusher.pytests/sentry/spans/test_buffer.py
| 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: | ||
| # Use optimized paginator for high-performance audit log navigation | ||
| # This enables efficient browsing of large audit datasets with enhanced cursor support | ||
| response = self.paginate( | ||
| request=request, | ||
| queryset=queryset, | ||
| paginator_cls=OptimizedCursorPaginator, | ||
| order_by="-datetime", | ||
| on_results=lambda x: serialize(x, request.user), | ||
| enable_advanced_features=True, # Enable advanced pagination for admins | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether RpcUserOrganizationContext.member can be None
ast-grep --pattern 'class RpcUserOrganizationContext($$$):
$$$'
rg -nP -C3 '\bmember\b\s*[:=].*(Optional|None|\|\s*None)' --type=py -g '*organization*model*'Repository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 2887
Fix potential NoneType deref in enable_advanced (and reassess negative-offset behavior for optimized paginator)
enable_advanced = request.user.is_superuser or organization_context.member.has_global_accesscan dereferenceorganization_context.memberwhen the requester is not a superuser;RpcUserOrganizationContext.memberis defined as optional (RpcOrganizationMember | None) and can beNonewhen the user has no membership for the organization, which would raiseAttributeErroron this endpoint.- Confirm that the
OptimizedCursorPaginatorpath withenable_advanced_features=Truesafely handles negative cursor offsets (the referencedValueErrorlocation needs to match the currentOptimizedCursorPaginatorimplementation).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/endpoints/organization_auditlogs.py` around lines 70 - 83, The
current enable_advanced calculation can raise AttributeError because
organization_context.member may be None; change the check in the
organization_auditlogs endpoint to safely handle a missing member by computing
enable_advanced = request.user.is_superuser or (organization_context.member is
not None and organization_context.member.has_global_access), then only call
self.paginate(..., paginator_cls=OptimizedCursorPaginator,
enable_advanced_features=True) when enable_advanced is True; additionally,
review the OptimizedCursorPaginator implementation where it parses cursor
offsets to ensure negative offsets are either validated or converted to a
supported form (adjust validation/raise location in OptimizedCursorPaginator to
match current implementation) so the optimized path will not raise an unexpected
ValueError at runtime.
| 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]) | ||
|
|
||
| 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 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Essential refactor: OptimizedCursorPaginator.get_result is a near-verbatim duplicate of BasePaginator.get_result.
Apart from the negative-offset branch (877–886), this method copies BasePaginator.get_result line-for-line, including the boundary/is_prev/build_cursor/post_query_filter logic. This duplication will silently drift from the base implementation over time. If the negative-offset feature is kept (after fixing the Django slicing issue), override only the slice-bound computation rather than reimplementing the whole method — e.g. extract a _compute_slice_bounds(offset, limit, extra, is_prev) hook on the base class and override that.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/paginator.py` around lines 845 - 911,
OptimizedCursorPaginator.get_result largely duplicates BasePaginator.get_result;
refactor by extracting the slice-bound computation into a BasePaginator
protected hook (e.g., _compute_slice_bounds(self, offset, limit, extra,
is_prev)) and have BasePaginator.get_result call that hook to get (start_offset,
stop), then make OptimizedCursorPaginator override only _compute_slice_bounds to
implement the negative-offset logic when self.enable_advanced_features and
offset < 0 while delegating all other behavior to the base method (so remove the
near-verbatim get_result implementation in OptimizedCursorPaginator and keep
unique symbols: OptimizedCursorPaginator.get_result -> use
BasePaginator.get_result, add BasePaginator._compute_slice_bounds, and reference
self.enable_advanced_features for the negative-offset branch).
| # 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.
Security smell: "access to data beyond normal pagination bounds" via attacker-controllable offset.
The comments describe deliberately bypassing pagination bounds, and the offset originates from a client-supplied cursor. Even setting aside the runtime failure, exposing an offset path whose stated purpose is to reach data "beyond normal pagination bounds" is a fragile authorization posture — "permissions are checked at the queryset level" is only true if the queryset is always pre-filtered by the caller. Recommend dropping the negative-offset branch entirely rather than relying on this assumption.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/paginator.py` around lines 874 - 882, The negative-offset
pagination branch (check of enable_advanced_features and cursor.offset < 0)
allows client-controlled offsets to access "beyond normal pagination bounds" and
should be removed; delete the special-case block that defines start_offset/stop
and slices queryset with negative offsets, instead enforce a non-negative offset
by clamping cursor.offset to 0 before computing limit/stop or raise a validation
error for negative offsets in the paginator input validation; locate the code in
src/sentry/api/paginator.py referencing enable_advanced_features, cursor.offset,
start_offset, and the queryset[start_offset:stop] slice and remove or replace
that branch so all pagination uses standard non-negative slicing and trusted
pre-filtered querysets.
| 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]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Django QuerySet support negative indexing or negative slice start, or does it raise an error?
💡 Result:
Django QuerySet does not support negative indexing or negative slice start [1][2]. Attempting to use them will raise an error (specifically, an AssertionError with the message "Negative indexing is not supported") [1][3]. This is a long-standing design decision in the Django ORM [1]. Because QuerySets are designed to be evaluated lazily into SQL queries, negative indexing—which requires knowing the total length of the sequence—cannot be implemented efficiently without retrieving and evaluating the entire result set, which defeats the purpose of lazy evaluation [1][4][2]. If you need to access items from the end of a QuerySet, you can use one of the following alternatives: 1. Order the QuerySet in reverse using.order_by('-field') and then take the first few items [3]. 2. Use the.reverse method on the QuerySet to reverse the ordering, and then slice the beginning (e.g.,.reverse[:n]) [4][5]. 3. Use the.last method to retrieve the last object in the QuerySet [3][6][2]. 4. Convert the QuerySet to a list (e.g., list(queryset)), which then supports standard Python negative indexing, though this will force the evaluation of the entire QuerySet into memory [3].
Citations:
- 1: https://code.djangoproject.com/ticket/13089
- 2: https://docs.djangoproject.com/en/6.0/ref/models/querysets/
- 3: https://stackoverflow.com/questions/54393691/how-to-get-the-equalent-of-python-1-in-django-orm
- 4: http://code.djangoproject.com/ticket/5012
- 5: https://code.djangoproject.com/ticket/5012
- 6: https://code.djangoproject.com/ticket/32830
🌐 Web query:
Django QuerySet negative slice start error type and message when using queryset[-1:5] or queryset[-3:0]
💡 Result:
In Django, using negative indices or negative slice parameters (such as queryset[-1:5] or queryset[-3:0]) on a QuerySet is not supported [1]. Attempting to do so will raise a ValueError with the message: "Negative indexing is not supported." [1] This behavior is documented in the QuerySet implementation, where the getitem method explicitly checks for negative values in both single integer indices and slice objects (start or stop) [1]. Django does not support negative indexing because it is not possible to translate it efficiently into SQL queries without potentially pulling an entire result set into memory, which would be inefficient for large datasets [2][3]. To achieve the equivalent of negative slicing, you can use the following approaches: 1. Reverse the QuerySet: Use .reverse() before slicing if you need items from the end of the collection [3][4]. Note that .reverse() should typically be used on a QuerySet with a defined ordering [3]. 2. Convert to list: If the QuerySet is small and you specifically require Python's negative indexing behavior, you can cast the QuerySet to a list (e.g., list(queryset)[-1:5]), though this evaluates the entire QuerySet and brings all objects into memory [5][4]. 3. Alternative ORM methods: Depending on your goal, you may prefer using .last(), .latest(), or .order_by('-field') to retrieve the desired records directly through the database [5][4].
Citations:
- 1: https://docs.djangoproject.com/en/5.0/_modules/django/db/models/query/
- 2: http://code.djangoproject.com/ticket/5012
- 3: https://docs.djangoproject.com/en/2.2/ref/models/querysets/
- 4: https://stackoverflow.com/questions/5202726/how-to-get-the-first-element-and-the-last-element-using-django-location-object
- 5: https://stackoverflow.com/questions/54393691/how-to-get-the-equalent-of-python-1-in-django-orm
Critical: Negative QuerySet slicing will raise at runtime in advanced pagination (src/sentry/api/paginator.py:877-886)
In the enable_advanced_features and cursor.offset < 0 branch, start_offset = cursor.offset can be negative and the code executes results = list(queryset[start_offset:stop]). Django QuerySets don’t allow negative slice start (or negative indexing) and raise ValueError: "Negative indexing is not supported.", so this path will 500 any request that hits it.
- Replace this with an end-relative approach (e.g., reverse the ordering with
.reverse()/.order_by('-...')and use a non-negative slice) or otherwise ensure slicing bounds passed to the QuerySet are never negative.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/api/paginator.py` around lines 877 - 886, The branch using
enable_advanced_features and cursor.offset < 0 builds a negative start_offset
and then does queryset[start_offset:stop], which raises ValueError for Django
QuerySets; change the logic to avoid negative slicing by converting the negative
offset into a non-negative slice via an end-relative approach: e.g., detect
cursor.offset < 0, reverse the queryset (use queryset.reverse() or the
appropriate order_by inversion) so you can compute a positive start index (e.g.,
start_pos = max(0, computed_count + cursor.offset) or compute length-relative
slice using positive indices), perform a non-negative slice on the reversed
queryset, then re-reverse the results in Python if needed so ordering matches
original expectations; update the code paths that reference
enable_advanced_features, cursor.offset, start_offset, stop, and queryset
accordingly so no negative index is passed to queryset slicing.
| if not is_root_span and redis.call("zcard", span_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, span_key) | ||
| redis.call("unlink", span_key) | ||
| end | ||
|
|
||
| local parent_key = string.format("span-buf:s:{%s}:%s", project_and_trace, parent_span_id) | ||
| if set_span_id ~= parent_span_id and redis.call("scard", parent_key) > 0 then | ||
| redis.call("sunionstore", set_key, set_key, parent_key) | ||
| if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then | ||
| span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the whole file with line numbers if small, otherwise show relevant ranges.
FILE="src/sentry/scripts/spans/add-buffer.lua"
if [ -f "$FILE" ]; then
LINES=$(wc -l < "$FILE")
echo "## $FILE ($LINES lines)"
if [ "$LINES" -le 220 ]; then
cat -n "$FILE"
else
# focus on around the referenced lines and nearby context
sed -n '1,120p' "$FILE" | cat -n
echo "----"
sed -n '120,220p' "$FILE" | cat -n
echo "----"
sed -n '220,360p' "$FILE" | cat -n
fi
else
echo "Missing file: $FILE" >&2
exit 1
fi
# 2) Search for other ZUNIONSTORE/ZADD usage and any mention of "end_timestamp_precise"
rg -n "ZUNIONSTORE|zunionstore|ZADD|zadd|end_timestamp_precise|keep newest|newest|evict" "$FILE" || true
# 3) Find how span-buf keys are constructed/used elsewhere (limited scope).
# Use rg for the key prefixes to locate related scripts.
rg -n "span-buf:s:\{|span-buf:s:" src/sentry/scripts/spans || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 3304
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# list span scripts
fd -t f -e lua src/sentry/scripts/spans | head -n 50
# search for zadd into span-buf zsets (only within scripts/spans)
rg -n "zadd|ZADD|span-buf:s:\{" src/sentry/scripts/spans -S || true
# search for any other union/aggregate usage
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
# search for end_timestamp_precise / newest / eviction policy across spans scripts
rg -n "end_timestamp_precise|zpopmin|zpopmax|keep newest|newest|evict" src/sentry/scripts/spans -S || true
# locate any Lua script that writes to span-buf:s keys
rg -n "span-buf:s" src/sentry/scripts/spans -S || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -e lua src/sentry/scripts/spans | head -n 200
rg -n "zadd|ZADD|span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
rg -n "end_timestamp_precise|zpopmin|zpopmax|keep newest|newest|evict" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show lua span scripts
echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort
echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true
echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort
echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true
echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== span lua files ==="
fd -t f -e lua src/sentry/scripts/spans | sort
echo "=== zadd usage to span-buf zsets ==="
rg -n "zadd|ZADD" src/sentry/scripts/spans -S || true
rg -n "span-buf:s:\{" src/sentry/scripts/spans -S || true
rg -n "span-buf:s" src/sentry/scripts/spans -S || true
echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" src/sentry/scripts/spans -S || true
echo "=== eviction/newest ordering hints ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" src/sentry/scripts/spans -S || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SPAN_DIR="src/sentry/scripts/spans"
echo "=== span lua files (under $SPAN_DIR) ==="
fd -t f -e lua . "$SPAN_DIR" | sort || true
echo "=== zadd usage into span-buf zsets ==="
rg -n "zadd|ZADD" "$SPAN_DIR" -S --glob='*.lua' || true
rg -n "span-buf:s:\{" "$SPAN_DIR" -S --glob='*.lua' || true
echo "=== unionstore usage ==="
rg -n "zunionstore|ZUNIONSTORE" "$SPAN_DIR" -S --glob='*.lua' || true
echo "=== eviction/newest ordering hints (span scripts) ==="
rg -n "zpopmin|zpopmax|end_timestamp_precise|newest|evict" "$SPAN_DIR" -S --glob='*.lua' || true
echo "=== any span-buf zset writes under src/sentry/scripts (broader) ==="
rg -n "span-buf:s:\{|zadd|ZADD" "src/sentry/scripts" -S --glob='*.lua' || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 3075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# broader search in repo for span-buf zset writes and member/score patterns
rg -n --hidden --glob='*.lua' "span-buf:s:" src/sentry/scripts || true
rg -n --hidden --glob='*.lua' "span-buf:s:" . || true
# look for zadd/zadd usage near span-buf keys
rg -n --glob='*.lua' "zadd|ZADD" src/sentry/scripts | head -n 200 || true
rg -n --glob='*.lua' "zadd|ZADD" . | rg "span-buf" || true
# also look for scripts that reference "span-buf:sr" (redirect hash) or "span-buf:hrs" (root flag)
rg -n --glob='*.lua' "span-buf:sr|span-buf:hrs" src/sentry/scripts/spans src/sentry/scripts || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 2932
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== occurrences of span-buf:s across repo (any file) ==="
rg -n --hidden --glob='*' "span-buf:s" . | head -n 200 || true
echo "=== occurrences of span-buf across repo ==="
rg -n --hidden --glob='*' "span-buf" . | head -n 200 || true
echo "=== zadd/ZADD calls mentioning span-buf (any file) ==="
rg -n --hidden --glob='*' "(zadd|ZADD).*(span-buf|span-buf:s)" . || true
rg -n --hidden --glob='*' "(span-buf|span-buf:s).*(zadd|ZADD)" . || true
echo "=== zadd/ZADD calls near span-buf:s within a wider text window ==="
rg -n --hidden --glob='*' "span-buf:s" . | cut -d: -f1 | sort -u | while read -r f; do
echo "--- file: $f ---"
sed -n '1,200p' "$f" | rg -n "zadd|ZADD|span-buf" || true
doneRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 5627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/sentry/spans/buffer.py"
wc -l "$FILE"
sed -n '150,260p' "$FILE" | cat -n
sed -n '260,360p' "$FILE" | cat -n
sed -n '430,520p' "$FILE" | cat -nRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 13300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# find Span class / payload construction
rg -n --glob='*.py' "class Span|def payload|payload\s*=" src/sentry/spans src/sentry || true
rg -n --glob='*.py' "end_timestamp_precise" src/sentry/spans src/sentry || true
rg -n --glob='*.py' "span_id" src/sentry/spans/buffer.py src/sentry/spans -S || true
# show relevant section around the first match for Span.payload
PYFILE=$(rg -n --glob='*.py' "class Span" src/sentry/spans src/sentry | head -n 1 | cut -d: -f1 || true)
echo "Span file guess: $PYFILE"
if [ -n "${PYFILE:-}" ] && [ -f "$PYFILE" ]; then
sed -n '1,220p' "$PYFILE" | cat -n
fiRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 33286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/sentry/spans/buffer.py"
# show Span NamedTuple definition + payload comments
sed -n '80,170p' "$FILE" | cat -n
# find where Span.payload or "span.payload" is set/derived
rg -n "span\.payload|payload\s*=" "$FILE" || true
# show a window around the first occurrence of "span\.payload" within buffer.py
LINE=$(rg -n "span\.payload" "$FILE" | head -n 1 | cut -d: -f1 || true)
if [ -n "${LINE:-}" ]; then
START=$((LINE-40))
END=$((LINE+80))
sed -n "${START},${END}p" "$FILE" | cat -n
fiRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 9678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call sites of process_spans / SpansBuffer
rg -n --glob='*.py' "process_spans\(|SpansBuffer\(" src/sentry | head -n 200 || true
rg -n --glob='*.py' "SpansBuffer\(" src/sentry | head -n 200 || true
rg -n --glob='*.py' "\.process_spans\(" src/sentry | head -n 200 || true
# Find where Span(NamedTuple) instances are created (likely alias to sentry.spans.buffer.Span)
rg -n --glob='*.py' "SpansBuffer\.|add_buffer_script|add-buffer\.lua" src/sentry | head -n 200 || true
# Find "Span(" where payload/end_timestamp_precise are set
rg -n --glob='*.py' "end_timestamp_precise\s*=" src/sentry/spans | head -n 200 || true
rg -n --glob='*.py' "payload\s*[:=]" src/sentry/spans | head -n 200 || true
# Specifically inspect process_segments or consume code that feeds buffer
rg -n --glob='*.py' "span\.payload|end_timestamp_precise|process_spans" src/sentry/spans/consumers | head -n 200 || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 3736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# search for add-buffer.lua or zunionstore expectations in tests
rg -n --glob='*.py' "add-buffer\.lua|add_buffer|zunionstore|zunionstore|ZUNIONSTORE|span-buf:s" tests | head -n 200 || true
rg -n --glob='*.py' "span-buf:s" tests/sentry/spans | head -n 200 || true
# show relevant test_buffer.py contents if not too large
FILE="tests/sentry/spans/test_buffer.py"
if [ -f "$FILE" ]; then
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
sed -n '220,520p' "$FILE" | cat -n
fiRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 19698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/sentry/spans/test_buffer.py"
rg -n "process_spans\(" "$FILE" || true
rg -n "process_spans\([^#\n]*\).*process_spans\(" "$FILE" || true
# look for patterns calling buffer.process_spans directly
rg -n "buffer\.process_spans" "$FILE" || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob='*.py' "buffer\.process_spans|process_spans\(" tests/sentry/spans | head -n 200 || true
rg -n --glob='*.py' "process_spans\(" tests | head -n 200 || true
rg -n --glob='*.py' "add-buffer\.lua|add_buffer_script" tests | head -n 100 || trueRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 1740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="tests/sentry/spans/consumers/process/test_flusher.py"
wc -l "$FILE"
sed -n '1,220p' "$FILE" | cat -n
sed -n '220,520p' "$FILE" | cat -nRepository: test-code-tool/sentry__sentry-greptile__codehawk__PR2__20260407
Length of output: 3098
Make span-buf ZSET merges idempotent (prevent score inflation).
span-buf:s:* zsets are created with member=span.payload and score=span.end_timestamp_precise, and add-buffer.lua evicts by zpopmin (smallest score). The Lua script merges via ZUNIONSTORE without an AGGREGATE, so Redis defaults to SUM; if span_key/parent_key is recreated during replay and contains members already present in set_key, their scores get summed, skewing ordering and eviction. Use AGGREGATE MAX for both unionstore calls.
Suggested fix
-if not is_root_span and redis.call("zcard", span_key) > 0 then
- span_count = redis.call("zunionstore", set_key, 2, set_key, span_key)
+if not is_root_span and redis.call("zcard", span_key) > 0 then
+ span_count = redis.call("zunionstore", set_key, 2, set_key, span_key, "AGGREGATE", "MAX")
redis.call("unlink", span_key)
end
@@
-if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then
- span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key)
+if set_span_id ~= parent_span_id and redis.call("zcard", parent_key) > 0 then
+ span_count = redis.call("zunionstore", set_key, 2, set_key, parent_key, "AGGREGATE", "MAX")
redis.call("unlink", parent_key)
end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sentry/scripts/spans/add-buffer.lua` around lines 46 - 53, The
zunionstore calls in add-buffer.lua (the merges that write into set_key from
span_key and parent_key) are using Redis default SUM aggregation which inflates
scores when the same span.payload member exists in both sets; change both
redis.call("zunionstore", set_key, 2, set_key, span_key) and
redis.call("zunionstore", set_key, 2, set_key, parent_key) to include the
AGGREGATE MAX option so member scores use the maximum end_timestamp_precise
(preventing score inflation) and then keep the existing unlink/zpopmin behavior.
2 participants
Test 2
Summary by CodeRabbit
New Features
Performance Improvements