Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -373,15 +373,51 @@ def post(
body = SizeAnalysisComparePOSTResponse(
status="exists",
message="A comparison already exists for the head and base size metrics.",
existing_comparisons=comparison_models,
comparisons=comparison_models,
)
return Response(body.dict(), status=200)

logger.info(
"preprod.size_analysis.compare.api.post.running_comparison",
"preprod.size_analysis.compare.api.post.creating_pending_comparisons",
extra={"head_artifact_id": head_artifact.id, "base_artifact_id": base_artifact.id},
)

# Create PENDING comparison records for each matching head/base metric pair
head_metrics_map = build_size_metrics_map(head_size_metrics)
base_metrics_map = build_size_metrics_map(base_size_metrics)

created_comparisons = []
for key, head_metric in head_metrics_map.items():
base_metric = base_metrics_map.get(key)
if base_metric:
comparison = PreprodArtifactSizeComparison.objects.create(
head_size_analysis=head_metric,
base_size_analysis=base_metric,
organization_id=project.organization_id,
state=PreprodArtifactSizeComparison.State.PENDING,
)
created_comparisons.append(
SizeAnalysisComparison(
head_size_metric_id=head_metric.id,
base_size_metric_id=base_metric.id,
metrics_artifact_type=head_metric.metrics_artifact_type,
identifier=head_metric.identifier,
state=PreprodArtifactSizeComparison.State.PENDING,
comparison_id=None,
error_code=None,
error_message=None,
)
)

logger.info(
"preprod.size_analysis.compare.api.post.running_comparison",
extra={
"head_artifact_id": head_artifact.id,
"base_artifact_id": base_artifact.id,
"pending_comparisons_count": len(created_comparisons),
},
)

manual_size_analysis_comparison.apply_async(
kwargs={
"project_id": project.id,
Expand All @@ -393,6 +429,16 @@ def post(

logger.info(
"preprod.size_analysis.compare.api.post.success",
extra={"head_artifact_id": head_artifact_id, "base_artifact_id": base_artifact_id},
extra={
"head_artifact_id": head_artifact_id,
"base_artifact_id": base_artifact_id,
"created_comparisons_count": len(created_comparisons),
},
)

body = SizeAnalysisComparePOSTResponse(
status="created",
message="Comparison records created and processing started.",
comparisons=created_comparisons,
)
return Response(status=200)
return Response(body.dict(), status=200)
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,4 @@ class SizeAnalysisCompareGETResponse(BaseModel):
class SizeAnalysisComparePOSTResponse(BaseModel):
status: str
message: str
existing_comparisons: list[SizeAnalysisComparison] | None
comparisons: list[SizeAnalysisComparison] | None
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't used by the frontend currently, so no need to update any FE types

68 changes: 51 additions & 17 deletions src/sentry/preprod/size_analysis/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,7 @@ def compare_preprod_artifact_size_analysis(
),
},
)
_run_size_analysis_comparison(
project_id, org_id, matching_head_size_metric, base_metric
)
_run_size_analysis_comparison(org_id, matching_head_size_metric, base_metric)
else:
logger.info(
"preprod.size_analysis.compare.no_matching_base_size_metric",
Expand Down Expand Up @@ -157,9 +155,7 @@ def compare_preprod_artifact_size_analysis(
"preprod.size_analysis.compare.running_comparison",
extra={"base_artifact_id": artifact_id, "head_artifact_id": head_artifact.id},
)
_run_size_analysis_comparison(
project_id, org_id, head_metric, matching_base_size_metric
)
_run_size_analysis_comparison(org_id, head_metric, matching_base_size_metric)
else:
logger.info(
"preprod.size_analysis.compare.no_matching_base_size_metric",
Expand Down Expand Up @@ -262,9 +258,7 @@ def manual_size_analysis_comparison(
),
},
)
_run_size_analysis_comparison(
project_id, org_id, matching_head_size_metric, base_metric
)
_run_size_analysis_comparison(org_id, matching_head_size_metric, base_metric)
else:
logger.info(
"preprod.size_analysis.compare.manual.no_matching_base_size_metric",
Expand All @@ -278,19 +272,19 @@ def manual_size_analysis_comparison(


def _run_size_analysis_comparison(
project_id: int,
org_id: int,
head_size_metric: PreprodArtifactSizeMetrics,
base_size_metric: PreprodArtifactSizeMetrics,
):
comparison = None
try:
comparison = PreprodArtifactSizeComparison.objects.get(
head_size_analysis=head_size_metric,
base_size_analysis=base_size_metric,
organization_id=org_id,
)

# Existing comparison exists or is already running,
# Skip if comparison is already complete or currently running
if comparison.state in [
PreprodArtifactSizeComparison.State.PROCESSING,
PreprodArtifactSizeComparison.State.SUCCESS,
Expand All @@ -301,10 +295,21 @@ def _run_size_analysis_comparison(
extra={
"head_artifact_size_metric_id": head_size_metric.id,
"base_artifact_size_metric_id": base_size_metric.id,
"state": comparison.state,
},
)
return

# PENDING state - transition to PROCESSING
if comparison.state == PreprodArtifactSizeComparison.State.PENDING:
logger.info(
"preprod.size_analysis.compare.transitioning_pending_to_processing",
extra={
"head_artifact_size_metric_id": head_size_metric.id,
"base_artifact_size_metric_id": base_size_metric.id,
},
)

Copy link
Member

Choose a reason for hiding this comment

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

I can't tag the line number but I think the error handling we have should now update the comparison to PreprodArtifactSizeComparison.State.FAILED for cases like except File.DoesNotExist. Right now it just returns from the function and the comparison is left PENDING. Eventually our timeout logic will catch it but will take a while.

except PreprodArtifactSizeComparison.DoesNotExist:
logger.info(
"preprod.size_analysis.compare.no_existing_comparison",
Expand All @@ -313,6 +318,14 @@ def _run_size_analysis_comparison(
"base_artifact_size_metric_id": base_size_metric.id,
},
)
with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)):
comparison = PreprodArtifactSizeComparison.objects.create(
head_size_analysis=head_size_metric,
base_size_analysis=base_size_metric,
organization_id=org_id,
state=PreprodArtifactSizeComparison.State.FAILED,
)
comparison.save()
pass

try:
Expand All @@ -325,6 +338,8 @@ def _run_size_analysis_comparison(
"head_artifact_id": head_size_metric.preprod_artifact.id,
},
)
comparison.state = PreprodArtifactSizeComparison.State.FAILED
comparison.save()
return

try:
Expand All @@ -337,6 +352,9 @@ def _run_size_analysis_comparison(
"base_artifact_id": base_size_metric.preprod_artifact.id,
},
)
with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)):
comparison.state = PreprodArtifactSizeComparison.State.FAILED
comparison.save()
return

head_size_analysis_results = SizeAnalysisResults.parse_raw(head_analysis_file.getfile().read())
Expand All @@ -350,12 +368,28 @@ def _run_size_analysis_comparison(
},
)

comparison = PreprodArtifactSizeComparison.objects.create(
head_size_analysis=head_size_metric,
base_size_analysis=base_size_metric,
organization_id=org_id,
state=PreprodArtifactSizeComparison.State.PROCESSING,
)
# Update existing PENDING comparison or create new one
with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)):
if comparison:
comparison.state = PreprodArtifactSizeComparison.State.PROCESSING
comparison.save()
else:
logger.info(
"preprod.size_analysis.compare.no_existing_comparison",
extra={
"head_artifact_size_metric_id": head_size_metric.id,
"base_artifact_size_metric_id": base_size_metric.id,
},
)
with transaction.atomic(router.db_for_write(PreprodArtifactSizeComparison)):
comparison = PreprodArtifactSizeComparison.objects.create(
head_size_analysis=head_size_metric,
base_size_analysis=base_size_metric,
organization_id=org_id,
state=PreprodArtifactSizeComparison.State.FAILED,
)
comparison.save()
return

comparison_results = compare_size_analysis(
head_size_analysis=head_size_metric,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,31 @@ def test_get_comparison_success_with_failed_comparison(self):
assert comparison_data["error_code"] == str(PreprodArtifactSizeComparison.ErrorCode.UNKNOWN)
assert comparison_data["error_message"] == "Comparison failed due to processing error"

@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
def test_get_comparison_success_with_pending_comparison(self):
"""Test GET endpoint returns processing state for pending comparison"""
# Create a pending comparison (which should be shown as PROCESSING to frontend)
comparison = PreprodArtifactSizeComparison.objects.create(
head_size_analysis=self.head_size_metric,
base_size_analysis=self.base_size_metric,
organization_id=self.organization.id,
state=PreprodArtifactSizeComparison.State.PENDING,
)

response = self.get_success_response(
self.organization.slug,
self.project.slug,
self.head_artifact.id,
self.base_artifact.id,
)
data = response.data
comparison_data = data["comparisons"][0]
# GET endpoint shows PENDING as PROCESSING to frontend
assert comparison_data["state"] == PreprodArtifactSizeComparison.State.PROCESSING
assert comparison_data["comparison_id"] == comparison.id
assert comparison_data["error_code"] is None
assert comparison_data["error_message"] is None

@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
def test_get_comparison_success_with_processing_comparison(self):
"""Test GET endpoint returns processing comparison when comparison is in progress"""
Expand Down Expand Up @@ -415,14 +440,16 @@ def test_get_comparison_multiple_metrics(self):
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
@patch("sentry.preprod.size_analysis.tasks.manual_size_analysis_comparison.apply_async")
def test_post_comparison_success(self, mock_apply_async):
"""Test POST endpoint successfully triggers comparison"""
self.get_success_response(
"""Test POST endpoint successfully triggers comparison and creates PENDING records"""
response = self.get_success_response(
self.organization.slug,
self.project.slug,
self.head_artifact.id,
self.base_artifact.id,
method="post",
)

# Verify task was enqueued
mock_apply_async.assert_called_once_with(
kwargs={
"project_id": self.project.id,
Expand All @@ -432,6 +459,27 @@ def test_post_comparison_success(self, mock_apply_async):
}
)

# Verify response contains created comparisons
data = response.data
assert data["status"] == "created"
assert "Comparison records created and processing started" in data["message"]
assert len(data["comparisons"]) == 1

# Verify comparison is in PENDING state
comparison_data = data["comparisons"][0]
assert comparison_data["state"] == PreprodArtifactSizeComparison.State.PENDING
assert comparison_data["head_size_metric_id"] == self.head_size_metric.id
assert comparison_data["base_size_metric_id"] == self.base_size_metric.id
assert comparison_data["comparison_id"] is None

# Verify PENDING record was created in database
comparison = PreprodArtifactSizeComparison.objects.get(
head_size_analysis=self.head_size_metric,
base_size_analysis=self.base_size_metric,
)
assert comparison.state == PreprodArtifactSizeComparison.State.PENDING
assert comparison.organization_id == self.organization.id

@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
def test_post_comparison_head_artifact_not_found(self):
"""Test POST endpoint returns 404 when head artifact doesn't exist"""
Expand Down Expand Up @@ -548,8 +596,8 @@ def test_post_comparison_existing_comparison(self):
data = response.json()
assert data["status"] == "exists"
assert "A comparison already exists for the head and base size metrics" in data["message"]
assert len(data["existing_comparisons"]) == 1
assert data["existing_comparisons"][0]["comparison_id"] == comparison.id
assert len(data["comparisons"]) == 1
assert data["comparisons"][0]["comparison_id"] == comparison.id

@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
def test_post_comparison_existing_failed_comparison(self):
Expand All @@ -569,8 +617,8 @@ def test_post_comparison_existing_failed_comparison(self):
assert response.status_code == 200
data = response.json()
assert data["status"] == "exists"
assert len(data["existing_comparisons"]) == 1
comparison_data = data["existing_comparisons"][0]
assert len(data["comparisons"]) == 1
comparison_data = data["comparisons"][0]
assert comparison_data["state"] == comparison.state
assert comparison_data["error_code"] == str(comparison.error_code)
assert comparison_data["error_message"] == comparison.error_message
Expand All @@ -596,17 +644,17 @@ def test_post_comparison_cannot_compare_size_metrics(self):
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
@patch("sentry.preprod.size_analysis.tasks.manual_size_analysis_comparison.apply_async")
def test_post_comparison_multiple_metrics(self, mock_apply_async):
"""Test POST endpoint handles multiple size metrics correctly"""
"""Test POST endpoint handles multiple size metrics correctly and creates PENDING records"""
# Create additional size metrics
PreprodArtifactSizeMetrics.objects.create(
head_watch_metric = PreprodArtifactSizeMetrics.objects.create(
preprod_artifact=self.head_artifact,
analysis_file_id=self.head_analysis_file.id,
metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.WATCH_ARTIFACT,
identifier="watch",
state=PreprodArtifactSizeMetrics.SizeAnalysisState.COMPLETED,
)

PreprodArtifactSizeMetrics.objects.create(
base_watch_metric = PreprodArtifactSizeMetrics.objects.create(
preprod_artifact=self.base_artifact,
analysis_file_id=self.base_analysis_file.id,
metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.WATCH_ARTIFACT,
Expand All @@ -617,6 +665,29 @@ def test_post_comparison_multiple_metrics(self, mock_apply_async):
response = self.client.post(self._get_url())

assert response.status_code == 200
data = response.json()

# Verify response
assert data["status"] == "created"
assert len(data["comparisons"]) == 2

# Verify both comparisons are PENDING
for comparison_data in data["comparisons"]:
assert comparison_data["state"] == PreprodArtifactSizeComparison.State.PENDING
assert comparison_data["comparison_id"] is None

# Verify both PENDING records were created in database
main_comparison = PreprodArtifactSizeComparison.objects.get(
head_size_analysis=self.head_size_metric,
base_size_analysis=self.base_size_metric,
)
assert main_comparison.state == PreprodArtifactSizeComparison.State.PENDING

watch_comparison = PreprodArtifactSizeComparison.objects.get(
head_size_analysis=head_watch_metric,
base_size_analysis=base_watch_metric,
)
assert watch_comparison.state == PreprodArtifactSizeComparison.State.PENDING

mock_apply_async.assert_called_once_with(
kwargs={
Expand Down
Loading
Loading