-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add ability to disable data export #1757
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1757 +/- ##
==========================================
+ Coverage 85.42% 85.44% +0.01%
==========================================
Files 464 464
Lines 10818 10839 +21
Branches 1601 1604 +3
==========================================
+ Hits 9241 9261 +20
Misses 860 860
- Partials 717 718 +1
|
stop() | ||
|
||
// delete any persisted data | ||
runCatching { |
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.
Should we do the delete even if the SDK hasn't started yet?
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.
Yep, this sounds reasonable.
* The SDK makes a best effort attempt. Some data capture/handlers may remain active until the next process launch | ||
* due to technical reasons, but any captured data will not be exported. | ||
*/ | ||
public fun disableDataExport() |
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.
Should we also disable session caching? Or perhaps just change the storage components to not actually persist any new payloads to disk? Trying to figure out if there are areas that might lead to writing any new data to disk.
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.
Calling stop()
shuts down all the workers which effectively stops all future data persistence. I can add a comment that makes this clearer
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 nice!
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.
LGTM
9e78ef9
to
338f190
Compare
338f190
to
a62357c
Compare
Goal
Adds a
disableDataExport
function to our public API that prevents exporting data via OTel exporters or HTTP requests. This callsstop()
which shuts down all executors internally, and deletes any persisted data/blocks OTel exporters from exporting more data.The intention is that an app would call this function if a user opted out of tracking halfway through a session, and on the next launch they would conditionally initialize the Embrace SDK.
Testing
Added an integration test & ran manual tests in example app.