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

Move ICFY stat generation to Github Actions #39646

Merged
merged 26 commits into from
Mar 2, 2020

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Feb 24, 2020

Changes proposed in this Pull Request

  • Move ICFY stat generation to Github Actions

Requires:

@blowery blowery requested a review from a team as a code owner February 24, 2020 21:04
@matticbot
Copy link
Contributor

@blowery blowery force-pushed the update/move-icfy-stats-to-github-action branch 3 times, most recently from 4284e1d to 2535df2 Compare February 24, 2020 21:09
@blowery blowery self-assigned this Feb 24, 2020
@blowery blowery added the Build label Feb 24, 2020
@blowery blowery force-pushed the update/move-icfy-stats-to-github-action branch from 2535df2 to 63a06d8 Compare February 24, 2020 21:11
with:
node-version: '>=12.15.0'
- name: Checkout code
uses: actions/checkout@master
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this? We need to fetch the refs, not just the commits:

Suggested change
uses: actions/checkout@master
uses: actions/checkout@v2
run: git fetch --no-tags --prune --depth=1 origin +refs/heads/master:refs/remotes/origin/master

Maybe we can drop the with/fetch-depth as well.

https://github.com/actions/checkout#fetch-all-tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sirreal
Copy link
Member

sirreal commented Feb 25, 2020

This seems ready to me, ICFY will need some change to handle this.

Automattic/icfy#23

ANCESTOR_SHA1=$(git merge-base HEAD origin/master)
CURRENT_BRANCH=$(git rev-parse --abbrev-ref HEAD)
curl -X POST --globoff \
"http://api.iscalypsofastyet.com:5000/submit-stats?secret=$ICFY_SECRET" \
Copy link
Contributor Author

@blowery blowery Feb 25, 2020

Choose a reason for hiding this comment

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

it hated the ${{}} syntax huh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm, maybe the secret has/had bad URL characters in it?

Copy link
Member

Choose a reason for hiding this comment

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

We could inline the secret again without the env, the important bit is the --globoff option.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the secret contains characters like %, |, } or *. Definitely not URL and shell friendly 🙂

-d '{
"payload": {
"branch": "'"$CURRENT_BRANCH"'",
"build_num": '"$GITHUB_RUN_NUMBER"',
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by the build numbering. For the feb1484 commit, the build_num received by ICFY was 16:

Received CircleCI success webhook notification: { payload:
   { branch: 'update/move-icfy-stats-to-github-action',
     build_num: 16,
     sha: 'feb1484add484d02d57e6728bd5ad0adf7529698',
     ancestor: '0c9f34fbee2020e8d771dce3c98a1439c30324e6',
     from: 'gh-action' } }

But the run numbers in GH web UI are 9 figures starting with 4 and the artifact download URL is:

https://github.com/Automattic/wp-calypso/suites/480255681/artifacts/2208657

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh. actions/upload-artifact#50

Yeah, the run_id would let you query for artifacts: https://developer.github.com/v3/actions/artifacts/

Copy link
Member

@jsnajdr jsnajdr Feb 25, 2020

Choose a reason for hiding this comment

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

Now after the latest commit the build number is 17, the build id is 44892545 and the artifact URL is https://github.com/Automattic/wp-calypso/suites/480418004/artifacts/2211405. Not quite there yet, but much closer 🙂

Copy link
Member

Choose a reason for hiding this comment

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

curl https://api.github.com/repos/automattic/wp-calypso/actions/runs/44892545/artifacts

returns:

{
  "total_count": 1,
  "artifacts": [
    {
      "id": 2211405,
      "node_id": "MDg6QXJ0aWZhY3QyMjExNDA1",
      "name": "icfy",
      "size_in_bytes": 192025220,
      "url": "https://api.github.com/repos/Automattic/wp-calypso/actions/artifacts/2211405",
      "archive_download_url": "https://api.github.com/repos/Automattic/wp-calypso/actions/artifacts/2211405/zip",
      "expired": false,
      "created_at": "2020-02-25T13:04:29Z",
      "updated_at": "2020-02-25T13:04:30Z"
    }
  ]
}

I guess that with the build ID, getting to the artifact download URL became possible 🎉

- uses: actions/upload-artifact@v1
with:
name: icfy
path: icfy-stats
Copy link
Member

Choose a reason for hiding this comment

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

Can we publish the artifacts as two plain files, stats.json and chart.json? That would be much more convenient for the ICFY client. It now downloads the files directly into memory. Now, it would need to unpack the ZIP to some tmp directory, read the JSON files from there, cleanup at the end...

Copy link
Member

Choose a reason for hiding this comment

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

This uses https://github.com/actions/upload-artifact. Docs make it sound like it will always be a zip:

You must upload artifacts during a workflow run. GitHub provides two actions that you can use to upload and download build artifacts. Files uploaded to a workflow run are archived using the .zip format. For more information, see the actions/upload-artifact and download-artifact actions.

https://help.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem like it. https://github.com/actions/upload-artifact

It wants a path to a directory and a name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure it helps, but zips can be used as streams with https://www.npmjs.com/package/node-stream-zip

Copy link
Member

Choose a reason for hiding this comment

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

node-stream-zip can only read from files 🙁 I need something that can read from a generic stream. Will continue looking for another package.

Copy link
Member

Choose a reason for hiding this comment

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

Found https://www.npmjs.com/package/unzipper which works great so far 👍

@matticbot
Copy link
Contributor

matticbot commented Feb 28, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~14305 bytes removed 📉 [gzipped])

