-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(Replay): Add Flag for EAP Query for Replay Details Page #103278
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103278 +/- ##
===========================================
+ Coverage 80.69% 80.73% +0.04%
===========================================
Files 9225 9232 +7
Lines 393916 394543 +627
Branches 25109 25109
===========================================
+ Hits 317887 318552 +665
+ Misses 75581 75543 -38
Partials 448 448 |
| if features.has("organizations:replay-details-eap-query", organization): | ||
| snuba_response = query_replay_instance_eap( | ||
| project_ids=project_ids, | ||
| replay_ids=[replay_id], | ||
| start=filter_params["start"], | ||
| end=filter_params["end"], | ||
| organization_id=organization.id, | ||
| request_user_id=request.user.id, | ||
| )["data"] |
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: Type mismatch in query_replay_instance_eap return value handling could cause TypeError if eap_read.query() returns a list directly.
Severity: MEDIUM | Confidence: 0.70
🔍 Detailed Analysis
The query_replay_instance_eap function assumes that eap_read.query() returns a dictionary containing a "data" key, which is then accessed as snuba_response = query_replay_instance_eap(...)["data"]. However, the non-EAP path, query_replay_instance, already unwraps the data by returning execute_query(...)["data"]. This inconsistency means that if eap_read.query() returns a list directly instead of a dictionary with a "data" key, accessing ["data"] will result in a TypeError at runtime when process_raw_response is called.
💡 Suggested Fix
Verify the return structure of eap_read.query() from src/sentry/replays/lib/eap/read.py. If it returns a list directly, modify line 171 to remove the ["data"] access.
🤖 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/replays/endpoints/organization_replay_details.py#L163-L171
Potential issue: The `query_replay_instance_eap` function assumes that
`eap_read.query()` returns a dictionary containing a `"data"` key, which is then
accessed as `snuba_response = query_replay_instance_eap(...)["data"]`. However, the
non-EAP path, `query_replay_instance`, already unwraps the data by returning
`execute_query(...)["data"]`. This inconsistency means that if `eap_read.query()`
returns a list directly instead of a dictionary with a `"data"` key, accessing
`["data"]` will result in a `TypeError` at runtime when `process_raw_response` is
called.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2662889
| end: datetime, | ||
| organization_id: int, | ||
| request_user_id: int, | ||
| request_user_id: int | None, |
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.
When can this be None?
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.
ah, i initially thought the request.user.id could have been null, but after looking further it seems we don't need this and i can just make it int since authenticated users should always have the id. i've updated it!
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.
request.user.id can still be none when request.user is an AnonymousUser, which happens when we query the api from a script for example
In this previous PR, a new helper function for querying EAP for replay details given a replay ID was added to the organization details file. This PR calls this new function behind a new feature flag organizations:replay-details-eap-query.
Relates To: https://linear.app/getsentry/issue/REPLAY-825/add-flag-for-reading-from-eap-for-replay-details