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

Jump to bottom

fix(releases): For semver releases, get latest release as the resolving one #80737

Merged

armenzg merged 9 commits into master from ref/releases_code/armenzg

Nov 15, 2024

Member

armenzg commented Nov 14, 2024 •

edited

Loading

Select the latest semver release for the project rather than the latest seen for the group.

This is a follow-up to #80655.
This fixes the customer issue #69937 (and possibly these: #74448 and #80194).

armenzg added 2 commits

November 14, 2024 10:10


          fix(releases): Change get_current_release_version_of_group for semver…

199a271

… releases

This is a follow-up to #80655.
This fixes customer issue #69937.


          The fix

b1eaf7d

armenzg added Component: Releases Product Area: Releases labels

armenzg self-assigned this

github-actions bot added the Scope: Backend label

vercel bot deployed to Preview

November 14, 2024 15:35

View deployment


          One more assertion

0d1b8c6

codecov bot commented Nov 14, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #80737    +/-   ##
========================================
  Coverage   78.39%   78.39%            
========================================
  Files        7206     7206            
  Lines      319290   319437   +147     
  Branches    43977    43991    +14     
========================================
+ Hits       250306   250434   +128     
- Misses      62607    62618    +11     
- Partials     6377     6385     +8

armenzg added 2 commits

November 14, 2024 15:27


          Refactor tests

73c2161


          Minor renaming

8a474b3

vercel bot deployed to Preview

November 14, 2024 20:39

View deployment

armenzg added 3 commits

November 14, 2024 15:47


          Fix it at the right place

614f598


          Minor touches

123d9bd


          Typing

c1c92cb

armenzg mentioned this pull request

Regression version comparison incorrectly identified issue as regressed, when version was older than resolved-in version #74448

Closed

vercel bot deployed to Preview

November 14, 2024 20:59

View deployment

armenzg commented

View reviewed changes

src/sentry/api/helpers/group_index/update.py

-              def get_current_release_version_of_group(
-                  group: Group, follows_semver: bool = False
-              ) -> Release | None:
+              def get_current_release_version_of_group(group: Group, follows_semver: bool = False) -> str | None:

Member Author

armenzg Nov 14, 2024

We need to add stronger typing enforcement here. It returns a string.

src/sentry/api/helpers/group_index/update.py

		@@ -140,13 +138,12 @@ def get_current_release_version_of_group(
		try:

Member Author

armenzg Nov 14, 2024

No changes in this block besides some minor refactoring to help comprehension.

src/sentry/api/helpers/group_index/update.py Outdated

+                                          "organizations:releases-resolve-next-release-semver-fix",
+                                          project.organization,
+                                      ):
+                                          resolving_release_version = get_latest_release(projects[0]).version

Member Author

armenzg Nov 14, 2024

The fix.

src/sentry/testutils/fixtures.py Outdated

                       if project is None:
                           project = self.project
+                      if "substatus" not in kwargs:
+                          kwargs["substatus"] = GroupSubStatus.NEW

Member Author

armenzg Nov 14, 2024

Minor improvement. You could see a logged error when this is not set.

tests/sentry/issues/endpoints/test_group_details.py

@@ @@ -328,7 +328,11 @@ def test_resolved_in_next_release_non_semver(self): @@
                       project = self.create_project()
                       project.flags.has_releases = True
                       project.save()
-                      group = self.create_group(project=project)
+                      # Using store_event() instead of create_group() produces GroupRelease objects
+                      # which is considered during the update_groups() call

Member Author

armenzg Nov 14, 2024

This change helps the test go through the ingestion pipeline, thus, creating GroupRelease objects.

Entrypoint:

sentry/src/sentry/event_manager.py

Line 509 in 3ae2f67

_get_or_create_release_many(jobs, projects)

Where is used:

sentry/src/sentry/api/helpers/group_index/update.py

Lines 125 to 149 in 3ae2f67

    
           def get_current_release_version_of_group( 
        
               group: Group, follows_semver: bool = False 
        
           ) -> Release | None: 
        
               """ 
        
               Function that returns the latest release version associated with a Group, and by latest we 
        
               mean either most recent (date) or latest in semver versioning scheme 
        
               Inputs: 
        
                   * group: Group of the issue 
        
                   * follows_semver: flag that determines whether the project of the group follows semantic 
        
                                     versioning or not. 
        
               Returns: 
        
                   current_release_version 
        
               """ 
        
               current_release_version = None 
        
               if follows_semver: 
        
                   try: 
        
                       # This sets current_release_version to the latest semver version associated with a group 
        
                       order_by_semver_desc = [f"-{col}" for col in Release.SEMVER_COLS] 
        
                       current_release_version = ( 
        
                           Release.objects.filter_to_semver() 
        
                           .filter( 
        
                               id__in=GroupRelease.objects.filter( 
        
                                   project_id=group.project.id, group_id=group.id 
        
                               ).values_list("release_id"), 
        
                           )

tests/sentry/issues/endpoints/test_group_details.py

+                      # For semver projects, we consider resolution based on an expression rather than a specific release,
+                      # thus, it is considered resolved in the release that has the highest semver
+                      assert group_resolution.type == GroupResolution.Type.in_release
+                      assert group_resolution.status == GroupResolution.Status.resolved

Member Author

armenzg Nov 14, 2024

This is evidence that we're going through the same code path as reported by Dropbox.

sentry/src/sentry/api/helpers/group_index/update.py

Lines 440 to 449 in 3ae2f67

    
           # In semver projects, and thereby semver releases, we determine 
        
           # resolutions by comparing against an expression rather than a 
        
           # specific release (i.e. >current_release_version). Consequently, 
        
           # at this point we can consider this GroupResolution as resolved 
        
           # in release 
        
           resolution_params.update( 
        
               { 
        
                   "type": GroupResolution.Type.in_release, 
        
                   "status": GroupResolution.Status.resolved, 
        
               }

tests/sentry/issues/endpoints/test_group_details.py

@@ @@ -351,17 +355,22 @@ def test_resolved_in_next_release_non_semver(self): @@
                       assert group_resolution.release.version == most_recent_version.version
                   # XXX: Remove this test once the feature flag is removed
-                  def test_resolved_in_next_release_semver_without_feature_flag(self):
+                  def test_resolved_in_next_release_semver_no_flag_and_first_release(self):

Member Author

armenzg Nov 14, 2024

I could have added one more test (one without setting the release), however, we're moving away from this logic, thus, I do not deem it necessary.

tests/sentry/issues/endpoints/test_group_details.py Outdated Show resolved Hide resolved

src/sentry/api/helpers/group_index/update.py

+                                          )
+                                          and follows_semver
+                                      ):
+                                          current_release_version = get_latest_release(projects[0]).version

Member Author

armenzg Nov 14, 2024

This is the actual fix.

tests/sentry/issues/endpoints/test_group_details.py

+                      new_release = Release.get_or_create(version="[email protected]+0", project=project)
+                      # A lesser release than 2.x but created more recently than 3.x
+                      old_version = Release.get_or_create(version="[email protected]+0", project=project)
+                      for release in releases + [old_version]:

Member Author

armenzg Nov 14, 2024

Without the fix, only [email protected]+0 would not unresolve.

armenzg marked this pull request as ready for review

November 14, 2024 21:00

armenzg requested a review from a team as a code owner

November 14, 2024 21:00

armenzg mentioned this pull request

"The oldest adopted release associated with the event's issue is newer than the latest adopted release in <environment>" should not consider "oldest adopted release" #80194

Open

snigdhas approved these changes

View reviewed changes

Member

snigdhas left a comment

Looks great!

JoshFerge approved these changes

View reviewed changes

wedamija reviewed

View reviewed changes

src/sentry/api/helpers/group_index/update.py

@@ @@ -419,9 +416,19 @@ def update_groups( @@
                                           release_version=release.version,
                                       )
-                                      current_release_version = get_current_release_version_of_group(

Member

wedamija Nov 14, 2024

I think we should look to remove this as a follow-up

sentry/src/sentry/integrations/tasks/sync_status_inbound.py

Lines 112 to 114 in c079722

    
           current_release_version = get_current_release_version_of_group( 
        
               group=group, follows_semver=follows_semver 
        
           )

and I think that actually that code path should have the same fix you're adding in here. So might be worth also adding the fix there as part of this

Member Author

armenzg Nov 15, 2024

Sounds good!

Member Author

armenzg Nov 20, 2024

I followed-up here: #80821


          Revert to original

7e2e8ad

vercel bot deployed to Preview

November 15, 2024 13:00

View deployment

armenzg enabled auto-merge (squash)

November 15, 2024 13:04

armenzg merged commit a087bb8 into master

50 checks passed

armenzg deleted the ref/releases_code/armenzg branch

November 15, 2024 13:32

armenzg mentioned this pull request

ref(releases): Make it easier to read #80820

Merged

armenzg added a commit that referenced this pull request


          fix(releases): Include fix within get_current_release_version_of_group()

e92c615

In #80737 we provided a fix within `get_latest_release()` but it makes more sense to include inside of `get_current_release_version_of_group()` for it will make it available to other parts of the codebase. For example:
https://github.com/getsentry/sentry/blob/c0797226ec0dc304d766796713121a880df38811/src/sentry/integrations/tasks/sync_status_inbound.py#L112-L114

armenzg mentioned this pull request

fix(releases): Include fix within get_current_release_version_of_group() #80821

Merged

armenzg added a commit that referenced this pull request


          fix(releases): Include fix within get_current_release_version_of_group()

a4c711f

In #80737 we provided a fix within `get_latest_release()` but it makes more sense to include inside of `get_current_release_version_of_group()` for it will make it available to other parts of the codebase. For example:
https://github.com/getsentry/sentry/blob/c0797226ec0dc304d766796713121a880df38811/src/sentry/integrations/tasks/sync_status_inbound.py#L112-L114

armenzg added a commit that referenced this pull request


          ref(releases): Make it easier to read (#80820)

b3bd7be

Follow-up to #80737.

armenzg added a commit that referenced this pull request


          fix(releases): Include fix within get_current_release_version_of_grou…

910df9c

…p() (#80821)

In #80737 we provided a fix within `get_latest_release()` but it makes
more sense to include it inside of
`get_current_release_version_of_group()` for it will make it available
to other parts of the codebase. For example:
https://github.com/getsentry/sentry/blob/c0797226ec0dc304d766796713121a880df38811/src/sentry/integrations/tasks/sync_status_inbound.py#L112-L114

Member Author

armenzg commented Nov 29, 2024

I will enable this for GA next week.

github-actions bot locked and limited conversation to collaborators

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

Labels

Component: Releases Product Area: Releases Scope: Backend