-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(eap): Sets up double deletion of occurrences with EAP #101385
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
base: master
Are you sure you want to change the base?
Conversation
logger = logging.getLogger(__name__) | ||
|
||
|
||
def delete_groups_from_eap_rpc( |
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.
Would like opinions on how to organize this code, e.g. in a separate file as drafted or alongside the Snuba eventstream impl
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 think having a general eventstream/eap.py
file to complement the existing eventstream/snuba.py
file would fit the existing structure best.
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.
More general question: do we want to keep the base EventStream
interface for the EAP implementation? Maybe a good discussion topic for our sync tomorrow.
from sentry.testutils.cases import TestCase | ||
|
||
|
||
class TestEAPDeletion(TestCase): |
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.
Basic tests for now, to be improved later
filter=combined_filter, | ||
) | ||
|
||
request = DeleteTraceItemsRequest( |
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.
Do the relevant traces have associated sentry.group_id
s? I had thought we would need to explicitly pull the trace IDs from the group and delete that way, but would be happy to hear that it does work this way.
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 see — we're filtering to TraceItems, not to traces themselves. (Comment)
Looks good!
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.
Yeah — I see that delete using filters is not yet supported in the endpoint implementation, but my assumption was that it eventually will be.
@onewland could you confirm?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101385 +/- ##
===========================================
+ Coverage 81.02% 81.05% +0.02%
===========================================
Files 8701 8678 -23
Lines 386084 385268 -816
Branches 24409 24247 -162
===========================================
- Hits 312837 312285 -552
+ Misses 72896 72629 -267
- Partials 351 354 +3 |
Sets up double deletion of occurrences with EAP, as described in ID-997.
When deleting events from eventstore, we should also delete from EAP using the in-progress
DeleteTraceItems
endpoint (protobuf definition).