Skip to content

Report binary size metrics on GitHub pull requests. #2744

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

Merged
merged 8 commits into from
Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test-all.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: Run All Tests

on:
on:
pull_request:
branches:
- master
Expand Down
24 changes: 24 additions & 0 deletions .github/workflows/test-changed.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,27 @@ jobs:
run: yarn build
- name: Run tests on changed packages
run: xvfb-run yarn test:changed

health-metrics-test:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Contributor Author

@yifanyang yifanyang Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@yifanyang yifanyang Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

name: Health Metrics Test
runs-on: ubuntu-latest
env:
METRICS_SERVICE_URL: ${{ secrets.METRICS_SERVICE_URL }}
GITHUB_PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }}
GITHUB_PULL_REQUEST_BASE_SHA: ${{ github.event.pull_request.base.sha }}
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: 10.x
- uses: GoogleCloudPlatform/github-actions/setup-gcloud@master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it for connecting to your service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with:
service_account_key: ${{ secrets.GCP_SA_KEY }}
- run: cp config/ci.config.json config/project.json
- run: yarn install
- run: yarn build

- name: Run health-metrics/binary-size test
run: yarn size-report

# TODO(yifany): Enable startup times testing on CI.
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"docgen:node": "node scripts/docgen/generate-docs.js --api node",
"docgen": "yarn docgen:js; yarn docgen:node",
"prettier": "prettier --config .prettierrc --write '**/*.{ts,js}'",
"lint": "lerna run --scope @firebase/* --scope rxfire lint"
"lint": "lerna run --scope @firebase/* --scope rxfire lint",
"size-report": "node scripts/report_binary_size.js"
},
"repository": {
"type": "git",
Expand Down
148 changes: 148 additions & 0 deletions scripts/report_binary_size.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
const { resolve } = require('path');
const fs = require('fs');
const { execSync } = require('child_process');
const https = require('https');

const repoRoot = resolve(__dirname, '..');

const runId = process.env.GITHUB_RUN_ID || 'local-run-id';

const METRICS_SERVICE_URL = process.env.METRICS_SERVICE_URL;

// CDN scripts
function generateReportForCDNScripts() {
const reports = [];
const firebaseRoot = resolve(__dirname, '../packages/firebase');
const pkgJson = require(`${firebaseRoot}/package.json`);

const special_files = [
'firebase-performance-standalone.es2017.js',
'firebase-performance-standalone.js',
'firebase.js'
];

const files = [
...special_files.map(file => `${firebaseRoot}/${file}`),
...pkgJson.components.map(component => `${firebaseRoot}/firebase-${component}.js`)
];

for (const file of files) {
const { size } = fs.statSync(file);
const fileName = file.split('/').slice(-1)[0];
reports.push(makeReportObject('firebase', fileName, size));
}

return reports;
}

// NPM packages
function generateReportForNPMPackages() {
const reports = [];
const fields = [
'main',
'module',
'esm2017',
'browser',
'react-native',
'lite',
'lite-esm2017'
];

const packageInfo = JSON.parse(
execSync('npx lerna ls --json --scope @firebase/*', { cwd: repoRoot }).toString()
);

for (const package of packageInfo) {
const packageJson = require(`${package.location}/package.json`);

for (const field of fields) {
if (packageJson[field]) {
const filePath = `${package.location}/${packageJson[field]}`;
const { size } = fs.statSync(filePath);
reports.push(makeReportObject(packageJson.name, field, size));
}
}
}

return reports;
}

function makeReportObject(sdk, type, value) {
return {
sdk,
type,
value
};
}

function generateSizeReport() {
const reports = [
...generateReportForCDNScripts(),
...generateReportForNPMPackages()
];

for (const r of reports) {
console.log(r.sdk, r.type, r.value);
}

console.log(`Github Action URL: https://github.com/${process.env.GITHUB_REPOSITORY}/actions/runs/${runId}`);

return {
log: `https://github.com/${process.env.GITHUB_REPOSITORY}/actions/runs/${runId}`,
metric: "BinarySize",
results: reports
};
}

function constructRequestPath() {
const repo = process.env.GITHUB_REPOSITORY;
const commit = process.env.GITHUB_SHA;
let path = `/repos/${repo}/commits/${commit}/reports`;
if (process.env.GITHUB_EVENT_NAME === 'pull_request') {
const pullRequestNumber = process.env.GITHUB_PULL_REQUEST_NUMBER;
const pullRequestBaseSha = process.env.GITHUB_PULL_REQUEST_BASE_SHA;
path += `?pull_request=${pullRequestNumber}&base_commit=${pullRequestBaseSha}`;
}
return path;
}

function constructRequestOptions(path) {
const accessToken = execSync('gcloud auth print-identity-token', { encoding: 'utf8' }).trim();
return {
path: path,
method: 'POST',
headers: {
'Authorization': `Bearer ${accessToken}`,
'Content-Type': 'application/json'
}
};
}

function upload(report) {
if (!process.env.GITHUB_ACTIONS) {
console.log('Metrics upload is only enabled on CI.');
return;
}

const path = constructRequestPath();
const options = constructRequestOptions(path);

console.log(`${report.metric} report:`, report);
console.log(`Posting to metrics service endpoint: ${path} ...`);

const request = https.request(METRICS_SERVICE_URL, options, response => {
response.setEncoding('utf8');
console.log(`Response status code: ${response.statusCode}`);
response.on('data', console.log);
response.on('end', () => {
if (response.statusCode !== 202) {
process.exit(1);
}
})
});
request.write(JSON.stringify(report));
request.end();
}

const report = generateSizeReport();
upload(report);