From 9e21f15bd1d46474c38f05d5adbc59b273aad6e6 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 18:05:23 -0500 Subject: [PATCH 1/6] fix --- src/sentry/preprod/size_analysis/compare.py | 177 +++++++++++++------- src/sentry/preprod/size_analysis/models.py | 3 + 2 files changed, 121 insertions(+), 59 deletions(-) diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 2475c629db9b7a..0a0df5a71bbaf2 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -2,6 +2,9 @@ import logging +from packaging.version import InvalidVersion +from packaging.version import parse as parse_version + from sentry.preprod.models import PreprodArtifactSizeMetrics from sentry.preprod.size_analysis.models import ( ComparisonResults, @@ -21,7 +24,9 @@ def compare_size_analysis( base_size_analysis: PreprodArtifactSizeMetrics, base_size_analysis_results: SizeAnalysisResults, ) -> ComparisonResults: - diff_items = [] + skip_diff_item_comparison = _should_skip_diff_comparison( + head_size_analysis_results, base_size_analysis_results + ) head_treemap = ( head_size_analysis_results.treemap.root if head_size_analysis_results.treemap else None @@ -35,70 +40,73 @@ def compare_size_analysis( all_paths = set(head_files.keys()) | set(base_files.keys()) - for path in sorted(all_paths): - head_elements = head_files.get(path, []) - base_elements = base_files.get(path, []) - - matched_pairs, unmatched_head, unmatched_base = _match_elements( - head_elements, base_elements - ) - - # Process matched pairs (modified or unchanged) - for head_element, base_element in matched_pairs: - head_size = head_element.size - base_size = base_element.size - size_diff = head_size - base_size - - if size_diff == 0: - continue # Skip items with no size change - - item_type = head_element.type - diff_type = DiffType.INCREASED if size_diff > 0 else DiffType.DECREASED - - diff_items.append( - DiffItem( - size_diff=size_diff, - head_size=head_size, - base_size=base_size, - path=path, - item_type=item_type, - type=diff_type, - ) - ) + diff_items = [] - # Process unmatched head elements (added) - for head_element in unmatched_head: - head_size = head_element.size - if head_size == 0: - continue + if not skip_diff_item_comparison: + for path in sorted(all_paths): + head_elements = head_files.get(path, []) + base_elements = base_files.get(path, []) - diff_items.append( - DiffItem( - size_diff=head_size, - head_size=head_size, - base_size=None, - path=path, - item_type=head_element.type, - type=DiffType.ADDED, - ) + matched_pairs, unmatched_head, unmatched_base = _match_elements( + head_elements, base_elements ) - # Process unmatched base elements (removed) - for base_element in unmatched_base: - base_size = base_element.size - if base_size == 0: - continue + # Process matched pairs (modified or unchanged) + for head_element, base_element in matched_pairs: + head_size = head_element.size + base_size = base_element.size + size_diff = head_size - base_size + + if size_diff == 0: + continue # Skip items with no size change + + item_type = head_element.type + diff_type = DiffType.INCREASED if size_diff > 0 else DiffType.DECREASED + + diff_items.append( + DiffItem( + size_diff=size_diff, + head_size=head_size, + base_size=base_size, + path=path, + item_type=item_type, + type=diff_type, + ) + ) - diff_items.append( - DiffItem( - size_diff=-base_size, - head_size=None, - base_size=base_size, - path=path, - item_type=base_element.type, - type=DiffType.REMOVED, + # Process unmatched head elements (added) + for head_element in unmatched_head: + head_size = head_element.size + if head_size == 0: + continue + + diff_items.append( + DiffItem( + size_diff=head_size, + head_size=head_size, + base_size=None, + path=path, + item_type=head_element.type, + type=DiffType.ADDED, + ) + ) + + # Process unmatched base elements (removed) + for base_element in unmatched_base: + base_size = base_element.size + if base_size == 0: + continue + + diff_items.append( + DiffItem( + size_diff=-base_size, + head_size=None, + base_size=base_size, + path=path, + item_type=base_element.type, + type=DiffType.REMOVED, + ) ) - ) size_metric_diff_item = SizeMetricDiffItem( metrics_artifact_type=head_size_analysis.metrics_artifact_type, @@ -112,9 +120,60 @@ def compare_size_analysis( return ComparisonResults( diff_items=diff_items, size_metric_diff_item=size_metric_diff_item, + skipped_diff_item_comparison=skip_diff_item_comparison, + head_analysis_version=head_size_analysis_results.analysis_version, + base_analysis_version=base_size_analysis_results.analysis_version, ) +def _should_skip_diff_comparison( + head_size_analysis_results: SizeAnalysisResults, + base_size_analysis_results: SizeAnalysisResults, +) -> bool: + """ + Determine if diff item comparison should be skipped based on analysis version compatibility. + + Skips comparison when: + - Major versions differ (incompatible analysis formats) + - Minor versions differ (potentially incompatible tree structures) + + Returns: + True if diff comparison should be skipped, False otherwise + """ + head_version = None + base_version = None + + if head_size_analysis_results.analysis_version: + try: + head_version = parse_version(head_size_analysis_results.analysis_version) + except InvalidVersion: + logger.warning( + "preprod.size_analysis.compare.invalid_version_format", + extra={ + "analysis_version": head_size_analysis_results.analysis_version, + }, + ) + + if base_size_analysis_results.analysis_version: + try: + base_version = parse_version(base_size_analysis_results.analysis_version) + except InvalidVersion: + logger.warning( + "preprod.size_analysis.compare.invalid_version_format", + extra={ + "analysis_version": head_size_analysis_results.analysis_version, + }, + ) + + if not head_version or not base_version: + return False + + has_mismatched_major = head_version.major != base_version.major + has_mismatched_minor = head_version.minor != base_version.minor + + return has_mismatched_major or has_mismatched_minor + + def _match_elements( head_elements: list[TreemapElement], base_elements: list[TreemapElement] ) -> tuple[list[tuple[TreemapElement, TreemapElement]], list[TreemapElement], list[TreemapElement]]: diff --git a/src/sentry/preprod/size_analysis/models.py b/src/sentry/preprod/size_analysis/models.py index 8a567aef627508..dc4f923fd7bde1 100644 --- a/src/sentry/preprod/size_analysis/models.py +++ b/src/sentry/preprod/size_analysis/models.py @@ -73,3 +73,6 @@ class SizeMetricDiffItem(BaseModel): class ComparisonResults(BaseModel): diff_items: list[DiffItem] size_metric_diff_item: SizeMetricDiffItem + skipped_diff_item_comparison: bool + head_analysis_version: str | None + base_analysis_version: str | None From c8fd428aecf8c73b3ec55115948997755d64e07a Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 18:09:42 -0500 Subject: [PATCH 2/6] fix --- src/sentry/preprod/size_analysis/compare.py | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 0a0df5a71bbaf2..947d6ca3777bd3 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -24,7 +24,7 @@ def compare_size_analysis( base_size_analysis: PreprodArtifactSizeMetrics, base_size_analysis_results: SizeAnalysisResults, ) -> ComparisonResults: - skip_diff_item_comparison = _should_skip_diff_comparison( + skip_diff_item_comparison = _should_skip_diff_item_comparison( head_size_analysis_results, base_size_analysis_results ) @@ -126,20 +126,10 @@ def compare_size_analysis( ) -def _should_skip_diff_comparison( +def _should_skip_diff_item_comparison( head_size_analysis_results: SizeAnalysisResults, base_size_analysis_results: SizeAnalysisResults, ) -> bool: - """ - Determine if diff item comparison should be skipped based on analysis version compatibility. - - Skips comparison when: - - Major versions differ (incompatible analysis formats) - - Minor versions differ (potentially incompatible tree structures) - - Returns: - True if diff comparison should be skipped, False otherwise - """ head_version = None base_version = None From 9bb7da95ad7c47cb320d8b621acc1d2e3b0e7365 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 18:25:28 -0500 Subject: [PATCH 3/6] tests --- .../preprod/size_analysis/test_compare.py | 269 +++++++++++++++++- 1 file changed, 268 insertions(+), 1 deletion(-) diff --git a/tests/sentry/preprod/size_analysis/test_compare.py b/tests/sentry/preprod/size_analysis/test_compare.py index 5dfd6ae75f51ef..6cb31201629a57 100644 --- a/tests/sentry/preprod/size_analysis/test_compare.py +++ b/tests/sentry/preprod/size_analysis/test_compare.py @@ -1,5 +1,8 @@ from sentry.preprod.models import PreprodArtifactSizeMetrics -from sentry.preprod.size_analysis.compare import compare_size_analysis +from sentry.preprod.size_analysis.compare import ( + _should_skip_diff_item_comparison, + compare_size_analysis, +) from sentry.preprod.size_analysis.models import ( ComparisonResults, DiffType, @@ -526,3 +529,267 @@ def test_compare_size_analysis_duplicate_paths(self): assert diff_items[2].path == "Assets.car/Primary-Light@2x.png" assert diff_items[2].size_diff == 802816 assert diff_items[2].type == DiffType.ADDED + + +class ShouldSkipDiffItemComparisonTest(TestCase): + def _create_size_analysis_results(self, analysis_version=None): + """Helper to create SizeAnalysisResults with specified analysis_version.""" + return SizeAnalysisResults( + download_size=500, + install_size=1000, + treemap=None, + analysis_version=analysis_version, + ) + + def test_skip_on_major_version_mismatch(self): + """Should skip diff comparison when major versions differ.""" + head_results = self._create_size_analysis_results(analysis_version="2.0.0") + base_results = self._create_size_analysis_results(analysis_version="1.0.0") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is True + + def test_skip_on_minor_version_mismatch(self): + """Should skip diff comparison when minor versions differ.""" + head_results = self._create_size_analysis_results(analysis_version="1.2.0") + base_results = self._create_size_analysis_results(analysis_version="1.1.0") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is True + + def test_no_skip_on_patch_version_mismatch(self): + """Should NOT skip diff comparison when only patch versions differ.""" + head_results = self._create_size_analysis_results(analysis_version="1.0.1") + base_results = self._create_size_analysis_results(analysis_version="1.0.0") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_on_same_versions(self): + """Should NOT skip diff comparison when versions are identical.""" + head_results = self._create_size_analysis_results(analysis_version="1.2.3") + base_results = self._create_size_analysis_results(analysis_version="1.2.3") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_when_head_version_missing(self): + """Should NOT skip diff comparison when head version is None.""" + head_results = self._create_size_analysis_results(analysis_version=None) + base_results = self._create_size_analysis_results(analysis_version="1.0.0") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_when_base_version_missing(self): + """Should NOT skip diff comparison when base version is None.""" + head_results = self._create_size_analysis_results(analysis_version="1.0.0") + base_results = self._create_size_analysis_results(analysis_version=None) + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_when_both_versions_missing(self): + """Should NOT skip diff comparison when both versions are None.""" + head_results = self._create_size_analysis_results(analysis_version=None) + base_results = self._create_size_analysis_results(analysis_version=None) + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_on_invalid_head_version(self): + """Should NOT skip diff comparison when head version is invalid.""" + head_results = self._create_size_analysis_results(analysis_version="invalid-version") + base_results = self._create_size_analysis_results(analysis_version="1.0.0") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_on_invalid_base_version(self): + """Should NOT skip diff comparison when base version is invalid.""" + head_results = self._create_size_analysis_results(analysis_version="1.0.0") + base_results = self._create_size_analysis_results(analysis_version="not-a-version") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_no_skip_when_both_versions_invalid(self): + """Should NOT skip diff comparison when both versions are invalid.""" + head_results = self._create_size_analysis_results(analysis_version="bad-version") + base_results = self._create_size_analysis_results(analysis_version="also-bad") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + def test_skip_with_prerelease_versions(self): + """Should skip when major/minor differ even with pre-release tags.""" + head_results = self._create_size_analysis_results(analysis_version="2.0.0-alpha") + base_results = self._create_size_analysis_results(analysis_version="1.0.0-beta") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is True + + def test_no_skip_with_same_major_minor_prerelease(self): + """Should NOT skip when major/minor match despite different pre-release tags.""" + head_results = self._create_size_analysis_results(analysis_version="1.2.0-alpha") + base_results = self._create_size_analysis_results(analysis_version="1.2.0-beta") + + result = _should_skip_diff_item_comparison(head_results, base_results) + + assert result is False + + +class CompareWithVersionSkippingTest(TestCase): + def setUp(self): + super().setUp() + self.organization = self.create_organization(owner=self.user) + self.project = self.create_project(organization=self.organization) + + def _create_treemap_element(self, name, size, path=None, children=None): + """Helper to create TreemapElement.""" + return TreemapElement( + name=name, + size=size, + path=path, + is_dir=children is not None, + children=children or [], + ) + + def _create_size_analysis_results( + self, download_size=500, install_size=1000, treemap_root=None, analysis_version=None + ): + """Helper to create SizeAnalysisResults.""" + treemap = None + if treemap_root: + treemap = TreemapResults( + root=treemap_root, + file_count=1, + category_breakdown={}, + platform="test", + ) + + return SizeAnalysisResults( + download_size=download_size, + install_size=install_size, + treemap=treemap, + analysis_version=analysis_version, + ) + + def test_compare_skips_diff_items_on_major_version_mismatch(self): + """Integration test: diff items should be skipped when major versions differ.""" + head_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=2000, + max_download_size=1000, + ) + base_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1500, + max_download_size=800, + ) + + # Create treemaps with differences + head_treemap = self._create_treemap_element("file.txt", 150) + base_treemap = self._create_treemap_element("file.txt", 100) + + head_results = self._create_size_analysis_results( + treemap_root=head_treemap, analysis_version="2.0.0" + ) + base_results = self._create_size_analysis_results( + treemap_root=base_treemap, analysis_version="1.0.0" + ) + + result = compare_size_analysis(head_metrics, head_results, base_metrics, base_results) + + # Diff items should be empty due to version mismatch + assert result.diff_items == [] + assert result.skipped_diff_item_comparison is True + assert result.head_analysis_version == "2.0.0" + assert result.base_analysis_version == "1.0.0" + # Size metrics should still be populated + assert result.size_metric_diff_item.head_install_size == 2000 + assert result.size_metric_diff_item.base_install_size == 1500 + + def test_compare_skips_diff_items_on_minor_version_mismatch(self): + """Integration test: diff items should be skipped when minor versions differ.""" + head_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=2000, + max_download_size=1000, + ) + base_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1500, + max_download_size=800, + ) + + head_treemap = self._create_treemap_element("file.txt", 150) + base_treemap = self._create_treemap_element("file.txt", 100) + + head_results = self._create_size_analysis_results( + treemap_root=head_treemap, analysis_version="1.2.0" + ) + base_results = self._create_size_analysis_results( + treemap_root=base_treemap, analysis_version="1.1.0" + ) + + result = compare_size_analysis(head_metrics, head_results, base_metrics, base_results) + + assert result.diff_items == [] + assert result.skipped_diff_item_comparison is True + assert result.head_analysis_version == "1.2.0" + assert result.base_analysis_version == "1.1.0" + + def test_compare_includes_diff_items_on_patch_version_mismatch(self): + """Integration test: diff items should be included when only patch versions differ.""" + head_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=2000, + max_download_size=1000, + ) + base_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1500, + max_download_size=800, + ) + + head_treemap = self._create_treemap_element("file.txt", 150) + base_treemap = self._create_treemap_element("file.txt", 100) + + head_results = self._create_size_analysis_results( + treemap_root=head_treemap, analysis_version="1.0.2" + ) + base_results = self._create_size_analysis_results( + treemap_root=base_treemap, analysis_version="1.0.1" + ) + + result = compare_size_analysis(head_metrics, head_results, base_metrics, base_results) + + # Diff items should be present since only patch version differs + assert len(result.diff_items) == 1 + assert result.skipped_diff_item_comparison is False + assert result.diff_items[0].size_diff == 50 + assert result.diff_items[0].type == DiffType.INCREASED From a645506444690ac37f13f0a2c208428ca14905b1 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 18:31:57 -0500 Subject: [PATCH 4/6] fix --- src/sentry/preprod/size_analysis/compare.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 947d6ca3777bd3..2a3ec4897ad4cf 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -151,7 +151,7 @@ def _should_skip_diff_item_comparison( logger.warning( "preprod.size_analysis.compare.invalid_version_format", extra={ - "analysis_version": head_size_analysis_results.analysis_version, + "analysis_version": base_size_analysis_results.analysis_version, }, ) From 179fd7bdad3bc83f5ddc4c2aa9df1378d743f216 Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 19:07:06 -0500 Subject: [PATCH 5/6] fix --- src/sentry/preprod/size_analysis/compare.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 2a3ec4897ad4cf..30c880b1ead4e2 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -107,6 +107,16 @@ def compare_size_analysis( type=DiffType.REMOVED, ) ) + else: + logger.info( + "preprod.size_analysis.compare.skipped_diff_item_comparison", + extra={ + "head_analysis_version": head_size_analysis_results.analysis_version, + "base_analysis_version": base_size_analysis_results.analysis_version, + "organization_id": head_size_analysis.organization_id, + "project_id": head_size_analysis.project_id, + }, + ) size_metric_diff_item = SizeMetricDiffItem( metrics_artifact_type=head_size_analysis.metrics_artifact_type, From 96b913c0e0e01031dca32e3816518ab7d2b4b80a Mon Sep 17 00:00:00 2001 From: Trevor Elkins Date: Tue, 11 Nov 2025 19:23:17 -0500 Subject: [PATCH 6/6] ugh --- src/sentry/preprod/size_analysis/compare.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 30c880b1ead4e2..afed78593f8db4 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -113,8 +113,7 @@ def compare_size_analysis( extra={ "head_analysis_version": head_size_analysis_results.analysis_version, "base_analysis_version": base_size_analysis_results.analysis_version, - "organization_id": head_size_analysis.organization_id, - "project_id": head_size_analysis.project_id, + "preprod_artifact_id": head_size_analysis.preprod_artifact_id, }, )