name              parsed_size           gzip_size
security             -29787 B  (-6.8%)    -6994 B  (-6.2%)
checkout             -29787 B  (-2.2%)    -6992 B  (-2.1%)
woocommerce            -367 B  (-0.0%)     -137 B  (-0.0%)
settings-writing       -367 B  (-0.1%)      -78 B  (-0.1%)
post-editor            -290 B  (-0.0%)     -104 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~296 bytes removed 📉 [gzipped])

name                                parsed_size           gzip_size
async-load-reader-search-stream          -292 B  (-0.3%)      -98 B  (-0.5%)
async-load-reader-following-manage       -292 B  (-0.3%)     -116 B  (-0.4%)
async-load-design-blocks                 -290 B  (-0.0%)      -82 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Moment.js Locales (~3979 bytes removed 📉 [gzipped])

name                    parsed_size            gzip_size
moment-locale-en-au        -12567 B  (-90.9%)    -4212 B  (-86.0%)
moment-locale-sw            -1357 B  (-53.9%)     -459 B  (-42.1%)
moment-locale-te            +1126 B  (+97.0%)     +327 B  (+51.7%)
moment-locale-tet           -1024 B  (-44.8%)     -270 B  (-28.2%)
moment-locale-ur            -1009 B  (-42.6%)     -289 B  (-28.3%)
moment-locale-kn             +993 B  (+56.4%)     +275 B  (+31.4%)
moment-locale-gu             +986 B  (+64.9%)     +351 B  (+47.6%)
moment-locale-sq             -858 B  (-39.9%)     -230 B  (-24.7%)
moment-locale-it-ch          -825 B  (-38.6%)     -245 B  (-25.4%)
moment-locale-ms             -822 B  (-37.6%)     -251 B  (-25.9%)
moment-locale-tg             +790 B  (+62.5%)     +292 B  (+42.4%)
moment-locale-my             +777 B  (+57.0%)     +219 B  (+30.5%)
moment-locale-pt-br          -751 B  (-36.3%)     -293 B  (-29.4%)
moment-locale-sr             +735 B  (+56.9%)     +249 B  (+35.4%)
moment-locale-si             +733 B  (+55.2%)     +155 B  (+21.9%)
moment-locale-ug-cn          +731 B  (+44.7%)     +153 B  (+17.6%)
moment-locale-ku             -664 B  (-24.1%)     -174 B  (-15.1%)
moment-locale-tr             -620 B  (-29.4%)      -61 B   (-7.1%)
moment-locale-he             -565 B  (-22.6%)     -210 B  (-19.3%)
moment-locale-sr-cyrl        +491 B  (+24.2%)     +139 B  (+14.6%)
moment-locale-mn             +435 B  (+24.9%)      +60 B   (+6.6%)
moment-locale-x-pseudo       +376 B  (+32.5%)     +253 B  (+42.2%)
moment-locale-kk             +347 B  (+24.5%)     +137 B  (+18.6%)
moment-locale-mk             -295 B  (-14.4%)      -47 B   (-4.9%)
moment-locale-lt             +276 B  (+13.5%)     +290 B  (+35.7%)
moment-locale-me             -275 B  (-11.9%)     -145 B  (-13.2%)
moment-locale-zh-cn          +226 B  (+14.7%)      +70 B   (+8.2%)
moment-locale-uz-latn        -200 B  (-14.7%)     -132 B  (-18.1%)
moment-locale-hr             +185 B   (+9.5%)      +61 B   (+6.9%)
moment-locale-gl             +171 B  (+12.7%)       +3 B   (+0.4%)
moment-locale-fy             +155 B  (+12.3%)      +83 B  (+12.1%)
moment-locale-tzl            +148 B  (+10.0%)      +68 B   (+8.5%)
moment-locale-nl             -147 B   (-6.9%)       -1 B   (-0.1%)
moment-locale-jv             +103 B   (+7.9%)      +17 B   (+2.4%)
moment-locale-sk              +90 B   (+4.4%)      +70 B   (+8.1%)
moment-locale-pl              +65 B   (+3.2%)      +55 B   (+5.9%)
moment-locale-ga              -63 B   (-4.5%)      -34 B   (-4.4%)
moment-locale-th              +54 B   (+2.6%)     -121 B  (-12.3%)
moment-locale-lo              -47 B   (-2.2%)     -164 B  (-16.8%)
moment-locale-sd              +40 B   (+3.0%)      +53 B   (+7.5%)
moment-locale-se              -29 B   (-2.1%)      -47 B   (-6.2%)
moment-locale-hy-am           +12 B   (+0.6%)      +26 B   (+2.8%)

Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@jsnajdr jsnajdr force-pushed the update/move-icfy-stats-to-github-action branch from 4642bb4 to d1abef8 Compare February 28, 2020 11:21
Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

@sirreal @blowery @sgomes ICFY was updated and the latest build in this branch was performed by the GitHub Action and recorded correctly by ICFY.

The stats reported are wrong, because the base master commit doesn't have valid stats -- it failed with the well-known "out of memory" error.

@jsnajdr jsnajdr force-pushed the update/move-icfy-stats-to-github-action branch from d1abef8 to e5c331b Compare February 28, 2020 21:19
@jsnajdr jsnajdr merged commit 732447e into master Mar 2, 2020
@jsnajdr jsnajdr deleted the update/move-icfy-stats-to-github-action branch March 2, 2020 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants