Skip to content

feat(workflow): Adding project slugs to Rule and AlertRule serializer #20075

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.

Sign up for GitHub

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

Closed
iProgramStuff wants to merge 3 commits into from
Closed

feat(workflow): Adding project slugs to Rule and AlertRule serializer #20075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
024d009
Adding project slugs to Rule and AlertRule serializer
iProgramStuff Jul 28, 2020
6f89920
Merge remote-tracking branch 'origin' into feat/projectsinruleserializer
iProgramStuff Jul 30, 2020
17f113e
Removing DetailedAlertRuleSerializer
iProgramStuff Jul 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 16 additions & 25 deletions src/sentry/api/serializers/models/alert_rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ def get_attrs(self, item_list, user, **kwargs):
)
alert_rule_triggers.append(serialized)

alert_rule_projects = AlertRule.objects.filter(
id__in=[item.id for item in item_list]
).values_list("id", "snuba_query__subscriptions__project__slug")
alert_rules = {item.id: item for item in item_list}
for alert_rule_id, project_slug in alert_rule_projects:
rule_result = result[alert_rules[alert_rule_id]].setdefault("projects", [])
rule_result.append(project_slug)

for alert_rule_id, project_slug in AlertRuleExcludedProjects.objects.filter(
alert_rule__in=item_list
).values_list("alert_rule_id", "project__slug"):
exclusions = result[alert_rules[alert_rule_id]].setdefault("excludedProjects", [])
exclusions.append(project_slug)
Comment on lines +39 to +43
Copy link
Copy Markdown
Member

@wedamija wedamija Jul 30, 2020

Choose a reason for hiding this comment

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

Do we need the excluded projects here? I'd leave them in the detailed serializer otherwise.

Copy link
Copy Markdown
Contributor Author

@iProgramStuff iProgramStuff Aug 10, 2020

Choose a reason for hiding this comment

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

Hmm, doesn't it seem appropriate? We could use the info to know which projects to display in the list (probably more useful once we have multi project alerts?).

Could put it back into the detailed serializer, but bringing it in here also makes the details serializer redundant (I was able to remove it)

Copy link
Copy Markdown
Member

@wedamija wedamija Aug 10, 2020

Choose a reason for hiding this comment

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

We have a general problem where we start putting more and more things into a serializer, and at some point our endpoints end up slow due to extra serialization + db queries. Unless we actually need this data now I'd say it should remain in the detailed serializer.

Copy link
Copy Markdown
Contributor Author

@iProgramStuff iProgramStuff Aug 11, 2020

Choose a reason for hiding this comment

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

Gotcha. Will put it back into the detailed serializer until we have an explicit need.


return result

def serialize(self, obj, attrs, user):
Expand All @@ -54,36 +68,13 @@ def serialize(self, obj, attrs, user):
"thresholdPeriod": obj.threshold_period,
"triggers": attrs.get("triggers", []),
"includeAllProjects": obj.include_all_projects,
"projects": attrs.get("projects", []),
"excludedProjects": attrs.get("excludedProjects", []),
"dateModified": obj.date_modified,
"dateCreated": obj.date_added,
}


class DetailedAlertRuleSerializer(AlertRuleSerializer):
def get_attrs(self, item_list, user, **kwargs):
result = super(DetailedAlertRuleSerializer, self).get_attrs(item_list, user, **kwargs)
alert_rule_projects = AlertRule.objects.filter(
id__in=[item.id for item in item_list]
).values_list("id", "snuba_query__subscriptions__project__slug")
alert_rules = {item.id: item for item in item_list}
for alert_rule_id, project_slug in alert_rule_projects:
rule_result = result[alert_rules[alert_rule_id]].setdefault("projects", [])
rule_result.append(project_slug)

for alert_rule_id, project_slug in AlertRuleExcludedProjects.objects.filter(
alert_rule__in=item_list
).values_list("alert_rule_id", "project__slug"):
exclusions = result[alert_rules[alert_rule_id]].setdefault("excludedProjects", [])
exclusions.append(project_slug)
return result

def serialize(self, obj, attrs, user):
data = super(DetailedAlertRuleSerializer, self).serialize(obj, attrs, user)
data["projects"] = sorted(attrs["projects"])
data["excludedProjects"] = sorted(attrs.get("excludedProjects", []))
return data


