diff --git a/src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py b/src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py index 919e3ac31ee0aa..8d8830b6050737 100644 --- a/src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py +++ b/src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py @@ -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, @@ -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) diff --git a/src/sentry/preprod/api/models/size_analysis/project_preprod_size_analysis_compare_models.py b/src/sentry/preprod/api/models/size_analysis/project_preprod_size_analysis_compare_models.py index 8ab754a9a9d1b1..8d67d211b04115 100644 --- a/src/sentry/preprod/api/models/size_analysis/project_preprod_size_analysis_compare_models.py +++ b/src/sentry/preprod/api/models/size_analysis/project_preprod_size_analysis_compare_models.py @@ -29,4 +29,4 @@ class SizeAnalysisCompareGETResponse(BaseModel): class SizeAnalysisComparePOSTResponse(BaseModel): status: str message: str - existing_comparisons: list[SizeAnalysisComparison] | None + comparisons: list[SizeAnalysisComparison] | None diff --git a/src/sentry/preprod/size_analysis/tasks.py b/src/sentry/preprod/size_analysis/tasks.py index a06d0dc6b99511..28909f2dc887d1 100644 --- a/src/sentry/preprod/size_analysis/tasks.py +++ b/src/sentry/preprod/size_analysis/tasks.py @@ -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", @@ -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", @@ -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", @@ -278,11 +272,11 @@ 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, @@ -290,7 +284,7 @@ def _run_size_analysis_comparison( 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, @@ -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, + }, + ) + except PreprodArtifactSizeComparison.DoesNotExist: logger.info( "preprod.size_analysis.compare.no_existing_comparison", @@ -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: @@ -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: @@ -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()) @@ -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, diff --git a/tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py b/tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py index 1682685b1965d8..f7ca752af9333f 100644 --- a/tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py +++ b/tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py @@ -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""" @@ -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, @@ -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""" @@ -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): @@ -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 @@ -596,9 +644,9 @@ 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, @@ -606,7 +654,7 @@ def test_post_comparison_multiple_metrics(self, mock_apply_async): 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, @@ -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={ diff --git a/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py b/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py index 3ec4196e99e4fd..1223056c29c4ff 100644 --- a/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py +++ b/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py @@ -185,10 +185,9 @@ def test_compare_preprod_artifact_size_analysis_success_as_head(self): mock_run_comparison.assert_called_once() call_args = mock_run_comparison.call_args[0] # Verify that the call was made with the correct parameters - assert call_args[0] == self.project.id - assert call_args[1] == self.organization.id - assert call_args[2].preprod_artifact.id == head_artifact.id - assert call_args[3].preprod_artifact.id == base_artifact.id + assert call_args[0] == self.organization.id + assert call_args[1].preprod_artifact.id == head_artifact.id + assert call_args[2].preprod_artifact.id == base_artifact.id def test_compare_preprod_artifact_size_analysis_success_as_base(self): """Test compare_preprod_artifact_size_analysis with artifact as base.""" @@ -271,10 +270,9 @@ def test_compare_preprod_artifact_size_analysis_success_as_base(self): mock_run_comparison.assert_called_once() call_args = mock_run_comparison.call_args[0] # Verify that the call was made with the correct parameters - assert call_args[0] == self.project.id - assert call_args[1] == self.organization.id - assert call_args[2].preprod_artifact.id == head_artifact.id - assert call_args[3].preprod_artifact.id == base_artifact.id + assert call_args[0] == self.organization.id + assert call_args[1].preprod_artifact.id == head_artifact.id + assert call_args[2].preprod_artifact.id == base_artifact.id def test_compare_preprod_artifact_size_analysis_no_matching_artifacts(self): """Test compare_preprod_artifact_size_analysis with no matching artifacts.""" @@ -528,7 +526,6 @@ def test_manual_size_analysis_comparison_success(self): ) mock_run_comparison.assert_called_once_with( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -659,7 +656,6 @@ def test_run_size_analysis_comparison_success(self): # Run comparison _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -706,7 +702,6 @@ def test_run_size_analysis_comparison_existing_comparison_processing(self): with patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger: _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -745,7 +740,6 @@ def test_run_size_analysis_comparison_existing_comparison_success(self): with patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger: _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -785,7 +779,6 @@ def test_run_size_analysis_comparison_missing_head_analysis_file(self): with patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger: _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -800,7 +793,8 @@ def test_run_size_analysis_comparison_missing_head_analysis_file(self): head_size_analysis=head_size_metrics, base_size_analysis=base_size_metrics, ) - assert len(comparisons) == 0 + assert len(comparisons) == 1 + assert comparisons[0].state == PreprodArtifactSizeComparison.State.FAILED def test_run_size_analysis_comparison_missing_base_analysis_file(self): """Test _run_size_analysis_comparison with missing base analysis file.""" @@ -823,7 +817,6 @@ def test_run_size_analysis_comparison_missing_base_analysis_file(self): with patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger: _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -838,7 +831,8 @@ def test_run_size_analysis_comparison_missing_base_analysis_file(self): head_size_analysis=head_size_metrics, base_size_analysis=base_size_metrics, ) - assert len(comparisons) == 0 + assert len(comparisons) == 1 + assert comparisons[0].state == PreprodArtifactSizeComparison.State.FAILED def test_run_size_analysis_comparison_invalid_json(self): """Test _run_size_analysis_comparison with invalid JSON in analysis files.""" @@ -885,7 +879,6 @@ def test_run_size_analysis_comparison_invalid_json(self): # Should raise an exception due to invalid JSON with pytest.raises(Exception): _run_size_analysis_comparison( - self.project.id, self.organization.id, head_size_metrics, base_size_metrics, @@ -896,4 +889,5 @@ def test_run_size_analysis_comparison_invalid_json(self): head_size_analysis=head_size_metrics, base_size_analysis=base_size_metrics, ) - assert len(comparisons) == 0 + assert len(comparisons) == 1 + assert comparisons[0].state == PreprodArtifactSizeComparison.State.FAILED