[AI-5800] Measure size of dependencies before PRs are merged #21331

lucia-sb · 2025-09-12T07:50:39Z

What does this PR do?

Adds logic that uploads dependency size data to a JSON artifact, so size measurements no longer depend on merged lockfiles.

Motivation

Size measuraments are inaccurate because dependency lockfiles are merged in a follow-up PR. As a result, updates or new dependencies aren’t reflected in the size delta of the originating PR.
This change uploads dependency sizes as a JSON artifact during the build, allowing us to measure size changes directly, without relying on lockfile merges.

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

github-actions · 2025-09-12T07:51:18Z

⚠️ Recommendation: Add qa/skip-qa Label

This PR does not modify any files shipped with the agent.

To help streamline the release process, please consider adding the qa/skip-qa label if these changes do not require QA testing.

codecov · 2025-09-12T10:13:59Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.96%. Comparing base (7b59ac0) to head (a481300).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AAraKKe

Thanks for the PR! It looks very promising and pretty well organized.

Although there are a bunch of comments I think we are pretty close to get it ready! Let me know if you want to discuss any of them.

My Feedback Legend

Here's a quick guide to the prefixes I use in my comments:

question: I need clarification or I'm seeking to understand your approach.
suggestion: I'm proposing an improvement. This is optional but recommended.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
request: A change I believe is necessary before this can be merged.

I would start by addressing the request and answering the question. Afterwards, you can move to suggestion and nit.

.github/workflows/resolve-build-deps.yaml

.github/workflows/measure-disk-usage.yml

AAraKKe · 2025-09-17T11:07:53Z

ddev/src/ddev/cli/size/status.py

+    platform: str | None,
+    version: str | None,


Thanks!!! 😊

AAraKKe · 2025-09-17T11:11:03Z

ddev/src/ddev/cli/size/status.py

@@ -55,6 +60,8 @@ def status(
            raise ValueError(f"Invalid platform: {platform}")


nit: You can do

raise ValueError(f"Invalid platform: {platform!r}")

the !r modifier prints the repr of the variable which for a string includes the quotes. I will result in a message like

Invalid platform: 'no-platform'

instead of

Invalid platform: no-platform

This is strictly personal but normally I prefer show that the value being printed is a string supplied by the user the fact that it is a string.

AAraKKe · 2025-09-17T16:28:06Z

ddev/src/ddev/cli/size/utils/common_funcs.py

+@cache
+def get_previous_dep_sizes_json(base_commit, platform):
+    print(f"Getting previous dependency sizes json for base_commit={base_commit}, platform={platform}")
+    run_id = get_run_id(base_commit, '.github/workflows/measure-disk-usage.yml')


suggestion: Can we extract this string to a module constant in the module? Just in case we need to modify or access it, avoid having several replicas or the same string.

AAraKKe · 2025-09-17T16:32:20Z

ddev/src/ddev/cli/size/utils/common_funcs.py

+    print(f"Previous run_id: {run_id}")
+    compressed_json = None
+    uncompressed_json = None
+    if run_id and check_artifact_exists(run_id, f'status_compressed_{platform}.json'):


question: I am wondering, we keep questioning first whether there are artifacts to download and when there are None we set whatever value to None.

Whey don't we just call get_artifacts and the same for everything where we validate first whether they exist, and return None on error? We would be saving several api calls.

For exampe, if I try to run gh run download for an artifact that does not exist I get

gh run download 17802166735 --name naaaan no artifact matches any of the names or patterns provided

which seems that it is already telling me that it does not exist, right? We can just get the output of the command we want to run and validate against the error mode to know they do not exist. No need to first ask whether they exist or not and then download it.

AAraKKe · 2025-09-17T16:39:31Z

ddev/tests/size/test_common.py

-    assert convert_to_human_readable_size(1024) == "1.0 KB"
-    assert convert_to_human_readable_size(1048576) == "1.0 MB"
-    assert convert_to_human_readable_size(1073741824) == "1.0 GB"
+    assert convert_to_human_readable_size(1024) == "1.0 KiB"


request: for these kind of tests lest parameterize them. These multiple asserts to test different cases can break without giving us the full picture.

AAraKKe · 2025-09-17T16:42:20Z

ddev/tests/cli/size/test_status.py

@@ -126,3 +127,10 @@ def test_status_wrong_plat_and_version(ddev):
    ):
        result = ddev("size", "status", "--platform", "linux", "--python", "2.10", "--compressed")
        assert result.exit_code != 0
