Skip to content

Conversation

@everettbu
Copy link
Contributor

@everettbu everettbu commented Jul 29, 2025

Test 7

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements a comprehensive cross-system issue synchronization feature for Sentry's integration ecosystem. The changes introduce assignment source tracking to prevent infinite sync loops when multiple integrations are connected to the same Sentry organization.

The core implementation adds a new AssignmentSource dataclass that tracks which integration initiated an assignment change. This source information flows through the entire sync pipeline:

  1. New AssignmentSource Service: A frozen dataclass with serialization methods (to_dict/from_dict) and factory methods (from_integration) for creating assignment sources from integration objects

  2. Enhanced Sync Logic: The should_sync method in IssueBasicMixin now accepts an optional sync_source parameter and prevents syncing when the source integration matches the target integration

  3. Model Layer Updates: The GroupAssignee.assign() and deassign() methods now accept an optional assignment_source parameter that gets passed to sync operations

  4. Task Integration: The sync_assignee_outbound task processes assignment source dictionaries and passes the parsed source to sync methods

  5. Utility Function Updates: Sync utility functions in sync.py now handle assignment source tracking for both inbound and outbound operations

The implementation maintains backward compatibility by making all new parameters optional and preserves existing sync behavior for single-integration scenarios. The changes are well-tested with comprehensive unit tests covering various scenarios including edge cases and the critical sync loop prevention logic.

PR Description Notes:

  • The PR body only contains "Test 7" which doesn't provide meaningful context about the changes

Confidence score: 4/5

  • Safe to merge with minor attention needed for the test file typo
  • High confidence due to comprehensive test coverage, backward compatibility, and well-structured implementation
  • tests/sentry/integrations/services/test_assignment_source.py needs attention for the typo in test method name ('inalid' should be 'invalid')

7 files reviewed, 3 comments

Edit Code Review Bot Settings | Greptile

result = AssignmentSource.from_dict(data)
assert result is None

def test_from_dict_inalid_data(self):
Copy link

Choose a reason for hiding this comment

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

syntax: Typo in method name: 'inalid' should be 'invalid'

Suggested change
def test_from_dict_inalid_data(self):
def test_from_dict_invalid_data(self):



class TestAssignmentSource(TestCase):
def test_from_dict_empty_array(self):
Copy link

Choose a reason for hiding this comment

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

style: Method name says 'empty_array' but tests empty dict - consider renaming to 'test_from_dict_empty_dict' for clarity

Suggested change
def test_from_dict_empty_array(self):
def test_from_dict_empty_dict(self):

class AssignmentSource:
source_name: str
integration_id: int
queued: datetime = timezone.now()
Copy link

Choose a reason for hiding this comment

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

logic: Using timezone.now() as a default creates a timestamp at class definition time, not instance creation time. Consider using field(default_factory=timezone.now) for proper per-instance timestamps.

Suggested change
queued: datetime = timezone.now()
queued: datetime = field(default_factory=timezone.now)

@GitHoobar
Copy link

Review Summary

🏷️ Draft Comments (7)

Skipped posting 7 draft comments that were valid but scored below your review threshold (>=13/15). Feel free to update them here.

src/sentry/integrations/mixins/issues.py (2)

66-67: should_sync in IssueBasicIntegration now takes a sync_source argument, but the method always returns False and ignores both arguments, breaking all subclasses that rely on the base implementation for correct sync logic.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 2/5
  • Urgency Impact: 3/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/integrations/mixins/issues.py, lines 66-67, the base `should_sync` method in `IssueBasicIntegration` was changed to accept a `sync_source` argument but now always returns `False`, breaking expected sync logic for subclasses. Update this method to implement the same logic as the `should_sync` in `IssueSyncIntegration`, checking the config and `sync_source` to determine if sync should occur.

