-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
ref(replay): move summary log parsing to a seer rpc #99547
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
| SEER_START_TASK_ENDPOINT_PATH, | ||
| { | ||
| "logs": [], | ||
| "use_rpc": True, |
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.
Used to toggle code paths in seer (default false)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #99547 +/- ##
=======================================
Coverage 81.20% 81.20%
=======================================
Files 8563 8563
Lines 380233 380265 +32
Branches 23953 23953
=======================================
+ Hits 308773 308804 +31
- Misses 71109 71110 +1
Partials 351 351 |
| if features.has( | ||
| "organizations:replay-ai-summaries-rpc", project.organization, actor=request.user | ||
| ): | ||
| start, end = default_start_end_dates() |
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.
deciding to not support date params, imo it's not necessary. We don't pass these in from the frontend atm
| "organizations:replay-ai-summaries-rpc", project.organization, actor=request.user | ||
| ): | ||
| start, end = default_start_end_dates() | ||
| snuba_response = query_replay_instance( |
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.
isn't the query_replace_instance call in the other (summarize.py) file already?
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 guess i'm confused why we have this call + also the other one
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 wanted to 404 early if the replay's missing, instead of making a seer task + db entry
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.
Colton is working on a more efficient existence check but hope this will do til then. Can test with the FF
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.
ohhh ok makes sense
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
Currently segment downloads and log parsing are done synchronously, before queueing a summary task in Seer. This PR moves that work to a RPC call which will be called by the task, allowing project_replay_summary endpoint to be fully async/return immediately.
Changes in both services can be rolled out together with a new FF.
Part of REPLAY-708: Move POST endpoint log parsing to Seer->Sentry RPC