-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(open periods): Add activities to open period serializer #100627
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #100627 +/- ##
===========================================
+ Coverage 81.14% 81.19% +0.05%
===========================================
Files 8599 8616 +17
Lines 380806 382141 +1335
Branches 23911 23911
===========================================
+ Hits 308993 310271 +1278
- Misses 71451 71508 +57
Partials 362 362 |
tests/sentry/workflow_engine/endpoints/test_organization_open_periods.py
Outdated
Show resolved
Hide resolved
saponifi3d
left a comment
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! (also thanks for the initial review malachi!)
src/sentry/workflow_engine/endpoints/serializers/group_open_period_serializer.py
Show resolved
Hide resolved
mifu67
left a comment
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.
Looks good, thank you for doing this!
src/sentry/workflow_engine/endpoints/serializers/group_open_period_serializer.py
Outdated
Show resolved
Hide resolved
| ) | ||
| activities = GroupOpenPeriodActivity.objects.filter( | ||
| group_open_period__in=item_list | ||
| ).order_by("id") |
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.
Could also order by date_added, but this works too :)
| return GroupOpenPeriodActivityResponse( | ||
| id=str(obj.id), | ||
| type=OpenPeriodActivityType(obj.type).to_str(), | ||
| value=PriorityLevel(obj.value).to_str() if obj.value else None, |
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.
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.
0 is not a valid PriorityLevel https://github.com/getsentry/sentry/blob/master/src/sentry/types/group.py#L63-L66
| ): | ||
| gopas[activity.group_open_period].append(serialized_activity) | ||
| for item in item_list: | ||
| result[item]["activities"] = gopas[item][:100] |
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.
Bug: Overfetching and Serialization Issues
The get_attrs method fetches and serializes all GroupOpenPeriodActivity records for each GroupOpenPeriod, even though only the first 100 are used. This can cause significant memory and processing overhead. Additionally, zipping the activities queryset with its serialized output risks an ordering mismatch, potentially leading to incorrect data associations.
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.
django lazy loads them
Add
GroupOpenPeriodActivityresults to the group open period serializer response.