Optimize spans buffer insertion with eviction during insert#9
Optimize spans buffer insertion with eviction during insert#9ShashankFC 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>
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # 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 |
There was a problem hiding this comment.
Suggestion: Accessing organization_context.member.has_global_access without checking that member is present can cause an AttributeError for superusers or other callers where the organization context has no member attached, breaking the endpoint before pagination is applied. [possible bug]
Severity Level: Critical 🚨
- ❌ Organization audit logs endpoint 500s for org auth tokens.
- ❌ Optimized audit log pagination unusable for service integrations.
- ⚠️ Breaks non-member access patterns relying on org:write scopes.| enable_advanced = request.user.is_superuser or organization_context.member.has_global_access | |
| member = getattr(organization_context, "member", None) | |
| enable_advanced = request.user.is_superuser or ( | |
| member is not None and getattr(member, "has_global_access", False) | |
| ) |
Steps of Reproduction ✅
1. Note from `src/sentry/api/urls.py:1425` (Grep output) that
`OrganizationAuditLogsEndpoint.as_view()` is registered as an organization-scoped API
endpoint, so its `get()` method in
`src/sentry/api/endpoints/organization_auditlogs.py:42-93` is reachable via the API.
2. In `src/sentry/api/bases/organization.py:11-25`, `OrganizationAuditPermission` (used by
this endpoint at line 40) defines `scope_map = {"GET": ["org:write"]}` and
`has_object_permission()` allows access for active superusers and for callers with the
required scope (e.g. organization auth tokens, as wired through `ScopedPermission` and
`is_org_auth_token_auth` in `src/sentry/api/permissions.py:9-22, 70-121`).
3. In `src/sentry/api/permissions.py:148-193`, `determine_access()` constructs a
`RpcUserOrganizationContext` via `organization_service.get_organization_by_id(...)` and
attaches it as `org_context`; the model definition in
`src/sentry/organizations/services/organization/model.py:20-27, 331-38` explicitly
documents `member: RpcOrganizationMember | None = None` and states that `member` can be
`None` when there is no user membership (for example, org auth token access where
`user_id` is not provided).
4. Call the audit logs endpoint as a non-superuser principal with `org:write` scope but no
organization membership (e.g. an organization auth token) and include
`?optimized_pagination=true` in the query string so that in
`OrganizationAuditLogsEndpoint.get()` at `organization_auditlogs.py:68-73` the code
executes `use_optimized = True` and then evaluates `enable_advanced =
request.user.is_superuser or organization_context.member.has_global_access`; since
`request.user.is_superuser` is `False` for this auth path and
`organization_context.member` is `None` per the model contract above, Python attempts to
access `None.has_global_access` and raises `AttributeError: 'NoneType' object has no
attribute 'has_global_access'`, causing a 500 response before pagination or serialization.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/sentry/api/endpoints/organization_auditlogs.py
**Line:** 71:71
**Comment:**
*Possible Bug: Accessing `organization_context.member.has_global_access` without checking that `member` is present can cause an AttributeError for superusers or other callers where the organization context has no member attached, breaking the endpoint before pagination is applied.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| # 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.
Suggestion: Allowing negative offsets to be passed directly into Django queryset slicing will raise a runtime AssertionError, so the advanced pagination branch in OptimizedCursorPaginator should clamp negative offsets to 0 (or otherwise reject them) instead of slicing with a negative index. [logic error]
Severity Level: Major ⚠️
- ❌ Optimized organization audit logs 500 on negative cursor.
- ⚠️ Any future OptimizedCursorPaginator users risk same crash.
- ⚠️ Reduces robustness of admin-only audit log navigation.| # 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]) | |
| # Note: Django ORM slicing does not support negative offsets, so clamp them to 0. | |
| if self.enable_advanced_features and cursor.offset < 0: | |
| # Treat negative offsets as starting from the beginning to avoid invalid queryset slicing | |
| start_offset = 0 |
Steps of Reproduction ✅
1. Start the Sentry backend with the PR code so that `OrganizationAuditLogsEndpoint` is
available (defined in `src/sentry/api/endpoints/organization_auditlogs.py:34-47`) and
imports `OptimizedCursorPaginator` from `src/sentry/api/paginator.py:821`.
2. Authenticate as a user who satisfies the advanced pagination condition in
`OrganizationAuditLogsEndpoint.get()` at `organization_auditlogs.py:68-73` (either
`request.user.is_superuser` or `organization_context.member.has_global_access` evaluates
to True).
3. Issue a GET request to the audit logs endpoint registered as
`"sentry-api-0-organization-audit-logs"` (see
`tests/sentry/api/endpoints/test_organization_auditlogs.py:15`) with query parameters
`optimized_pagination=true` (so `use_optimized` is True and `OptimizedCursorPaginator` is
selected at lines 73-83) and a crafted negative-offset cursor, for example
`cursor=0:-10:0`. The cursor string is parsed by `Cursor.from_string` in
`src/sentry/utils/cursors.py:52-59`, which constructs `Cursor(value=0, offset=-10,
is_prev=0)`.
4. Inside `OptimizedCursorPaginator.get_result()` in
`src/sentry/api/paginator.py:845-882`, `enable_advanced_features=True` (passed from
`OrganizationAuditLogsEndpoint.get()` at line 82) and `cursor.offset` is -10, so the
branch at lines 874-882 is taken, setting `start_offset = cursor.offset` and executing
`queryset[start_offset:stop]` with a negative slice start. Django QuerySet slicing does
not support negative slice indices and raises an `AssertionError`, causing this audit log
request to error with HTTP 500.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/sentry/api/paginator.py
**Line:** 876:882
**Comment:**
*Logic Error: Allowing negative offsets to be passed directly into Django queryset slicing will raise a runtime AssertionError, so the advanced pagination branch in `OptimizedCursorPaginator` should clamp negative offsets to 0 (or otherwise reject them) instead of slicing with a negative index.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
2 participants
User description
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
CodeAnt-AI Description
Use timestamp-scored span buffer with eviction and enable optimized audit-log pagination for admins
What Changed
Impact
✅ Fewer oversized span segments during ingestion✅ Deterministic eviction of oldest spans✅ Faster audit log browsing for admins💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.