+
+
+def test_status_dependency_sizes(ddev, mock_size_status):


question: what is this test testing? The name is not very descriptive and it seems we are just really testing that we cannot pass commit and dependency-sizes at the same time.

I refactored the tests

AAraKKe · 2025-09-17T16:56:56Z

ddev/tests/size/test_common.py

+        }
+    }
+
+    mock_file_handler = mock_open()


suggestion: this first mock doe snot need to be a mock_open, can be a simple MagicMock. The mock_open helps by returning whatever is on read_data when the return_value of the __enter__ method of the context manager is called but since here we want to give 2 different bheaviors, we just need something that provides 2 different side effects.

create dependency json

e13b5fc

typing

cd9c2eb

lucia-sb changed the title ~~create dependency json~~ [AI-5800] Measure size of dependencies before PRs are merged Sep 12, 2025

lucia-sb added 2 commits September 12, 2025 10:36

order sizes

d0f0fdd

send metrics

0ab6d1e

datadog-agent-integrations-bot bot added dev/testing dev/tooling ddev labels Sep 12, 2025

lucia-sb added 20 commits September 12, 2025 12:25

send metrics

3a1d897

change on

77a22a0

change on

05e7206

change on

6c60125

change on

915ae41

change on

58fac6b

change on

09ae290

change on

414c32d

change on

1a0f483

check files

80e0f15

check files

fbf217d

check files

897eb33

check files

b1fe7fd

check files

8b4752d

check files

534f64e

check files

dfb1b54

check files

443546a

check files

132d3d3

measure disk usage

658f89d

fix

a8e5286

lucia-sb added 16 commits September 16, 2025 12:32

changelog

7408a26

build deps folder

8572155

comment build

1d0acac

change

a9e63af

Merge branch 'master' into lucia-sb/calculate-dep-sizes-2

8920542

uncomment

ef47024

workflow_run

544eeaf

default

18615ae

sha

af8aff0

test

684a03a

dep files

094ce49

comment

07c142d

tests

611e868

comment

ba9fb8f

spaces

43b422b

test

385f351

lucia-sb marked this pull request as ready for review September 17, 2025 10:44

lucia-sb requested review from a team as code owners September 17, 2025 10:44

datadog-agent-integrations-bot bot added team/agent-integrations team/agent-build labels Sep 17, 2025

lucia-sb added the qa/skip-qa Automatically skip this PR for the next QA label Sep 17, 2025

change metric name

bccad63

AAraKKe requested changes Sep 17, 2025

View reviewed changes

lucia-sb added 6 commits September 18, 2025 15:08

Review Changes

a866603

Review Changes

65392bc

Review Changes

eba6310

Review Changes

147dd4b

Review Changes

7dc210f

Review Changes

a481300

		@@ -55,6 +60,8 @@ def status(
		raise ValueError(f"Invalid platform: {platform}")

[AI-5800] Measure size of dependencies before PRs are merged #21331

Are you sure you want to change the base?

[AI-5800] Measure size of dependencies before PRs are merged #21331

Conversation

lucia-sb commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Review checklist (to be filled by reviewers)

Uh oh!

github-actions bot commented Sep 12, 2025

Uh oh!

codecov bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AAraKKe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lucia-sb commented Sep 12, 2025 •

edited

Loading

codecov bot commented Sep 12, 2025 •

edited

Loading

AAraKKe left a comment •

edited

Loading