-
-
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
Draft
shashjar
wants to merge
7
commits into
master
Choose a base branch
from
id-997-double-deletions-with-eap
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+224
−3
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
8095794
Add double deletion of occurrences with EAP
shashjar 184b394
Add tests for EAP double deletion logic
shashjar c22bd21
Merge branch 'master' into id-997-double-deletions-with-eap
shashjar 521a963
Change project allowlist to organization allowlist for EAP deletion
shashjar a816dfe
Merge branch 'master' into id-997-double-deletions-with-eap
shashjar 5589ba5
Rename `eventstream/eap_delete.py` to `eventstream/eap.py`
shashjar f4a60ab
Merge branch 'master' into id-997-double-deletions-with-eap
shashjar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
from __future__ import annotations | ||
|
||
import logging | ||
from collections.abc import Sequence | ||
|
||
from sentry_protos.snuba.v1.endpoint_delete_trace_items_pb2 import ( | ||
DeleteTraceItemsRequest, | ||
DeleteTraceItemsResponse, | ||
) | ||
from sentry_protos.snuba.v1.request_common_pb2 import ( | ||
TRACE_ITEM_TYPE_OCCURRENCE, | ||
RequestMeta, | ||
TraceItemFilterWithType, | ||
) | ||
from sentry_protos.snuba.v1.trace_item_attribute_pb2 import AttributeKey, AttributeValue, IntArray | ||
from sentry_protos.snuba.v1.trace_item_filter_pb2 import ( | ||
AndFilter, | ||
ComparisonFilter, | ||
TraceItemFilter, | ||
) | ||
|
||
from sentry.utils import snuba_rpc | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def delete_groups_from_eap_rpc( | ||
organization_id: int, | ||
project_id: int, | ||
group_ids: Sequence[int], | ||
referrer: str = "deletions.group", | ||
) -> DeleteTraceItemsResponse: | ||
""" | ||
Delete occurrences from EAP for the given group IDs. | ||
""" | ||
|
||
if not group_ids: | ||
raise ValueError("group_ids must not be empty") | ||
|
||
project_filter = _create_project_filter(project_id) | ||
group_id_filter = _create_group_id_filter(list(group_ids)) | ||
combined_filter = TraceItemFilter( | ||
and_filter=AndFilter(filters=[project_filter, group_id_filter]) | ||
) | ||
filter_with_type = TraceItemFilterWithType( | ||
item_type=TRACE_ITEM_TYPE_OCCURRENCE, | ||
shashjar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
filter=combined_filter, | ||
) | ||
|
||
request = DeleteTraceItemsRequest( | ||
shashjar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
meta=RequestMeta( | ||
organization_id=organization_id, | ||
project_ids=[project_id], | ||
referrer=referrer, | ||
cogs_category="deletions", | ||
), | ||
filters=[filter_with_type], | ||
) | ||
response = snuba_rpc.rpc(request, DeleteTraceItemsResponse) | ||
|
||
return response | ||
|
||
|
||
def _create_project_filter(project_id: int) -> TraceItemFilter: | ||
return TraceItemFilter( | ||
comparison_filter=ComparisonFilter( | ||
key=AttributeKey( | ||
type=AttributeKey.TYPE_INT, | ||
name="sentry.project_id", | ||
), | ||
op=ComparisonFilter.OP_EQUALS, | ||
value=AttributeValue(val_int=project_id), | ||
) | ||
) | ||
|
||
|
||
def _create_group_id_filter(group_ids: list[int]) -> TraceItemFilter: | ||
return TraceItemFilter( | ||
comparison_filter=ComparisonFilter( | ||
key=AttributeKey( | ||
type=AttributeKey.TYPE_INT, | ||
name="sentry.group_id", | ||
), | ||
op=ComparisonFilter.OP_IN, | ||
value=AttributeValue(val_int_array=IntArray(values=group_ids)), | ||
) | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
from unittest.mock import patch | ||
|
||
import pytest | ||
from sentry_protos.snuba.v1.endpoint_delete_trace_items_pb2 import DeleteTraceItemsResponse | ||
from sentry_protos.snuba.v1.request_common_pb2 import TRACE_ITEM_TYPE_OCCURRENCE, ResponseMeta | ||
|
||
from sentry.deletions.tasks.nodestore import delete_events_from_eap | ||
from sentry.eventstream.eap import delete_groups_from_eap_rpc | ||
from sentry.snuba.dataset import Dataset | ||
from sentry.testutils.cases import TestCase | ||
|
||
|
||
class TestEAPDeletion(TestCase): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basic tests for now, to be improved later |
||
def setUp(self): | ||
self.organization_id = 1 | ||
self.project_id = 123 | ||
self.group_ids = [1, 2, 3] | ||
|
||
@patch("sentry.eventstream.eap.snuba_rpc.rpc") | ||
def test_deletion_with_error_dataset(self, mock_rpc): | ||
mock_rpc.return_value = DeleteTraceItemsResponse( | ||
meta=ResponseMeta(), | ||
matching_items_count=150, | ||
) | ||
|
||
with self.options( | ||
{"eventstream.eap.deletion_enabled.organization_allowlist": [self.organization_id]} | ||
): | ||
delete_events_from_eap( | ||
self.organization_id, self.project_id, self.group_ids, Dataset.Events | ||
) | ||
|
||
assert mock_rpc.call_count == 1 | ||
|
||
request = mock_rpc.call_args[0][0] | ||
assert request.meta.organization_id == self.organization_id | ||
assert request.meta.project_ids == [self.project_id] | ||
assert request.meta.referrer == "deletions.group.eap" | ||
assert request.meta.cogs_category == "deletions" | ||
|
||
assert len(request.filters) == 1 | ||
assert request.filters[0].item_type == TRACE_ITEM_TYPE_OCCURRENCE | ||
|
||
@patch("sentry.eventstream.eap.snuba_rpc.rpc") | ||
def test_multiple_group_ids(self, mock_rpc): | ||
mock_rpc.return_value = DeleteTraceItemsResponse( | ||
meta=ResponseMeta(), | ||
matching_items_count=500, | ||
) | ||
|
||
many_group_ids = [10, 20, 30, 40, 50] | ||
|
||
with self.options( | ||
{"eventstream.eap.deletion_enabled.organization_allowlist": [self.organization_id]} | ||
): | ||
delete_events_from_eap( | ||
self.organization_id, self.project_id, many_group_ids, Dataset.Events | ||
) | ||
|
||
request = mock_rpc.call_args[0][0] | ||
group_filter = request.filters[0].filter.and_filter.filters[1] | ||
assert list(group_filter.comparison_filter.value.val_int_array.values) == many_group_ids | ||
|
||
@patch("sentry.eventstream.eap.snuba_rpc.rpc") | ||
def test_organization_not_in_allowlist_skips_deletion(self, mock_rpc): | ||
with self.options({"eventstream.eap.deletion_enabled.organization_allowlist": [456, 789]}): | ||
delete_events_from_eap( | ||
self.organization_id, self.project_id, self.group_ids, Dataset.Events | ||
) | ||
|
||
mock_rpc.assert_not_called() | ||
|
||
@patch("sentry.eventstream.eap.snuba_rpc.rpc") | ||
def test_empty_allowlist_skips_deletion(self, mock_rpc): | ||
with self.options({"eventstream.eap.deletion_enabled.organization_allowlist": []}): | ||
delete_events_from_eap( | ||
self.organization_id, self.project_id, self.group_ids, Dataset.Events | ||
) | ||
|
||
mock_rpc.assert_not_called() | ||
|
||
def test_empty_group_ids_raises_error(self): | ||
with pytest.raises(ValueError, match="group_ids must not be empty"): | ||
delete_groups_from_eap_rpc( | ||
organization_id=self.organization_id, | ||
project_id=self.project_id, | ||
group_ids=[], | ||
) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 existingeventstream/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.