class CombinedRuleSerializer(Serializer):
def get_attrs(self, item_list, user, **kwargs):
results = super(CombinedRuleSerializer, self).get_attrs(item_list, user)
Expand Down
1 change: 1 addition & 0 deletions src/sentry/api/serializers/models/rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def serialize(self, obj, attrs, user):
"actionMatch": obj.data.get("action_match") or Rule.DEFAULT_ACTION_MATCH,
"frequency": obj.data.get("frequency") or Rule.DEFAULT_FREQUENCY,
"name": obj.label,
"projects": [obj.project.slug],
"dateCreated": obj.date_added,
"environment": environment.name if environment is not None else None,
}
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/incidents/endpoints/organization_alert_rule_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from rest_framework.response import Response

from sentry.api.serializers import serialize
from sentry.api.serializers.models.alert_rule import DetailedAlertRuleSerializer
from sentry.api.serializers.models.alert_rule import AlertRuleSerializer
from sentry.incidents.endpoints.bases import OrganizationAlertRuleEndpoint
from sentry.incidents.endpoints.serializers import AlertRuleSerializer as DrfAlertRuleSerializer
from sentry.incidents.logic import AlreadyDeletedError, delete_alert_rule
Expand All @@ -17,7 +17,7 @@ def get(self, request, organization, alert_rule):
``````````````````
:auth: required
"""
data = serialize(alert_rule, request.user, DetailedAlertRuleSerializer())
data = serialize(alert_rule, request.user, AlertRuleSerializer())
return Response(data)

def put(self, request, organization, alert_rule):
Expand Down
20 changes: 5 additions & 15 deletions tests/sentry/api/serializers/test_alert_rule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from sentry.api.serializers import serialize
from sentry.api.serializers.models.alert_rule import (
DetailedAlertRuleSerializer,
AlertRuleSerializer,
CombinedRuleSerializer,
)
from sentry.models import Rule
Expand Down Expand Up @@ -96,12 +96,10 @@ def test_environment(self):
result = serialize(alert_rule)
self.assert_alert_rule_serialized(alert_rule, result)


class DetailedAlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase):
def test_simple(self):
def test_projects(self):
projects = [self.project, self.create_project()]
alert_rule = self.create_alert_rule(projects=projects)
result = serialize(alert_rule, serializer=DetailedAlertRuleSerializer())
result = serialize(alert_rule, serializer=AlertRuleSerializer())
self.assert_alert_rule_serialized(alert_rule, result)
assert sorted(result["projects"]) == sorted([p.slug for p in projects])
assert result["excludedProjects"] == []
Expand All @@ -112,25 +110,17 @@ def test_excluded_projects(self):
alert_rule = self.create_alert_rule(
projects=[], include_all_projects=True, excluded_projects=excluded
)
result = serialize(alert_rule, serializer=DetailedAlertRuleSerializer())
result = serialize(alert_rule, serializer=AlertRuleSerializer())
self.assert_alert_rule_serialized(alert_rule, result)
assert result["projects"] == [p.slug for p in projects]
assert result["excludedProjects"] == [p.slug for p in excluded]

alert_rule = self.create_alert_rule(projects=projects, include_all_projects=False)
result = serialize(alert_rule, serializer=DetailedAlertRuleSerializer())
result = serialize(alert_rule, serializer=AlertRuleSerializer())
self.assert_alert_rule_serialized(alert_rule, result)
assert result["projects"] == [p.slug for p in projects]
assert result["excludedProjects"] == []

def test_triggers(self):
alert_rule = self.create_alert_rule()
other_alert_rule = self.create_alert_rule()
trigger = create_alert_rule_trigger(alert_rule, "test", 1000)
result = serialize([alert_rule, other_alert_rule], serializer=DetailedAlertRuleSerializer())
assert result[0]["triggers"] == [serialize(trigger)]
assert result[1]["triggers"] == []


class CombinedRuleSerializerTest(BaseAlertRuleSerializerTest, APITestCase, TestCase):
def test_combined_serializer(self):
Expand Down
4 changes: 3 additions & 1 deletion tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@


from sentry.api.serializers import serialize
from sentry.api.serializers.models.alert_rule import DetailedAlertRuleSerializer
from sentry.api.serializers.models.alert_rule import (
AlertRuleSerializer as DetailedAlertRuleSerializer,
)
from sentry.auth.access import OrganizationGlobalAccess
from sentry.incidents.endpoints.serializers import AlertRuleSerializer
from sentry.incidents.models import AlertRule, AlertRuleStatus, Incident, IncidentStatus
Expand Down