Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bundle Analysis] FEATURE: gate configuration option for PR based on bundle size and/or changes #1491

Closed
5 tasks done
codecovdesign opened this issue Apr 2, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request epic this label is used to mark issues as epics

Comments

@codecovdesign
Copy link
Contributor

codecovdesign commented Apr 2, 2024

Problem to solve

As projects grow, maintaining performance standards becomes increasingly challenging. One way to ensure code quality is by gating PRs based on criteria such as bundle size, changes, and/or newly added dependencies.

Exploration here is around 1) implementing a gate that triggers specific actions (ex: fail the check, or pass with warning) when a PR's bundle size exceeds and/or other predefined thresholds and 2) a gating system to prevent performance degradation by enforcing conditional bundle limits as set by org/repo owner/maintainer.

related discovery: #1420

Solution discovery

MVP draft for bundle gate feature

  1. gating item

    • users can select entire bundle
  2. gating criteria

    • default and/or user-defined thresholds: the system to use a default percentage threshold for bundle size increases, such as 5% (auto) - users also have the option to specify their own thresholds, either as a percentage or an absolute value in MB, to suit their specific needs. (notes: confirmed in scope)
    • GH check behavior: IF auto or input is exceeded, check is passed with warning - user may update to opt-in to fail
  3. configuration

    • by YAML, the gating configurations will be managed through a YAML file, allowing users to integrate settings directly with their existing CI/CD pipeline configurations
    • in-app could show display if/not YAML is configured (to further market the configuration)

PR comment designs

View current + updated copy – the copy aims to be descriptive helping user understand why the message is shown, while not being prescriptive about next steps:

view designs

questions, considerations, and later iterations

more open questions and considerations

  • default behavior: should the default behavior be to fail the gate when thresholds are exceeded, or should it pass with a warning unless configured otherwise?
    • we can use neutral?
    • is this configurable or default to one or the other?

issues for later:

  • type-specific thresholds, if this is a gating item then info-display in PR comment needs updating;

  • out of box could be type: js, css, images and/or 1st loading (removing lazy loading)

  • handling new vs. existing assets: do we want the system to differentiate between increases due to new assets versus changes to existing ones

ideation

What is the criteria and/or configuration options for gate?

  1. Bundle size threshold: a specific size limit (e.g., X MB) for the bundle that, when exceeded, triggers a gate.
  2. Percentage increase: a gate triggers if the bundle size increases by more than a set percentage compared to the base branch or the last commit.
  3. Critical dependency addition: introduction of a new dependency known for large size or performance impact triggers the gate.
    • dependency gate?
  4. Large file inclusion: including any single file exceeding a specified size (e.g., Y MB) in the bundle triggers the gate.
  5. Number of changes: exceeding a certain number of file changes in the bundle might indicate significant changes requiring additional review.

What are the gate outcomes and/or configuration options for gate?

ideation
  1. Fail the check: strict enforcement of bundle size criteria, blocking merges that exceed size limits.
  2. Pass with warning: issues a warning for review without blocking the merge, suitable for expected but notable size increases.
  3. Request for manual review: flags the PR for manual review under specific conditions, highlighting concerns in PR comments.
  4. Conditional pass/fail: logic to allow more flexibility based on additional criteria like feature importance or user experience impact.

Another consideration is: should the system display suggestions about actions to take if gate fails and/or there are warnings? or maybe that could be input by customer?

copy examples from the concepts designs in gate is activated:
Please consider reducing the size or removing assets to meet the required standards
OR
Please reach out to insert name for further review

Summary of Considerations for Initial Iteration:

configuration criteria:

  • by percentage increase: allow setting a percentage increase threshold for the bundle size or 3G/high-speed download time change
  • total bundle size gate: enable configuring a maximum allowable total bundle size
  • by change sizes: set thresholds for acceptable change sizes within the bundle
  • per bundle: allow different configurations for each bundle within a project

gate outcomes:

  • gate behavior: configurable to pass, fail, or be neutral
  • system guidance copy to user: generic and/or TBD for later
  • otherwise, configuration by codecov.yaml? and global yam
    - further about configuring in package as alternative?

WIP designs

Next Steps

Preview Give feedback
  1. giovanni-guidini
  2. giovanni-guidini
  3. giovanni-guidini
@codecovdesign codecovdesign added the in discovery The design, product, and specifications require refinement label Apr 2, 2024
@codecovdesign
Copy link
Contributor Author

codecovdesign commented Apr 2, 2024

feedback review notes, post review WIP:

configuration criteria:

  • by percentage increase: allow setting a percentage increase threshold for the bundle size or 3G/high-speed download time change
  • total bundle size gate: enable configuring a maximum allowable total bundle size
  • by change sizes: set thresholds for acceptable change sizes within the bundle
  • per bundle: allow different configurations for each bundle within a project

gate outcomes:

  • gate behavior: configurable to pass, fail, or be neutral
  • system guidance copy to user: generic and/or TBD for later
  • otherwise, configuration by codecov.yaml

@codecovdesign codecovdesign changed the title DISCOVERY: gate configuration option for PR based on bundle size and/or changes DISCOVERY/ISSUE: gate configuration option for PR based on bundle size and/or changes Apr 17, 2024
@codecovdesign
Copy link
Contributor Author

codecovdesign commented Apr 29, 2024

