-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(explorer): change transactions ranking #102966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| result = Spans.run_table_query( | ||
| params=snuba_params, | ||
| query_string=f"is_transaction:true project.id:{project_id}", | ||
| query_string="is_transaction:true", | ||
| selected_columns=[ | ||
| "transaction", | ||
| "count()", | ||
| "sum(span.duration)", | ||
| ], | ||
| orderby=["-count()"], # Sort by count descending (highest volume first) | ||
| orderby=["-sum(span.duration)"], | ||
| offset=0, | ||
| limit=limit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: get_transactions_for_project() lacks an explicit project.id filter in its query_string, potentially returning data from all projects.
Severity: CRITICAL | Confidence: 0.95
🔍 Detailed Analysis
The get_transactions_for_project() function constructs a query_string="is_transaction:true" without explicitly including project.id:{project_id}. Despite snuba_params containing projects=[project], this metadata is not automatically applied as a server-side filter in the RPC layer, as indicated by similar filtering requirements in SearchResolver.resolve_query(). This results in the function returning transactions from all projects in the organization, rather than being scoped to the specified project_id.
💡 Suggested Fix
Modify the query_string in get_transactions_for_project() to explicitly include project.id:{project_id} to ensure results are correctly scoped.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/seer/explorer/index_data.py#L79-L88
Potential issue: The `get_transactions_for_project()` function constructs a
`query_string="is_transaction:true"` without explicitly including
`project.id:{project_id}`. Despite `snuba_params` containing `projects=[project]`, this
metadata is not automatically applied as a server-side filter in the RPC layer, as
indicated by similar filtering requirements in `SearchResolver.resolve_query()`. This
results in the function returning transactions from all projects in the organization,
rather than being scoped to the specified `project_id`.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe this is wrong because the snuba params has the project filter
Instead of picking the top transactions for a project by volume, pick them by total time spent. This mirrors what's done on the Insights page and seems to select the more meaningful parts of the app (rather than stuff like health checks and middleware for example).
Instead of picking the top transactions for a project by volume, pick them by total time spent. This mirrors what's done on the Insights page and seems to select the more meaningful parts of the app (rather than stuff like health checks and middleware for example).
Instead of picking the top transactions for a project by volume, pick them by total time spent. This mirrors what's done on the Insights page and seems to select the more meaningful parts of the app (rather than stuff like health checks and middleware for example).