Skip to content
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

feat(replays): Delete videos on replay delete request #68463

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Apr 8, 2024

If we did not delete this the videos would remain in storage until the retention-period expires.

Relates to: #63255

@cmanallen cmanallen requested a review from a team as a code owner April 8, 2024 21:09
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 8, 2024
Copy link

codecov bot commented Apr 8, 2024

Bundle Report

Changes will decrease total bundle size by 770 bytes ⬇️

Bundle name Size Change
sentry-webpack-bundle-array-push 26.17MB 770 bytes ⬇️

if direct_storage_segments:
with cf.ThreadPoolExecutor(max_workers=100) as pool:
with cf.ThreadPoolExecutor(max_workers=100) as pool:
pool.map(_delete_if_exists, video_filenames)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we guard this with the video replay feature flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its necessary. If you have a video it deletes. If you don't it does nothing.

Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no way to know if a replay segment is a video or a rrweb payload from segment metadata?

@cmanallen
Copy link
Member Author

@JoshFerge It would require downloading and parsing the RRWeb.

@JoshFerge
Copy link
Member

@JoshFerge It would require downloading and parsing the RRWeb.

got it. how does the replay UI for example know whether or not a replay is a mobile replay or a rrweb replay?

@cmanallen
Copy link
Member Author

@JoshFerge I believe they download the RRWeb first and then fetch the video once they know its a mobile replay.

@JoshFerge
Copy link
Member

@JoshFerge I believe they download the RRWeb first and then fetch the video once they know its a mobile replay.

got it, makes sense.

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 79.72%. Comparing base (229a26a) to head (ed9f9c9).
Report is 38 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #68463       +/-   ##
===========================================
+ Coverage   66.44%   79.72%   +13.28%     
===========================================
  Files        6417     6422        +5     
  Lines      284175   284646      +471     
  Branches    48979    49034       +55     
===========================================
+ Hits       188808   226942    +38134     
+ Misses      94994    57337    -37657     
+ Partials      373      367        -6     
Files Coverage Δ
src/sentry/replays/tasks.py 96.07% <83.33%> (+53.22%) ⬆️

... and 1577 files with indirect coverage changes

@cmanallen cmanallen merged commit 4221751 into master Apr 15, 2024
48 of 49 checks passed
@cmanallen cmanallen deleted the cmanallen/replays-delete-videos branch April 15, 2024 19:07
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants