Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
55 changes: 47 additions & 8 deletions src/sentry/api/helpers/group_index/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from collections import defaultdict
from collections.abc import Mapping, MutableMapping, Sequence
from http import HTTPStatus
from typing import Any, NotRequired, TypedDict
from typing import Any, NotRequired, TypedDict, cast
from urllib.parse import urlparse

import rest_framework
Expand Down Expand Up @@ -41,6 +41,7 @@
from sentry.models.groupinbox import GroupInboxRemoveAction, remove_group_from_inbox
from sentry.models.grouplink import GroupLink
from sentry.models.groupopenperiod import update_group_open_period
from sentry.models.grouprelease import GroupRelease
from sentry.models.groupresolution import GroupResolution
from sentry.models.groupseen import GroupSeen
from sentry.models.groupshare import GroupShare
Expand Down Expand Up @@ -154,7 +155,31 @@ def get_current_release_version_of_group(group: Group, follows_semver: bool = Fa
"""
current_release_version = None
if follows_semver:
release = greatest_semver_release(group.project)
# Fetch all the release-packages associated with the group. We'll find the largest semver
# version for one of these packages.
release_ids = list(
GroupRelease.objects.filter(
group_id=group.id,
project_id=group.project_id,
)
.distinct()
.values_list("release_id", flat=True)
)

group_packages = cast(
list[str],
list(
Release.objects.filter(
organization_id=group.project.organization_id,
id__in=release_ids,
package__isnull=False,
)
.distinct()
.values_list("package", flat=True)
),
)

release = greatest_semver_release(group.project, packages=group_packages)
if release is not None:
current_release_version = release.version
else:
Expand Down Expand Up @@ -537,6 +562,10 @@ def process_group_resolution(
# in release
resolution_params.update(
{
"release": Release.objects.filter(
organization_id=release.organization_id,
version=current_release_version,
).get(),
Copy link
Member

Choose a reason for hiding this comment

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

PR looks good to me. One small question about the extra query we have here to get the release that matches current_release_version--can/should we remove the need for this? Disadvantages are a little bit of extra latency and edge cases where get() raises Release.DoesNotExist.

We could do an easy fix in the semver path by returning current_release alongside current_release_version in get_current_release_version_of_group, then doing current_release or Release.objects.filter( here.

The non-semver path also queries the release and returns only the version (ugh), but the logic is buried fairly deep and uses caching so I understand if we don't want to change anything in this path. But it would be cleaner and save us a query if we change get_current_release_version_of_group to get_current_release_of_group altogether. If we want to merge this PR as is, I'm happy to investigate more and potentially write a followup ref PR that does this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Its not ideal. Definitely something we could address in a follow-up. For this PR, we have enough to worry about so we'll limit the blast radius.

On performance, yes its another query but its indexed so it will be fast. These things do add up so its best to not add queries too cavalierly. My long term goal is to refactor this code path entirely. The code, as is, is incomprehensible. An intermediate step might be the change you suggested or it might be something deeper. It just depends on the performance we measure in production and if its causing issues or not. If its not causing issues then we might focus our efforts on more substantial improvements.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. Was gonna ask how I can follow up on measuring perf in prod but I think your DD walkthrough is coming up anyway!

Copy link
Member Author

Choose a reason for hiding this comment

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

Sentry's tracing product would be better! You can filter to the PUT request and find this span in there. You can see how the system performs in aggregate and determine if you need to make a perf change or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Unhandled Exception When Fetching Nonexistent Release

During issue resolution in semver projects, the code fetches a Release object using .get() with current_release_version. If current_release_version doesn't correspond to an existing Release in the database, this raises Release.DoesNotExist, causing an unhandled exception.

Fix in Cursor Fix in Web

Copy link
Member Author

Choose a reason for hiding this comment

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

True but since the current_release_version has already been shown to exist in the db the only way this could happen is if the release was deleted (which we don't allow). Technically this could raise but I'm not sure what I'd do with the exception anyway. Raising seems to be appropriate for now.

"type": GroupResolution.Type.in_release,
"status": GroupResolution.Status.resolved,
}
Expand Down Expand Up @@ -819,7 +848,11 @@ def get_release_to_resolve_by(project: Project) -> Release | None:
follows_semver = follows_semver_versioning_scheme(
org_id=project.organization_id, project_id=project.id
)
return greatest_semver_release(project) if follows_semver else most_recent_release(project)
return (
greatest_semver_release(project, packages=[])
if follows_semver
else most_recent_release(project)
)


def most_recent_release(project: Project) -> Release | None:
Expand All @@ -841,14 +874,20 @@ def most_recent_release_matching_commit(
)


def greatest_semver_release(project: Project) -> Release | None:
return get_semver_releases(project).first()
def greatest_semver_release(project: Project, packages: list[str]) -> Release | None:
return get_semver_releases(project, packages).first()


def get_semver_releases(project: Project) -> QuerySet[Release]:
def get_semver_releases(project: Project, packages: list[str]) -> QuerySet[Release]:
query = Release.objects.filter(projects=project, organization_id=project.organization_id)

# Multiple packages may exist for a single project. If we were able to infer the packages
# associated with an issue we'll include them.
if packages:
query = query.filter(package__in=packages)

return (
Release.objects.filter(projects=project, organization_id=project.organization_id)
.filter_to_semver() # type: ignore[attr-defined]
query.filter_to_semver() # type: ignore[attr-defined]
.annotate_prerelease_column()
.order_by(*[f"-{col}" for col in Release.SEMVER_COLS])
)
Expand Down
60 changes: 60 additions & 0 deletions tests/sentry/issues/endpoints/test_organization_group_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -3588,6 +3588,66 @@ def test_set_resolved_in_next_release(self) -> None:
)
assert activity.data["version"] == ""

def test_set_resolved_in_next_semver_release(self) -> None:
release = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release.add_project(self.project)

# Smaller than 1.0.0 but more recent.
release2 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release2.add_project(self.project)

# Bigger than 1.0.0 and more recent but different package.
release3 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release3.add_project(self.project)

# Smaller than 1.0.0, a different package, and associated to the group. This package's
# release should be in contention for largest semver but not selected.
release4 = Release.objects.create(
organization_id=self.project.organization_id, version="[email protected]"
)
release4.add_project(self.project)

group = self.create_group(status=GroupStatus.UNRESOLVED)

# Record the release as a group release so the group is scoped to at least one package.
self.create_group_release(self.project, group=group, release=release)
self.create_group_release(self.project, group=group, release=release4)

self.login_as(user=self.user)

response = self.get_success_response(
qs_params={"id": group.id}, status="resolved", statusDetails={"inNextRelease": True}
)
assert response.data["status"] == "resolved"
assert response.data["statusDetails"]["inNextRelease"]
assert response.data["statusDetails"]["actor"]["id"] == str(self.user.id)
assert "activity" in response.data

group = Group.objects.get(id=group.id)
assert group.status == GroupStatus.RESOLVED

resolution = GroupResolution.objects.get(group=group)
assert resolution.release == release
assert resolution.type == GroupResolution.Type.in_release
assert resolution.status == GroupResolution.Status.resolved
assert resolution.actor_id == self.user.id

assert GroupSubscription.objects.filter(
user_id=self.user.id, group=group, is_active=True
).exists()

activity = Activity.objects.get(
group=group, type=ActivityType.SET_RESOLVED_IN_RELEASE.value
)
assert activity.data["version"] == ""
assert activity.data["current_release_version"] == "[email protected]"

def test_set_resolved_in_next_release_legacy(self) -> None:
release = Release.objects.create(organization_id=self.project.organization_id, version="a")
release.add_project(self.project)
Expand Down
Loading