334-355: get_annotations_for_group_list performs N^2 queries/operations for large group lists due to per-group and per-link filtering; this will cause major slowdowns and DB load at scale.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 5/5
  • Urgency Impact: 3/5
  • Total Score: 12/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Optimize the `get_annotations_for_group_list` method in src/sentry/integrations/mixins/issues.py (lines 334-355). The current implementation performs O(N^2) filtering by iterating over all external issues for each group link, which causes major slowdowns for large group lists. Refactor the code to build a mapping from external issue id to object for O(1) lookup, and use this mapping to efficiently retrieve the relevant external issue for each group link. Ensure the new implementation preserves the original output structure and logic.

src/sentry/integrations/services/assignment_source.py (1)

18-18: queued: datetime = timezone.now() sets the default value at class definition time, causing all instances to share the same timestamp, which is incorrect for per-instance creation time.

📊 Impact Scores:

  • Production Impact: 3/5
  • Fix Specificity: 4/5
  • Urgency Impact: 2/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/integrations/services/assignment_source.py, line 18, the dataclass field `queued` uses `timezone.now()` as a default value, which evaluates only once at class definition. Change it to use `dataclasses.field(default_factory=timezone.now)` so each instance gets the correct creation time.

src/sentry/integrations/tasks/sync_assignee_outbound.py (1)

56-61: installation.should_sync and installation.sync_assignee_outbound are now called with an extra assignment_source argument, but if the installation implementation does not accept this argument, it will raise a runtime TypeError and crash.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 2/5
  • Urgency Impact: 3/5
  • Total Score: 9/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/integrations/tasks/sync_assignee_outbound.py, lines 56-61, the code unconditionally passes the new `assignment_source` argument to `installation.should_sync` and `installation.sync_assignee_outbound`. If the installation implementation does not accept this argument, a runtime TypeError will occur. Update the code to check the function signatures and only pass `assignment_source` if the function supports it, falling back to the old call signature otherwise.

src/sentry/integrations/utils/sync.py (1)

44-49: where_should_sync calls integration.get_installation(...).should_sync(key) inside a list comprehension, causing N+1 queries/requests for each organization, which can severely degrade performance for many organizations.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 3/5
  • Urgency Impact: 3/5
  • Total Score: 10/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Optimize the `where_should_sync` function in src/sentry/integrations/utils/sync.py (lines 44-49). The current implementation calls `integration.get_installation(organization_id=organization.id).should_sync(key)` inside a list comprehension, causing an N+1 query/request problem for each organization. Refactor the code to pre-fetch all installations for the relevant organizations in a single pass, then use the pre-fetched installations in the list comprehension to avoid repeated calls.

src/sentry/models/groupassignee.py (2)

193-194, 239-240: assignment_source is passed to sync_group_assignee_outbound in assign and deassign, but if the integration code does not expect this new kwarg, it may cause a runtime TypeError if the function signature is not updated everywhere.

📊 Impact Scores:

  • Production Impact: 4/5
  • Fix Specificity: 4/5
  • Urgency Impact: 3/5
  • Total Score: 11/15

🤖 AI Agent Prompt (Copy & Paste Ready):

In src/sentry/models/groupassignee.py, lines 193-194 and 239-240, the code calls `sync_group_assignee_outbound` with a new `assignment_source` kwarg. If the function definition does not accept this kwarg everywhere, it will cause a runtime TypeError. Update these calls to only pass `assignment_source` if it is not None, using `**({'assignment_source': assignment_source} if assignment_source is not None else {})` to avoid breaking existing function signatures.

92-110: remove_old_assignees repeatedly queries and deletes subscriptions for each reassignment, causing multiple DB hits per operation for large teams.

📊 Impact Scores:

  • Production Impact: 2/5
  • Fix Specificity: 4/5
  • Urgency Impact: 2/5
  • Total Score: 8/15

🤖 AI Agent Prompt (Copy & Paste Ready):

Optimize the `remove_old_assignees` method in src/sentry/models/groupassignee.py (lines 92-110). The current code fetches team members as a list and may perform unnecessary DB deletes for empty sets. Change `team_members` to a set, use `discard` instead of `remove` to avoid exceptions, and only perform the delete if `team_members` is non-empty. This reduces unnecessary DB operations for large teams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants