Comment the disk usage difference on pull requests - #24895
Draft
lucia-sb wants to merge 33 commits into
Draft
Conversation
Dependency sizes are read from the .deps/resolved lockfiles in the measured commit, so the workflow no longer has to wait for wheels to be built. It can trigger directly on the commit instead of subscribing to resolve-build-deps via workflow_run, which in turn removes the reason the master-only companion workflow existed: resolve-build-deps does not run on master, so the workflow_run path was dead there. Everything dropped here was load-bearing only for artifact fetching: - workflow_run on resolve-build-deps - measure-disk-usage-master.yml - GITHUB_TOKEN, needed for the gh calls in get_run_id and get_artifact - the actions: read permission, needed to list runs and artifacts - the "[<sha>]" run-name suffix, which get_run_id_measure_disk_usage matched against to find a run PR runs measure and upload the JSON but do not send. The metrics carry no branch tag yet, so PR measurements would land in the same series as master and skew it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lockfile URLs carry a ${INTEGRATIONS_WHEELS_STORAGE} placeholder, and the tier
was substituted while parsing the lockfile. A wheel missing from the chosen tier
then raised straight out of requests.raise_for_status, and because the exception
escapes the per-platform loop it aborted the whole measurement rather than
losing a single dependency.
Resolution now happens when the wheel is requested, so request_wheel can try the
preferred tier and fall back to the other one. A wheel is the same object
whichever tier serves it, but retention differs: older builds are pruned from
dev while stable keeps them, so falling back lets older commits still be
measured by diff and timeline. Only a missing wheel triggers a retry; other HTTP
errors are raised immediately rather than masked by trying the next tier.
The workflow now asks for dev, which is a superset of stable for current wheels,
with stable as the fallback for anything already pruned. Both tiers serve
identical bytes for the same wheel, so this does not shift the metric.
timeline duplicated the tier substitution and the download, so both now go
through the shared helper.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Fold every tried tier URL into the final HTTPError from request_wheel and debug-log each attempt, so a failure shows more than just the last candidate. - Share a single WheelsStorageTier Literal alias between common_params and common_funcs instead of duplicating the dev/stable pair, and clarify the --wheels-storage help text about the fallback. - Reject --commit passed without --to-dd-org/--to-dd-key, matching the existing validation for the reverse case. - Strengthen test coverage: get_dependencies_list now asserts the storage placeholder is left unresolved, request_wheel gets a 403 fallback case and close() assertions, and the status --commit test actually exercises metric forwarding. - Add a changelog entry for the tier-fallback behavior itself, which was missing alongside the --dependency-sizes removal entry.
…ddev app logger Consolidate the two 23831 changelog fragments into a single .removed entry, and thread app: Application through request_wheel/get_dependencies(_sizes) so debug logging uses app.display_debug instead of a stdlib logger.
measure-disk-usage.yml always passed --commit but only conditionally passed --to-dd-key, tripping the new "commit without a dd target" validation on every non-push run.
Gate the dd-sts step on push only so DD_API_KEY is never present in the job environment when running untrusted PR-head code.
… size calculation The wheels themselves no longer include the test/type-stub files this was filtering out, so the filtering logic is dead weight.
Parametrize the 404/403 fallback tests and hoist the requests import; pin the "Tried: ..." error message with a match assertion; make test_get_dependencies_sizes exercise placeholder resolution end-to-end; document the 403-for-absent-key rationale in request_wheel's docstring; and use the WheelsStorageTier alias consistently instead of str.
Builds on the unified measure-disk-usage workflow by adding the size diff to it, in two forms: - On master, per-module deltas are sent to Datadog as datadog.agent_integrations.size_diff, attributed to the later of the two compared commits so a change lands on the commit that introduced it. This brings over the --to-dd-org/--to-dd-key options for `ddev size diff`. - On a pull request, the diff is posted as a comment with one collapsible section per platform, so a reader can open just the platform they care about. The section summary carries that platform's total, so the numbers are visible without expanding anything. The comparison is between the ephemeral merge commit and its first parent. That parent is the base branch commit GitHub built the merge from, which isolates the pull request's own contribution; diffing against the current tip of master would fold in whatever landed since. save_markdown now emits those collapsible sections. It drops the Platform and Python_Version columns, which only repeated what each section heading already says, and takes a section_total argument for the summary. That defaults to off because summing rows is only meaningful when they partition a single total, and timeline rows are the size at successive commits rather than parts of one. It also no longer titles every export "Status"; diff exports say "Diff". Credential resolution and the per-module tag list were duplicated between the two send paths, so they are now initialize_dd_client() and build_module_tags(). METRIC_VERSION stays at 2, since this adds a metric rather than changing the shape of an existing one.
evalya-impact-summaryevalya impact analysis |
lucia-sb
changed the base branch from
master
to
lucia-sb/remove-size-artifact-fetching
August 18, 2026 09:55
lucia-sb
changed the base branch from
lucia-sb/remove-size-artifact-fetching
to
master
August 18, 2026 09:56
Contributor
Disk usage changeMerge commit UncompressedNo integration or dependency changed size. CompressedNo integration or dependency changed size. |
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0dec242 | Docs | View more details | Give us feedback! |
Contributor
|
|
Adds ~5.4 KiB of comment lines to produce a non-zero size diff so the pull request comment can be verified end to end. To be reverted before review.
Platforms reach save_markdown from a set, so the compressed and uncompressed reports of the same commit listed their sections in different orders.
The size diff comment is verified; the padding that produced a non-zero diff is no longer needed.
The two are independent measurements, so serialising them delayed whichever ran second: the diff takes about twice as long as the status measurement, which on a pull request pushed the comment out to roughly six minutes. Splitting them halves the time to a comment while keeping the absolute sizes, which are the primary telemetry on master, sent as early as before. It also narrows pull-requests: write to the job that actually posts the comment. The comment now distinguishes a size-neutral pull request from a measurement that failed. Both leave no export behind, and since the diff steps continue on error, a crash was being reported as 'No integration or dependency changed size'.
The comment step already skipped forks, but the job still spent the full measurement, about four minutes of runner time, on a report that could never be posted: a fork's pull_request GITHUB_TOKEN is read-only. Moving the condition to the job puts the rule in one place and stops the work before it starts. Widening the token instead would mean pull_request_target, which runs with secrets against pull-request-authored code that this workflow installs and executes.
Splits the workflow in two: a trigger that runs on the commit and does nothing, and the measurement itself, which now runs on that trigger's completion. workflow_run always runs the default-branch version of its own file, so the commit being measured cannot alter the measuring workflow. That is what makes it a usable place to hold a credential later; it does not make the measured code trusted, and this job still checks that code out, so the Datadog credential stays gated to push, where the code is master's. Nothing has to be passed from the trigger. The workflow_run event already carries head_sha, which is the head commit on a pull request and the pushed commit on a push, and event, which distinguishes the two. That replaces the Resolve measurement context step outright. The trigger is skipped for fork pull requests, which are not measured, and the measurement re-checks that since it is the job that can hold a credential.
The fork condition was in both workflows. Only the measurement needs it: it is the job that checks out the measured code and can hold a credential, and it cannot rely on the trigger's conclusion, which is not clearly defined when a run's only job is skipped. Guarding the trigger as well bought nothing and gave the condition somewhere to drift.
# Conflicts: # .github/workflows/measure-disk-usage-master.yml # .github/workflows/measure-disk-usage.yml
The two tiers were asserted in one test body, so a failure did not say which ordering broke and the second assertion never ran when the first failed.
…ataDog/integrations-core into lucia-sb/size-diff-comment-on-pr # Conflicts: # .github/workflows/measure-disk-usage.yml
Contributor
Validation ReportAll 21 validations passed. Show details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Builds on #23831 (unified
measure-disk-usage.yml) by adding the size diff to that workflow, in two forms.On master, per-module deltas go to Datadog as
datadog.agent_integrations.size_diff— this is the content of #24768, re-targeted at the unified workflow since #23831 deletesmeasure-disk-usage-master.yml:--to-dd-org/--to-dd-keytoddev size diff, mirroringddev size status. The two are mutually exclusive.datadog.agent_integrations.size. Values are deltas and may be negative.On a pull request, the diff is posted as a comment: a dropdown for uncompressed and one for compressed, each containing a dropdown per platform whose summary line carries that platform's total, so the numbers are readable without expanding anything and the full table is one click away.
What is compared. On
pull_request,github.shais the ephemeral merge commit of the PR into the base branch, and the workflow now checks that out rather than the head commit. The diff is against that merge commit's first parent, which is the base branch commit GitHub built the merge from. That isolates the PR's own contribution — diffing against the current tip of master would fold in whatever landed since.github.event.pull_request.base.shais not used because it can lag the branch the merge was actually built from.Reused rather than duplicated. The comment is assembled from
ddev size diff --format markdown, so no new script was needed. To make that output fit a PR comment,save_markdownnow emits a collapsible section per platform instead of a##heading, and:PlatformandPython_Versioncolumns, which only repeated what the section heading already says;section_totalargument for the summary total. It defaults tonone, because summing rows is only meaningful when they partition a single total —timelinerows are the size at successive commits, so their sum would be nonsense;Status. Diff exports previously claimed to beStatus; they now sayDiff.Credential resolution and the per-module tag list were duplicated between the two send paths, so they are now
initialize_dd_client()andbuild_module_tags().send_metrics_to_ddis otherwise unchanged, andMETRIC_VERSIONstays at2since this adds a metric rather than changing the shape of an existing one.Two parallel jobs. The absolute sizes and the diff are independent measurements, so they run concurrently rather than in sequence. They cost about the same (~250s each in a representative run), so serialising them roughly doubled wall-clock and, on a pull request, pushed the comment out to the end. Split, the comment arrives as soon as the diff finishes and the absolute sizes are still sent as early as before. This also narrows
pull-requests: writeto the one job that posts the comment.Two things worth a reviewer's attention:
diffchecks out both commits and resolves dependencies for every platform × Python version, twice per invocation. That is the largest cost this PR adds, and it now applies to pull requests as well as master. It buys back most of the wall-clock by running alongside the status job. Both diff steps arecontinue-on-error: true: this is supplementary to the absolute size metrics the workflow already sends, and a failure here should not mask them.pull_requestGITHUB_TOKENis read-only, so the comment step is skipped for them by an explicit condition rather than left to fail. Making it work on forks would mean reintroducing theworkflow_runindirection that Remove dependency size artifact fetching and unify the disk usage workflows #23831 exists to remove.The comment is updated in place on each push, keyed by a hidden HTML marker, rather than stacking a new comment per push.
A missing export means either a size-neutral pull request or a measurement that never finished, and because the diff steps continue on error those two cases would otherwise read identically. The comment checks the step outcome so a crashed measurement is reported as a failure rather than as "No integration or dependency changed size".
Motivation
#23831 collapsed the two disk usage workflows into one and #24768 added size deltas to master, but the two conflict textually: #24768 edits the file #23831 deletes. This combines them on top of the unified workflow.
The pull request comment is the part that changes day-to-day behavior. The workflow currently measures PRs and uploads the JSON, but nothing surfaces it, so a size regression is only noticed after it lands on master. Reporting the delta against the merge base on the PR itself puts it in front of the author and reviewer while the change can still be reconsidered.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged