diff --git a/src/sentry/preprod/size_analysis/compare.py b/src/sentry/preprod/size_analysis/compare.py index 42d9b309236bdd..90990860c28669 100644 --- a/src/sentry/preprod/size_analysis/compare.py +++ b/src/sentry/preprod/size_analysis/compare.py @@ -13,6 +13,10 @@ FileSavingsResultGroup, FilesInsightResult, GroupsInsightResult, + ImageOptimizationInsightResult, + OptimizableImageFile, + StripBinaryFileInfo, + StripBinaryInsightResult, ) from sentry.preprod.size_analysis.models import ( ComparisonResults, @@ -546,6 +550,138 @@ def _compare_groups( return diff_items +def _compare_optimizable_images( + head_files: list[OptimizableImageFile], base_files: list[OptimizableImageFile] +) -> list[DiffItem]: + """Compare optimizable image files between head and base builds.""" + diff_items = [] + + head_files_dict = {f.file_path: f for f in head_files} + base_files_dict = {f.file_path: f for f in base_files} + + all_paths = set(head_files_dict.keys()) | set(base_files_dict.keys()) + + for path in sorted(all_paths): + head_file = head_files_dict.get(path) + base_file = base_files_dict.get(path) + + if head_file and base_file: + # File exists in both - check for savings change + size_diff = head_file.potential_savings - base_file.potential_savings + + # Skip files with no savings change + if size_diff == 0: + continue + + diff_type = DiffType.INCREASED if size_diff > 0 else DiffType.DECREASED + + diff_items.append( + DiffItem( + size_diff=size_diff, + head_size=head_file.potential_savings, + base_size=base_file.potential_savings, + path=path, + item_type=None, + type=diff_type, + diff_items=None, + ) + ) + elif head_file: + # File added in head + diff_items.append( + DiffItem( + size_diff=head_file.potential_savings, + head_size=head_file.potential_savings, + base_size=None, + path=path, + item_type=None, + type=DiffType.ADDED, + diff_items=None, + ) + ) + elif base_file: + # File removed from head + diff_items.append( + DiffItem( + size_diff=-base_file.potential_savings, + head_size=None, + base_size=base_file.potential_savings, + path=path, + item_type=None, + type=DiffType.REMOVED, + diff_items=None, + ) + ) + + return diff_items + + +def _compare_strip_binary_files( + head_files: list[StripBinaryFileInfo], base_files: list[StripBinaryFileInfo] +) -> list[DiffItem]: + """Compare strip binary files between head and base builds.""" + diff_items = [] + + head_files_dict = {f.file_path: f for f in head_files} + base_files_dict = {f.file_path: f for f in base_files} + + all_paths = set(head_files_dict.keys()) | set(base_files_dict.keys()) + + for path in sorted(all_paths): + head_file = head_files_dict.get(path) + base_file = base_files_dict.get(path) + + if head_file and base_file: + # File exists in both - check for savings change + size_diff = head_file.total_savings - base_file.total_savings + + # Skip files with no savings change + if size_diff == 0: + continue + + diff_type = DiffType.INCREASED if size_diff > 0 else DiffType.DECREASED + + diff_items.append( + DiffItem( + size_diff=size_diff, + head_size=head_file.total_savings, + base_size=base_file.total_savings, + path=path, + item_type=None, + type=diff_type, + diff_items=None, + ) + ) + elif head_file: + # File added in head + diff_items.append( + DiffItem( + size_diff=head_file.total_savings, + head_size=head_file.total_savings, + base_size=None, + path=path, + item_type=None, + type=DiffType.ADDED, + diff_items=None, + ) + ) + elif base_file: + # File removed from head + diff_items.append( + DiffItem( + size_diff=-base_file.total_savings, + head_size=None, + base_size=base_file.total_savings, + path=path, + item_type=None, + type=DiffType.REMOVED, + diff_items=None, + ) + ) + + return diff_items + + def _diff_insight( insight_type: str, head_insight: BaseInsightResult | None, @@ -564,6 +700,16 @@ def _diff_insight( base_files = base_insight.files if isinstance(base_insight, FilesInsightResult) else [] head_groups = [] base_groups = base_insight.groups if isinstance(base_insight, GroupsInsightResult) else [] + head_optimizable_images = [] + base_optimizable_images = ( + base_insight.optimizable_files + if isinstance(base_insight, ImageOptimizationInsightResult) + else [] + ) + head_strip_binary_files = [] + base_strip_binary_files = ( + base_insight.files if isinstance(base_insight, StripBinaryInsightResult) else [] + ) elif base_insight is None: # Should never happen, but here for mypy passing assert head_insight is not None @@ -574,6 +720,16 @@ def _diff_insight( base_files = [] head_groups = head_insight.groups if isinstance(head_insight, GroupsInsightResult) else [] base_groups = [] + head_optimizable_images = ( + head_insight.optimizable_files + if isinstance(head_insight, ImageOptimizationInsightResult) + else [] + ) + base_optimizable_images = [] + head_strip_binary_files = ( + head_insight.files if isinstance(head_insight, StripBinaryInsightResult) else [] + ) + base_strip_binary_files = [] else: status = "unresolved" # Unresolved insight - compare both @@ -582,9 +738,25 @@ def _diff_insight( base_files = base_insight.files if isinstance(base_insight, FilesInsightResult) else [] head_groups = head_insight.groups if isinstance(head_insight, GroupsInsightResult) else [] base_groups = base_insight.groups if isinstance(base_insight, GroupsInsightResult) else [] + head_optimizable_images = ( + head_insight.optimizable_files + if isinstance(head_insight, ImageOptimizationInsightResult) + else [] + ) + base_optimizable_images = ( + base_insight.optimizable_files + if isinstance(base_insight, ImageOptimizationInsightResult) + else [] + ) + head_strip_binary_files = ( + head_insight.files if isinstance(head_insight, StripBinaryInsightResult) else [] + ) + base_strip_binary_files = ( + base_insight.files if isinstance(base_insight, StripBinaryInsightResult) else [] + ) - file_diffs = [] - group_diffs = [] + file_diffs: list[DiffItem] = [] + group_diffs: list[DiffItem] = [] if head_files or base_files: file_diffs = _compare_files(head_files, base_files) @@ -596,8 +768,16 @@ def _diff_insight( # If no group diffs, we don't want to show an irrelevant insight diff item if len(group_diffs) == 0: return None - - # TODO(EME-678) Implement non-File/GroupinsightsResult insight diffs in future + elif head_optimizable_images or base_optimizable_images: + file_diffs = _compare_optimizable_images(head_optimizable_images, base_optimizable_images) + # If no file diffs, we don't want to show an irrelevant insight diff item + if len(file_diffs) == 0: + return None + elif head_strip_binary_files or base_strip_binary_files: + file_diffs = _compare_strip_binary_files(head_strip_binary_files, base_strip_binary_files) + # If no file diffs, we don't want to show an irrelevant insight diff item + if len(file_diffs) == 0: + return None return InsightDiffItem( insight_type=insight_type, @@ -621,11 +801,12 @@ def _compare_insights( return insight_diff_items # Convert insights to dictionaries for easier comparison + # Use isinstance check to filter out non-insight fields like 'platform' discriminator head_insight_dict = ( { field_name: value for field_name, value in vars(head_insights).items() - if value is not None + if isinstance(value, BaseInsightResult) } if head_insights else {} @@ -634,7 +815,7 @@ def _compare_insights( { field_name: value for field_name, value in vars(base_insights).items() - if value is not None + if isinstance(value, BaseInsightResult) } if base_insights else {} diff --git a/src/sentry/preprod/size_analysis/models.py b/src/sentry/preprod/size_analysis/models.py index 85ad4317f67b13..5a4f2d035bd4b0 100644 --- a/src/sentry/preprod/size_analysis/models.py +++ b/src/sentry/preprod/size_analysis/models.py @@ -1,8 +1,9 @@ from __future__ import annotations from enum import Enum +from typing import Annotated, Literal -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, Field from sentry.preprod.models import PreprodArtifactSizeMetrics from sentry.preprod.size_analysis.insight_models import ( @@ -31,6 +32,7 @@ class AndroidInsightResults(BaseModel): + platform: Literal["android"] = "android" duplicate_files: DuplicateFilesInsightResult | None = None webp_optimization: WebPOptimizationInsightResult | None = None large_images: LargeImageFileInsightResult | None = None @@ -41,6 +43,7 @@ class AndroidInsightResults(BaseModel): class AppleInsightResults(BaseModel): + platform: Literal["apple"] = "apple" duplicate_files: DuplicateFilesInsightResult | None = None large_images: LargeImageFileInsightResult | None = None large_audios: LargeAudioFileInsightResult | None = None @@ -134,7 +137,10 @@ class SizeAnalysisResults(BaseModel): download_size: int install_size: int treemap: TreemapResults | None = None - insights: AndroidInsightResults | AppleInsightResults | None = None + insights: ( + Annotated[AndroidInsightResults | AppleInsightResults, Field(discriminator="platform")] + | None + ) = None analysis_version: str | None = None file_analysis: FileAnalysis | None = None app_components: list[AppComponent] | None = None diff --git a/static/app/views/preprod/buildComparison/main/insightComparisonSection.tsx b/static/app/views/preprod/buildComparison/main/insightComparisonSection.tsx index 82bb7fd0f33bdd..8fb820f6cd2396 100644 --- a/static/app/views/preprod/buildComparison/main/insightComparisonSection.tsx +++ b/static/app/views/preprod/buildComparison/main/insightComparisonSection.tsx @@ -33,6 +33,7 @@ const FILE_DIFF_INSIGHT_TYPES = [ 'multiple_native_library_archs', 'video_compression', 'image_optimization', + 'strip_binary', ]; const GROUP_DIFF_INSIGHT_TYPES = ['duplicate_files', 'loose_images']; diff --git a/tests/sentry/preprod/size_analysis/test_compare.py b/tests/sentry/preprod/size_analysis/test_compare.py index 469b823bae14f3..25d39d74f9cce0 100644 --- a/tests/sentry/preprod/size_analysis/test_compare.py +++ b/tests/sentry/preprod/size_analysis/test_compare.py @@ -7,7 +7,11 @@ DuplicateFilesInsightResult, FileSavingsResult, FileSavingsResultGroup, + ImageOptimizationInsightResult, LargeImageFileInsightResult, + OptimizableImageFile, + StripBinaryFileInfo, + StripBinaryInsightResult, WebPOptimizationInsightResult, ) from sentry.preprod.size_analysis.models import ( @@ -1306,13 +1310,17 @@ def _create_size_analysis_results( insights: AndroidInsightResults | AppleInsightResults | None = None, analysis_version="1.0.0", ): - return SizeAnalysisResults( + # Use construct() to bypass Pydantic union coercion which would + # otherwise convert AppleInsightResults to AndroidInsightResults + return SizeAnalysisResults.construct( analysis_duration=1.0, download_size=500, install_size=1000, treemap=None, insights=insights, analysis_version=analysis_version, + file_analysis=None, + app_components=None, ) def test_compare_insights_new_insight_in_head(self): @@ -2100,3 +2108,233 @@ def test_compare_insights_groups_with_identical_groups(self): # No insight diff items should be returned assert len(result.insight_diff_items) == 0 + + def test_compare_insights_image_optimization_with_diffs(self): + """Test ImageOptimizationInsightResult with file-level differences.""" + head_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1000, + max_download_size=500, + ) + base_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=2, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1000, + max_download_size=500, + ) + + # Head: icon.png (increased), banner.png (new) + # Base: icon.png (original), logo.png (removed) + head_insights = AppleInsightResults( + duplicate_files=None, + large_images=None, + large_audios=None, + large_videos=None, + strip_binary=None, + localized_strings_minify=None, + small_files=None, + loose_images=None, + hermes_debug_info=None, + image_optimization=ImageOptimizationInsightResult( + total_savings=6000, + optimizable_files=[ + OptimizableImageFile( + file_path="/path/icon.png", + current_size=10000, + minify_savings=3000, + conversion_savings=4000, + ), + OptimizableImageFile( + file_path="/path/banner.png", + current_size=5000, + minify_savings=2000, + conversion_savings=1000, + ), + ], + ), + main_binary_exported_symbols=None, + unnecessary_files=None, + audio_compression=None, + video_compression=None, + alternate_icons_optimization=None, + ) + base_insights = AppleInsightResults( + duplicate_files=None, + large_images=None, + large_audios=None, + large_videos=None, + strip_binary=None, + localized_strings_minify=None, + small_files=None, + loose_images=None, + hermes_debug_info=None, + image_optimization=ImageOptimizationInsightResult( + total_savings=5000, + optimizable_files=[ + OptimizableImageFile( + file_path="/path/icon.png", + current_size=10000, + minify_savings=2000, + conversion_savings=2000, + ), + OptimizableImageFile( + file_path="/path/logo.png", + current_size=6000, + minify_savings=1000, + conversion_savings=3000, + ), + ], + ), + main_binary_exported_symbols=None, + unnecessary_files=None, + audio_compression=None, + video_compression=None, + alternate_icons_optimization=None, + ) + + head_results = self._create_size_analysis_results(insights=head_insights) + base_results = self._create_size_analysis_results(insights=base_insights) + + result = compare_size_analysis(head_metrics, head_results, base_metrics, base_results) + + assert len(result.insight_diff_items) == 1 + insight_diff = result.insight_diff_items[0] + assert insight_diff.insight_type == "image_optimization" + assert insight_diff.status == "unresolved" + assert insight_diff.total_savings_change == 1000 + + # Should have 3 file diffs: icon.png increased, banner.png added, logo.png removed + assert len(insight_diff.file_diffs) == 3 + file_diffs_by_path = {d.path: d for d in insight_diff.file_diffs} + + # icon.png - increased (potential_savings: 2000 -> 4000) + assert file_diffs_by_path["/path/icon.png"].type.value == "increased" + assert file_diffs_by_path["/path/icon.png"].size_diff == 2000 + + # banner.png - added (potential_savings: 2000) + assert file_diffs_by_path["/path/banner.png"].type.value == "added" + assert file_diffs_by_path["/path/banner.png"].size_diff == 2000 + + # logo.png - removed (potential_savings: 3000) + assert file_diffs_by_path["/path/logo.png"].type.value == "removed" + assert file_diffs_by_path["/path/logo.png"].size_diff == -3000 + + def test_compare_insights_strip_binary_with_diffs(self): + """Test StripBinaryInsightResult with file-level differences.""" + head_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=1, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1000, + max_download_size=500, + ) + base_metrics = PreprodArtifactSizeMetrics( + preprod_artifact_id=2, + metrics_artifact_type=PreprodArtifactSizeMetrics.MetricsArtifactType.MAIN_ARTIFACT, + identifier="test", + max_install_size=1000, + max_download_size=500, + ) + + # Head: MyApp (increased), NewFramework (added) + # Base: MyApp (original), OldFramework (removed) + head_insights = AppleInsightResults( + duplicate_files=None, + large_images=None, + large_audios=None, + large_videos=None, + strip_binary=StripBinaryInsightResult( + total_savings=18000, + files=[ + StripBinaryFileInfo( + file_path="/path/MyApp", + debug_sections_savings=8000, + symbol_table_savings=4000, + total_savings=12000, + ), + StripBinaryFileInfo( + file_path="/path/NewFramework", + debug_sections_savings=4000, + symbol_table_savings=2000, + total_savings=6000, + ), + ], + total_debug_sections_savings=12000, + total_symbol_table_savings=6000, + ), + localized_strings_minify=None, + small_files=None, + loose_images=None, + hermes_debug_info=None, + image_optimization=None, + main_binary_exported_symbols=None, + unnecessary_files=None, + audio_compression=None, + video_compression=None, + alternate_icons_optimization=None, + ) + base_insights = AppleInsightResults( + duplicate_files=None, + large_images=None, + large_audios=None, + large_videos=None, + strip_binary=StripBinaryInsightResult( + total_savings=15000, + files=[ + StripBinaryFileInfo( + file_path="/path/MyApp", + debug_sections_savings=6000, + symbol_table_savings=4000, + total_savings=10000, + ), + StripBinaryFileInfo( + file_path="/path/OldFramework", + debug_sections_savings=3000, + symbol_table_savings=2000, + total_savings=5000, + ), + ], + total_debug_sections_savings=9000, + total_symbol_table_savings=6000, + ), + localized_strings_minify=None, + small_files=None, + loose_images=None, + hermes_debug_info=None, + image_optimization=None, + main_binary_exported_symbols=None, + unnecessary_files=None, + audio_compression=None, + video_compression=None, + alternate_icons_optimization=None, + ) + + head_results = self._create_size_analysis_results(insights=head_insights) + base_results = self._create_size_analysis_results(insights=base_insights) + + result = compare_size_analysis(head_metrics, head_results, base_metrics, base_results) + + assert len(result.insight_diff_items) == 1 + insight_diff = result.insight_diff_items[0] + assert insight_diff.insight_type == "strip_binary" + assert insight_diff.status == "unresolved" + assert insight_diff.total_savings_change == 3000 + + # Should have 3 file diffs: MyApp increased, NewFramework added, OldFramework removed + assert len(insight_diff.file_diffs) == 3 + file_diffs_by_path = {d.path: d for d in insight_diff.file_diffs} + + # MyApp - increased (10000 -> 12000) + assert file_diffs_by_path["/path/MyApp"].type.value == "increased" + assert file_diffs_by_path["/path/MyApp"].size_diff == 2000 + + # NewFramework - added (6000) + assert file_diffs_by_path["/path/NewFramework"].type.value == "added" + assert file_diffs_by_path["/path/NewFramework"].size_diff == 6000 + + # OldFramework - removed (5000) + assert file_diffs_by_path["/path/OldFramework"].type.value == "removed" + assert file_diffs_by_path["/path/OldFramework"].size_diff == -5000 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 12fafe22cfe0f2..c39b53680fba13 100644 --- a/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py +++ b/tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py @@ -780,6 +780,74 @@ def test_run_size_analysis_comparison_success(self): assert "diff_items" in file_content assert "size_metric_diff_item" in file_content + def test_run_size_analysis_comparison_preserves_apple_insights(self): + """Test _run_size_analysis_comparison with Apple-only insights.""" + head_analysis_data = { + "analysis_duration": 0.5, + "download_size": 1000, + "install_size": 2000, + "insights": { + "platform": "apple", + "image_optimization": { + "total_savings": 1200, + "optimizable_files": [ + { + "file_path": "Assets/foo.png", + "current_size": 2000, + "minify_savings": 1200, + "minified_size": 800, + "conversion_savings": 0, + "heic_size": None, + } + ], + }, + "strip_binary": { + "total_savings": 500, + "files": [ + { + "file_path": "HackerNews", + "debug_sections_savings": 300, + "symbol_table_savings": 200, + "total_savings": 500, + } + ], + "total_debug_sections_savings": 300, + "total_symbol_table_savings": 200, + }, + }, + } + base_analysis_data = { + "analysis_duration": 0.4, + "download_size": 800, + "install_size": 1500, + } + + head_size_metrics = self._create_size_metrics_with_analysis_file(head_analysis_data) + base_size_metrics = self._create_size_metrics_with_analysis_file(base_analysis_data) + + self.create_preprod_artifact_size_comparison( + head_size_analysis=head_size_metrics, + base_size_analysis=base_size_metrics, + organization=self.organization, + state=PreprodArtifactSizeComparison.State.PENDING, + ) + + _run_size_analysis_comparison( + self.organization.id, + head_size_metrics, + base_size_metrics, + ) + + comparison = PreprodArtifactSizeComparison.objects.get( + head_size_analysis=head_size_metrics, + base_size_analysis=base_size_metrics, + ) + comparison_file = File.objects.get(id=comparison.file_id) + file_content = json.loads(comparison_file.getfile().read().decode()) + insight_types = [item["insight_type"] for item in file_content["insight_diff_items"]] + assert "image_optimization" in insight_types + assert "strip_binary" in insight_types + def test_run_size_analysis_comparison_existing_comparison_processing(self): """Test _run_size_analysis_comparison with existing processing comparison.""" # Create size metrics