-
Notifications
You must be signed in to change notification settings - Fork 938
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
Changes from 4 commits
5c13ff3
c06eea0
1c34983
6266328
b90abcf
7b129c9
573e4f2
768ac9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
name: Run All Tests | ||
|
||
on: | ||
on: | ||
pull_request: | ||
branches: | ||
- master | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,27 @@ jobs: | |
run: yarn build | ||
- name: Run tests on changed packages | ||
run: xvfb-run yarn test:changed | ||
|
||
health-metrics-test: | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it for connecting to your service? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
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); | ||
Feiyang1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}) | ||
}); | ||
request.write(JSON.stringify(report)); | ||
request.end(); | ||
} | ||
|
||
const report = generateSizeReport(); | ||
upload(report); |
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 (theorigin/master
) with this line. In case when theHEAD
is actually the master branch, thatgit
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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:
and
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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