fix(package_size): Sort the size in display message

tasks/libs/package/size.py

@@ -7,7 +7,7 @@
 from tasks.libs.common.constants import ORIGIN_CATEGORY, ORIGIN_PRODUCT, ORIGIN_SERVICE
 from tasks.libs.common.git import get_default_branch
 from tasks.libs.common.utils import get_metric_origin
-from tasks.libs.package.utils import get_package_path
+from tasks.libs.package.utils import find_package
 
 DEBIAN_OS = "debian"
 CENTOS_OS = "centos"
@@ -158,33 +158,23 @@ def compute_package_size_metrics(
     return series
 
 
-def compare(ctx, package_sizes, ancestor, arch, flavor, os_name, threshold):
+def compare(ctx, package_sizes, ancestor, pkg_size):
     """
-    Compare (or update) a package size with the ancestor package size.
+    Compare (or update, when on main branch) a package size with the ancestor package size.
     """
-    if os_name == 'suse':
-        dir = os.environ['OMNIBUS_PACKAGE_DIR_SUSE']
-        path = f'{dir}/{flavor}-7*{arch}.rpm'
-    else:
-        dir = os.environ['OMNIBUS_PACKAGE_DIR']
-        separator = '_' if os_name == 'deb' else '-'
-        path = f'{dir}/{flavor}{separator}7*{arch}.{os_name}'
-    package_size = _get_uncompressed_size(ctx, get_package_path(path), os_name)
+    current_size = _get_uncompressed_size(ctx, find_package(pkg_size.path()), pkg_size.os)
     if os.environ['CI_COMMIT_REF_NAME'] == get_default_branch():
-        package_sizes[ancestor][arch][flavor][os_name] = package_size
+        # On main, ancestor is the current commit, so we set the current value
+        package_sizes[ancestor][pkg_size.arch][pkg_size.flavor][pkg_size.os] = current_size
         return
-    previous_size = package_sizes[ancestor][arch][flavor][os_name]
-    diff = package_size - previous_size
-
-    message = f"{flavor}-{arch}-{os_name} size {mb(package_size)} is OK: {mb(diff)} diff with previous {mb(previous_size)} (max: {mb(threshold)})"
+    previous_size = package_sizes[ancestor][pkg_size.arch][pkg_size.flavor][pkg_size.os]
+    pkg_size.compare(current_size, previous_size)
 
-    if diff > threshold:
-        emoji = "❌"
-        print(color_message(message.replace('OK', 'too large'), Color.RED), file=sys.stderr)
+    if pkg_size.ko():
+        print(color_message(pkg_size.log(), Color.RED), file=sys.stderr)
     else:
-        emoji = "✅" if diff <= 0 else "⚠️"
-        print(message)
-    return f"|{flavor}-{arch}-{os_name}|{mb(diff)}|{emoji}|{mb(package_size)}|{mb(previous_size)}|{mb(threshold)}|"
+        print(pkg_size.log())
+    return pkg_size
 
 
 def mb(value):

tasks/libs/package/utils.py

@@ -1,5 +1,6 @@
 import glob
 import json
+import os
 
 from invoke import Exit, UnexpectedExit
 
@@ -11,7 +12,67 @@
 PACKAGE_SIZE_S3_CI_BUCKET_URL = "s3://dd-ci-artefacts-build-stable/datadog-agent/package_size"
 
 
-def get_package_path(glob_pattern):
+class PackageSize:
+    def __init__(self, arch, flavor, os_name, threshold):
+        self.arch = arch
+        self.flavor = flavor
+        self.os = os_name
+        self.size = 0
+        self.ancestor_size = 0
+        self.diff = 0
+        self.threshold = threshold
+        self.emoji = "✅"
+
+    @property
+    def name(self):
+        return f"{self.flavor}-{self.arch}-{self.os}"
+
+    def arch_name(self):
+        if self.arch in ["x86_64", "amd64"]:
+            return "amd"
+        return "arm"
+
+    def ko(self):
+        return self.diff > self.threshold
+
+    def path(self):
+        if self.os == 'suse':
+            dir = os.environ['OMNIBUS_PACKAGE_DIR_SUSE']
+            return f'{dir}/{self.flavor}-7*{self.arch}.rpm'
+        else:
+            dir = os.environ['OMNIBUS_PACKAGE_DIR']
+            separator = '_' if self.os == 'deb' else '-'
+            return f'{dir}/{self.flavor}{separator}7*{self.arch}.{self.os}'
+
+    def compare(self, size, ancestor_size):
+        self.size = size
+        self.ancestor_size = ancestor_size
+        self.diff = self.size - self.ancestor_size
+        if self.ko():
+            self.emoji = "❌"
+        elif self.diff > 0:
+            self.emoji = "⚠️"
+
+    @staticmethod
+    def mb(value):
+        return f"{value / 1000000:.2f}MB"
+
+    def log(self):
+        return f"{self.emoji} - {self.name} size {self.mb(self.size)}: {self.mb(self.diff)} diff[{self.diff}] with previous {self.mb(self.ancestor_size)} (max: {self.mb(self.threshold)})"
+
+    def markdown(self):
+        elements = (
+            self.name,
+            self.mb(self.diff),
+            self.emoji,
+            self.mb(self.size),
+            self.mb(self.ancestor_size),
+            self.mb(self.threshold),
+        )
+        return f'|{"|".join(map(str, elements))}|'
+
+
+def find_package(glob_pattern):
     package_paths = glob.glob(glob_pattern)
     if len(package_paths) > 1:
         raise Exit(code=1, message=color_message(f"Too many files matching {glob_pattern}: {package_paths}", "red"))
@@ -103,4 +164,4 @@ def display_message(ctx, ancestor, rows, decision):
 ## Decision
 {decision}
 """
-    pr_commenter(ctx, title="Package size comparison", body=message)
+    pr_commenter(ctx, title="Uncompressed package size comparison", body=message)

tasks/package.py

@@ -14,9 +14,10 @@
     compute_package_size_metrics,
 )
 from tasks.libs.package.utils import (
+    PackageSize,
     display_message,
+    find_package,
     get_ancestor,
-    get_package_path,
     list_packages,
     retrieve_package_sizes,
     upload_package_sizes,
@@ -33,26 +34,29 @@ def check_size(ctx, filename: str = 'package_sizes.json', dry_run: bool = False)
         if ancestor in package_sizes:
             # The test already ran on this commit
             return
-        package_sizes[ancestor] = PACKAGE_SIZE_TEMPLATE
+        package_sizes[ancestor] = PACKAGE_SIZE_TEMPLATE.copy()
         package_sizes[ancestor]['timestamp'] = int(datetime.now().timestamp())
     # Check size of packages
     print(
         color_message(f"Checking package sizes from {os.environ['CI_COMMIT_REF_NAME']} against {ancestor}", Color.BLUE)
     )
-    size_table = ""
+    size_table = []
     for package_info in list_packages(PACKAGE_SIZE_TEMPLATE):
-        size_table += f"{compare(ctx, package_sizes, ancestor, *package_info)}\n"
+        pkg_size = PackageSize(*package_info)
+        size_table.append(compare(ctx, package_sizes, ancestor, pkg_size))
 
     if on_main:
         upload_package_sizes(ctx, package_sizes, filename, distant=not dry_run)
     else:
-        if "❌" in size_table:
+        size_table.sort(key=lambda x: (-x.diff, x.flavor, x.arch_name()))
+        size_message = "".join(f"{pkg_size.markdown()}\n" for pkg_size in size_table)
+        if "❌" in size_message:
             decision = "❌ Failed"
-        elif "⚠️" in size_table:
+        elif "⚠️" in size_message:
             decision = "⚠️ Warning"
         else:
             decision = "✅ Passed"
-        display_message(ctx, ancestor, size_table, decision)
+        display_message(ctx, ancestor, size_message, decision)
         if "Failed" in decision:
             raise Exit(code=1)
 
@@ -62,11 +66,11 @@ def compare_size(ctx, new_package, stable_package, package_type, last_stable, th
     mb = 1000000
 
     if package_type.endswith('deb'):
-        new_package_size = _get_deb_uncompressed_size(ctx, get_package_path(new_package))
-        stable_package_size = _get_deb_uncompressed_size(ctx, get_package_path(stable_package))
+        new_package_size = _get_deb_uncompressed_size(ctx, find_package(new_package))
+        stable_package_size = _get_deb_uncompressed_size(ctx, find_package(stable_package))
     else:
-        new_package_size = _get_rpm_uncompressed_size(ctx, get_package_path(new_package))
-        stable_package_size = _get_rpm_uncompressed_size(ctx, get_package_path(stable_package))
+        new_package_size = _get_rpm_uncompressed_size(ctx, find_package(new_package))
+        stable_package_size = _get_rpm_uncompressed_size(ctx, find_package(stable_package))
 
     threshold = int(threshold)
 

tasks/unit_tests/package_lib_tests.py

@@ -6,13 +6,12 @@
 from invoke import MockContext, Result
 
 from tasks.libs.package.size import (
-    PACKAGE_SIZE_TEMPLATE,
     SCANNED_BINARIES,
     _get_uncompressed_size,
     compare,
     compute_package_size_metrics,
 )
-from tasks.libs.package.utils import get_ancestor, list_packages
+from tasks.libs.package.utils import PackageSize, get_ancestor, list_packages
 
 
 class TestProduceSizeStats(unittest.TestCase):
@@ -167,6 +166,28 @@ def test_get_suse_uncompressed_size(self):
         self.assertEqual(_get_uncompressed_size(c, flavor, 'suse'), 69)
 
 
+class TestPackageSizeMethods(unittest.TestCase):
+    def test_markdown_row(self):
+        size = PackageSize("amd64", "datadog-agent", "deb", 70000000)
+        size.compare(67000000, 68000000)
+        self.assertEqual("|datadog-agent-amd64-deb|-1.00MB|✅|67.00MB|68.00MB|70.00MB|", size.markdown())
+
+    @patch.dict('os.environ', {'OMNIBUS_PACKAGE_DIR': 'root'})
+    def test_path_deb(self):
+        size = PackageSize("amd64", "datadog-agent", "deb", 70000000)
+        self.assertEqual("root/datadog-agent_7*amd64.deb", size.path())
+
+    @patch.dict('os.environ', {'OMNIBUS_PACKAGE_DIR': 'root'})
+    def test_path_rpm(self):
+        size = PackageSize("x86_64", "datadog-agent", "rpm", 70000000)
+        self.assertEqual("root/datadog-agent-7*x86_64.rpm", size.path())
+
+    @patch.dict('os.environ', {'OMNIBUS_PACKAGE_DIR_SUSE': 'rout'})
+    def test_path_suse(self):
+        size = PackageSize("x86_64", "datadog-agent", "suse", 70000000)
+        self.assertEqual("rout/datadog-agent-7*x86_64.rpm", size.path())
+
+
 class TestCompare(unittest.TestCase):
     package_sizes = {}
     pkg_root = 'tasks/unit_tests/testdata/packages'
@@ -181,6 +202,7 @@ def setUp(self) -> None:
     @patch('builtins.print')
     def test_on_main(self, mock_print):
         flavor, arch, os_name = 'datadog-heroku-agent', 'amd64', 'deb'
+        s = PackageSize(arch, flavor, os_name, 2001)
         c = MockContext(
             run={
                 'git merge-base HEAD origin/main': Result('12345'),
@@ -189,9 +211,9 @@ def test_on_main(self, mock_print):
                 ),
             }
         )
-        self.package_sizes['12345'] = PACKAGE_SIZE_TEMPLATE
+        self.package_sizes['12345'] = {arch: {flavor: {os_name: 70000000}}}
         self.assertEqual(self.package_sizes['12345'][arch][flavor][os_name], 70000000)
-        res = compare(c, self.package_sizes, '12345', arch, flavor, os_name, 2001)
+        res = compare(c, self.package_sizes, '12345', s)
         self.assertIsNone(res)
         self.assertEqual(self.package_sizes['12345'][arch][flavor][os_name], 43008)
         mock_print.assert_not_called()
@@ -203,16 +225,17 @@ def test_on_main(self, mock_print):
     @patch('builtins.print')
     def test_on_branch_warning(self, mock_print):
         flavor, arch, os_name = 'datadog-agent', 'aarch64', 'suse'
+        s = PackageSize(arch, flavor, os_name, 70000000)
         c = MockContext(
             run={
                 'git merge-base HEAD origin/main': Result('25'),
                 f"rpm -qip {self.pkg_root}/{flavor}-7.{arch}.rpm | grep Size | cut -d : -f 2 | xargs": Result(69000000),
             }
         )
-        res = compare(c, self.package_sizes, '25', arch, flavor, os_name, 70000000)
-        self.assertEqual(res, "|datadog-agent-aarch64-suse|1.00MB|⚠️|69.00MB|68.00MB|70.00MB|")
+        res = compare(c, self.package_sizes, '25', s)
+        self.assertEqual(res.markdown(), "|datadog-agent-aarch64-suse|1.00MB|⚠️|69.00MB|68.00MB|70.00MB|")
         mock_print.assert_called_with(
-            f"{flavor}-{arch}-{os_name} size 69.00MB is OK: 1.00MB diff with previous 68.00MB (max: 70.00MB)"
+            f"⚠️ - {flavor}-{arch}-{os_name} size 69.00MB: 1.00MB diff[1000000] with previous 68.00MB (max: 70.00MB)"
         )
 
     @patch.dict(
@@ -221,6 +244,7 @@ def test_on_branch_warning(self, mock_print):
     @patch('builtins.print')
     def test_on_branch_ok_rpm(self, mock_print):
         flavor, arch, os_name = 'datadog-iot-agent', 'x86_64', 'rpm'
+        s = PackageSize(arch, flavor, os_name, 70000000)
         c = MockContext(
             run={
                 'git merge-base HEAD origin/main': Result('25'),
@@ -229,10 +253,10 @@ def test_on_branch_ok_rpm(self, mock_print):
                 ),
             }
         )
-        res = compare(c, self.package_sizes, '25', arch, flavor, os_name, 70000000)
-        self.assertEqual(res, "|datadog-iot-agent-x86_64-rpm|-9.00MB|✅|69.00MB|78.00MB|70.00MB|")
+        res = compare(c, self.package_sizes, '25', s)
+        self.assertEqual(res.markdown(), "|datadog-iot-agent-x86_64-rpm|-9.00MB|✅|69.00MB|78.00MB|70.00MB|")
         mock_print.assert_called_with(
-            f"{flavor}-{arch}-{os_name} size 69.00MB is OK: -9.00MB diff with previous 78.00MB (max: 70.00MB)"
+            f"✅ - {flavor}-{arch}-{os_name} size 69.00MB: -9.00MB diff[-9000000] with previous 78.00MB (max: 70.00MB)"
         )
 
     @patch.dict(
@@ -242,6 +266,7 @@ def test_on_branch_ok_rpm(self, mock_print):
     @patch('builtins.print')
     def test_on_branch_ko(self, mock_print):
         flavor, arch, os_name = 'datadog-agent', 'aarch64', 'suse'
+        s = PackageSize(arch, flavor, os_name, 70000000)
         c = MockContext(
             run={
                 'git merge-base HEAD origin/main': Result('25'),
@@ -250,9 +275,9 @@ def test_on_branch_ko(self, mock_print):
                 ),
             }
         )
-        res = compare(c, self.package_sizes, '25', arch, flavor, os_name, 70000000)
-        self.assertEqual(res, "|datadog-agent-aarch64-suse|71.00MB|❌|139.00MB|68.00MB|70.00MB|")
+        res = compare(c, self.package_sizes, '25', s)
+        self.assertEqual(res.markdown(), "|datadog-agent-aarch64-suse|71.00MB|❌|139.00MB|68.00MB|70.00MB|")
         mock_print.assert_called_with(
-            "\x1b[91mdatadog-agent-aarch64-suse size 139.00MB is too large: 71.00MB diff with previous 68.00MB (max: 70.00MB)\x1b[0m",
+            "\x1b[91m❌ - datadog-agent-aarch64-suse size 139.00MB: 71.00MB diff[71000000] with previous 68.00MB (max: 70.00MB)\x1b[0m",
             file=sys.stderr,
         )

tasks/unit_tests/package_tests.py

@@ -16,19 +16,25 @@ class TestCheckSize(unittest.TestCase):
             'CI_COMMIT_REF_NAME': 'pikachu',
         },
     )
-    @patch('tasks.libs.package.size.get_package_path', new=MagicMock(return_value='datadog-agent'))
-    @patch('tasks.package.display_message', new=MagicMock())
-    def test_dev_branch_ko(self):
+    @patch('tasks.libs.package.size.find_package', new=MagicMock(return_value='datadog-agent'))
+    @patch('tasks.package.display_message')
+    def test_dev_branch_ko(self, display_mock):
         flavor = 'datadog-agent'
         c = MockContext(
             run={
                 'git merge-base HEAD origin/main': Result('25'),
                 f"dpkg-deb --info {flavor} | grep Installed-Size | cut -d : -f 2 | xargs": Result(42),
-                f"rpm -qip {flavor} | grep Size | cut -d : -f 2 | xargs": Result(69000000),
+                f"rpm -qip {flavor} | grep Size | cut -d : -f 2 | xargs": Result(141000000),
             }
         )
         with self.assertRaises(Exit):
             check_size(c, filename='tasks/unit_tests/testdata/package_sizes_real.json', dry_run=True)
+        display_mock.assert_called_with(
+            c,
+            '12345',
+            '|datadog-dogstatsd-x86_64-rpm|131.00MB|❌|141.00MB|10.00MB|10.00MB|\n|datadog-dogstatsd-x86_64-suse|131.00MB|❌|141.00MB|10.00MB|10.00MB|\n|datadog-iot-agent-x86_64-rpm|131.00MB|❌|141.00MB|10.00MB|10.00MB|\n|datadog-iot-agent-x86_64-suse|131.00MB|❌|141.00MB|10.00MB|10.00MB|\n|datadog-iot-agent-aarch64-rpm|131.00MB|❌|141.00MB|10.00MB|10.00MB|\n|datadog-agent-x86_64-rpm|1.00MB|⚠️|141.00MB|140.00MB|140.00MB|\n|datadog-agent-x86_64-suse|1.00MB|⚠️|141.00MB|140.00MB|140.00MB|\n|datadog-agent-aarch64-rpm|1.00MB|⚠️|141.00MB|140.00MB|140.00MB|\n|datadog-dogstatsd-amd64-deb|-9.96MB|✅|0.04MB|10.00MB|10.00MB|\n|datadog-dogstatsd-arm64-deb|-9.96MB|✅|0.04MB|10.00MB|10.00MB|\n|datadog-iot-agent-amd64-deb|-9.96MB|✅|0.04MB|10.00MB|10.00MB|\n|datadog-iot-agent-arm64-deb|-9.96MB|✅|0.04MB|10.00MB|10.00MB|\n|datadog-heroku-agent-amd64-deb|-69.96MB|✅|0.04MB|70.00MB|70.00MB|\n|datadog-agent-amd64-deb|-139.96MB|✅|0.04MB|140.00MB|140.00MB|\n|datadog-agent-arm64-deb|-139.96MB|✅|0.04MB|140.00MB|140.00MB|\n',
+            '❌ Failed',
+        )
 
     @patch('builtins.print')
     @patch.dict(
@@ -39,7 +45,7 @@ def test_dev_branch_ko(self):
             'CI_COMMIT_REF_NAME': 'pikachu',
         },
     )
-    @patch('tasks.libs.package.size.get_package_path', new=MagicMock(return_value='datadog-agent'))
+    @patch('tasks.libs.package.size.find_package', new=MagicMock(return_value='datadog-agent'))
     @patch('tasks.package.display_message', new=MagicMock())
     @patch('tasks.package.upload_package_sizes')
     def test_dev_branch_ok(self, upload_mock, print_mock):
@@ -64,7 +70,7 @@ def test_dev_branch_ok(self, upload_mock, print_mock):
             'CI_COMMIT_REF_NAME': 'main',
         },
     )
-    @patch('tasks.libs.package.size.get_package_path', new=MagicMock(return_value='datadog-agent'))
+    @patch('tasks.libs.package.size.find_package', new=MagicMock(return_value='datadog-agent'))
     @patch('tasks.package.display_message', new=MagicMock())
     def test_main_branch_ok(self):
         flavor = 'datadog-agent'