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

ref(build): Use rollup to build AWS lambda layer #5146

Merged
merged 16 commits into from
May 25, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented May 20, 2022

Our current system for building the AWS lambda layer involves a script which manually traces the serverless package's dependencies, including other monorepo packages, and symlinks them into a node_modules folder in a directory set up to mimic an installation from npm. There are a few disadvantages to this system:

  • The signals we rely on to figure out what is and isn't a dependency aren't exhaustive, and we're not experts at this, both of which have made getting the right files and only the right files into the layer a brittle process, which has broken down more than once and caused issue for our users.
  • The script is complicated, which makes it harder to figure out exactly what's causing it when something does go wrong.
  • We symlink in entire packages at a time, regardless of whether or not we're using most of the code. This is true even of the serverless package itself, where we include all of the GCP code along with the AWS code.

This refactors our lambda layer build process to use the new bundling config functions we use for CDN bundles, to create a bundle which can take the place of the the index file, but which contains everything it needs internally. This has the following advantages:

  • This puts Rollup in charge of figuring out dependencies instead of us.
  • The config is in line with existing configs and is therefore much easier to reason about and maintain.
  • It lets us easily exclude anything GCP-related in the SDK. Between that, Rollup's treeshaking, and terser's minifying, the layer will now take up much less of the finite size allotted to each lambda function.
  • Removing extraneous files means less to cache and retrieve from the cache in each GHA job.

Key changes:

  • The layer now builds in packages/serverless/build/aws rather than at the top level of the repo. (It is now moved to the top level only in our GHA workflow creating the lambda zip.)
  • In that workflow, the process to determine the SDK version has been simplified.
  • The bundle builds based not on the main index file but on a new bundle-specific index file, which only includes AWS code.
  • There is new rollup config just for the layer, which uses the bundle-building functions to make the main bundle and the npm-package-building functions to create a separate module for the optional script which automatically starts up the SDK in AWS.
  • The old build script has been replaced by one which runs rollup with that config, and then symlinks the built auto-startup script into its legacy location, so as to be backwards compatible with folks who run it using an environment variable pointing to the old path.
  • The building of the layer has temporarily been shifted from build:awslambda-layer to build:bundle, so that it will run during the first phase of the repo-level yarn build. (The goal is to eventually do everything in one phase, for greater parallelization and shorter overall build time.)

h/t to @antonpirker for all his help testing and thinking through this with me.

@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.36 KB (-3.86% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.05 KB (-7.06% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.21 KB (-3.45% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.67 KB (-7.43% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.92 KB (-14.26% 🔽)
@sentry/browser - Webpack (minified) 63.13 KB (-22.75% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.95 KB (-14.31% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 43.77 KB (-8.93% 🔽)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.38 KB (-2.67% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23.92 KB (-2.28% 🔽)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! I left some remarks and questions but overall this looks good to 🚀

"build": "run-p build:rollup build:types && yarn build:extras",
"build:awslambda-layer": "node scripts/build-awslambda-layer.js",
"build": "run-p build:rollup build:types build:bundle && yarn build:extras",
"build:awslambda-layer": "echo 'WARNING: AWS lambda layer build emporarily moved to \\`build:bundle\\`.'",
Copy link
Member

Choose a reason for hiding this comment

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

This will be refactored in an upcoming PR, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on your definition of "upcoming." 😛

But yes, eventually, once I've applied the "wait only as long as you need to" to all of the "extras" steps, then the "extras" build can happen in parallel with the others, and this can move back under that umbrella.

packages/serverless/rollup.aws.config.js Outdated Show resolved Hide resolved
console.log('Creating symlink for `awslambda-auto.js` in legacy `dist` directory.');
fsForceMkdirSync('build/aws/dist-serverless/nodejs/node_modules/@sentry/serverless/dist');
fs.symlinkSync(
'../build/cjs/awslambda-auto.js',
Copy link
Member

Choose a reason for hiding this comment

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

I might be mistaken but shouldn't this file be in build/npm?

Suggested change
'../build/cjs/awslambda-auto.js',
'../build/npm/cjs/awslambda-auto.js',

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow. Not only are you right, you're making me realize that there's a number of other places I should have fixed this, too. Yikes. Thank you for catching that!

@@ -0,0 +1,66 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to convert this script into a node script at some point in the future because we recently moved from bash scripts to node scripts for platform independence (see #4720 for context). But this can easily be done some time in the future in a follow-up PR since it's a new script and our contributors won't need it

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a note to the top. I think it makes sense to keep it in bash, because it's supposed to be a mirror of the GHA, which is also written in bash.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh that makes sense 👍

@lobsterkatie lobsterkatie marked this pull request as draft May 23, 2022 13:58
@lobsterkatie lobsterkatie force-pushed the kmclb-use-rollup-for-lambda-layer branch 5 times, most recently from 7565aa0 to 5b31c75 Compare May 24, 2022 07:52
@lobsterkatie lobsterkatie force-pushed the kmclb-use-rollup-for-lambda-layer branch 2 times, most recently from 5e56e20 to cce54c0 Compare May 25, 2022 13:58
@lobsterkatie lobsterkatie marked this pull request as ready for review May 25, 2022 14:18
@lobsterkatie lobsterkatie merged commit 549512d into 7.x May 25, 2022
@lobsterkatie lobsterkatie deleted the kmclb-use-rollup-for-lambda-layer branch May 25, 2022 14:22
AbhiPrasad pushed a commit that referenced this pull request May 30, 2022
Our current system for building the AWS lambda layer involves a script which manually traces the serverless package's dependencies, including other monorepo packages, and symlinks them into a `node_modules` folder in a directory set up to mimic an installation from npm. There are a few disadvantages to this system:
- The signals we rely on to figure out what is and isn't a dependency aren't exhaustive, and we're not experts at this, both of which have made getting the right files and only the right files into the layer a brittle process, which has broken down more than once and caused issue for our users.
- The script is complicated, which makes it harder to figure out exactly what's causing it when something does go wrong.
- We symlink in entire packages at a time, regardless of whether or not we're using most of the code. This is true even of the serverless package itself, where we include all of the GCP code along with the AWS code.

This refactors our lambda layer build process to use the new bundling config functions we use for CDN bundles, to create a bundle which can take the place of the the index file, but which contains everything it needs internally. This has the following advantages:

- This puts Rollup in charge of figuring out dependencies instead of us. 
- The config is in line with existing configs and is therefore much easier to reason about and maintain.
- It lets us easily exclude anything GCP-related in the SDK. Between that, Rollup's treeshaking, and terser's minifying, the layer will now take up much less of the finite size allotted to each lambda function.
- Removing extraneous files means less to cache and retrieve from the cache in each GHA job.

Key changes:

- The layer now builds in `packages/serverless/build/aws` rather than at the top level of the repo. (It is now moved to the top level only in our GHA workflow creating the lambda zip.)
- In that workflow, the process to determine the SDK version has been simplified.
- The bundle builds based not on the main index file but on a new bundle-specific index file, which only includes AWS code.
- There is new rollup config just for the layer, which uses the bundle-building functions to make the main bundle and the npm-package-building functions to create a separate module for the optional script which automatically starts up the SDK in AWS.
- The old build script has been replaced by one which runs rollup with that config, and then symlinks the built auto-startup script into its legacy location, so as to be backwards compatible with folks who run it using an environment variable pointing to the old path.
- The building of the layer has temporarily been shifted from `build:awslambda-layer` to `build:bundle`, so that it will run during the first phase of the repo-level `yarn build`. (The goal is to eventually do everything in one phase, for greater parallelization and shorter overall build time.)

h/t to @antonpirker for all his help testing and thinking through this with me.
@AbhiPrasad AbhiPrasad added this to the 7.0.0 milestone May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants