Report binary size metrics on GitHub pull requests.

[yifanyang]

No description provided.

[yifanyang]

I'm able to enable and configure it in my forked repo. See this pull request comment.

Github Actions are failing since the workflow requires adding new secrets in this repo, which I don't have permission to do. @Feiyang1 Can you help me here?

[Feiyang1]

Do we want to run it on push? I think we want to limit it to run on PR only, with a condition like

if: github.event_name == 'pull_request'

[yifanyang]

It will need to be run on push for master branch. When we want to conduct the comparison for a pull request, the metrics for the PR head commit is from the pull request run, the metrics for the comparison base commit (usually a commit on master) actually comes from an earlier push run for master. I'm okay will limiting it to pr runs and push runs for master only.

Also you mentioned that not all pull requests are to be merged to master. Let's call their merge targets "feature branch". If we also want to support diffing in those pull requests. We will also need to include the size test in push builds for those "feature branch"es. I'm not sure about the priority for this one though.

Does it make sense?

[Feiyang1]

This trigger doesn't run when a PR is merged to master, but test-all workflow runs instead. I think it is because merging is not the same as directly pushing a commit.

"feature branch" diff will be nice to have. Will it require substantial change to enable?

[yifanyang]

Ahh, I'm not sure... For example this "Push/PR" run seems to be from master branch. If we take a look at its log for checking out source code, the commit being checked out and tested is actually the commit on master. The commit sha (4277e4c) is identical to the one in this "Run All Tests" run, which is for merging this pull request. git log origin/master shows all commits on master, and it seems that they all can be found in this history of "Push/PR" runs.

I think here is what happened: after a pull request is merged to master, the "merge" event triggers the "Run All Tests" workflow, and the new commit on master triggers the "Push/PR" workflow. Both are testing the new commit on master. The reason why there is no package specific test run in the "Push/PR" workflow is that the "Push/PR" workflow runs this script, in which we always compare the current checked out code (the HEAD) to the master (the origin/master) with this line. In case when the HEAD is actually the master branch, that git command returns no code change, thus no package specific test got run, as shown in this line.

In case of our size test here, since size test is an independent job in the workflow yaml file (not triggered by the run_changed.js script), as long as the "Push/PR" workflow is triggered, the size test will run. And I think it does get triggered by all new commits on master branch. In fact, our "Push/PR" workflow now is set to be triggered by new commit on all branches including master and "feature branches", thus this PR actually supports diffing for "feature branches".