MVP review:

  1. gating item: user may select bundle + specific section asset

    • vu: i would use the grouping/custom and/or 1st loading (removing lazy loading)
  2. gating criteria: increased bundle by <user entry threshold (ex: )>, some ideation:

    • percentage increase: instead of or in addition to absolute size increases, users to set percentage-based thresholds. For example, a 5% increase might be acceptable for a small module but too much for a larger bundle.
      • vu: % vs absolute is hard to say, everyone would have different preference
        • strongly prefer a warning, not a block
      • handling diff between new or existing asset criteria/outcome
    • baseline comparisons: options to compare against a baseline build (e.g., the main branch's last stable build) or against the previous commit.
    • type-specific thresholds: since users can select specific assets (1), enabling different thresholds for different file types could be beneficial. for example, a larger threshold might be set for images and fonts compared to JavaScript or CSS files, considering their different impacts on load times and user experience.
      • vu: this is important^
  3. configuration: configuring in the YAML OR the package

    • what are the tradeoffs, per review with Eli question
    • nick had preference of YAML

@codecovdesign
Copy link
Contributor Author

codecovdesign commented May 13, 2024

MVP draft for bundle gate feature

  1. gating item

    • users can select entire bundle
  2. gating criteria

    • default and/or user-defined thresholds: the system to use a default percentage threshold for bundle size increases, such as 5% (auto) - users also have the option to specify their own thresholds, either as a percentage or an absolute value in MB, to suit their specific needs. (notes: confirmed in scope)
    • GH check behavior: IF auto or input is exceeded, check is passed with warning - user may update to opt-in to fail
  3. configuration

    • by YAML, the gating configurations will be managed through a YAML file, allowing users to integrate settings directly with their existing CI/CD pipeline configurations
    • in-app could show display if/not YAML is configured (to further market the configuration)

PR comment designs

View current + updated copy – the copy aims to be descriptive helping user understand why the message is shown, while not being prescriptive about next steps:

view designs

questions, considerations, and later iterations

more open questions and considerations

  • default behavior: should the default behavior be to fail the gate when thresholds are exceeded, or should it pass with a warning unless configured otherwise?
    • we can use neutral?
    • is this configurable or default to one or the other?

issues for later:

  • type-specific thresholds, if this is a gating item then info-display in PR comment needs updating;

  • out of box could be type: js, css, images and/or 1st loading (removing lazy loading)

  • handling new vs. existing assets: do we want the system to differentiate between increases due to new assets versus changes to existing ones

@rohan-at-sentry
Copy link

rohan-at-sentry commented May 13, 2024

My thoughts on open questions

handling of new vs. existing assets: do we want the system to differentiate between increases due to new assets versus changes to existing ones? Or is this differentiation a later iteration?

This can be later

@nicholas-codecov
Copy link

Just noting some stuff down after chatting with @giovanni-guidini

  1. Users should be able to define gates using %, but also be able to define sizes in kB, MB, etc. so we'll need to do some extra handling around that.
  2. Users will need to provide the bundle name associated with the given gate, however this name will not be the exact same as defined in the plugin. As in the plugin we add some information to the plugin such as the format that the bundle is being exported as, as well for frameworks where they output different bundles for different envs we also append stuff like client, and server to the bundle name. Users will also probably care differently between a server bundle and a client bundle. So we will need to be quite clear of this to users and make sure they know this information.

@thomasrockhu-codecov thomasrockhu-codecov changed the title DISCOVERY/ISSUE: gate configuration option for PR based on bundle size and/or changes [Bundle Analysis] DISCOVERY/ISSUE: gate configuration option for PR based on bundle size and/or changes Jun 10, 2024
@thomasrockhu-codecov thomasrockhu-codecov added enhancement New feature or request and removed in discovery The design, product, and specifications require refinement labels Jun 10, 2024
@thomasrockhu-codecov thomasrockhu-codecov changed the title [Bundle Analysis] DISCOVERY/ISSUE: gate configuration option for PR based on bundle size and/or changes [Bundle Analysis] FEATURE: gate configuration option for PR based on bundle size and/or changes Jun 10, 2024
@codecov-hooky codecov-hooky bot added the epic this label is used to mark issues as epics label Jun 18, 2024
@giovanni-guidini
Copy link

giovanni-guidini commented Jul 22, 2024

TODO: 1st PR message confirming config is OK
Link to UI where user can see bundle details.

Maybe we can add a tag to the URL and trigger a quick tour of the UI?

TODO: Handle missing comparison
If the commit is not part of a PR we can still emit commit checks if there's a parent commit to compare against.

With the way we're doing bundle caching there "should" always be a parent, since we're copying forward on every commit creation, tho in those cases we would probably skip the checks anyways because no new bundle stuff has been uploaded
~Nick

That's a good condition to add. "some bundle must have been uploaded before sending notifications"

giovanni-guidini added a commit to codecov/worker that referenced this issue Sep 6, 2024
Customizes the bundle analysis PR comment based on the commit status outcome.
matches designs in codecov/engineering-team#1491 except for the table warnings (we can't get a per-bundle percentage yet)

Removing some unit tests in favor of larger scoped ones in `test_bundle_analysis.py`
giovanni-guidini added a commit to codecov/shared that referenced this issue Sep 9, 2024
The design for codecov/engineering-team#1491 includes warnings in the bundle table for individual bundles.

In order to do that we need a percentage change on the bundle level.
The calculation of percentage follows the same idea as the `Comparison`: the percentage is taken using the base bundle size as the 100%
github-merge-queue bot pushed a commit to codecov/shared that referenced this issue Sep 17, 2024
* feat: add percentage change for bundles

The design for codecov/engineering-team#1491 includes warnings in the bundle table for individual bundles.

In order to do that we need a percentage change on the bundle level.
The calculation of percentage follows the same idea as the `Comparison`: the percentage is taken using the base bundle size as the 100%

* remove incorrect comment
@codecov-hooky codecov-hooky bot closed this as completed Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request epic this label is used to mark issues as epics
Projects
None yet
Development

No branches or pull requests

5 participants