-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(replays): Add data export #100778
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): Add data export #100778
Conversation
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
|
||
def save_to_storage(destination_bucket: str, filename: str, contents: str) -> None: | ||
storage = get_storage(None) | ||
assert isinstance(storage, GoogleCloudStorage) |
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.
Potential bug: The save_to_storage
function incorrectly asserts the storage backend is GoogleCloudStorage
, which will cause a crash if the default filesystem or S3 storage is used.
-
Description: The
save_to_storage
function, used for replay data exports, includes an assertion that the storage backend must be an instance ofGoogleCloudStorage
. However, the default storage backend for Sentry isfilesystem
, and it can also be configured to use other backends like S3. When the replay data export feature is triggered in an environment not using Google Cloud Storage, thisisinstance
check will fail, raising anAssertionError
and crashing the background task handling the export. This will break the data export functionality, which is a compliance-related feature, in all non-GCS deployments. -
Suggested fix: Remove the hard assertion
assert isinstance(storage, GoogleCloudStorage)
. The function should either be made storage-agnostic or include logic to handle different storage backends. Alternatively, add configuration validation to ensure this feature is only used when Google Cloud Storage is the configured filestore backend.
severity: 0.85, confidence: 0.98
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.
We want it to crash.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #100778 +/- ##
=========================================
Coverage 81.13% 81.13%
=========================================
Files 8607 8609 +2
Lines 381775 381945 +170
Branches 24015 24015
=========================================
+ Hits 309743 309897 +154
- Misses 71705 71721 +16
Partials 327 327 |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
Added data-export for Session Replay to comply with EU data-portability regulations.