(I'm not sure if I have been clear enough in this comment, as the reasoning process involved a lot of concepts. We can probably have a quick GVC to discuss it through.)

[Feiyang1]

Ah I see. I don't know if it is the intended behavior to run Push/PR on master because it does nothing. @hsubox76 Can you please clarify?

Commits to master should always run the full tests IMO. Right now I think our triggers are a little confusing, is it possible to add comment in yaml file or is there alternative triggers that we can use that makes the intent clearer?

[hsubox76]

I think it might be simpler to just change that trigger to

on:
  push:
    branches:
      - master

The only unwanted case there will be when we merge release branch to master every week, it will run, which we don't need, but that doesn't seem a big deal and in all the other cases will do what we want, I think, and avoid doing the above.

[yifanyang]

SGTM. Do you want me to include the trigger changes in this PR, or do you prefer to leave it in a separate PR?

[hsubox76]

The current plan is that we are trying to separate push and PR into different workflows because push can often be redundant and gets incorrect diff results on stale branches.

• The PR workflow would only cover PR updates not including merge to master. It will run tests on changed files and it will block PR submission.
• The push workflow would cover all pushes, including merge to master. It will run all tests, and NOT block PR submission.

I could do it in a separate PR but I think you probably want to decide where to put your job based on this separation, or if neither trigger is appropriate, or if there is a flaw in this plan overall. I think the triggers would be:

on: 
  pull_request:
    branches:
      - '*'
      - '!master'

and

on: push

[yifanyang]

This size test need to be run on all pull_requests and pushes (at least the ones from master).

How about we do this -- I'll hold off this PR for now, wait for your changes for workflows to merge in first, and then accommodate your change here. Does it make sense?

[hsubox76]

Sure, feel free to double check I am not missing something: #2763

[Feiyang1]

Is it for connecting to your service?

[yifanyang]

Yes. The metrics service, the one that receives size measurements, calculates the diff and posts reports back to pull requests as a comment, is hosted on GCP. Google cloud SDK is needed here to authenticate our size test so that it is allowed to call the metrics service.

[google-oss-bot]

Binary Size Report

Affected SDKs

SDK	Type	Base (afaf982)	Head (debd056)	Diff
@firebase/logger	module	?	4778.00	? (?)
	esm2017	?	3223.00	? (?)
	main	?	5087.00	? (?)
@firebase/storage	module	?	62277.00	? (?)
	esm2017	?	56137.00	? (?)
	main	?	62483.00	? (?)
@firebase/util	module	?	18551.00	? (?)
	browser	?	19454.00	? (?)
	esm2017	?	17369.00	? (?)
	main	?	19481.00	? (?)
@firebase/app	react-native	?	9862.00	? (?)
	lite-esm2017	?	7748.00	? (?)
	module	?	11011.00	? (?)
	browser	?	11108.00	? (?)
	esm2017	?	9461.00	? (?)
	main	?	10105.00	? (?)
	lite	?	9102.00	? (?)
@firebase/firestore	module	?	271630.00	? (?)
	browser	?	275008.00	? (?)
	esm2017	?	271630.00	? (?)
	main	?	494047.00	? (?)
@firebase/installations	module	?	21544.00	? (?)
	esm2017	?	16530.00	? (?)
	main	?	21995.00	? (?)
@firebase/functions	module	?	8994.00	? (?)
	browser	?	9174.00	? (?)
	esm2017	?	7010.00	? (?)
	main	?	9209.00	? (?)
@firebase/webchannel-wrapper	module	?	41226.00	? (?)
	main	?	40383.00	? (?)
@firebase/performance	module	?	24914.00	? (?)
	browser	?	25158.00	? (?)
	esm2017	?	23188.00	? (?)
	main	?	25158.00	? (?)
firebase	firebase-installations.js	?	19436.00	? (?)
	firebase-storage.js	?	41262.00	? (?)
	firebase-messaging.js	?	39431.00	? (?)
	firebase.js	?	848935.00	? (?)
	firebase-auth.js	?	175057.00	? (?)
	firebase-analytics.js	?	26646.00	? (?)
	firebase-performance-standalone.js	?	46946.00	? (?)
	firebase-remote-config.js	?	37217.00	? (?)
	firebase-performance.js	?	37211.00	? (?)
	firebase-app.js	?	20008.00	? (?)
	firebase-firestore.js	?	315800.00	? (?)
	firebase-database.js	?	185548.00	? (?)
	firebase-performance-standalone.es2017.js	?	70398.00	? (?)
	firebase-functions.js	?	9642.00	? (?)
@firebase/analytics	module	?	9197.00	? (?)
	esm2017	?	8508.00	? (?)
	main	?	9515.00	? (?)
@firebase/auth	module	?	177089.00	? (?)
	main	?	177099.00	? (?)
@firebase/remote-config	module	?	22720.00	? (?)
	browser	?	23103.00	? (?)
	esm2017	?	17705.00	? (?)
	main	?	23103.00	? (?)
@firebase/testing	main	?	6598.00	? (?)
@firebase/messaging	module	?	30549.00	? (?)
	esm2017	?	22806.00	? (?)
	main	?	31006.00	? (?)
@firebase/database	module	?	264057.00	? (?)
	browser	?	265575.00	? (?)
	esm2017	?	232641.00	? (?)
	main	?	266339.00	? (?)
@firebase/component	module	?	5046.00	? (?)
	browser	?	5164.00	? (?)
	esm2017	?	3898.00	? (?)
	main	?	5164.00	? (?)
@firebase/polyfill	module	?	666.00	? (?)
	main	?	733.00	? (?)

Metric Unit: byte

Test Logs

• https://github.com/firebase/firebase-js-sdk/actions/runs/61851404

[yifanyang]

I'm making the size test in its own workflow for these reasons:

• Size test need to be run on both push and pull_request. This way I don't need to duplicate the job setup in two files (test-all.yml and test-changed.yaml).
• The displayed name seems clearer: "Health Metrics / Binary Size". Later I would like to surface startup times measurement in pull request as well, and it will be "Health Metrics / Startup Times".

What do you think? @Feiyang1 @hsubox76

[Feiyang1]

LGTM